Start addressing comments from issue project-chip#34565

Author: PeterC1965

examples/all-clusters-app/all-clusters-common/include/WhmDelegate.h

@@ -104,8 +104,8 @@ class WaterHeaterManagementDelegate : public WaterHeaterManagement::Delegate
      * SHALL be rejected with appropriate error.
      */
     Protocols::InteractionModel::Status HandleBoost(uint32_t duration, Optional<bool> oneShot, Optional<bool> emergencyBoost,
-                                                    Optional<int16_t> temporarySetpoint, Optional<chip::Percent> targetPercentage,
-                                                    Optional<chip::Percent> targetReheat) override;
+                                                    Optional<int16_t> temporarySetpoint, Optional<Percent> targetPercentage,
+                                                    Optional<Percent> targetReheat) override;
 
     /**
      * @brief Delegate should implement a handler to cancel a boost command.
@@ -162,9 +162,12 @@ class WaterHeaterManagementDelegate : public WaterHeaterManagement::Delegate
     void SetTargetWaterTemperature(uint16_t targetWaterTemperature);
 
     /**
-     * @brief Determine whether the heating sources need to be turned on or off
+     * @brief Determine whether the heating sources need to be turned on or off or left unchanged.
+     *
+     * @return Success if the heating was successfully turned on or off or left unchanged otherwise an error
+     *         code is returned if turning the heating on or off failed.
      */
-    Protocols::InteractionModel::Status CheckIfHeatNeedsToBeTurnedOnOrOff();
+    Protocols::InteractionModel::Status ChangeHeatingIfNecessary();
 
     /**
      * @brief Static timer callback for when Boost timer expires.
@@ -194,9 +197,34 @@ class WaterHeaterManagementDelegate : public WaterHeaterManagement::Delegate
      * @param replacedWaterTemperature  The temperature of the
      * percentageReplaced water.
      */
-    void DrawOffHotWater(chip::Percent percentageReplaced, uint16_t replacedWaterTemperature);
+    void DrawOffHotWater(Percent percentageReplaced, uint16_t replacedWaterTemperature);
+
+    /**
+     * Set the temperature of the cold water that fills the tank as the hot water
+     * is drawn off.
+     *
+     * @param oldWaterTemperature  The cold water temperature in 100th of a C
+     */
+    void SetColdWaterTemperature(uint16_t coldWaterTemperature);
 
 private:
