FanController: Use raw RPM as input to fan PID loop
The fan PID loop was wrongly using normalized RPM as input, instead of
raw RPM. This meant that the input RPM was between 0.0 and 1.0, the
wrong units, an unusable low value for RPM.
What's more, the inputProc() function used int64_t instead of double,
for an unknown reason, as the input and output of this function is
double. This integer truncation caused the normalized RPM to always be
zero, making this bug harder to notice.
Cleaned up the inputProc() function to always use double, and
correctly use the raw RPM.
I am really glad I had earlier added a feature to maintain the raw
unscaled value, along with the normalized scaled value, in the cache!
https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-pid-control/+/36697
This made it easy to recover the raw value. Otherwise, this bug would
have been much harder to fix.
Tested: The RPM input values now use same units as the setpoint,
restoring proper fan PID loop operation, as logged in the new PID core
debugging feature here:
https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-pid-control/+/38087
Signed-off-by: Josh Lehan <krellan@google.com>
Change-Id: I4607d9ebee57cea04b8f83d658913e24200a6428
diff --git a/pid/fancontroller.cpp b/pid/fancontroller.cpp
index 7d4a92d..44852a0 100644
--- a/pid/fancontroller.cpp
+++ b/pid/fancontroller.cpp
@@ -46,15 +46,17 @@
double FanController::inputProc(void)
{
- double value = 0;
- std::vector<int64_t> values;
- std::vector<int64_t>::iterator result;
+ double value = 0.0;
+ std::vector<double> values;
+ std::vector<double>::iterator result;
try
{
for (const auto& name : _inputs)
{
- value = _owner->getCachedValue(name);
+ // Read the unscaled value, to correctly recover the RPM
+ value = _owner->getCachedValues(name).unscaled;
+
/* If we have a fan we can't read, its value will be 0 for at least
* some boards, while others... the fan will drop off dbus (if
* that's how it's being read and in that case its value will never
@@ -67,7 +69,7 @@
{
continue;
}
- if (value <= 0)
+ if (value <= 0.0)
{
continue;
}
@@ -82,7 +84,7 @@
}
/* Reset the value from the above loop. */
- value = 0;
+ value = 0.0;
if (values.size() > 0)
{
/* the fan PID algorithm was unstable with average, and seemed to work
@@ -166,7 +168,7 @@
}
// value and kFanFailSafeDutyCycle are 10 for 10% so let's fix that.
- percent /= 100;
+ percent /= 100.0;
// PidSensorMap for writing.
for (const auto& it : _inputs)
diff --git a/pid/zone.cpp b/pid/zone.cpp
index 69464e1..e9aaafc 100644
--- a/pid/zone.cpp
+++ b/pid/zone.cpp
@@ -493,12 +493,10 @@
void DbusPidZone::initializeCache(void)
{
- auto nan = std::numeric_limits<double>::quiet_NaN();
-
for (const auto& f : _fanInputs)
{
- _cachedValuesByName[f] = {nan, nan};
- _cachedFanOutputs[f] = {nan, nan};
+ _cachedValuesByName[f] = {0, 0};
+ _cachedFanOutputs[f] = {0, 0};
// Start all fans in fail-safe mode.
_failSafeSensors.insert(f);
@@ -506,7 +504,7 @@
for (const auto& t : _thermalInputs)
{
- _cachedValuesByName[t] = {nan, nan};
+ _cachedValuesByName[t] = {0, 0};
// Start all sensors in fail-safe mode.
_failSafeSensors.insert(t);
diff --git a/test/zone_mock.hpp b/test/zone_mock.hpp
index b63408f..ba167f4 100644
--- a/test/zone_mock.hpp
+++ b/test/zone_mock.hpp
@@ -18,7 +18,14 @@
MOCK_METHOD0(updateSensors, void());
MOCK_METHOD0(initializeCache, void());
MOCK_METHOD1(getCachedValue, double(const std::string&));
- MOCK_METHOD1(getCachedValues, ValueCacheEntry(const std::string&));
+
+ // Compatibility interface for getCachedValues
+ ValueCacheEntry getCachedValues(const std::string& s)
+ {
+ auto v = getCachedValue(s);
+ return ValueCacheEntry(v, v);
+ }
+
MOCK_CONST_METHOD0(getRedundantWrite, bool(void));
MOCK_METHOD2(addSetPoint, void(double, const std::string&));
MOCK_METHOD2(setOutputCache,