pid/zone: Adding unscaled to cache and logging
The "ReadReturn" structure, and the cache within DbusPidZone, have
been widened, to hold both the scaled and the original unscaled values
at the same time. This allows logging to show both at once, and also
clears up confusion/bugs resulting from storing one or the other and
losing track of which was which.
Compatibility setValue() and getCachedValue() functions still
retained, so this will not break other sensors. These functions still
only take a single argument/return, which will be used for both value
and unscaled, indicating scaling is unknown or irrelevant to this
sensor.
Also, the PWM output of the PID loop appears in the log file,
conveniently right alongside the RPM input of the PID loop.
An output cache has been added to the zone interface, and, unlike the
input cache, use of it is optional. It is only to help populate the
logging, so subclasses are free to ignore it if they want.
Tested: In the logging files, I can see both PWM and RPM, and they are
consistent, showing how the PID loop is trying to update the PWM to
target the desired RPM.
Example: Here's /tmp/zone_0.log on my system
epoch_ms,setpt,fan0_tach,fan0_tach_raw,fan0_tach_pwm,fan0_tach_pwm_raw,bmcmargin_zone0,bmcmargin_zone0_raw,thermal_zone0,thermal_zone0_raw,failsafe
3097918,3818.42,0.748267,11224,0,0,0.724753,56.812,0.745098,62,0
3098022,3818.42,0.748267,11224,0.266666,67,0.724753,56.812,0.745098,62,0
3098132,3818.42,0.748267,11224,0.266666,67,0.724753,56.812,0.745098,62,0
Here's what we can now learn:
The desired setpoint is 3818 RPM.
The fan is at 74.8% of scale, which is 11224 RPM.
The written PWM, after the first PID loop pass, is a raw value of 67,
which is 26.6% of scale.
The first margin temperature is 56.8 degrees of margin, which is 72.4%
of scale.
The second margin temperature is 62 degrees of margin, which is 74.5%
of scale.
This zone is not in failsafe mode.
As you can see, this will be rather useful for PID loop tuning.
Signed-off-by: Josh Lehan <krellan@google.com>
Change-Id: I972a4e4a3b787255f0dcafa10d4498ee58b682f0
diff --git a/dbus/dbuspassive.cpp b/dbus/dbuspassive.cpp
index 41047a1..a567e96 100644
--- a/dbus/dbuspassive.cpp
+++ b/dbus/dbuspassive.cpp
@@ -115,19 +115,26 @@
{
std::lock_guard<std::mutex> guard(_lock);
- ReadReturn r = {_value, _updated};
+ ReadReturn r = {_value, _updated, _unscaled};
return r;
}
-void DbusPassive::setValue(double value)
+void DbusPassive::setValue(double value, double unscaled)
{
std::lock_guard<std::mutex> guard(_lock);
_value = value;
+ _unscaled = unscaled;
_updated = std::chrono::high_resolution_clock::now();
}
+void DbusPassive::setValue(double value)
+{
+ // First param is scaled, second param is unscaled, assume same here
+ setValue(value, value);
+}
+
bool DbusPassive::getFailed(void) const
{
if (redundancy)
@@ -253,7 +260,7 @@
}
}
- setValue(value);
+ setValue(value, unscaled);
}
int handleSensorValue(sdbusplus::message_t& msg, DbusPassive* owner)
diff --git a/dbus/dbuspassive.hpp b/dbus/dbuspassive.hpp
index 72ad9a5..3813622 100644
--- a/dbus/dbuspassive.hpp
+++ b/dbus/dbuspassive.hpp
@@ -57,6 +57,7 @@
bool getFailed(void) const override;
void updateValue(double value, bool force);
+ void setValue(double value, double unscaled);
void setValue(double value);
void setFailed(bool value);
@@ -76,6 +77,7 @@
std::mutex _lock;
double _value = 0;
+ double _unscaled = 0;
double _max = 0;
double _min = 0;
bool _failed = false;
diff --git a/interfaces.hpp b/interfaces.hpp
index 8ec474b..73ef643 100644
--- a/interfaces.hpp
+++ b/interfaces.hpp
@@ -9,13 +9,24 @@
{
double value;
std::chrono::high_resolution_clock::time_point updated;
+ double unscaled = value;
bool operator==(const ReadReturn& rhs) const
{
- return (this->value == rhs.value && this->updated == rhs.updated);
+ return ((this->value == rhs.value) && (this->updated == rhs.updated) &&
+ (this->unscaled == rhs.unscaled));
}
};
+struct ValueCacheEntry
+{
+ // This is normalized to (0.0, 1.0) range, using configured min and max
+ double scaled;
+
+ // This is the raw value, as recieved from the input/output sensors
+ double unscaled;
+};
+
/*
* A ReadInterface is a plug-in for the PluggableSensor and anyone implementing
* this basically is providing a way to read a sensor.
diff --git a/pid/fancontroller.cpp b/pid/fancontroller.cpp
index 33e2d9f..57902ab 100644
--- a/pid/fancontroller.cpp
+++ b/pid/fancontroller.cpp
@@ -159,6 +159,12 @@
auto redundantWrite = _owner->getRedundantWrite();
int64_t rawWritten;
sensor->write(percent, redundantWrite, &rawWritten);
+
+ // The outputCache will be used later,
+ // to store a record of the PWM commanded,
+ // so that this information can be included during logging.
+ auto unscaledWritten = static_cast<double>(rawWritten);
+ _owner->setOutputCache(sensor->getName(), {percent, unscaledWritten});
}
return;
diff --git a/pid/zone.cpp b/pid/zone.cpp
index 1eda992..585cbeb 100644
--- a/pid/zone.cpp
+++ b/pid/zone.cpp
@@ -153,9 +153,20 @@
double DbusPidZone::getCachedValue(const std::string& name)
{
+ return _cachedValuesByName.at(name).scaled;
+}
+
+ValueCacheEntry DbusPidZone::getCachedValues(const std::string& name)
+{
return _cachedValuesByName.at(name);
}
+void DbusPidZone::setOutputCache(std::string_view name,
+ const ValueCacheEntry& values)
+{
+ _cachedFanOutputs[std::string{name}] = values;
+}
+
void DbusPidZone::addFanInput(const std::string& fan)
{
_fanInputs.push_back(fan);
@@ -284,29 +295,28 @@
void DbusPidZone::initializeLog(void)
{
/* Print header for log file:
- * epoch_ms,setpt,fan1,fan2,fanN,sensor1,sensor2,sensorN,failsafe
+ * epoch_ms,setpt,fan1,fan1_raw,fan1_pwm,fan1_pwm_raw,fan2,fan2_raw,fan2_pwm,fan2_pwm_raw,fanN,fanN_raw,fanN_pwm,fanN_pwm_raw,sensor1,sensor1_raw,sensor2,sensor2_raw,sensorN,sensorN_raw,failsafe
*/
_log << "epoch_ms,setpt,requester";
for (const auto& f : _fanInputs)
{
- _log << "," << f;
+ _log << "," << f << "," << f << "_raw";
+ _log << "," << f << "_pwm," << f << "_pwm_raw";
}
for (const auto& t : _thermalInputs)
{
- _log << "," << t;
+ _log << "," << t << "," << t << "_raw";
}
+
_log << ",failsafe";
_log << std::endl;
-
- return;
}
void DbusPidZone::writeLog(const std::string& value)
{
_log << value;
- return;
}
/*
@@ -342,7 +352,7 @@
{
auto sensor = _mgr.getSensor(f);
ReadReturn r = sensor->read();
- _cachedValuesByName[f] = r.value;
+ _cachedValuesByName[f] = {r.value, r.unscaled};
int64_t timeout = sensor->getTimeout();
tstamp then = r.updated;
@@ -357,7 +367,10 @@
*/
if (loggingEnabled)
{
- _log << "," << r.value;
+ const auto& v = _cachedValuesByName[f];
+ _log << "," << v.scaled << "," << v.unscaled;
+ const auto& p = _cachedFanOutputs[f];
+ _log << "," << p.scaled << "," << p.unscaled;
}
// check if fan fail.
@@ -384,7 +397,8 @@
{
for (const auto& t : _thermalInputs)
{
- _log << "," << _cachedValuesByName[t];
+ const auto& v = _cachedValuesByName[t];
+ _log << "," << v.scaled << "," << v.unscaled;
}
}
@@ -403,7 +417,7 @@
ReadReturn r = sensor->read();
int64_t timeout = sensor->getTimeout();
- _cachedValuesByName[t] = r.value;
+ _cachedValuesByName[t] = {r.value, r.unscaled};
tstamp then = r.updated;
auto duration = duration_cast<std::chrono::seconds>(now - then).count();
@@ -434,9 +448,12 @@
void DbusPidZone::initializeCache(void)
{
+ auto nan = std::numeric_limits<double>::quiet_NaN();
+
for (const auto& f : _fanInputs)
{
- _cachedValuesByName[f] = 0;
+ _cachedValuesByName[f] = {nan, nan};
+ _cachedFanOutputs[f] = {nan, nan};
// Start all fans in fail-safe mode.
_failSafeSensors.insert(f);
@@ -444,7 +461,7 @@
for (const auto& t : _thermalInputs)
{
- _cachedValuesByName[t] = 0;
+ _cachedValuesByName[t] = {nan, nan};
// Start all sensors in fail-safe mode.
_failSafeSensors.insert(t);
@@ -456,7 +473,15 @@
std::cerr << "Cache values now: \n";
for (const auto& [name, value] : _cachedValuesByName)
{
- std::cerr << name << ": " << value << "\n";
+ std::cerr << name << ": " << value.scaled << " " << value.unscaled
+ << "\n";
+ }
+
+ std::cerr << "Fan outputs now: \n";
+ for (const auto& [name, value] : _cachedFanOutputs)
+ {
+ std::cerr << name << ": " << value.scaled << " " << value.unscaled
+ << "\n";
}
}
diff --git a/pid/zone.hpp b/pid/zone.hpp
index 12e47a6..179b24d 100644
--- a/pid/zone.hpp
+++ b/pid/zone.hpp
@@ -74,6 +74,7 @@
void updateFanTelemetry(void) override;
void updateSensors(void) override;
void initializeCache(void) override;
+ void setOutputCache(std::string_view, const ValueCacheEntry&) override;
void dumpCache(void);
void processFans(void) override;
@@ -82,6 +83,8 @@
void addFanPID(std::unique_ptr<Controller> pid);
void addThermalPID(std::unique_ptr<Controller> pid);
double getCachedValue(const std::string& name) override;
+ ValueCacheEntry getCachedValues(const std::string& name) override;
+
void addFanInput(const std::string& fan);
void addThermalInput(const std::string& therm);
@@ -111,7 +114,8 @@
std::vector<double> _RPMCeilings;
std::vector<std::string> _fanInputs;
std::vector<std::string> _thermalInputs;
- std::map<std::string, double> _cachedValuesByName;
+ std::map<std::string, ValueCacheEntry> _cachedValuesByName;
+ std::map<std::string, ValueCacheEntry> _cachedFanOutputs;
const SensorManager& _mgr;
std::vector<std::unique_ptr<Controller>> _fans;
diff --git a/pid/zone_interface.hpp b/pid/zone_interface.hpp
index 982e051..7d59497 100644
--- a/pid/zone_interface.hpp
+++ b/pid/zone_interface.hpp
@@ -42,8 +42,23 @@
*/
virtual void initializeCache(void) = 0;
+ /** Optionally adds fan outputs to an output cache, which is different
+ * from the input cache accessed by getCachedValue(), so it is possible
+ * to have entries with the same name in both the output cache and
+ * the input cache. The output cache is used for logging, to show
+ * the PWM values determined by the PID loop, next to the resulting RPM.
+ */
+ virtual void setOutputCache(std::string_view name,
+ const ValueCacheEntry& values) = 0;
+
/** Return cached value for sensor by name. */
virtual double getCachedValue(const std::string& name) = 0;
+ /** Return cached values, both scaled and original unscaled values,
+ * for sensor by name. Subclasses can add trivial return {value, value},
+ * for subclasses that only implement getCachedValue() and do not care
+ * about maintaining the distinction between scaled and unscaled values.
+ */
+ virtual ValueCacheEntry getCachedValues(const std::string& name) = 0;
/** Add a set point value for the Max Set Point computation. */
virtual void addSetPoint(double setpoint, const std::string& name) = 0;
diff --git a/test/zone_mock.hpp b/test/zone_mock.hpp
index dd98c15..f2620d5 100644
--- a/test/zone_mock.hpp
+++ b/test/zone_mock.hpp
@@ -18,8 +18,11 @@
MOCK_METHOD0(updateSensors, void());
MOCK_METHOD0(initializeCache, void());
MOCK_METHOD1(getCachedValue, double(const std::string&));
+ MOCK_METHOD1(getCachedValues, ValueCacheEntry(const std::string&));
MOCK_CONST_METHOD0(getRedundantWrite, bool(void));
MOCK_METHOD2(addSetPoint, void(double, const std::string&));
+ MOCK_METHOD2(setOutputCache,
+ void(std::string_view name, const ValueCacheEntry& values));
MOCK_METHOD0(clearSetPoints, void());
MOCK_METHOD1(addRPMCeiling, void(double));
MOCK_METHOD0(clearRPMCeilings, void());