+    /**
+     * Return the target temperature.
+     * If a boost command is in progress and has a mBoostTemporarySetpoint value use that as the
+     * target temperature otherwise use the temperature set via SetTargetWaterTemperature().
+     *
+     * @return the target temperature
+     */
+    uint16_t GetActiveTargetWaterTemperature() const;
+
+    /**
+     * @brief Calculate the percentage of the water in the tank at the target
+     *        temperature.
+     *
+     * @return  Percentage of water at the target temperature
+     */
+    uint8_t CalculateTankPercentage() const;
+
     /**
      * @brief Determine whether heating needs to be turned on or off or left as
      * is.
@@ -228,8 +256,8 @@ class WaterHeaterManagementDelegate : public WaterHeaterManagement::Delegate
     // Actual water temperature in 100ths of a C
     uint16_t mWaterTemperature;
 
-    // The % of water at temperature mReplacedWaterTemperature
-    uint16_t mReplacedWaterTemperature;
+    // The cold water temperature in 100ths of a C
+    uint16_t mColdWaterTemperature;
 
     // Boost command parameters
 
@@ -253,7 +281,7 @@ class WaterHeaterManagementDelegate : public WaterHeaterManagement::Delegate
     // amount of water that SHALL be heated by this Boost command before the
     // heater is switched off. This field is optional, however it SHALL be
     // included if the TargetReheat field is included.
-    Optional<chip::Percent> mBoostTargetPercentage;
+    Optional<Percent> mBoostTargetPercentage;
 
     // If the tank supports the TankPercent feature, and the heating by this
     // Boost command has ceased because the TargetPercentage of the water in the
@@ -266,7 +294,7 @@ class WaterHeaterManagementDelegate : public WaterHeaterManagement::Delegate
     // heat back up to 80% of hot water. If this field and the OneShot field
     // were both omitted, heating would begin again after any water draw which
     // reduced the TankPercentage below 80%.
-    Optional<chip::Percent> mBoostTargetReheat;
+    Optional<Percent> mBoostTargetReheat;
 
     // Track whether the water temperature has reached the water temperature
     // specified in the boost command. Used in conjunction with the boost

examples/all-clusters-app/all-clusters-common/src/WhmDelegateImpl.cpp

@@ -21,6 +21,8 @@
 #include <WhmManufacturer.h>
 #include <water-heater-mode.h>
 
+#include <algorithm>
+
 using namespace chip;
 using namespace chip::app;
 using namespace chip::app::Clusters;
@@ -29,10 +31,11 @@ using namespace chip::app::Clusters::WaterHeaterManagement;
 using Protocols::InteractionModel::Status;
 
 WaterHeaterManagementDelegate::WaterHeaterManagementDelegate(EndpointId clustersEndpoint) :
-    mpWhmInstance(nullptr), mpWhmManufacturer(nullptr), mWaterTemperature(0), mReplacedWaterTemperature(0),
-    mBoostTargetTemperatureReached(false), mTankVolume(0), mEstimatedHeatRequired(0), mTankPercentage(0),
-    mBoostState(BoostStateEnum::kInactive)
-{}
+    mpWhmInstance(nullptr), mpWhmManufacturer(nullptr), mTargetWaterTemperature(0), mWaterTemperature(0),
+    mColdWaterTemperature(0), mBoostTargetTemperatureReached(false), mTankVolume(0),
+    mEstimatedHeatRequired(0), mTankPercentage(0), mBoostState(BoostStateEnum::kInactive)
+{
+}
 
 void WaterHeaterManagementDelegate::SetWaterHeaterManagementInstance(WaterHeaterManagement::Instance & instance)
 {
@@ -128,8 +131,6 @@ void WaterHeaterManagementDelegate::SetTankPercentage(Percent tankPercentage)
         {
             mTankPercentage = tankPercentage;
 
-            CheckIfHeatNeedsToBeTurnedOnOrOff();
-
             MatterReportingAttributeChangeCallback(mEndpointId, WaterHeaterManagement::Id, Attributes::TankPercentage::Id);
         }
     }
@@ -201,13 +202,22 @@ Status WaterHeaterManagementDelegate::HandleBoost(uint32_t durationS, Optional<b
         ChipLogError(AppServer, "HandleBoost: mpWhmManufacturer == nullptr");
     }
 
-    if (status == Status::Success)
+    VerifyOrReturnValue(status == Status::Success, status);
+
+    // See if the heat needs to be turned on or off as a result of this boost command
+    status = ChangeHeatingIfNecessary();
+    VerifyOrReturnValue(status == Status::Success, status);
+
+    // Now generate a BoostStarted event
+    err = GenerateBoostStartedEvent(durationS, oneShot, emergencyBoost, temporarySetpoint, targetPercentage, targetReheat);
+    if (err != CHIP_NO_ERROR)
     {
-        // See if the heat needs to be turned on or off as a result of this boost command
-        status = CheckIfHeatNeedsToBeTurnedOnOrOff();
+        ChipLogError(AppServer, "HandleBoost: Failed to send BoostStarted event: %" CHIP_ERROR_FORMAT, err.Format());
+
+        return Status::Failure;
     }
 
-    return status;
+    return Status::Success;
 }
 
 void WaterHeaterManagementDelegate::BoostTimerExpiry(System::Layer * systemLayer, void * delegate)
@@ -236,7 +246,15 @@ void WaterHeaterManagementDelegate::HandleBoostTimerExpiry()
         ChipLogError(AppServer, "HandleBoostTimerExpiry: mpWhmManufacturer == nullptr");
     }
 
-    CheckIfHeatNeedsToBeTurnedOnOrOff();
+    // Note ChangeHeatingIfNecessary can generate a BoostEnded event but only if the boost state is kActive
+    // which it cannot be when called from here.
+    ChangeHeatingIfNecessary();
+
+    CHIP_ERROR err = GenerateBoostEndedEvent();
+    if (err != CHIP_NO_ERROR)
+    {
+        ChipLogError(AppServer, "HandleBoostTimerExpiry: Failed to send BoostEnded event: %" CHIP_ERROR_FORMAT, err.Format());
+    }
 }
 
 /**
@@ -260,8 +278,18 @@ Status WaterHeaterManagementDelegate::HandleCancelBoost()
         Status status = mpWhmManufacturer->BoostCommandCancelled();
         VerifyOrReturnValue(status == Status::Success, status);
 
-        status = CheckIfHeatNeedsToBeTurnedOnOrOff();
+        // Note ChangeHeatingIfNecessary can generate a BoostEnded event but only if the boost state is kActive
+        // which it cannot be when called from here.
+        status = ChangeHeatingIfNecessary();
         VerifyOrReturnValue(status == Status::Success, status);
+
+        CHIP_ERROR err = GenerateBoostEndedEvent();
+        if (err != CHIP_NO_ERROR)
+        {
+            ChipLogError(AppServer, "HandleCancelBoost: Failed to send BoostEnded event: %" CHIP_ERROR_FORMAT, err.Format());
+
+            return Status::Failure;
+        }
     }
 
     return Status::Success;
@@ -273,59 +301,75 @@ Status WaterHeaterManagementDelegate::HandleCancelBoost()
  *
  *********************************************************************************/
 
