phosphor-pid-control: MissingIsAcceptable patch This is a partial implementation of the ideas here: https://github.com/openbmc/phosphor-pid-control/issues/31 Renumbering and reordering the recipe file, to help keep the patches in sequence: TempToMargin first, MissingIsAcceptable next. A new configuration item is supported in the PID object, named "MissingIsAcceptable" (for D-Bus) or "missingIsAcceptable" (for the old config.json). The value is an array of strings. If these strings match sensor names, those sensors will be flagged as "missing is acceptable", that is, they can go missing and the zone will not be thrown into failsafe mode as a result. This can be handy for sensors that are not always available on your particular machine. It is independent of the existing Availability interface, because the decision to go into failsafe mode or not is a property of the PID loop, not of the sensor itself. Tested: It worked for me. Also, added a unit test case. Patch Tracking Bug: b/269219517 Upstream info / review: https://gerrit.openbmc.org/c/openbmc/phosphor-pid-control/+/60888 Upstream-Status: Submitted Justification: Awaiting upstream review Google-Bug-Id: 266217080 Google-Bug-Id: 269219517 Signed-off-by: Josh Lehan <krellan@google.com> Change-Id: I26bc83001a5398ee9e87427b52fe4f15b7f81de7 (cherry picked from commit bc8ac9a7d523b2cfb17229e16f4add75b2f29595) Signed-off-by: Josh Lehan <krellan@google.com>
diff --git a/recipes-phosphor/fans/phosphor-pid-control/0002-Add-MissingIsAcceptable-feature-to-avoid-failsafe.patch b/recipes-phosphor/fans/phosphor-pid-control/0002-Add-MissingIsAcceptable-feature-to-avoid-failsafe.patch new file mode 100644 index 0000000..729de06 --- /dev/null +++ b/recipes-phosphor/fans/phosphor-pid-control/0002-Add-MissingIsAcceptable-feature-to-avoid-failsafe.patch
@@ -0,0 +1,512 @@ +From 14880286c4dc0784aaa9b3832be1efbef1ccfd72 Mon Sep 17 00:00:00 2001 +From: Josh Lehan <krellan@google.com> +Date: Mon, 13 Feb 2023 01:45:29 -0800 +Subject: [PATCH] Add MissingIsAcceptable feature to avoid failsafe + +Patch Tracking Bug: b/269219517 +Upstream info / review: https://gerrit.openbmc.org/c/openbmc/phosphor-pid-control/+/60888 +Upstream-Status: Submitted +Justification: Awaiting upstream review + +This is a partial implementation of the ideas here: +https://github.com/openbmc/phosphor-pid-control/issues/31 + +A new configuration item is supported in the PID object, named +"MissingIsAcceptable" (for D-Bus) or "missingIsAcceptable" (for the old +config.json). The value is an array of strings. If these strings match +sensor names, those sensors will be flagged as "missing is acceptable", +that is, they can go missing and the zone will not be thrown into +failsafe mode as a result. + +This can be handy for sensors that are not always available on your +particular machine. It is independent of the existing Availability +interface, because the decision to go into failsafe mode or not is a +property of the PID loop, not of the sensor itself. + +Tested: It worked for me. Also, added a unit test case. + +Signed-off-by: Josh Lehan <krellan@google.com> +--- + conf.hpp | 6 ++- + dbus/dbusconfiguration.cpp | 24 +++++++++- + pid/builder.cpp | 10 +++-- + pid/buildjson.cpp | 9 +++- + pid/zone.cpp | 30 +++++++++++-- + pid/zone.hpp | 13 ++++-- + test/pid_zone_unittest.cpp | 91 ++++++++++++++++++++++++++++++++++---- + util.cpp | 24 ++++++++-- + util.hpp | 3 +- + 9 files changed, 182 insertions(+), 28 deletions(-) + +diff --git a/conf.hpp b/conf.hpp +index 486e2e5..5ea7fc9 100644 +--- a/conf.hpp ++++ b/conf.hpp +@@ -34,13 +34,17 @@ struct SensorConfig + + /* + * Structure for decorating an input sensor's name with additional +- * information, to help out with TempToMargin conversion. ++ * information, such as TempToMargin and MissingIsAcceptable. ++ * This information comes from the PID loop configuration, ++ * not from SensorConfig, in order for it to be able to work ++ * with dynamic sensors from entity-manager. + */ + struct SensorInput + { + std::string name; + double convertMarginZero = std::numeric_limits<double>::quiet_NaN(); + bool convertTempToMargin = false; ++ bool missingIsAcceptable = false; + }; + + /* +diff --git a/dbus/dbusconfiguration.cpp b/dbus/dbusconfiguration.cpp +index c20f1e0..1155578 100644 +--- a/dbus/dbusconfiguration.cpp ++++ b/dbus/dbusconfiguration.cpp +@@ -845,8 +845,18 @@ bool init(sdbusplus::bus_t& bus, boost::asio::steady_timer& timer, + std::get<std::vector<double>>(findTempToMargin->second); + } + ++ std::vector<std::string> missingAcceptableNames; ++ ++ auto findMissingAcceptable = base.find("MissingIsAcceptable"); ++ if (findMissingAcceptable != base.end()) ++ { ++ missingAcceptableNames = std::get<std::vector<std::string>>( ++ findMissingAcceptable->second); ++ } ++ + std::vector<pid_control::conf::SensorInput> sensorInputs = +- spliceInputs(inputSensorNames, inputTempToMargin); ++ spliceInputs(inputSensorNames, inputTempToMargin, ++ missingAcceptableNames); + + if (offsetType.empty()) + { +@@ -943,7 +953,17 @@ bool init(sdbusplus::bus_t& bus, boost::asio::steady_timer& timer, + std::get<std::vector<double>>(findTempToMargin->second); + } + +- info.inputs = spliceInputs(inputs, inputTempToMargin); ++ std::vector<std::string> missingAcceptableNames; ++ ++ auto findMissingAcceptable = base.find("MissingIsAcceptable"); ++ if (findMissingAcceptable != base.end()) ++ { ++ missingAcceptableNames = std::get<std::vector<std::string>>( ++ findMissingAcceptable->second); ++ } ++ ++ info.inputs = spliceInputs(inputs, inputTempToMargin, ++ missingAcceptableNames); + + info.type = "stepwise"; + info.stepwiseInfo.ts = 1.0; // currently unused +diff --git a/pid/builder.cpp b/pid/builder.cpp +index 043a393..86273bf 100644 +--- a/pid/builder.cpp ++++ b/pid/builder.cpp +@@ -90,7 +90,7 @@ std::unordered_map<int64_t, std::shared_ptr<ZoneInterface>> + for (const auto& i : info.inputs) + { + inputs.push_back(i); +- zone->addFanInput(i.name); ++ zone->addFanInput(i.name, i.missingIsAcceptable); + } + + auto pid = FanController::createFanPid( +@@ -102,7 +102,7 @@ std::unordered_map<int64_t, std::shared_ptr<ZoneInterface>> + for (const auto& i : info.inputs) + { + inputs.push_back(i); +- zone->addThermalInput(i.name); ++ zone->addThermalInput(i.name, i.missingIsAcceptable); + } + + auto pid = ThermalController::createThermalPid( +@@ -116,7 +116,7 @@ std::unordered_map<int64_t, std::shared_ptr<ZoneInterface>> + for (const auto& i : info.inputs) + { + inputs.push_back(i); +- zone->addThermalInput(i.name); ++ zone->addThermalInput(i.name, i.missingIsAcceptable); + } + auto stepwise = StepwiseController::createStepwiseController( + zone.get(), name, splitNames(inputs), info.stepwiseInfo); +@@ -131,6 +131,10 @@ std::unordered_map<int64_t, std::shared_ptr<ZoneInterface>> + { + std::cerr << "[" << i.convertMarginZero << "]"; + } ++ if (i.missingIsAcceptable) ++ { ++ std::cerr << "?"; ++ } + std::cerr << ", "; + } + std::cerr << "\n"; +diff --git a/pid/buildjson.cpp b/pid/buildjson.cpp +index 62e3058..420822a 100644 +--- a/pid/buildjson.cpp ++++ b/pid/buildjson.cpp +@@ -37,6 +37,7 @@ namespace conf + void from_json(const json& j, conf::ControllerInfo& c) + { + std::vector<std::string> inputNames; ++ std::vector<std::string> missingAcceptableNames; + + j.at("type").get_to(c.type); + j.at("inputs").get_to(inputNames); +@@ -49,8 +50,14 @@ void from_json(const json& j, conf::ControllerInfo& c) + { + findTempToMargin->get_to(inputTempToMargin); + } ++ ++ auto findMissingAcceptable = j.find("missingIsAcceptable"); ++ if (findMissingAcceptable != j.end()) ++ { ++ findMissingAcceptable->get_to(missingAcceptableNames); ++ } + +- c.inputs = spliceInputs(inputNames, inputTempToMargin); ++ c.inputs = spliceInputs(inputNames, inputTempToMargin, missingAcceptableNames); + + /* TODO: We need to handle parsing other PID controller configurations. + * We can do that by checking for different keys and making the decision +diff --git a/pid/zone.cpp b/pid/zone.cpp +index c88f41b..98121da 100644 +--- a/pid/zone.cpp ++++ b/pid/zone.cpp +@@ -96,6 +96,17 @@ bool DbusPidZone::getFailSafeMode(void) const + return !_failSafeSensors.empty(); + } + ++void DbusPidZone::markSensorMissing(const std::string& name) ++{ ++ if (_missingAcceptable.find(name) != _missingAcceptable.end()) ++ { ++ // Disallow sensors in MissingIsAcceptable list from causing failsafe ++ return; ++ } ++ ++ _failSafeSensors.emplace(name); ++} ++ + int64_t DbusPidZone::getZoneID(void) const + { + return _zoneId; +@@ -129,6 +140,7 @@ void DbusPidZone::clearSetPoints(void) + { + _SetPoints.clear(); + _maximumSetPoint = 0; ++ _maximumSetPointName.clear(); + } + + double DbusPidZone::getFailSafePercent(void) const +@@ -177,14 +189,24 @@ void DbusPidZone::setOutputCache(std::string_view name, + _cachedFanOutputs[std::string{name}] = values; + } + +-void DbusPidZone::addFanInput(const std::string& fan) ++void DbusPidZone::addFanInput(const std::string& fan, bool missingAcceptable) + { + _fanInputs.push_back(fan); ++ ++ if (missingAcceptable) ++ { ++ _missingAcceptable.emplace(fan); ++ } + } + +-void DbusPidZone::addThermalInput(const std::string& therm) ++void DbusPidZone::addThermalInput(const std::string& therm, bool missingAcceptable) + { + _thermalInputs.push_back(therm); ++ ++ if (missingAcceptable) ++ { ++ _missingAcceptable.emplace(therm); ++ } + } + + // Updates desired RPM setpoint from optional text file +@@ -388,7 +410,7 @@ void DbusPidZone::initializeCache(void) + _cachedFanOutputs[f] = {0, 0}; + + // Start all fans in fail-safe mode. +- _failSafeSensors.insert(f); ++ markSensorMissing(f); + } + + for (const auto& t : _thermalInputs) +@@ -396,7 +418,7 @@ void DbusPidZone::initializeCache(void) + _cachedValuesByName[t] = {0, 0}; + + // Start all sensors in fail-safe mode. +- _failSafeSensors.insert(t); ++ markSensorMissing(t); + } + } + +diff --git a/pid/zone.hpp b/pid/zone.hpp +index b985f5f..36ff9bf 100644 +--- a/pid/zone.hpp ++++ b/pid/zone.hpp +@@ -60,6 +60,7 @@ class DbusPidZone : public ZoneInterface, public ModeObject + bool getRedundantWrite(void) const override; + void setManualMode(bool mode); + bool getFailSafeMode(void) const override; ++ void markSensorMissing(const std::string& name); + + int64_t getZoneID(void) const override; + void addSetPoint(double setPoint, const std::string& name) override; +@@ -88,8 +89,8 @@ class DbusPidZone : public ZoneInterface, public ModeObject + 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); ++ void addFanInput(const std::string& fan, bool missingAcceptable); ++ void addThermalInput(const std::string& therm, bool missingAcceptable); + + void initializeLog(void) override; + void writeLog(const std::string& value) override; +@@ -141,7 +142,8 @@ class DbusPidZone : public ZoneInterface, public ModeObject + // check if fan fail. + if (sensor->getFailed()) + { +- _failSafeSensors.insert(sensorInput); ++ markSensorMissing(sensorInput); ++ + if (debugEnabled) + { + std::cerr << sensorInput << " sensor get failed\n"; +@@ -149,7 +151,8 @@ class DbusPidZone : public ZoneInterface, public ModeObject + } + else if (timeout != 0 && duration >= period) + { +- _failSafeSensors.insert(sensorInput); ++ markSensorMissing(sensorInput); ++ + if (debugEnabled) + { + std::cerr << sensorInput << " sensor timeout\n"; +@@ -166,6 +169,7 @@ class DbusPidZone : public ZoneInterface, public ModeObject + std::cerr << sensorInput + << " is erased from failsafe sensor set\n"; + } ++ + _failSafeSensors.erase(kt); + } + } +@@ -185,6 +189,7 @@ class DbusPidZone : public ZoneInterface, public ModeObject + const conf::CycleTime _cycleTime; + + std::set<std::string> _failSafeSensors; ++ std::set<std::string> _missingAcceptable; + + std::vector<double> _SetPoints; + std::vector<double> _RPMCeilings; +diff --git a/test/pid_zone_unittest.cpp b/test/pid_zone_unittest.cpp +index 3a5871d..a4628bb 100644 +--- a/test/pid_zone_unittest.cpp ++++ b/test/pid_zone_unittest.cpp +@@ -286,8 +286,8 @@ TEST_F(PidZoneTest, FanInputTest_VerifiesFanValuesCached) + EXPECT_EQ(mgr.getSensor(name2), sensor_ptr2); + + // Now that the sensors exist, add them to the zone. +- zone->addFanInput(name1); +- zone->addFanInput(name2); ++ zone->addFanInput(name1, false); ++ zone->addFanInput(name2, false); + + // Initialize Zone + zone->initializeCache(); +@@ -333,8 +333,8 @@ TEST_F(PidZoneTest, ThermalInput_ValueTimeoutEntersFailSafeMode) + mgr.addSensor(type, name2, std::move(sensor2)); + EXPECT_EQ(mgr.getSensor(name2), sensor_ptr2); + +- zone->addThermalInput(name1); +- zone->addThermalInput(name2); ++ zone->addThermalInput(name1, false); ++ zone->addThermalInput(name2, false); + + // Initialize Zone + zone->initializeCache(); +@@ -370,6 +370,79 @@ TEST_F(PidZoneTest, ThermalInput_ValueTimeoutEntersFailSafeMode) + EXPECT_TRUE(zone->getFailSafeMode()); + } + ++TEST_F(PidZoneTest, ThermalInput_MissingIsAcceptableNoFailSafe) ++{ ++ // This is similar to the above test, but because missingIsAcceptable ++ // is set, the zone should not enter failsafe mode when it goes missing. ++ ++ int64_t timeout = 1; ++ ++ std::string name1 = "temp1"; ++ std::unique_ptr<Sensor> sensor1 = ++ std::make_unique<SensorMock>(name1, timeout); ++ SensorMock* sensor_ptr1 = reinterpret_cast<SensorMock*>(sensor1.get()); ++ ++ std::string name2 = "temp2"; ++ std::unique_ptr<Sensor> sensor2 = ++ std::make_unique<SensorMock>(name2, timeout); ++ SensorMock* sensor_ptr2 = reinterpret_cast<SensorMock*>(sensor2.get()); ++ ++ std::string type = "unchecked"; ++ mgr.addSensor(type, name1, std::move(sensor1)); ++ EXPECT_EQ(mgr.getSensor(name1), sensor_ptr1); ++ mgr.addSensor(type, name2, std::move(sensor2)); ++ EXPECT_EQ(mgr.getSensor(name2), sensor_ptr2); ++ ++ zone->addThermalInput(name1, true); ++ zone->addThermalInput(name2, false); ++ ++ // Initialize Zone ++ zone->initializeCache(); ++ ++ // Verify now in failsafe mode. ++ EXPECT_TRUE(zone->getFailSafeMode()); ++ ++ ReadReturn r1; ++ r1.value = 10.0; ++ r1.updated = std::chrono::high_resolution_clock::now(); ++ EXPECT_CALL(*sensor_ptr1, read()).WillOnce(Return(r1)); ++ ++ ReadReturn r2; ++ r2.value = 11.0; ++ r2.updated = std::chrono::high_resolution_clock::now(); ++ EXPECT_CALL(*sensor_ptr2, read()).WillOnce(Return(r2)); ++ ++ zone->updateSensors(); ++ EXPECT_FALSE(zone->getFailSafeMode()); ++ ++ // Ok, so we're not in failsafe mode, so let's set updated to the past. ++ // sensor1 will have an updated field older than its timeout value, but ++ // sensor2 will be fine. :D ++ r1.updated -= std::chrono::seconds(3); ++ r2.updated = std::chrono::high_resolution_clock::now(); ++ ++ EXPECT_CALL(*sensor_ptr1, read()).WillOnce(Return(r1)); ++ EXPECT_CALL(*sensor_ptr2, read()).WillOnce(Return(r2)); ++ ++ // MissingIsAcceptable is true for sensor1, so the zone should not be ++ // thrown into failsafe mode. ++ zone->updateSensors(); ++ EXPECT_FALSE(zone->getFailSafeMode()); ++ ++ // Do the same thing, but for the opposite sensors: r1 is good, ++ // but r2 is set to some time in the past. ++ r1.updated = std::chrono::high_resolution_clock::now(); ++ r2.updated -= std::chrono::seconds(3); ++ ++ EXPECT_CALL(*sensor_ptr1, read()).WillOnce(Return(r1)); ++ EXPECT_CALL(*sensor_ptr2, read()).WillOnce(Return(r2)); ++ ++ // Now, the zone should be in failsafe mode, because sensor2 does not ++ // have MissingIsAcceptable set true, it is still subject to failsafe. ++ zone->updateSensors(); ++ EXPECT_TRUE(zone->getFailSafeMode()); ++} ++ + TEST_F(PidZoneTest, FanInputTest_FailsafeToValid_ReadsSensors) + { + // This will add a couple fan inputs, and verify the values are cached. +@@ -393,8 +466,8 @@ TEST_F(PidZoneTest, FanInputTest_FailsafeToValid_ReadsSensors) + EXPECT_EQ(mgr.getSensor(name2), sensor_ptr2); + + // Now that the sensors exist, add them to the zone. +- zone->addFanInput(name1); +- zone->addFanInput(name2); ++ zone->addFanInput(name1, false); ++ zone->addFanInput(name2, false); + + // Initialize Zone + zone->initializeCache(); +@@ -446,8 +519,8 @@ TEST_F(PidZoneTest, FanInputTest_ValueTimeoutEntersFailSafeMode) + EXPECT_EQ(mgr.getSensor(name2), sensor_ptr2); + + // Now that the sensors exist, add them to the zone. +- zone->addFanInput(name1); +- zone->addFanInput(name2); ++ zone->addFanInput(name1, false); ++ zone->addFanInput(name2, false); + + // Initialize Zone + zone->initializeCache(); +@@ -497,7 +570,7 @@ TEST_F(PidZoneTest, GetSensorTest_ReturnsExpected) + mgr.addSensor(type, name1, std::move(sensor1)); + EXPECT_EQ(mgr.getSensor(name1), sensor_ptr1); + +- zone->addThermalInput(name1); ++ zone->addThermalInput(name1, false); + + // Verify method under test returns the pointer we expect. + EXPECT_EQ(mgr.getSensor(name1), zone->getSensor(name1)); +diff --git a/util.cpp b/util.cpp +index cc23f9f..529cd76 100644 +--- a/util.cpp ++++ b/util.cpp +@@ -104,15 +104,16 @@ void debugPrint(const std::map<std::string, conf::SensorConfig>& sensorConfig, + + std::vector<conf::SensorInput> + spliceInputs(const std::vector<std::string>& inputNames, +- const std::vector<double>& inputTempToMargin) ++ const std::vector<double>& inputTempToMargin, ++ const std::vector<std::string>& missingAcceptableNames) + { + std::vector<conf::SensorInput> results; + +- // Default to the TempToMargin feature disabled ++ // Default to TempToMargin and MissingIsAcceptable disabled + for (const auto& inputName : inputNames) + { + conf::SensorInput newInput{ +- inputName, std::numeric_limits<double>::quiet_NaN(), false}; ++ inputName, std::numeric_limits<double>::quiet_NaN(), false, false}; + + results.emplace_back(newInput); + } +@@ -133,6 +134,23 @@ std::vector<conf::SensorInput> + results[index].convertTempToMargin = true; + } + ++ std::set<std::string> acceptableSet; ++ ++ // Copy vector to set, to avoid O(n^2) runtime below ++ for (const auto& name : missingAcceptableNames) ++ { ++ acceptableSet.emplace(name); ++ } ++ ++ // Flag missingIsAcceptable true if name found in that set ++ for (auto& result : results) ++ { ++ if (acceptableSet.find(result.name) != acceptableSet.end()) ++ { ++ result.missingIsAcceptable = true; ++ } ++ } ++ + return results; + } + +diff --git a/util.hpp b/util.hpp +index 0588934..7617562 100644 +--- a/util.hpp ++++ b/util.hpp +@@ -46,7 +46,8 @@ std::string FixupPath(std::string original); + */ + std::vector<conf::SensorInput> + spliceInputs(const std::vector<std::string>& inputNames, +- const std::vector<double>& inputTempToMargin); ++ const std::vector<double>& inputTempToMargin, ++ const std::vector<std::string>& missingAcceptableNames); + + /* + * Recovers the original "Inputs" vector from spliceInputs(). +-- +2.39.2.637.g21b0678d19-goog +
diff --git a/recipes-phosphor/fans/phosphor-pid-control_%.bbappend b/recipes-phosphor/fans/phosphor-pid-control_%.bbappend index 0df0387..6ed8f0f 100644 --- a/recipes-phosphor/fans/phosphor-pid-control_%.bbappend +++ b/recipes-phosphor/fans/phosphor-pid-control_%.bbappend
@@ -1,7 +1,8 @@ FILESEXTRAPATHS:prepend:gbmc := "${THISDIR}/${PN}:" SRC_URI:append:gbmc = " \ - file://0001-Implementing-the-TempToMargin-feature.patch \ - file://0001-Skipping-over-Association-Definitions-messages.patch \ - file://0001-pid-reuse-the-code-for-processing-sensors-input.patch \ + file://0001-Implementing-the-TempToMargin-feature.patch \ + file://0001-Skipping-over-Association-Definitions-messages.patch \ + file://0001-pid-reuse-the-code-for-processing-sensors-input.patch \ + file://0002-Add-MissingIsAcceptable-feature-to-avoid-failsafe.patch \ "