Fix: DPL: ensure inverter reaches requested state

we previously only called commitPowerLimit() if the desired limit
changed such that the change was bigger than the hysteresis. we found
that if the limit update was not received and the desired limit would
not change much, the limit of the inverter was wrong for a long time.

to mitigate this, we introduced re-sending the limit update every 60
seconds, regardless of what the limit reported by the inverter was at
that time.

if the power-up command was not received, we also would repeat it only
once every 60 seconds.

this leads to a new kind of staleness and the actual inverter state was
still not matching the desired state.

this new approach effectively adds an additional control loop at the
start of the DPL loop(). that new function compares the requested
inverter state to the actual reported state. it sends updates (limit
update or power on state) until the desired inverter state is reached,
or until a (hard-coded) timeout occurs.

this approach also allows us to send power-up, power-down, and limit
update commands independent from one another and in a particular order.

this should make sure that the inverter is in the desired state even if
conditions change slowly and commands were not received as expected.

include/PowerLimiter.h

@@ -7,6 +7,7 @@
 #include <Hoymiles.h>
 #include <memory>
 #include <functional>
+#include <optional>
 #include <TaskSchedulerDeclarations.h>
 #include <frozen/string.h>
 
@@ -15,10 +16,6 @@
 #define PL_UI_STATE_USE_SOLAR_ONLY 2
 #define PL_UI_STATE_USE_SOLAR_AND_BATTERY 3
 