+uint16_t WaterHeaterManagementDelegate::GetActiveTargetWaterTemperature() const
+{
+    // Determine the target temperature. If a boost command is in progress and has a mBoostTemporarySetpoint value use that as the
+    // target temperature.
+    // Note, in practise the actual heating is likely to be controlled by the thermostat's occupiedHeatingSetpoint most of the
+    // time, and the TemporarySetpoint (if not null) would be overiding the thermostat's occupiedHeatingSetpoint.
+    // However, this code doesn't rely upon the thermostat cluster.
+    uint16_t targetTemperature = (mBoostState == BoostStateEnum::kActive && mBoostTemporarySetpoint.HasValue())
+        ? static_cast<uint16_t>(mBoostTemporarySetpoint.Value())
+        : mTargetWaterTemperature;
+
+    return targetTemperature;
+}
+
+uint8_t WaterHeaterManagementDelegate::CalculateTankPercentage() const
+{
+    int16_t tankPercentage = 100 * (mWaterTemperature - mColdWaterTemperature) / (GetActiveTargetWaterTemperature() - mColdWaterTemperature);
+
+    tankPercentage = std::min(tankPercentage, static_cast<int16_t>(100));
+    tankPercentage = std::max(tankPercentage, static_cast<int16_t>(0));
+
+    return static_cast<uint8_t>(tankPercentage);
+}
+
+void WaterHeaterManagementDelegate::SetColdWaterTemperature(uint16_t coldWaterTemperature)
+{
+    mColdWaterTemperature = coldWaterTemperature;
+}
+
 void WaterHeaterManagementDelegate::SetWaterTemperature(uint16_t waterTemperature)
 {
     mWaterTemperature = waterTemperature;
 
     if (mpWhmInstance != nullptr && mpWhmInstance->HasFeature(Feature::kTankPercent))
     {
-        mTankPercentage = 100;
+        // Recalculate the tankPercentage as the waterTemperature has changed
+        SetTankPercentage(CalculateTankPercentage());
     }
 
     // See if the heat needs to be turned on or off
-    CheckIfHeatNeedsToBeTurnedOnOrOff();
+    ChangeHeatingIfNecessary();
 }
 
 void WaterHeaterManagementDelegate::SetTargetWaterTemperature(uint16_t targetWaterTemperature)
 {
     mTargetWaterTemperature = targetWaterTemperature;
 
     // See if the heat needs to be turned on or off
-    CheckIfHeatNeedsToBeTurnedOnOrOff();
+    ChangeHeatingIfNecessary();
 }
 
 void WaterHeaterManagementDelegate::DrawOffHotWater(Percent percentageReplaced, uint16_t replacedWaterTemperature)
 {
-    // Only supported in the kTankPercent is supported.
+    // First calculate the new average water temperature
+    mWaterTemperature = (mWaterTemperature * (100 - percentageReplaced) + replacedWaterTemperature * percentageReplaced) / 100;
+
     // Replaces percentageReplaced% of the water in the tank with water of a temperature replacedWaterTemperature
+    // Only supported if the kTankPercent feature is supported.
     if (mpWhmInstance != nullptr && mpWhmInstance->HasFeature(Feature::kTankPercent))
     {
-        // See if all of the water has now been replaced with replacedWaterTemperature
-        if (mTankPercentage >= percentageReplaced)
-        {
-            mTankPercentage = static_cast<Percent>(mTankPercentage - percentageReplaced);
-        }
-        else
-        {
-            mTankPercentage = 0;
-        }
-
-        mReplacedWaterTemperature = replacedWaterTemperature;
+        SetTankPercentage(CalculateTankPercentage());
 
-        CheckIfHeatNeedsToBeTurnedOnOrOff();
+        ChangeHeatingIfNecessary();
     }
 }
 
 bool WaterHeaterManagementDelegate::HasWaterTemperatureReachedTarget() const
 {
-    // Determine the target temperature. If a boost command is in progress and has a mBoostTemporarySetpoint value use that as the
-    // target temperature.
-    // Note, in practise the actual heating is likely to be controlled by the thermostat's occupiedHeatingSetpoint most of the
-    // time, and the TemporarySetpoint (if not null) would be overiding the thermostat's occupiedHeatingSetpoint.
-    // However, this code doesn't rely upon the thermostat cluster.
-    uint16_t targetTemperature = (mBoostState == BoostStateEnum::kActive && mBoostTemporarySetpoint.HasValue())
-        ? static_cast<uint16_t>(mBoostTemporarySetpoint.Value())
-        : mTargetWaterTemperature;
+    uint16_t targetTemperature = GetActiveTargetWaterTemperature();
 
     VerifyOrReturnValue(mWaterTemperature >= targetTemperature, false);
 
@@ -357,7 +401,7 @@ bool WaterHeaterManagementDelegate::HasWaterTemperatureReachedTarget() const
     return true;
 }
 
