Skip to content

Commit

Permalink
Allow disabling PID loops at runtime
Browse files Browse the repository at this point in the history 
<design concept>
Add the map of object enable interface to pid loops
in the zone then we can disable/enable each pid loop
process in a zone by dbus command.

[note]
Enable = true  : enable process (default)
Enable = false : disable process

Tested:
In this case: we set Enable = false to disable
pidloop:Zone_Temp_0, and see how it affects
the zone final pwm, when pidloop: Zone_Temp_0
in zone 0 is disabled.

then even we are trying to heat up the temperature
of a sensor: Temp_0 in pidloop: Zone_Temp_0, this
set point of the pidloop will not be taken into the
calculation for the final set point of the whole zone.

```
<service object>
root@openbmc:/tmp# busctl tree xyz.openbmc_project.State.FanCtrl
`-/xyz
  `-/xyz/openbmc_project
    `-/xyz/openbmc_project/settings
      `-/xyz/openbmc_project/settings/fanctrl
        |-/xyz/openbmc_project/settings/fanctrl/zone0
        | |-/xyz/openbmc_project/settings/fanctrl/zone0/Zone_Temp
        | |-/xyz/openbmc_project/settings/fanctrl/zone0/Zone_Temp_0
        | `-/xyz/openbmc_project/settings/fanctrl/zone0/Zone_Temp_1

====Enable process for pidloop:Zone_Temp_0 with p-switch temperature sensor:Temp_0 at runtime====
root@openbmc:~# busctl introspect xyz.openbmc_project.State.FanCtrl /xyz/openbmc_project/settings/fanctrl/zone0/Zone_Temp_0
NAME                                TYPE      SIGNATURE RESULT/VALUE FLAGS
xyz.openbmc_project.Object.Enable   interface -         -            -
.Enabled                            property  b         true         emits-change writable

