diff --git a/dbus/dbusconfiguration.cpp b/dbus/dbusconfiguration.cpp index 7bb96f1b..4f86ff66 100644 --- a/dbus/dbusconfiguration.cpp +++ b/dbus/dbusconfiguration.cpp @@ -634,7 +634,8 @@ bool init(sdbusplus::bus_t& bus, boost::asio::steady_timer& timer, { const auto& base = configuration.second.at(pidConfigurationInterface); - const std::string pidName = std::get(base.at("Name")); + const std::string pidName = + sensorNameToDbusName(std::get(base.at("Name"))); const std::string pidClass = std::get(base.at("Class")); const std::vector& zones = @@ -833,8 +834,7 @@ bool init(sdbusplus::bus_t& bus, boost::asio::steady_timer& timer, if (offsetType.empty()) { - conf::ControllerInfo& info = - conf[std::get(base.at("Name"))]; + conf::ControllerInfo& info = conf[pidName]; info.inputs = std::move(inputSensorNames); populatePidInfo(bus, base, info, nullptr, sensorConfig); } @@ -857,6 +857,8 @@ bool init(sdbusplus::bus_t& bus, boost::asio::steady_timer& timer, if (findStepwise != configuration.second.end()) { const auto& base = findStepwise->second; + const std::string pidName = + sensorNameToDbusName(std::get(base.at("Name"))); const std::vector& zones = std::get>(base.at("Zones")); for (const std::string& zone : zones) @@ -913,8 +915,7 @@ bool init(sdbusplus::bus_t& bus, boost::asio::steady_timer& timer, { continue; } - conf::ControllerInfo& info = - conf[std::get(base.at("Name"))]; + conf::ControllerInfo& info = conf[pidName]; info.inputs = std::move(inputs); info.type = "stepwise"; diff --git a/interfaces.hpp b/interfaces.hpp index 5e99d195..7f4b4cd1 100644 --- a/interfaces.hpp +++ b/interfaces.hpp @@ -7,7 +7,7 @@ namespace pid_control struct ReadReturn { - double value; + double value = std::numeric_limits::quiet_NaN(); std::chrono::high_resolution_clock::time_point updated; double unscaled = value; diff --git a/pid/builder.cpp b/pid/builder.cpp index d7a60b4d..08526fa5 100644 --- a/pid/builder.cpp +++ b/pid/builder.cpp @@ -44,6 +44,11 @@ static std::string getControlPath(int64_t zone) return std::string(objectPath) + std::to_string(zone); } +static std::string getPidControlPath(int64_t zone, std::string pidname) +{ + return std::string(objectPath) + std::to_string(zone) + "/" + pidname; +} + std::unordered_map> buildZones(const std::map& zonePids, std::map& zoneConfigs, @@ -109,6 +114,9 @@ std::unordered_map> getThermalType(info.type)); zone->addThermalPID(std::move(pid)); + zone->addPidControlProcess(name, modeControlBus, + getPidControlPath(zoneId, name), + deferSignals); } else if (info.type == "stepwise") { @@ -120,6 +128,9 @@ std::unordered_map> auto stepwise = StepwiseController::createStepwiseController( zone.get(), name, inputs, info.stepwiseInfo); zone->addThermalPID(std::move(stepwise)); + zone->addPidControlProcess(name, modeControlBus, + getPidControlPath(zoneId, name), + deferSignals); } std::cerr << "inputs: "; diff --git a/pid/buildjson.cpp b/pid/buildjson.cpp index c37a5f63..1122f09a 100644 --- a/pid/buildjson.cpp +++ b/pid/buildjson.cpp @@ -187,6 +187,12 @@ std::pair, std::map> auto name = pid["name"]; auto item = pid.get(); + if (thisZone.find(name) != thisZone.end()) + { + std::cerr << "Warning: zone " << id + << " have the same pid name " << name << std::endl; + } + thisZone[name] = item; } diff --git a/pid/zone.cpp b/pid/zone.cpp index c88f41ba..b76fdecf 100644 --- a/pid/zone.cpp +++ b/pid/zone.cpp @@ -103,6 +103,12 @@ int64_t DbusPidZone::getZoneID(void) const void DbusPidZone::addSetPoint(double setPoint, const std::string& name) { + /* exclude disabled pidloop from _maximumSetPoint calculation*/ + if (!isPidProcessEnabled(name)) + { + return; + } + _SetPoints.push_back(setPoint); /* * if there are multiple thermal controllers with the same @@ -129,6 +135,7 @@ void DbusPidZone::clearSetPoints(void) { _SetPoints.clear(); _maximumSetPoint = 0; + _maximumSetPointName.clear(); } double DbusPidZone::getFailSafePercent(void) const @@ -264,7 +271,7 @@ void DbusPidZone::determineMaxSetPointRequest(void) if (minThermalThreshold >= _maximumSetPoint) { _maximumSetPoint = minThermalThreshold; - _maximumSetPointName = ""; + _maximumSetPointName = "Minimum"; } else if (_maximumSetPointName.compare(_maximumSetPointNamePrev)) { @@ -461,4 +468,20 @@ bool DbusPidZone::failSafe() const return getFailSafeMode(); } +void DbusPidZone::addPidControlProcess(std::string name, sdbusplus::bus_t& bus, + std::string objPath, bool defer) +{ + _pidsControlProcess[name] = std::make_unique( + bus, objPath.c_str(), + defer ? ProcessObject::action::defer_emit + : ProcessObject::action::emit_object_added); + // Default enable setting = true + _pidsControlProcess[name]->enabled(true); +} + +bool DbusPidZone::isPidProcessEnabled(std::string name) +{ + return _pidsControlProcess[name]->enabled(); +} + } // namespace pid_control diff --git a/pid/zone.hpp b/pid/zone.hpp index b985f5f4..9604c73d 100644 --- a/pid/zone.hpp +++ b/pid/zone.hpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -24,6 +25,9 @@ template using ServerObject = typename sdbusplus::server::object_t; using ModeInterface = sdbusplus::xyz::openbmc_project::Control::server::Mode; using ModeObject = ServerObject; +using ProcessInterface = + sdbusplus::xyz::openbmc_project::Object::server::Enable; +using ProcessObject = ServerObject; namespace pid_control { @@ -98,6 +102,10 @@ class DbusPidZone : public ZoneInterface, public ModeObject bool manual(bool value) override; /* Method for reading whether in fail-safe mode over dbus */ bool failSafe() const override; + /* Method for control process for each loop at runtime */ + void addPidControlProcess(std::string name, sdbusplus::bus_t& bus, + std::string objPath, bool defer); + bool isPidProcessEnabled(std::string name); private: template @@ -196,6 +204,8 @@ class DbusPidZone : public ZoneInterface, public ModeObject std::vector> _fans; std::vector> _thermals; + + std::map> _pidsControlProcess; }; } // namespace pid_control diff --git a/test/helpers.hpp b/test/helpers.hpp index 8f81bc69..4d3faea9 100644 --- a/test/helpers.hpp +++ b/test/helpers.hpp @@ -45,6 +45,7 @@ void SetupDbusObject(sdbusplus::SdBusMock* sdbus_mock, bool defer, EXPECT_CALL(*sdbus_mock, sd_bus_add_object_vtable(IsNull(), NotNull(), StrEq(path), StrEq(intf), NotNull(), NotNull())) + .Times(::testing::AnyNumber()) .WillOnce(Return(0)); if (!defer) diff --git a/test/pid_zone_unittest.cpp b/test/pid_zone_unittest.cpp index 8e743672..4eb34870 100644 --- a/test/pid_zone_unittest.cpp +++ b/test/pid_zone_unittest.cpp @@ -26,6 +26,7 @@ using ::testing::Return; using ::testing::StrEq; static std::string modeInterface = "xyz.openbmc_project.Control.Mode"; +static std::string enableInterface = "xyz.openbmc_project.Object.Enable"; namespace { @@ -34,10 +35,12 @@ TEST(PidZoneConstructorTest, BoringConstructorTest) { // Build a PID Zone. - sdbusplus::SdBusMock sdbus_mock_passive, sdbus_mock_host, sdbus_mock_mode; + sdbusplus::SdBusMock sdbus_mock_passive, sdbus_mock_host, sdbus_mock_mode, + sdbus_mock_enable; auto bus_mock_passive = sdbusplus::get_mocked_new(&sdbus_mock_passive); auto bus_mock_host = sdbusplus::get_mocked_new(&sdbus_mock_host); auto bus_mock_mode = sdbusplus::get_mocked_new(&sdbus_mock_mode); + auto bus_mock_enable = sdbusplus::get_mocked_new(&sdbus_mock_enable); EXPECT_CALL(sdbus_mock_host, sd_bus_add_object_manager( @@ -58,6 +61,15 @@ TEST(PidZoneConstructorTest, BoringConstructorTest) SetupDbusObject(&sdbus_mock_mode, defer, objPath, modeInterface, properties, &d); + std::string sensorname = "temp1"; + std::string pidsensorpath = "/xyz/openbmc_project/settings/fanctrl/zone1/" + + sensorname; + + double de; + std::vector propertiesenable; + SetupDbusObject(&sdbus_mock_enable, defer, pidsensorpath.c_str(), + enableInterface, propertiesenable, &de); + DbusPidZone p(zone, minThermalOutput, failSafePercent, cycleTime, m, bus_mock_mode, objPath, defer); // Success. @@ -70,7 +82,7 @@ class PidZoneTest : public ::testing::Test protected: PidZoneTest() : property_index(), properties(), sdbus_mock_passive(), sdbus_mock_host(), - sdbus_mock_mode() + sdbus_mock_mode(), sdbus_mock_enable() { EXPECT_CALL(sdbus_mock_host, sd_bus_add_object_manager( @@ -80,6 +92,7 @@ class PidZoneTest : public ::testing::Test auto bus_mock_passive = sdbusplus::get_mocked_new(&sdbus_mock_passive); auto bus_mock_host = sdbusplus::get_mocked_new(&sdbus_mock_host); auto bus_mock_mode = sdbusplus::get_mocked_new(&sdbus_mock_mode); + auto bus_mock_enable = sdbusplus::get_mocked_new(&sdbus_mock_enable); // Compiler weirdly not happy about just instantiating mgr(...); SensorManager m(bus_mock_passive, bus_mock_host); @@ -88,6 +101,10 @@ class PidZoneTest : public ::testing::Test SetupDbusObject(&sdbus_mock_mode, defer, objPath, modeInterface, properties, &property_index); + SetupDbusObject(&sdbus_mock_enable, defer, pidsensorpath.c_str(), + enableInterface, propertiesenable, + &propertyenable_index); + zone = std::make_unique(zoneId, minThermalOutput, failSafePercent, cycleTime, mgr, bus_mock_mode, objPath, defer); @@ -96,10 +113,13 @@ class PidZoneTest : public ::testing::Test // unused double property_index; std::vector properties; + double propertyenable_index; + std::vector propertiesenable; sdbusplus::SdBusMock sdbus_mock_passive; sdbusplus::SdBusMock sdbus_mock_host; sdbusplus::SdBusMock sdbus_mock_mode; + sdbusplus::SdBusMock sdbus_mock_enable; int64_t zoneId = 1; double minThermalOutput = 1000.0; double failSafePercent = 0.75; @@ -108,6 +128,10 @@ class PidZoneTest : public ::testing::Test SensorManager mgr; conf::CycleTime cycleTime; + std::string sensorname = "temp1"; + std::string pidsensorpath = "/xyz/openbmc_project/settings/fanctrl/zone1/" + + sensorname; + std::unique_ptr zone; }; @@ -128,6 +152,28 @@ TEST_F(PidZoneTest, GetAndSetManualModeTest_BehavesAsExpected) EXPECT_TRUE(zone->getManualMode()); } +TEST_F(PidZoneTest, AddPidControlProcessGetAndSetEnableTest_BehavesAsExpected) +{ + // Verifies that the zone starts in enable mode. Verifies that one can set + // enable the mode. + auto bus_mock_enable = sdbusplus::get_mocked_new(&sdbus_mock_enable); + + EXPECT_CALL(sdbus_mock_mode, sd_bus_emit_properties_changed_strv( + IsNull(), StrEq(pidsensorpath.c_str()), + StrEq(enableInterface), NotNull())) + .Times(::testing::AnyNumber()) + .WillOnce(Invoke( + [&]([[maybe_unused]] sd_bus* bus, [[maybe_unused]] const char* path, + [[maybe_unused]] const char* interface, const char** names) { + EXPECT_STREQ("Enable", names[0]); + return 0; + })); + + zone->addPidControlProcess(sensorname, bus_mock_enable, + pidsensorpath.c_str(), defer); + EXPECT_TRUE(zone->isPidProcessEnabled(sensorname)); +} + TEST_F(PidZoneTest, SetManualMode_RedundantWritesEnabledOnceAfterManualMode) { // Tests adding a fan PID controller to the zone, and verifies it's @@ -166,12 +212,31 @@ TEST_F(PidZoneTest, RpmSetPoints_AddMaxClear_BehaveAsExpected) // Tests addSetPoint, clearSetPoints, determineMaxSetPointRequest // and getMinThermalSetPoint. + // Need to add pid control process for the zone that can enable + // the process and add the set point. + auto bus_mock_enable = sdbusplus::get_mocked_new(&sdbus_mock_enable); + + EXPECT_CALL(sdbus_mock_mode, sd_bus_emit_properties_changed_strv( + IsNull(), StrEq(pidsensorpath.c_str()), + StrEq(enableInterface), NotNull())) + .Times(::testing::AnyNumber()) + .WillOnce(Invoke( + [&]([[maybe_unused]] sd_bus* bus, [[maybe_unused]] const char* path, + [[maybe_unused]] const char* interface, const char** names) { + EXPECT_STREQ("Enable", names[0]); + return 0; + })); + + zone->addPidControlProcess(sensorname, bus_mock_enable, + pidsensorpath.c_str(), defer); + // At least one value must be above the minimum thermal setpoint used in // the constructor otherwise it'll choose that value std::vector values = {100, 200, 300, 400, 500, 5000}; + for (auto v : values) { - zone->addSetPoint(v, ""); + zone->addSetPoint(v, sensorname); } // This will pull the maximum RPM setpoint request. @@ -191,10 +256,29 @@ TEST_F(PidZoneTest, RpmSetPoints_AddBelowMinimum_BehavesAsExpected) // Tests adding several RPM setpoints, however, they're all lower than the // configured minimal thermal setpoint RPM value. + // Need to add pid control process for the zone that can enable + // the process and add the set point. + auto bus_mock_enable = sdbusplus::get_mocked_new(&sdbus_mock_enable); + + EXPECT_CALL(sdbus_mock_mode, sd_bus_emit_properties_changed_strv( + IsNull(), StrEq(pidsensorpath.c_str()), + StrEq(enableInterface), NotNull())) + .Times(::testing::AnyNumber()) + .WillOnce(Invoke( + [&]([[maybe_unused]] sd_bus* bus, [[maybe_unused]] const char* path, + [[maybe_unused]] const char* interface, const char** names) { + EXPECT_STREQ("Enable", names[0]); + return 0; + })); + + zone->addPidControlProcess(sensorname, bus_mock_enable, + pidsensorpath.c_str(), defer); + std::vector values = {100, 200, 300, 400, 500}; + for (auto v : values) { - zone->addSetPoint(v, ""); + zone->addSetPoint(v, sensorname); } // This will pull the maximum RPM setpoint request.