Preserve original discrete trigger value
Currently, there are no 'real' discrete sensors, so discrete trigger is
working with numeric ones. Dbus api is using string as thresholdValue,
but internally service is converting it to double. This resulted in
side-effect of malformed value of Thresholds property, e.g., 90.0 being
represented as 90.000000. This change stores original value in order to
not confuse potential users.
Additionally, check was added to validate whole string of thesholdValue.
Now, it must consist only of numeric characters, values like '12.3FOO'
will be rejected on AddTrigger call.
Testing done:
- UTs added and are passing,
- dbus get-property on Thresholds confirms unchanged initial value.
Signed-off-by: Szymon Dompke <szymon.dompke@intel.com>
Change-Id: Iec3514ac1479587e610f8da31ecf9ba6fc0bdb62
diff --git a/src/discrete_threshold.cpp b/src/discrete_threshold.cpp
index d7640f6..1bb250f 100644
--- a/src/discrete_threshold.cpp
+++ b/src/discrete_threshold.cpp
@@ -1,15 +1,19 @@
#include "discrete_threshold.hpp"
+#include "utils/conversion_trigger.hpp"
+
#include <phosphor-logging/log.hpp>
DiscreteThreshold::DiscreteThreshold(
boost::asio::io_context& ioc, Sensors sensorsIn,
std::vector<std::unique_ptr<interfaces::TriggerAction>> actionsIn,
- Milliseconds dwellTimeIn, double thresholdValueIn,
+ Milliseconds dwellTimeIn, const std::string& thresholdValueIn,
const std::string& nameIn, const discrete::Severity severityIn) :
ioc(ioc),
actions(std::move(actionsIn)), dwellTime(dwellTimeIn),
- thresholdValue(thresholdValueIn), name(nameIn), severity(severityIn)
+ thresholdValue(thresholdValueIn),
+ numericThresholdValue(utils::stodStrict(thresholdValue)), name(nameIn),
+ severity(severityIn)
{
for (const auto& sensor : sensorsIn)
{
@@ -45,17 +49,14 @@
auto& details = getDetails(sensor);
auto& [sensorName, dwell, timer] = details;
- if (thresholdValue)
+ if (dwell && value != numericThresholdValue)
{
- if (dwell && value != thresholdValue)
- {
- timer.cancel();
- dwell = false;
- }
- else if (value == thresholdValue)
- {
- startTimer(details, timestamp, value);
- }
+ timer.cancel();
+ dwell = false;
+ }
+ else if (value == numericThresholdValue)
+ {
+ startTimer(details, timestamp, value);
}
}
@@ -100,5 +101,5 @@
LabeledThresholdParam DiscreteThreshold::getThresholdParam() const
{
return discrete::LabeledThresholdParam(name, severity, dwellTime.count(),
- std::to_string(thresholdValue));
+ thresholdValue);
}
diff --git a/src/discrete_threshold.hpp b/src/discrete_threshold.hpp
index 2a51a1a..545a980 100644
--- a/src/discrete_threshold.hpp
+++ b/src/discrete_threshold.hpp
@@ -24,8 +24,8 @@
DiscreteThreshold(
boost::asio::io_context& ioc, Sensors sensors,
std::vector<std::unique_ptr<interfaces::TriggerAction>> actions,
- Milliseconds dwellTime, double thresholdValue, const std::string& name,
- const discrete::Severity severity);
+ Milliseconds dwellTime, const std::string& thresholdValue,
+ const std::string& name, const discrete::Severity severity);
DiscreteThreshold(const DiscreteThreshold&) = delete;
DiscreteThreshold(DiscreteThreshold&&) = delete;
@@ -38,7 +38,8 @@
boost::asio::io_context& ioc;
const std::vector<std::unique_ptr<interfaces::TriggerAction>> actions;
const Milliseconds dwellTime;
- const double thresholdValue;
+ const std::string thresholdValue;
+ const double numericThresholdValue;
const std::string name;
const discrete::Severity severity;
bool initialized = false;
diff --git a/src/trigger_factory.cpp b/src/trigger_factory.cpp
index da5a64c..6c9ceb9 100644
--- a/src/trigger_factory.cpp
+++ b/src/trigger_factory.cpp
@@ -162,8 +162,7 @@
thresholds.emplace_back(std::make_shared<DiscreteThreshold>(
bus->get_io_context(), sensors, std::move(actions),
- Milliseconds(dwellTime), std::stod(thresholdValue), thresholdName,
- severity));
+ Milliseconds(dwellTime), thresholdValue, thresholdName, severity));
}
void TriggerFactory::makeNumericThreshold(
diff --git a/src/utils/conversion_trigger.cpp b/src/utils/conversion_trigger.cpp
index bb0c3bc..d65eae9 100644
--- a/src/utils/conversion_trigger.cpp
+++ b/src/utils/conversion_trigger.cpp
@@ -140,4 +140,16 @@
labeledThresholdParams);
}
+double stodStrict(const std::string& str)
+{
+ size_t pos = 0;
+ double result = std::stod(str, &pos);
+ if (pos < str.length())
+ {
+ throw std::invalid_argument(
+ "non-numeric characters at the end of string");
+ }
+ return result;
+}
+
} // namespace utils
diff --git a/src/utils/conversion_trigger.hpp b/src/utils/conversion_trigger.hpp
index a749962..9d5d4a5 100644
--- a/src/utils/conversion_trigger.hpp
+++ b/src/utils/conversion_trigger.hpp
@@ -55,4 +55,6 @@
return std::holds_alternative<AlternativeT>(*collection.begin());
}
+double stodStrict(const std::string& str);
+
} // namespace utils
diff --git a/tests/src/test_discrete_threshold.cpp b/tests/src/test_discrete_threshold.cpp
index ebd50cc..b68d1c3 100644
--- a/tests/src/test_discrete_threshold.cpp
+++ b/tests/src/test_discrete_threshold.cpp
@@ -24,7 +24,7 @@
std::shared_ptr<DiscreteThreshold> sut;
std::shared_ptr<DiscreteThreshold>
- makeThreshold(Milliseconds dwellTime, double thresholdValue,
+ makeThreshold(Milliseconds dwellTime, std::string thresholdValue,
discrete::Severity severity = discrete::Severity::ok)
{
std::vector<std::unique_ptr<interfaces::TriggerAction>> actions;
@@ -46,7 +46,7 @@
.WillByDefault(Return(sensorNames[idx]));
}
- sut = makeThreshold(0ms, 90.0, discrete::Severity::critical);
+ sut = makeThreshold(0ms, "90.0", discrete::Severity::critical);
}
};
@@ -68,13 +68,35 @@
EXPECT_CALL(actionMock, commit(_, _, _)).Times(0);
}
-TEST_F(TestDiscreteThreshold, getLabeledParamsReturnsCorrectly)
+class TestDiscreteThresholdValues :
+ public TestDiscreteThreshold,
+ public WithParamInterface<std::string>
+{};
+
+INSTANTIATE_TEST_SUITE_P(_, TestDiscreteThresholdValues,
+ Values("90", ".90", "90.123", "0.0"));
+
+TEST_P(TestDiscreteThresholdValues, thresholdValueIsNumericAndStoredCorrectly)
{
+ sut = makeThreshold(0ms, GetParam(), discrete::Severity::critical);
LabeledThresholdParam expected = discrete::LabeledThresholdParam(
- "treshold name", discrete::Severity::critical, 0, std::to_string(90.0));
+ "treshold name", discrete::Severity::critical, 0, GetParam());
EXPECT_EQ(sut->getThresholdParam(), expected);
}
+class TestBadDiscreteThresholdValues :
+ public TestDiscreteThreshold,
+ public WithParamInterface<std::string>
+{};
+
+INSTANTIATE_TEST_SUITE_P(_, TestBadDiscreteThresholdValues,
+ Values("90ad", "ab.90", "x90", "On", "Off", ""));
+
+TEST_P(TestBadDiscreteThresholdValues, throwsWhenNotNumericValues)
+{
+ EXPECT_THROW(makeThreshold(0ms, GetParam()), std::invalid_argument);
+}
+
struct DiscreteParams
{
struct UpdateParams
@@ -117,9 +139,9 @@
return *this;
}
- DiscreteParams& ThresholdValue(double val)
+ DiscreteParams& ThresholdValue(std::string val)
{
- thresholdValue = val;
+ thresholdValue = std::move(val);
return *this;
}
@@ -152,7 +174,7 @@
std::vector<UpdateParams> updates;
std::vector<ExpectedParams> expected;
- double thresholdValue = 0.0;
+ std::string thresholdValue = "0.0";
Milliseconds dwellTime = 0ms;
};
@@ -227,18 +249,18 @@
INSTANTIATE_TEST_SUITE_P(_, TestDiscreteThresholdNoDwellTime,
Values(DiscreteParams()
- .ThresholdValue(90.0)
+ .ThresholdValue("90.0")
.Updates({{0, 1ms, 80.0}, {0, 2ms, 89.0}})
.Expected({}),
DiscreteParams()
- .ThresholdValue(90.0)
+ .ThresholdValue("90.0")
.Updates({{0, 1ms, 80.0},
{0, 2ms, 90.0},
{0, 3ms, 80.0},
{0, 4ms, 90.0}})
.Expected({{0, 2ms, 90.0}, {0, 4ms, 90.0}}),
DiscreteParams()
- .ThresholdValue(90.0)
+ .ThresholdValue("90.0")
.Updates({{0, 1ms, 90.0},
{0, 2ms, 99.0},
{1, 3ms, 100.0},
@@ -270,17 +292,17 @@
_, TestDiscreteThresholdWithDwellTime,
Values(DiscreteParams()
.DwellTime(200ms)
- .ThresholdValue(90.0)
+ .ThresholdValue("90.0")
.Updates({{0, 1ms, 90.0, 100ms}, {0, 2ms, 91.0}, {0, 3ms, 90.0}})
.Expected({{0, 3ms, 90.0, 300ms}}),
DiscreteParams()
.DwellTime(100ms)
- .ThresholdValue(90.0)
+ .ThresholdValue("90.0")
.Updates({{0, 1ms, 90.0, 100ms}})
.Expected({{0, 1ms, 90.0, 100ms}}),
DiscreteParams()
.DwellTime(1000ms)
- .ThresholdValue(90.0)
+ .ThresholdValue("90.0")
.Updates({{0, 1ms, 90.0, 700ms},
{0, 1ms, 91.0, 100ms},
{0, 1ms, 90.0, 300ms},
@@ -288,7 +310,7 @@
.Expected({}),
DiscreteParams()
.DwellTime(200ms)
- .ThresholdValue(90.0)
+ .ThresholdValue("90.0")
.Updates({{0, 1ms, 90.0},
{1, 2ms, 89.0, 100ms},
{1, 3ms, 90.0, 100ms},