-Status WaterHeaterManagementDelegate::CheckIfHeatNeedsToBeTurnedOnOrOff()
+Status WaterHeaterManagementDelegate::ChangeHeatingIfNecessary()
 {
     VerifyOrReturnError(mpWhmManufacturer != nullptr, Status::InvalidInState);
 
@@ -384,6 +428,12 @@ Status WaterHeaterManagementDelegate::CheckIfHeatNeedsToBeTurnedOnOrOff()
             mBoostEmergencyBoost.ClearValue();
 
             status = mpWhmManufacturer->BoostCommandCancelled();
+
+            CHIP_ERROR err = GenerateBoostEndedEvent();
+            if (err != CHIP_NO_ERROR)
+            {
+                ChipLogError(AppServer, "ChangeHeatingIfNecessary: Failed to send BoostEnded event: %" CHIP_ERROR_FORMAT, err.Format());
+            }
         }
 
         // Turn the heating off
@@ -457,5 +507,6 @@ Status WaterHeaterManagementDelegate::SetWaterHeaterMode(uint8_t modeValue)
         return status;
     }
 
-    return CheckIfHeatNeedsToBeTurnedOnOrOff();
+    return ChangeHeatingIfNecessary();
 }
+

examples/all-clusters-app/all-clusters-common/src/WhmManufacturer.cpp

