Add new PID Class types "power" and "powersum"
Implements this feature:
https://github.com/openbmc/phosphor-pid-control/issues/24
In addition to the existing "temp" and "margin" classes, adding
new "power" and "powersum" Class types.
The "power" class is the same as "temp", but expects D-Bus power
sensors, instead of D-Bus temperature sensors.
The "powersum" class is the same as "power", but sums together all of
the given inputs, instead of finding the maximum.
Signed-off-by: Josh Lehan <krellan@google.com>
Change-Id: I11d8ad8385632658ed061671134be87a560cd02a
diff --git a/dbus/dbusconfiguration.cpp b/dbus/dbusconfiguration.cpp
index 460e9e3..29f0ccb 100644
--- a/dbus/dbusconfiguration.cpp
+++ b/dbus/dbusconfiguration.cpp
@@ -646,7 +646,8 @@
// All non-fan PID classes should be initialized this way.
// As for why a fan should not use this code path, see
// the ed1dafdf168def37c65bfb7a5efd18d9dbe04727 commit.
- if ((pidClass == "temp") || (pidClass == "margin"))
+ if ((pidClass == "temp") || (pidClass == "margin") ||
+ (pidClass == "power") || (pidClass == "powersum"))
{
std::string inputSensorName =
getSensorNameFromPath(inputSensorPath);
diff --git a/dbus/dbusutil.cpp b/dbus/dbusutil.cpp
index 03cf41e..31526ca 100644
--- a/dbus/dbusutil.cpp
+++ b/dbus/dbusutil.cpp
@@ -103,6 +103,14 @@
{
layer = "temperature";
}
+ else if (type == "power")
+ {
+ layer = "power";
+ }
+ else if (type == "powersum")
+ {
+ layer = "power";
+ }
else
{
layer = "unknown"; // TODO(venture): Need to handle.
@@ -122,7 +130,8 @@
bool validType(const std::string& type)
{
- static std::set<std::string> valid = {"fan", "temp", "margin"};
+ static std::set<std::string> valid = {"fan", "temp", "margin", "power",
+ "powersum"};
return (valid.find(type) != valid.end());
}
diff --git a/pid/stepwisecontroller.cpp b/pid/stepwisecontroller.cpp
index d141f87..b93c349 100644
--- a/pid/stepwisecontroller.cpp
+++ b/pid/stepwisecontroller.cpp
@@ -75,7 +75,6 @@
if (inputs.empty())
{
throw ControllerBuildException("Stepwise controller missing inputs");
- return nullptr;
}
auto thermal = std::make_unique<StepwiseController>(id, inputs, owner);
diff --git a/pid/thermalcontroller.cpp b/pid/thermalcontroller.cpp
index 8778719..db4763f 100644
--- a/pid/thermalcontroller.cpp
+++ b/pid/thermalcontroller.cpp
@@ -30,13 +30,26 @@
ThermalType getThermalType(const std::string& typeString)
{
- /* Currently it only supports the two types. */
- return (typeString == "temp") ? ThermalType::absolute : ThermalType::margin;
+ if (typeString == "margin")
+ {
+ return ThermalType::margin;
+ }
+ if ((typeString == "temp") || (typeString == "power"))
+ {
+ return ThermalType::absolute;
+ }
+ if (typeString == "powersum")
+ {
+ return ThermalType::summation;
+ }
+
+ throw ControllerBuildException("Unrecognized PID Type/Class string");
}
bool isThermalType(const std::string& typeString)
{
- static const std::vector<std::string> thermalTypes = {"temp", "margin"};
+ static const std::vector<std::string> thermalTypes = {"temp", "margin",
+ "power", "powersum"};
return std::count(thermalTypes.begin(), thermalTypes.end(), typeString);
}
@@ -49,7 +62,6 @@
if (inputs.empty())
{
throw ControllerBuildException("Thermal controller missing inputs");
- return nullptr;
}
auto thermal = std::make_unique<ThermalController>(id, inputs, type, owner);
@@ -67,16 +79,27 @@
{
double value;
const double& (*compare)(const double&, const double&);
+ bool doSummation = false;
+
if (type == ThermalType::margin)
{
value = std::numeric_limits<double>::max();
compare = std::min<double>;
}
- else
+ else if (type == ThermalType::absolute)
{
value = std::numeric_limits<double>::lowest();
compare = std::max<double>;
}
+ else if (type == ThermalType::summation)
+ {
+ doSummation = true;
+ value = 0.0;
+ }
+ else
+ {
+ throw ControllerBuildException("Unrecognized ThermalType");
+ }
bool acceptable = false;
for (const auto& in : _inputs)
@@ -89,7 +112,15 @@
continue;
}
- value = compare(value, cachedValue);
+ if (doSummation)
+ {
+ value += cachedValue;
+ }
+ else
+ {
+ value = compare(value, cachedValue);
+ }
+
acceptable = true;
}
diff --git a/pid/thermalcontroller.hpp b/pid/thermalcontroller.hpp
index 6144d49..752c105 100644
--- a/pid/thermalcontroller.hpp
+++ b/pid/thermalcontroller.hpp
@@ -13,12 +13,16 @@
/*
* A ThermalController is a PID controller that reads a number of sensors and
* provides the setpoints for the fans.
+ * With addition of support for power sensors, this name is misleading,
+ * as it now works for power sensors also, not just thermal sensors.
+ * If rewritten today, a better name would be "ComputationType".
*/
enum class ThermalType
{
margin,
- absolute
+ absolute,
+ summation
};
/**
diff --git a/sensors/builder.cpp b/sensors/builder.cpp
index 82f54a1..7c335c6 100644
--- a/sensors/builder.cpp
+++ b/sensors/builder.cpp
@@ -162,7 +162,8 @@
name, info->timeout, std::move(ri), std::move(wi));
mgmr.addSensor(info->type, name, std::move(sensor));
}
- else if (info->type == "temp" || info->type == "margin")
+ else if (info->type == "temp" || info->type == "margin" ||
+ info->type == "power" || info->type == "powersum")
{
// These sensors are read-only, but only for this application
// which only writes to fan sensors.
diff --git a/test/dbus_util_unittest.cpp b/test/dbus_util_unittest.cpp
index 4896e03..09d1b5c 100644
--- a/test/dbus_util_unittest.cpp
+++ b/test/dbus_util_unittest.cpp
@@ -40,7 +40,11 @@
std::make_tuple("margin", "9",
"/xyz/openbmc_project/sensors/temperature/9"),
std::make_tuple("temp", "123",
- "/xyz/openbmc_project/sensors/temperature/123")));
+ "/xyz/openbmc_project/sensors/temperature/123"),
+ std::make_tuple("power", "9000",
+ "/xyz/openbmc_project/sensors/power/9000"),
+ std::make_tuple("powersum", "total",
+ "/xyz/openbmc_project/sensors/power/total")));
class FindSensorsTest : public ::testing::Test
{
diff --git a/test/pid_thermalcontroller_unittest.cpp b/test/pid_thermalcontroller_unittest.cpp
index 4e71a3b..ab45661 100644
--- a/test/pid_thermalcontroller_unittest.cpp
+++ b/test/pid_thermalcontroller_unittest.cpp
@@ -152,6 +152,27 @@
EXPECT_EQ(5.0, p->inputProc());
}
+TEST(ThermalControllerTest, InputProc_MultipleInputsSummation)
+{
+ // This test verifies inputProc behaves as expected with multiple summation
+ // inputs.
+
+ ZoneMock z;
+
+ std::vector<std::string> inputs = {"fleeting0", "fleeting1"};
+ double setpoint = 10.0;
+ ec::pidinfo initial;
+
+ std::unique_ptr<PIDController> p = ThermalController::createThermalPid(
+ &z, "therm1", inputs, setpoint, initial, ThermalType::summation);
+ EXPECT_FALSE(p == nullptr);
+
+ EXPECT_CALL(z, getCachedValue(StrEq("fleeting0"))).WillOnce(Return(5.0));
+ EXPECT_CALL(z, getCachedValue(StrEq("fleeting1"))).WillOnce(Return(10.0));
+
+ EXPECT_EQ(15.0, p->inputProc());
+}
+
TEST(ThermalControllerTest, NegHysteresis_BehavesAsExpected)
{