====Disable process for pidloop:Zone_Temp_0 with p-switch temperature sensor: Temp_0====
root@openbmc:~# busctl set-property xyz.openbmc_project.State.FanCtrl /xyz/openbmc_project/settings/fanctrl/zone0/Zone_Temp_0 xyz.openbmc_project.Object.Enable Enabled b false
root@openbmc:~# busctl introspect xyz.openbmc_project.State.FanCtrl /xyz/openbmc_project/settings/fanctrl/zone0/Zone_Temp_0
NAME                                TYPE      SIGNATURE RESULT/VALUE FLAGS
xyz.openbmc_project.Object.Enable   interface -         -            -
.Enabled                            property  b         false        emits-change writable
```

when Disable the process of the pidloop: Zone_Temp_0,
the requester switches from Zone_Temp_0 to the others,
when you enable the pidloop: Zone_Temp_0, the setpoint
of Zone_Temp_0 will be take into consideration again

Change-Id: I95ae700144f0d16049fff8b309f05ae690a7ef72
Signed-off-by: ykchiu <Chiu.YK@inventec.com>
  • Loading branch information
@chiuyikai24
chiuyikai24 committed Jun 1, 2023
1 parent 02839e3 commit 7c6d35d
Show file tree
Hide file tree
Showing 8 changed files with 147 additions and 11 deletions.
11 changes: 6 additions & 5 deletions dbus/dbusconfiguration.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string>(base.at("Name"));
const std::string pidName =
sensorNameToDbusName(std::get<std::string>(base.at("Name")));
const std::string pidClass =
std::get<std::string>(base.at("Class"));
const std::vector<std::string>& zones =
Expand Down Expand Up @@ -833,8 +834,7 @@ bool init(sdbusplus::bus_t& bus, boost::asio::steady_timer& timer,

if (offsetType.empty())
{
conf::ControllerInfo& info =
conf[std::get<std::string>(base.at("Name"))];
conf::ControllerInfo& info = conf[pidName];
info.inputs = std::move(inputSensorNames);
populatePidInfo(bus, base, info, nullptr, sensorConfig);
}
Expand All @@ -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<std::string>(base.at("Name")));
const std::vector<std::string>& zones =
std::get<std::vector<std::string>>(base.at("Zones"));
for (const std::string& zone : zones)
Expand Down Expand Up @@ -913,8 +915,7 @@ bool init(sdbusplus::bus_t& bus, boost::asio::steady_timer& timer,
{
continue;
}
conf::ControllerInfo& info =
conf[std::get<std::string>(base.at("Name"))];
conf::ControllerInfo& info = conf[pidName];
info.inputs = std::move(inputs);

info.type = "stepwise";
Expand Down
2 changes: 1 addition & 1 deletion interfaces.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace pid_control

struct ReadReturn
{
double value;
double value = std::numeric_limits<double>::quiet_NaN();
std::chrono::high_resolution_clock::time_point updated;
double unscaled = value;

Expand Down
11 changes: 11 additions & 0 deletions pid/builder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<int64_t, std::shared_ptr<ZoneInterface>>
buildZones(const std::map<int64_t, conf::PIDConf>& zonePids,
std::map<int64_t, conf::ZoneConfig>& zoneConfigs,
Expand Down Expand Up @@ -109,6 +114,9 @@ std::unordered_map<int64_t, std::shared_ptr<ZoneInterface>>
getThermalType(info.type));

zone->addThermalPID(std::move(pid));
zone->addPidControlProcess(name, modeControlBus,
getPidControlPath(zoneId, name),
deferSignals);
}
else if (info.type == "stepwise")
{
Expand All @@ -120,6 +128,9 @@ std::unordered_map<int64_t, std::shared_ptr<ZoneInterface>>
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: ";
Expand Down
6 changes: 6 additions & 0 deletions pid/buildjson.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,12 @@ std::pair<std::map<int64_t, conf::PIDConf>, std::map<int64_t, conf::ZoneConfig>>
auto name = pid["name"];
auto item = pid.get<conf::ControllerInfo>();

if (thisZone.find(name) != thisZone.end())
{
std::cerr << "Warning: zone " << id
<< " have the same pid name " << name << std::endl;
}

thisZone[name] = item;
}

Expand Down
25 changes: 24 additions & 1 deletion pid/zone.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -129,6 +135,7 @@ void DbusPidZone::clearSetPoints(void)
{
_SetPoints.clear();
_maximumSetPoint = 0;
_maximumSetPointName.clear();
}

double DbusPidZone::getFailSafePercent(void) const
Expand Down Expand Up @@ -264,7 +271,7 @@ void DbusPidZone::determineMaxSetPointRequest(void)
if (minThermalThreshold >= _maximumSetPoint)
{
_maximumSetPoint = minThermalThreshold;
_maximumSetPointName = "";
_maximumSetPointName = "Minimum";
}
else if (_maximumSetPointName.compare(_maximumSetPointNamePrev))
{
Expand Down Expand Up @@ -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<ProcessObject>(
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
10 changes: 10 additions & 0 deletions pid/zone.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <sdbusplus/bus.hpp>
#include <sdbusplus/server.hpp>
#include <xyz/openbmc_project/Control/Mode/server.hpp>
#include <xyz/openbmc_project/Object/Enable/server.hpp>

#include <fstream>
#include <iostream>
Expand All @@ -24,6 +25,9 @@ template <typename... T>
using ServerObject = typename sdbusplus::server::object_t<T...>;
using ModeInterface = sdbusplus::xyz::openbmc_project::Control::server::Mode;
using ModeObject = ServerObject<ModeInterface>;
using ProcessInterface =
sdbusplus::xyz::openbmc_project::Object::server::Enable;
using ProcessObject = ServerObject<ProcessInterface>;

namespace pid_control
{
Expand Down Expand Up @@ -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 <bool fanSensorLogging>
Expand Down Expand Up @@ -196,6 +204,8 @@ class DbusPidZone : public ZoneInterface, public ModeObject

std::vector<std::unique_ptr<Controller>> _fans;
std::vector<std::unique_ptr<Controller>> _thermals;

std::map<std::string, std::unique_ptr<ProcessObject>> _pidsControlProcess;
};

} // namespace pid_control
1 change: 1 addition & 0 deletions test/helpers.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
92 changes: 88 additions & 4 deletions test/pid_zone_unittest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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(
Expand All @@ -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<std::string> 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.
Expand All @@ -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(
Expand All @@ -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);
Expand All @@ -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<DbusPidZone>(zoneId, minThermalOutput,
failSafePercent, cycleTime, mgr,
bus_mock_mode, objPath, defer);
Expand All @@ -96,10 +113,13 @@ class PidZoneTest : public ::testing::Test
// unused
double property_index;
std::vector<std::string> properties;
double propertyenable_index;
std::vector<std::string> 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;
Expand All @@ -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<DbusPidZone> zone;
};

Expand All @@ -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
Expand Down Expand Up @@ -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<double> 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.
Expand All @@ -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<double> 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.
Expand Down

0 comments on commit 7c6d35d

Please sign in to comment.