@@ -45,7 +45,7 @@ CHIP_ERROR WhmManufacturer::Init()
 
     dg->SetHeaterTypes(BitMask<WaterHeaterHeatSourceBitmap>(WaterHeaterHeatSourceBitmap::kImmersionElement1));
     dg->SetHeatDemand(BitMask<WaterHeaterHeatSourceBitmap>(WaterHeaterHeatSourceBitmap::kImmersionElement1));
-    dg->SetEstimatedHeatRequired(10000);
+    dg->SetColdWaterTemperature(2000);
 
     return CHIP_NO_ERROR;
 }
@@ -65,38 +65,30 @@ BitMask<WaterHeaterHeatSourceBitmap> WhmManufacturer::DetermineHeatingSources()
     }
 
     // A list of valid heaterTypes
-    uint8_t waterHeaterTypeValues[] = {
-        static_cast<uint8_t>(WaterHeaterHeatSourceBitmap::kImmersionElement1),
-        static_cast<uint8_t>(WaterHeaterHeatSourceBitmap::kImmersionElement2),
-        static_cast<uint8_t>(WaterHeaterHeatSourceBitmap::kHeatPump),
-        static_cast<uint8_t>(WaterHeaterHeatSourceBitmap::kBoiler),
-        static_cast<uint8_t>(WaterHeaterHeatSourceBitmap::kOther),
-    };
-
-    // The corresponding list of valid headerDemands
-    uint8_t waterHeaterDemandValues[] = {
-        static_cast<uint8_t>(WaterHeaterHeatSourceBitmap::kImmersionElement1),
-        static_cast<uint8_t>(WaterHeaterHeatSourceBitmap::kImmersionElement2),
-        static_cast<uint8_t>(WaterHeaterHeatSourceBitmap::kHeatPump),
-        static_cast<uint8_t>(WaterHeaterHeatSourceBitmap::kBoiler),
-        static_cast<uint8_t>(WaterHeaterHeatSourceBitmap::kOther),
+    BitMask<WaterHeaterHeatSourceBitmap> waterHeaterTypeValues[] = {
+        BitMask<WaterHeaterHeatSourceBitmap>(WaterHeaterHeatSourceBitmap::kImmersionElement1),
+        BitMask<WaterHeaterHeatSourceBitmap>(WaterHeaterHeatSourceBitmap::kImmersionElement2),
+        BitMask<WaterHeaterHeatSourceBitmap>(WaterHeaterHeatSourceBitmap::kHeatPump),
+        BitMask<WaterHeaterHeatSourceBitmap>(WaterHeaterHeatSourceBitmap::kBoiler),
+        BitMask<WaterHeaterHeatSourceBitmap>(WaterHeaterHeatSourceBitmap::kOther),
     };
 
     // Iterate across the valid waterHeaterTypes seeing which heating sources are available based on heaterTypes.
     // Set the corresponding bit in the heaterDemand bitmap.
-    BitMask<WaterHeaterHeatSourceBitmap> heaterTypes = dg->GetHeaterTypes();
+    BitMask<WaterHeaterHeatSourceBitmap> waterHeaterTypes = dg->GetHeaterTypes();
+
+    BitMask<WaterHeaterHeatSourceBitmap> waterHeaterDemand;
 
-    uint8_t heaterDemandMask = 0;
-    for (uint16_t idx = 0; idx < static_cast<uint16_t>(sizeof(waterHeaterTypeValues) / sizeof(waterHeaterTypeValues[0])); idx++)
+    for (auto & waterHeaterType : waterHeaterTypeValues)
     {
         // Is this heating source being used?
-        if (heaterTypes.Raw() & waterHeaterTypeValues[idx])
+        if (waterHeaterTypes.Has(waterHeaterType))
         {
-            heaterDemandMask |= waterHeaterDemandValues[idx];
+            waterHeaterDemand.Set(waterHeaterType);
         }
     }
 
-    return BitMask<WaterHeaterHeatSourceBitmap>(heaterDemandMask);
+    return waterHeaterDemand;
 }
 
 Status WhmManufacturer::TurnHeatingOn(bool emergencyBoost)