-#define PL_MODE_ENABLE_NORMAL_OP 0
-#define PL_MODE_FULL_DISABLE 1
-#define PL_MODE_SOLAR_PT_ONLY 2
-
 typedef enum {
     EMPTY_WHEN_FULL= 0, 
     EMPTY_AT_NIGHT
@@ -51,7 +48,7 @@ public:
 
     void init(Scheduler& scheduler);
     uint8_t getPowerLimiterState();
-    int32_t getLastRequestedPowerLimit();
+    int32_t getLastRequestedPowerLimit() { return _lastRequestedPowerLimit; }
 
     enum class Mode : unsigned {
         Normal = 0,
@@ -69,8 +66,10 @@ private:
     Task _loopTask;
 
     int32_t _lastRequestedPowerLimit = 0;
-    uint32_t _lastPowerLimitMillis = 0;
+    bool _shutdownPending = false;
-    uint32_t _shutdownTimeout = 0;
+    std::optional<uint32_t> _oUpdateStartMillis = std::nullopt;
+    std::optional<int32_t> _oTargetPowerLimitWatts = std::nullopt;
+    std::optional<bool> _oTargetPowerState = std::nullopt;
     Status _lastStatus = Status::Initializing;
     uint32_t _lastStatusPrinted = 0;
     uint32_t _lastCalculation = 0;
@@ -93,7 +92,7 @@ private:
     void unconditionalSolarPassthrough(std::shared_ptr<InverterAbstract> inverter);
     bool canUseDirectSolarPower();
     int32_t calcPowerLimit(std::shared_ptr<InverterAbstract> inverter, bool solarPowerEnabled, bool batteryDischargeEnabled);
-    void commitPowerLimit(std::shared_ptr<InverterAbstract> inverter, int32_t limit, bool enablePowerProduction);
+    bool updateInverter();
     bool setNewPowerLimit(std::shared_ptr<InverterAbstract> inverter, int32_t newPowerLimit);
     int32_t getSolarChargePower();
     float getLoadCorrectedVoltage();

src/PowerLimiter.cpp

@@ -30,7 +30,7 @@ frozen::string const& PowerLimiterClass::getStatusText(PowerLimiterClass::Status
 {
     static const frozen::string missing = "programmer error: missing status text";
 
-    static const frozen::map<Status, frozen::string, 19> texts = {
+    static const frozen::map<Status, frozen::string, 18> texts = {
         { Status::Initializing, "initializing (should not see me)" },
         { Status::DisabledByConfig, "disabled by configuration" },
         { Status::DisabledByMqtt, "disabled by MQTT" },
@@ -48,7 +48,6 @@ frozen::string const& PowerLimiterClass::getStatusText(PowerLimiterClass::Status
         { Status::InverterStatsPending, "waiting for sufficiently recent inverter data" },
         { Status::UnconditionalSolarPassthrough, "unconditionally passing through all solar power (MQTT override)" },
         { Status::NoVeDirect, "VE.Direct disabled, connection broken, or data outdated" },
-        { Status::Settling, "waiting for the system to settle" },
         { Status::Stable, "the system is stable, the last power limit is still valid" },
     };
 
@@ -79,36 +78,18 @@ void PowerLimiterClass::announceStatus(PowerLimiterClass::Status status)
 /**
  * returns true if the inverter state was changed or is about to change, i.e.,
  * if it is actually in need of a shutdown. returns false otherwise, i.e., the
- * inverter is already (assumed to be) shut down.
+ * inverter is already shut down and the inverter limit is set to the configured
+ * lower power limit.
  */
 bool PowerLimiterClass::shutdown(PowerLimiterClass::Status status)
 {
     announceStatus(status);
 
-    if (_inverter == nullptr || !_inverter->isProducing() ||
+    _shutdownPending = true;
-            (_shutdownTimeout > 0 && _shutdownTimeout < millis()) ) {
-        // we are actually (already) done with shutting down the inverter,
-        // or a shutdown attempt was initiated but it timed out.
-        _inverter = nullptr;
-        _shutdownTimeout = 0;
-        return false;
-    }
 
-    if (!_inverter->isReachable()) { return true; } // retry later (until timeout)
+    _oTargetPowerState = false;
-
+    _oTargetPowerLimitWatts = Configuration.get().PowerLimiter.LowerPowerLimit;
-    // retry shutdown for a maximum amount of time before giving up
+    return updateInverter();
-    if (_shutdownTimeout == 0) { _shutdownTimeout = millis() + 10 * 1000; }
-
-    auto lastLimitCommandState = _inverter->SystemConfigPara()->getLastLimitCommandSuccess();
-    if (CMD_PENDING == lastLimitCommandState) { return true; }
-
-    auto lastPowerCommandState = _inverter->PowerCommand()->getLastPowerCommandSuccess();
-    if (CMD_PENDING == lastPowerCommandState) { return true; }
-
-    CONFIG_T& config = Configuration.get();
-    commitPowerLimit(_inverter, config.PowerLimiter.LowerPowerLimit, false);
-
-    return true;
 }
 
 void PowerLimiterClass::loop()
@@ -124,12 +105,13 @@ void PowerLimiterClass::loop()
         return announceStatus(Status::WaitingForValidTimestamp);
     }
 
-    if (_shutdownTimeout > 0) {
+    // take care that the last requested power
-        // we transition from SHUTDOWN to OFF when we know the inverter was
+    // limit and power state are actually reached
-        // shut down. until then, we retry shutting it down. in this case we
+    if (updateInverter()) { return; }
-        // preserve the original status that lead to the decision to shut down.
+
-        shutdown();
+    if (_shutdownPending) {
-        return;
+        _shutdownPending = false;
+        _inverter = nullptr;
     }
 
     if (!config.PowerLimiter.Enabled) {
@@ -172,18 +154,6 @@ void PowerLimiterClass::loop()
         return announceStatus(Status::InverterCommandsDisabled);
     }
 
-    // concerns active power commands (power limits) only (also from web app or MQTT)
-    auto lastLimitCommandState = _inverter->SystemConfigPara()->getLastLimitCommandSuccess();
-    if (CMD_PENDING == lastLimitCommandState) {
-        return announceStatus(Status::InverterLimitPending);
-    }
-
-    // concerns power commands (start, stop, restart) only (also from web app or MQTT)
-    auto lastPowerCommandState = _inverter->PowerCommand()->getLastPowerCommandSuccess();
-    if (CMD_PENDING == lastPowerCommandState) {
-        return announceStatus(Status::InverterPowerCmdPending);
-    }
-
     // a calculated power limit will always be limited to the reported
     // device's max power. that upper limit is only known after the first
     // DevInfoSimpleCommand succeeded.
@@ -214,16 +184,11 @@ void PowerLimiterClass::loop()
             _inverter->SystemConfigPara()->getLastUpdateCommand(),
             _inverter->PowerCommand()->getLastUpdateCommand());
 
-    // wait for power meter and inverter stat updates after a settling phase
+    if (_inverter->Statistics()->getLastUpdate() <= lastUpdateCmd) {
-    auto settlingEnd = lastUpdateCmd + 3 * 1000;
-
-    if (millis() < settlingEnd) { return announceStatus(Status::Settling); }
-
-    if (_inverter->Statistics()->getLastUpdate() <= settlingEnd) {
         return announceStatus(Status::InverterStatsPending);
     }
 
-    if (PowerMeter.getLastPowerMeterUpdate() <= settlingEnd) {
+    if (PowerMeter.getLastPowerMeterUpdate() <= lastUpdateCmd) {
         return announceStatus(Status::PowerMeterPending);
     }
 
@@ -323,12 +288,6 @@ void PowerLimiterClass::loop()
     int32_t newPowerLimit = calcPowerLimit(_inverter, canUseDirectSolarPower(), _batteryDischargeEnabled);
     bool limitUpdated = setNewPowerLimit(_inverter, newPowerLimit);
 
-    if (_verboseLogging) {
-        MessageOutput.printf("[DPL::loop] ******************* Leaving PL, calculated limit: %d W, requested limit: %d W (%s)\r\n",
-                newPowerLimit, _lastRequestedPowerLimit,
-                (limitUpdated?"updated from calculated":"kept last requested"));
-    }
-
     _lastCalculation = millis();
 
     if (!limitUpdated) {
@@ -441,10 +400,6 @@ uint8_t PowerLimiterClass::getPowerLimiterState() {
     return PL_UI_STATE_INACTIVE;
 }
 
-int32_t PowerLimiterClass::getLastRequestedPowerLimit() {
-	    return _lastRequestedPowerLimit;
-}
-
 bool PowerLimiterClass::canUseDirectSolarPower()
 {
     CONFIG_T& config = Configuration.get();
@@ -527,34 +482,141 @@ int32_t PowerLimiterClass::calcPowerLimit(std::shared_ptr<InverterAbstract> inve
     return newPowerLimit;
 }
 
-void PowerLimiterClass::commitPowerLimit(std::shared_ptr<InverterAbstract> inverter, int32_t limit, bool enablePowerProduction)
+/**
+ * updates the inverter state (power production and limit). returns true if a
+ * change to its state was requested or is pending. this function only requests
+ * one change (limit value or production on/off) at a time.
+ */
+bool PowerLimiterClass::updateInverter()
 {
+    auto reset = [this]() -> bool {
+        _oTargetPowerState = std::nullopt;
+        _oTargetPowerLimitWatts = std::nullopt;
+        _oUpdateStartMillis = std::nullopt;
+        return false;
+    };
+
+    if (nullptr == _inverter) { return reset(); }
+
+    if (!_oUpdateStartMillis.has_value()) {
+        _oUpdateStartMillis = millis();
+    }
+
+    if ((millis() - *_oUpdateStartMillis) > 30 * 1000) {
+        MessageOutput.printf("[DPL::updateInverter] timeout, "
+                "state transition pending: %s, limit pending: %s\r\n",
+                (_oTargetPowerState.has_value()?"yes":"no"),
+                (_oTargetPowerLimitWatts.has_value()?"yes":"no"));
+        return reset();
+    }
+
+    auto constexpr halfOfAllMillis = std::numeric_limits<uint32_t>::max() / 2;
+
+    auto switchPowerState = [this](bool transitionOn) -> bool {
+        // no power state transition requested at all
+        if (!_oTargetPowerState.has_value()) { return false; }
+
+        // the transition that may be started is not the one which is requested
+        if (transitionOn != *_oTargetPowerState) { return false; }
+
+        // wait for pending power command(s) to complete
+        auto lastPowerCommandState = _inverter->PowerCommand()->getLastPowerCommandSuccess();
+        if (CMD_PENDING == lastPowerCommandState) {
+            announceStatus(Status::InverterPowerCmdPending);
+            return true;
+        }
+
+        // we need to wait for statistics that are more recent than the last
+        // power update command to reliably use _inverter->isProducing()
+        auto lastPowerCommandMillis = _inverter->PowerCommand()->getLastUpdateCommand();
+        auto lastStatisticsMillis = _inverter->Statistics()->getLastUpdate();
+        if ((lastStatisticsMillis - lastPowerCommandMillis) > halfOfAllMillis) { return true; }
+
+        if (_inverter->isProducing() != *_oTargetPowerState) {
+            MessageOutput.printf("[DPL::updateInverter] %s inverter...\r\n",
+                    ((*_oTargetPowerState)?"Starting":"Stopping"));
+            _inverter->sendPowerControlRequest(*_oTargetPowerState);
+            return true;
+        }
+
+        _oTargetPowerState = std::nullopt; // target power state reached
+        return false;
+    };
+
+    // we use a lambda function here to be able to use return statements,
+    // which allows to avoid if-else-indentions and improves code readability
+    auto updateLimit = [this]() -> bool {
+        // no limit update requested at all
+        if (!_oTargetPowerLimitWatts.has_value()) { return false; }
+
+        // wait for pending limit command(s) to complete
+        auto lastLimitCommandState = _inverter->SystemConfigPara()->getLastLimitCommandSuccess();
+        if (CMD_PENDING == lastLimitCommandState) {
+            announceStatus(Status::InverterLimitPending);
+            return true;
+        }
+
+        auto maxPower = _inverter->DevInfo()->getMaxPower();
+        auto newRelativeLimit = static_cast<float>(*_oTargetPowerLimitWatts * 100) / maxPower;
+
+        // if no limit command is pending, the SystemConfigPara does report the
+        // current limit, as the answer by the inverter to a limit command is
+        // the canonical source that updates the known current limit.
+        auto currentRelativeLimit = _inverter->SystemConfigPara()->getLimitPercent();
+
+        // we assume having exclusive control over the inverter. if the last
+        // limit command was successful and sent after we started the last
+        // update cycle, we should assume *our* requested limit was set.
+        uint32_t lastLimitCommandMillis = _inverter->SystemConfigPara()->getLastUpdateCommand();
+        if ((lastLimitCommandMillis - *_oUpdateStartMillis) < halfOfAllMillis &&
+                CMD_OK == lastLimitCommandState) {
+            MessageOutput.printf("[DPL:updateInverter] actual limit is %.1f %% "
+                    "(%.0f W respectively), effective %d ms after update started, "
+                    "requested were %.1f %%\r\n",
+                    currentRelativeLimit,
+                    (currentRelativeLimit * maxPower / 100),
+                    (lastLimitCommandMillis - *_oUpdateStartMillis),
+                    newRelativeLimit);
+
+            if (std::abs(newRelativeLimit - currentRelativeLimit) > 2.0) {
+                MessageOutput.printf("[DPL:updateInverter] NOTE: expected limit of %.1f %% "
+                        "and actual limit of %.1f %% mismatch by more than 2 %%, "
+                        "is the DPL in exclusive control over the inverter?\r\n",
+                        newRelativeLimit, currentRelativeLimit);
+            }
+
+            _oTargetPowerLimitWatts = std::nullopt;
+            return false;
+        }
+
+        MessageOutput.printf("[DPL::updateInverter] sending limit of %.1f %% "
+                "(%.0f W respectively), max output is %d W\r\n",
+                newRelativeLimit, (newRelativeLimit * maxPower / 100), maxPower);
+
+        _inverter->sendActivePowerControlRequest(static_cast<float>(newRelativeLimit),
+                PowerLimitControlType::RelativNonPersistent);
+
+        _lastRequestedPowerLimit = *_oTargetPowerLimitWatts;
+        return true;
+    };
+
     // disable power production as soon as possible.
-    // setting the power limit is less important.
+    // setting the power limit is less important once the inverter is off.
-    if (!enablePowerProduction && inverter->isProducing()) {
+    if (switchPowerState(false)) { return true; }
-        MessageOutput.println("[DPL::commitPowerLimit] Stopping inverter...");
-        inverter->sendPowerControlRequest(false);
-    }
 
-    inverter->sendActivePowerControlRequest(static_cast<float>(limit),
+    if (updateLimit()) { return true; }
-            PowerLimitControlType::AbsolutNonPersistent);
 
-    _lastRequestedPowerLimit = limit;
+    // enable power production only after setting the desired limit
-    _lastPowerLimitMillis = millis();
+    if (switchPowerState(true)) { return true; }
 
-    // enable power production only after setting the desired limit,
+    return reset();
-    // such that an older, greater limit will not cause power spikes.
-    if (enablePowerProduction && !inverter->isProducing()) {
-        MessageOutput.println("[DPL::commitPowerLimit] Starting up inverter...");
-        inverter->sendPowerControlRequest(true);
-    }
 }
 
 /**
- * enforces limits and a hystersis on the requested power limit, after scaling
+ * enforces limits on the requested power limit, after scaling the power limit
- * the power limit to the ratio of total and producing inverter channels.
+ * to the ratio of total and producing inverter channels. commits the sanitized
- * commits the sanitized power limit. returns true if a limit update was
+ * power limit. returns true if an inverter update was committed, false
- * committed, false otherwise.
+ * otherwise.
  */
 bool PowerLimiterClass::setNewPowerLimit(std::shared_ptr<InverterAbstract> inverter, int32_t newPowerLimit)
 {
@@ -587,31 +649,29 @@ bool PowerLimiterClass::setNewPowerLimit(std::shared_ptr<InverterAbstract> inver
         effPowerLimit = round(effPowerLimit * static_cast<float>(dcTotalChnls) / dcProdChnls);
     }
 
-    effPowerLimit = std::min<int32_t>(effPowerLimit, inverter->DevInfo()->getMaxPower());
+    // early in the loop we make it a pre-requisite that this
+    // value is non-zero, so we can assume it to be valid.
+    auto maxPower = inverter->DevInfo()->getMaxPower();
 
-    // Check if the new value is within the limits of the hysteresis
+    effPowerLimit = std::min<int32_t>(effPowerLimit, maxPower);
-    auto diff = std::abs(effPowerLimit - _lastRequestedPowerLimit);
+
+    float currentLimitPercent = inverter->SystemConfigPara()->getLimitPercent();
+    auto currentLimitAbs = static_cast<int32_t>(currentLimitPercent * maxPower / 100);
+    auto diff = std::abs(currentLimitAbs - effPowerLimit);
     auto hysteresis = config.PowerLimiter.TargetPowerConsumptionHysteresis;
 
-    // (re-)send power limit in case the last was sent a long time ago. avoids
-    // staleness in case a power limit update was not received by the inverter.
-    auto ageMillis = millis() - _lastPowerLimitMillis;
-
-    if (diff < hysteresis && ageMillis < 60 * 1000) {
-        if (_verboseLogging) {
-            MessageOutput.printf("[DPL::setNewPowerLimit] requested: %d W, last limit: %d W, diff: %d W, hysteresis: %d W, age: %ld ms\r\n",
-                    newPowerLimit, _lastRequestedPowerLimit, diff, hysteresis, ageMillis);
-        }
-        return false;
-    }
-
     if (_verboseLogging) {
-        MessageOutput.printf("[DPL::setNewPowerLimit] requested: %d W, (re-)sending limit: %d W\r\n",
+        MessageOutput.printf("[DPL::setNewPowerLimit] calculated: %d W, "
-                newPowerLimit, effPowerLimit);
+                "requesting: %d W, reported: %d W, diff: %d W, hysteresis: %d W\r\n",
+                newPowerLimit, effPowerLimit, currentLimitAbs, diff, hysteresis);
     }
 
-    commitPowerLimit(inverter, effPowerLimit, true);
+    if (diff > hysteresis) {
-    return true;
+        _oTargetPowerLimitWatts = effPowerLimit;
+    }
+
+    _oTargetPowerState = true;
+    return updateInverter();
 }
 
 int32_t PowerLimiterClass::getSolarChargePower()