Allow multiple inputs to thermal and stepwise controllers

Use std::max to determine which input value to apply.
Also start throwing when inputs are empty as otherwise
there will be a nullptr dereference.

Tested-by: Added multiple inputs and application no longer
segfaults and verifed max was being used. Also added unit
tests.

Change-Id: I7c8eda45b99247b8e92e629f036c9a46c98d9fe2
Signed-off-by: James Feist <james.feist@linux.intel.com>

6 files changed, 127 insertions, 40 deletions

diff --git a/errors/exception.hpp b/errors/exception.hpp
index 69d63be..c1c789a 100644
--- a/errors/exception.hpp
+++ b/errors/exception.hpp
@@ -18,3 +18,19 @@ class SensorBuildException : public std::exception
   private:
     std::string message;
 };
+
+class ControllerBuildException : public std::exception
+{
+  public:
+    ControllerBuildException(const std::string& message) : message(message)
+    {
+    }
+
+    virtual const char* what() const noexcept override
+    {
+        return message.c_str();
+    }
+
+  private:
+    std::string message;
+};
diff --git a/pid/builder.cpp b/pid/builder.cpp
index 55f3e35..261a165 100644
--- a/pid/builder.cpp
+++ b/pid/builder.cpp
@@ -102,8 +102,19 @@ std::unordered_map<int64_t, std::unique_ptr<PIDZone>>
                     zone->addThermalInput(i);
                 }
 
+                ThermalType type;
+                if (info->type == "temp")
+                {
+                    type = ThermalType::absolute;
+                }
+                else
+                {
+                    type = ThermalType::margin;
+                }
+
                 auto pid = ThermalController::createThermalPid(
-                    zone.get(), name, inputs, info->setpoint, info->pidInfo);
+                    zone.get(), name, inputs, info->setpoint, info->pidInfo,
+                    type);
 
                 zone->addThermalPID(std::move(pid));
             }
diff --git a/pid/stepwisecontroller.cpp b/pid/stepwisecontroller.cpp
index b81f8b1..43fd241 100644
--- a/pid/stepwisecontroller.cpp
+++ b/pid/stepwisecontroller.cpp
@@ -17,6 +17,7 @@
 #include "stepwisecontroller.hpp"
 
 #include "ec/stepwise.hpp"
+#include "errors/exception.hpp"
 #include "util.hpp"
 #include "zone.hpp"
 
@@ -66,9 +67,10 @@ std::unique_ptr<Controller> StepwiseController::createStepwiseController(
     ZoneInterface* owner, const std::string& id,
     const std::vector<std::string>& inputs, const ec::StepwiseInfo& initial)
 {
-    // StepwiseController currently only supports precisely one input.
-    if (inputs.size() != 1)
+    // StepwiseController requires at least 1 input
+    if (inputs.empty())
     {
+        throw ControllerBuildException("Stepwise controller missing inputs");
         return nullptr;
     }
 
@@ -83,7 +85,11 @@ std::unique_ptr<Controller> StepwiseController::createStepwiseController(
 
 double StepwiseController::inputProc(void)
 {
-    double value = _owner->getCachedValue(_inputs.at(0));
+    double value = std::numeric_limits<double>::lowest();
+    for (const auto& in : _inputs)
+    {
+        value = std::max(value, _owner->getCachedValue(in));
+    }
     return value;
 }
 
diff --git a/pid/thermalcontroller.cpp b/pid/thermalcontroller.cpp
index 485688a..ff2c7bc 100644
--- a/pid/thermalcontroller.cpp
+++ b/pid/thermalcontroller.cpp
@@ -16,21 +16,23 @@
 
 #include "thermalcontroller.hpp"
 
+#include "errors/exception.hpp"
 #include "util.hpp"
 #include "zone.hpp"
 
 std::unique_ptr<PIDController> ThermalController::createThermalPid(
     ZoneInterface* owner, const std::string& id,
     const std::vector<std::string>& inputs, double setpoint,
-    const ec::pidinfo& initial)
+    const ec::pidinfo& initial, const ThermalType& type)
 {
-    // ThermalController currently only supports precisely one input.
-    if (inputs.size() != 1)
+    // ThermalController requires at least 1 input
+    if (inputs.empty())
     {
+        throw ControllerBuildException("Thermal controller missing inputs");
         return nullptr;
     }
 
-    auto thermal = std::make_unique<ThermalController>(id, inputs, owner);
+    auto thermal = std::make_unique<ThermalController>(id, inputs, type, owner);
 
     ec::pid_info_t* info = thermal->getPIDInfo();
     thermal->setSetpoint(setpoint);
@@ -43,11 +45,24 @@ std::unique_ptr<PIDController> ThermalController::createThermalPid(
 // bmc_host_sensor_value_double
 double ThermalController::inputProc(void)
 {
-    /*
-     * This only supports one thermal input because it doesn't yet know how to
-     * handle merging them, probably max?
-     */
-    double value = _owner->getCachedValue(_inputs.at(0));
+    double value;
+    const double& (*compare)(const double&, const double&);
+    if (type == ThermalType::margin)
+    {
+        value = std::numeric_limits<double>::max();
+        compare = std::min<double>;
+    }
+    else
+    {
+        value = std::numeric_limits<double>::lowest();
+        compare = std::max<double>;
+    }
+
+    for (const auto& in : _inputs)
+    {
+        value = compare(value, _owner->getCachedValue(in));
+    }
+
     return value;
 }
 
diff --git a/pid/thermalcontroller.hpp b/pid/thermalcontroller.hpp
index 8089da1..317219c 100644
--- a/pid/thermalcontroller.hpp
+++ b/pid/thermalcontroller.hpp
@@ -11,19 +11,27 @@
  * A ThermalController is a PID controller that reads a number of sensors and
  * provides the set-points for the fans.
  */
+
+enum class ThermalType
+{
+    margin,
+    absolute
+};
+
 class ThermalController : public PIDController
 {
   public:
     static std::unique_ptr<PIDController>
         createThermalPid(ZoneInterface* owner, const std::string& id,
                          const std::vector<std::string>& inputs,
-                         double setpoint, const ec::pidinfo& initial);
+                         double setpoint, const ec::pidinfo& initial,
+                         const ThermalType& type);
 
     ThermalController(const std::string& id,
                       const std::vector<std::string>& inputs,
-                      ZoneInterface* owner) :
+                      const ThermalType& type, ZoneInterface* owner) :
         PIDController(id, owner),
-        _inputs(inputs)
+        _inputs(inputs), type(type)
     {
     }
 
@@ -33,4 +41,5 @@ class ThermalController : public PIDController
 
   private:
     std::vector<std::string> _inputs;
+    ThermalType type;
 };
diff --git a/test/pid_thermalcontroller_unittest.cpp b/test/pid_thermalcontroller_unittest.cpp
index 6da309f..804d84a 100644
--- a/test/pid_thermalcontroller_unittest.cpp
+++ b/test/pid_thermalcontroller_unittest.cpp
@@ -23,7 +23,7 @@ TEST(ThermalControllerTest, BoringFactoryTest)
     ec::pidinfo initial;
 
     std::unique_ptr<PIDController> p = ThermalController::createThermalPid(
-        &z, "therm1", inputs, setpoint, initial);
+        &z, "therm1", inputs, setpoint, initial, ThermalType::margin);
     // Success
     EXPECT_FALSE(p == nullptr);
 }
