dbuspassive: Input processing cleanup and allow "margin"
The sensor input processing steps have been cleaned up,
consolidating code duplicated in two places into one.
The new "updateValue" function does the processing,
then calls "setValue" to set it, if it was acceptable.
Initialization of sensor value now uses the same processing
path as regular sensor updates, solving a strange discontinuity
seen at initialization, where the values had wrong scaling,
until the first asynchronous D-Bus sensor update was received.
Also cleaned up handling of floating-point NAN received,
which has same meaning as old -1 sentinel value,
as used back when the D-Bus schema was integer millidegrees.
Sensors of type "margin" are now accepted,
as they are now processed correctly.
Adding new member "_typeMargin" flag to remember margin type,
avoiding repeated string computation.
Adding new member "_badReading" flag to track the case in which
sensor reading is working but gave us bad reading, such as NAN,
as this situation differs from already-existing "_failed" flag.
Adding new member "_marginHot" flag to request failsafe mode,
even though sensor is functional and reading is good,
because if we have ran out of margin, PID is not doing the job,
and thus we need failsafe mode, to make sure fans spin fast.
Signed-off-by: Josh Lehan <krellan@google.com>
Change-Id: Ia6e2f58967f58ae2999489e3e9342388f7b5fb1a
diff --git a/dbus/dbuspassive.cpp b/dbus/dbuspassive.cpp
index a90f5ea..6667bc4 100644
--- a/dbus/dbuspassive.cpp
+++ b/dbus/dbuspassive.cpp
@@ -89,10 +89,14 @@
{
_scale = settings.scale;
- _value = settings.value * pow(10, _scale);
- _min = settings.min * pow(10, _scale);
- _max = settings.max * pow(10, _scale);
- _updated = std::chrono::high_resolution_clock::now();
+ _min = settings.min * std::pow(10.0, _scale);
+ _max = settings.max * std::pow(10.0, _scale);
+
+ // Cache this type knowledge, to avoid repeated string comparison
+ _typeMargin = (type == "margin");
+
+ // Force value to be stored, otherwise member would be uninitialized
+ updateValue(settings.value, true);
}
ReadReturn DbusPassive::read(void)
@@ -123,6 +127,26 @@
}
}
+ // If a reading has came in,
+ // but its value bad in some way (determined by sensor type),
+ // indicate this sensor has failed,
+ // until another value comes in that is no longer bad.
+ // This is different from the overall _failed flag,
+ // which is set and cleared by other causes.
+ if (_badReading)
+ {
+ return true;
+ }
+
+ // If a reading has came in, and it is not a bad reading,
+ // but it indicates there is no more thermal margin left,
+ // that is bad, something is wrong with the PID loops,
+ // they are not cooling the system, enable failsafe mode also.
+ if (_marginHot)
+ {
+ return true;
+ }
+
return _failed || !_functional;
}
@@ -156,6 +180,52 @@
return _min;
}
+void DbusPassive::updateValue(double value, bool force)
+{
+ _badReading = false;
+
+ // Do not let a NAN, or other floating-point oddity, be used to update
+ // the value, as that indicates the sensor has no valid reading.
+ if (!(std::isfinite(value)))
+ {
+ _badReading = true;
+
+ // Do not continue with a bad reading, unless caller forcing
+ if (!force)
+ {
+ return;
+ }
+ }
+
+ value *= std::pow(10.0, _scale);
+
+ auto unscaled = value;
+ scaleSensorReading(_min, _max, value);
+
+ if (_typeMargin)
+ {
+ _marginHot = false;
+
+ // Unlike an absolute temperature sensor,
+ // where 0 degrees C is a good reading,
+ // a value received of 0 (or negative) margin is worrisome,
+ // and should be flagged.
+ // Either it indicates margin not calculated properly,
+ // or somebody forgot to set the margin-zero setpoint,
+ // or the system is really overheating that much.
+ // This is a different condition from _failed
+ // and _badReading, so it merits its own flag.
+ // The sensor has not failed, the reading is good, but the zone
+ // still needs to know that it should go to failsafe mode.
+ if (unscaled <= 0.0)
+ {
+ _marginHot = true;
+ }
+ }
+
+ setValue(value);
+}
+
int handleSensorValue(sdbusplus::message::message& msg, DbusPassive* owner)
{
std::string msgSensor;
@@ -171,11 +241,7 @@
double value =
std::visit(VariantToDoubleVisitor(), valPropMap->second);
- value *= std::pow(10, owner->getScale());
-
- scaleSensorReading(owner->getMin(), owner->getMax(), value);
-
- owner->setValue(value);
+ owner->updateValue(value, false);
}
}
else if (msgSensor == "xyz.openbmc_project.Sensor.Threshold.Critical")
diff --git a/dbus/dbuspassive.hpp b/dbus/dbuspassive.hpp
index 57ba04b..3b091dd 100644
--- a/dbus/dbuspassive.hpp
+++ b/dbus/dbuspassive.hpp
@@ -56,9 +56,12 @@
ReadReturn read(void) override;
bool getFailed(void) const override;
+ void updateValue(double value, bool force);
void setValue(double value);
+
void setFailed(bool value);
void setFunctional(bool value);
+
int64_t getScale(void);
std::string getID(void);
double getMax(void);
@@ -77,6 +80,10 @@
bool _failed = false;
bool _functional = true;
+ bool _typeMargin = false;
+ bool _badReading = false;
+ bool _marginHot = false;
+
std::string path;
std::shared_ptr<DbusPassiveRedundancy> redundancy;
/* The last time the value was refreshed, not necessarily changed. */
diff --git a/dbus/dbusutil.cpp b/dbus/dbusutil.cpp
index b80dff8..1596628 100644
--- a/dbus/dbusutil.cpp
+++ b/dbus/dbusutil.cpp
@@ -23,6 +23,10 @@
{
layer = "temperature";
}
+ else if (type == "margin")
+ {
+ layer = "temperature";
+ }
else
{
layer = "unknown"; // TODO(venture): Need to handle.
@@ -42,7 +46,7 @@
bool validType(const std::string& type)
{
- static std::set<std::string> valid = {"fan", "temp"};
+ static std::set<std::string> valid = {"fan", "temp", "margin"};
return (valid.find(type) != valid.end());
}