commit c612c051a5c414180f8ebe273c24543ab95d16ee
author Josh Lehan <krellan@google.com> Mon Dec 12 09:56:47 2022 -0800
committer Josh Lehan <krellan@google.com> Mon Dec 12 10:59:40 2022 -0800
tree 1859b3df4c5b0d5a59a9a5dd1484ee87b23b8133
parent 5897fcc104df9d07e41f0b71d52b273b6c46f901

Allow derivativeCoeff and DCoefficient optional

To avoid breaking existing configurations in the field, treat the
new "derivativeCoeff" parameter as optional, not mandatory.

This affects both the old JSON parser, and the new D-Bus
entity-manager parser (it's called "DCoefficient" there).

Signed-off-by: Josh Lehan <krellan@google.com>
Change-Id: Ifcaf47d66e009b48e41b510a2ef1686b8860ad35

dbus/dbusconfiguration.cpp

diff --git a/dbus/dbusconfiguration.cpp b/dbus/dbusconfiguration.cpp
index 29f0ccb..02eda38 100644
--- a/dbus/dbusconfiguration.cpp
+++ b/dbus/dbusconfiguration.cpp
@@ -309,6 +309,7 @@
         VariantToDoubleVisitor(), getPIDAttribute(base, "PCoefficient"));
     info.pidInfo.integralCoeff = std::visit(
         VariantToDoubleVisitor(), getPIDAttribute(base, "ICoefficient"));
+    // DCoefficient is below, it is optional, same reason as in buildjson.cpp
     info.pidInfo.feedFwdOffset = std::visit(
         VariantToDoubleVisitor(), getPIDAttribute(base, "FFOffCoefficient"));
     info.pidInfo.feedFwdGain = std::visit(
@@ -325,25 +326,34 @@
         std::visit(VariantToDoubleVisitor(), getPIDAttribute(base, "SlewNeg"));
     info.pidInfo.slewPos =
         std::visit(VariantToDoubleVisitor(), getPIDAttribute(base, "SlewPos"));
+
     double negativeHysteresis = 0;
     double positiveHysteresis = 0;
+    double derivativeCoeff = 0;
 
     auto findNeg = base.find("NegativeHysteresis");
     auto findPos = base.find("PositiveHysteresis");
+    auto findDerivative = base.find("DCoefficient");
 
     if (findNeg != base.end())
     {
         negativeHysteresis =
             std::visit(VariantToDoubleVisitor(), findNeg->second);
     }
-
     if (findPos != base.end())
     {
         positiveHysteresis =
             std::visit(VariantToDoubleVisitor(), findPos->second);
     }
+    if (findDerivative != base.end())
+    {
+        derivativeCoeff =
+            std::visit(VariantToDoubleVisitor(), findDerivative->second);
+    }
+
     info.pidInfo.negativeHysteresis = negativeHysteresis;
     info.pidInfo.positiveHysteresis = positiveHysteresis;
+    info.pidInfo.derivativeCoeff = derivativeCoeff;
 }
 
 bool init(sdbusplus::bus_t& bus, boost::asio::steady_timer& timer,

pid/buildjson.cpp

diff --git a/pid/buildjson.cpp b/pid/buildjson.cpp
index cf7df67..8dcbfbc 100644
--- a/pid/buildjson.cpp
+++ b/pid/buildjson.cpp
@@ -45,15 +45,21 @@
 
     auto positiveHysteresis = p.find("positiveHysteresis");
     auto negativeHysteresis = p.find("negativeHysteresis");
+    auto derivativeCoeff = p.find("derivativeCoeff");
     auto positiveHysteresisValue = 0.0;
     auto negativeHysteresisValue = 0.0;
+    auto derivativeCoeffValue = 0.0;
     if (positiveHysteresis != p.end())
     {
-        p.at("positiveHysteresis").get_to(positiveHysteresisValue);
+        positiveHysteresis->get_to(positiveHysteresisValue);
     }
     if (negativeHysteresis != p.end())
     {
-        p.at("negativeHysteresis").get_to(negativeHysteresisValue);
+        negativeHysteresis->get_to(negativeHysteresisValue);
+    }
+    if (derivativeCoeff != p.end())
+    {
+        derivativeCoeff->get_to(derivativeCoeffValue);
     }
 
     if (c.type != "stepwise")
@@ -61,7 +67,6 @@
         p.at("samplePeriod").get_to(c.pidInfo.ts);
         p.at("proportionalCoeff").get_to(c.pidInfo.proportionalCoeff);
         p.at("integralCoeff").get_to(c.pidInfo.integralCoeff);
-        p.at("derivativeCoeff").get_to(c.pidInfo.derivativeCoeff);
         p.at("feedFwdOffsetCoeff").get_to(c.pidInfo.feedFwdOffset);
         p.at("feedFwdGainCoeff").get_to(c.pidInfo.feedFwdGain);
         p.at("integralLimit_min").get_to(c.pidInfo.integralLimit.min);
@@ -71,8 +76,12 @@
         p.at("slewNeg").get_to(c.pidInfo.slewNeg);
         p.at("slewPos").get_to(c.pidInfo.slewPos);
 
+        // Unlike other coefficients, treat derivativeCoeff as an optional
+        // parameter, as support for it is fairly new, to avoid breaking
+        // existing configurations in the field that predate it.
         c.pidInfo.positiveHysteresis = positiveHysteresisValue;
         c.pidInfo.negativeHysteresis = negativeHysteresisValue;
+        c.pidInfo.derivativeCoeff = derivativeCoeffValue;
     }
     else
     {

test/pid_json_unittest.cpp

diff --git a/test/pid_json_unittest.cpp b/test/pid_json_unittest.cpp
index b6963cd..eb9ca58 100644
--- a/test/pid_json_unittest.cpp
+++ b/test/pid_json_unittest.cpp
@@ -31,6 +31,7 @@
 TEST(ZoneFromJson, oneZoneOnePid)
 {
     // Parse a valid configuration with one zone and one PID.
+    // Intentionally omits "derivativeCoeff" to test that it is optional.
 
     std::map<int64_t, conf::PIDConf> pidConfig;
     std::map<int64_t, conf::ZoneConfig> zoneConfig;
@@ -50,7 +51,6 @@
               "samplePeriod": 0.1,
               "proportionalCoeff": 0.0,
               "integralCoeff": 0.0,
-              "derivativeCoeff": 0.0,
               "feedFwdOffsetCoeff": 0.0,
               "feedFwdGainCoeff": 0.010,
               "integralLimit_min": 0.0,