@@ -37,25 +37,13 @@ TEST(ThermalControllerTest, VerifyFactoryFailsWithZeroInputs)
     std::vector<std::string> inputs = {};
     double setpoint = 10.0;
     ec::pidinfo initial;
-
-    std::unique_ptr<PIDController> p = ThermalController::createThermalPid(
-        &z, "therm1", inputs, setpoint, initial);
-    EXPECT_TRUE(p == nullptr);
-}
-
-TEST(ThermalControllerTest, VerifyFactoryFailsForMoreThanOneInput)
-{
-    // ThermalControllers currently only support one input, so don't let
-    // someone accidentally specify more.
-
-    ZoneMock z;
-
-    std::vector<std::string> inputs = {"fleeting0", "asdf"};
-    double setpoint = 10.0;
-    ec::pidinfo initial;
-
-    std::unique_ptr<PIDController> p = ThermalController::createThermalPid(
-        &z, "therm1", inputs, setpoint, initial);
+    std::unique_ptr<PIDController> p;
+    EXPECT_THROW(
+        {
+            p = ThermalController::createThermalPid(
+                &z, "therm1", inputs, setpoint, initial, ThermalType::margin);
+        },
+        std::exception);
     EXPECT_TRUE(p == nullptr);
 }
 
@@ -70,7 +58,7 @@ TEST(ThermalControllerTest, InputProc_BehavesAsExpected)
     ec::pidinfo initial;
 
     std::unique_ptr<PIDController> p = ThermalController::createThermalPid(
-        &z, "therm1", inputs, setpoint, initial);
+        &z, "therm1", inputs, setpoint, initial, ThermalType::margin);
     EXPECT_FALSE(p == nullptr);
 
     EXPECT_CALL(z, getCachedValue(StrEq("fleeting0"))).WillOnce(Return(5.0));
@@ -89,7 +77,7 @@ TEST(ThermalControllerTest, SetPtProc_BehavesAsExpected)
     ec::pidinfo initial;
 
     std::unique_ptr<PIDController> p = ThermalController::createThermalPid(
-        &z, "therm1", inputs, setpoint, initial);
+        &z, "therm1", inputs, setpoint, initial, ThermalType::margin);
     EXPECT_FALSE(p == nullptr);
 
     EXPECT_EQ(setpoint, p->setptProc());
@@ -97,7 +85,7 @@ TEST(ThermalControllerTest, SetPtProc_BehavesAsExpected)
 
 TEST(ThermalControllerTest, OutputProc_BehavesAsExpected)
 {
-    // This test just verifies inputProc behaves as expected.
+    // This test just verifies outputProc behaves as expected.
 
     ZoneMock z;
 
@@ -106,7 +94,7 @@ TEST(ThermalControllerTest, OutputProc_BehavesAsExpected)
     ec::pidinfo initial;
 
     std::unique_ptr<PIDController> p = ThermalController::createThermalPid(
-        &z, "therm1", inputs, setpoint, initial);
+        &z, "therm1", inputs, setpoint, initial, ThermalType::margin);
     EXPECT_FALSE(p == nullptr);
 
     double value = 90.0;
@@ -114,3 +102,45 @@ TEST(ThermalControllerTest, OutputProc_BehavesAsExpected)
 
     p->outputProc(value);
 }
+
+TEST(ThermalControllerTest, InputProc_MultipleInputsAbsolute)
+{
+    // This test verifies inputProc behaves as expected with multiple absolute
+    // 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::absolute);
+    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(10.0, p->inputProc());
+}
+
+TEST(ThermalControllerTest, InputProc_MultipleInputsMargin)
+{
+    // This test verifies inputProc behaves as expected with multiple margin
+    // 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::margin);
+    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(5.0, p->inputProc());
+}
\ No newline at end of file