Address review comments from JamesH

Author: PeterC1965

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

@@ -19,7 +19,6 @@
 #pragma once
 
 #include <app-common/zap-generated/cluster-objects.h>
-#include <app/clusters/mode-base-server/mode-base-server.h>
 #include <app/clusters/water-heater-management-server/water-heater-management-server.h>
 
 #include <protocols/interaction_model/StatusCode.h>
@@ -34,7 +33,7 @@ using ModeTagStructType = detail::Structs::ModeTagStruct::Type;
 class WhmManufacturer;
 
 // This is an application level delegate to handle operational state commands according to the specific business logic.
-class WaterHeaterManagementDelegate : public WaterHeaterManagement::Delegate, public ModeBase::Delegate
+class WaterHeaterManagementDelegate : public WaterHeaterManagement::Delegate
 {
 public:
     WaterHeaterManagementDelegate(EndpointId clustersEndpoint);
@@ -47,29 +46,33 @@ class WaterHeaterManagementDelegate : public WaterHeaterManagement::Delegate, pu
 
     /*********************************************************************************
      *
-     * Methods implementing the WaterHeaterManagement::Delegate interace
+     * Methods implementing the WaterHeaterManagement::Delegate interface
      *
      *********************************************************************************/
 
     /**
      * @brief Delegate should implement a handler to start boosting the water temperature as required.
-     *        Upon receipt, the Water Heater SHALL transition into the BOOST state, which SHALL cause the water in the tank (or the
-     * TargetPercentage of the water, if included) to be heated towards the set point (or the TemporarySetpoint, if included), which
-     * in turn may cause a call for heat, even if the mode is OFF, or is TIMED and it is during one of the Off periods.
+     *        Upon receipt, the Water Heater SHALL transition into the BOOST state, which SHALL cause the
+     *        water in the tank (or the TargetPercentage of the water, if included) to be heated towards
+     *        the set point (or the TemporarySetpoint, if included), which in turn may cause a call for heat,
+     *        even if the mode is OFF, or is TIMED and it is during one of the Off periods.
      *
-     * @param duration  Indicates the time period in seconds for which the BOOST state is activated before it automatically reverts
-     * to the previous mode (e.g. OFF, MANUAL or TIMED).
-     * @param oneShot  Indicates whether the BOOST state should be automatically canceled once the hot water has first reached the
-     * set point temperature (or the TemporarySetpoint temperature, if specified) for the TargetPercentage (if specified).
-     * @param emergencyBoost  Indicates that the consumer wants the water to be heated as quickly as practicable. This MAY cause
-     * multiple heat sources to be activated (e.g. a heat pump and direct electric heating element).
-     * @param temporarySetpoint  Indicates the target temperature to which to heat the hot water for this Boost command. It SHALL be
-     * used instead of the normal set point temperature whilst the BOOST state is active.
-     * @param targetPercentage  If the tank supports the TankPercent feature, this field indicates the amount of water that SHALL be
-     * heated by this Boost command before the heater is switched off.
-     * @param targetReheat  If the tank supports the TankPercent feature, and the heating by this Boost command has ceased because
-     * the TargetPercentage of the water in the tank has been heated to the set point (or TemporarySetpoint if included), this field
-     * indicates the percentage to which the hot water in the tank SHALL be allowed to fall before again beginning to reheat it.
+     * @param duration  Indicates the time period in seconds for which the BOOST state is activated before it
+     *                  automatically reverts to the previous mode (e.g. OFF, MANUAL or TIMED).
+     * @param oneShot  Indicates whether the BOOST state should be automatically canceled once the hot water has
+     *                 first reached the set point temperature (or the TemporarySetpoint temperature, if specified)
+     *                 for the TargetPercentage (if specified).
+     * @param emergencyBoost  Indicates that the consumer wants the water to be heated as quickly as practicable.
+     *                        This MAY cause multiple heat sources to be activated (e.g. a heat pump and direct
+     *                        electric heating element).
+     * @param temporarySetpoint  Indicates the target temperature to which to heat the hot water for this Boost command.
+     *                           It SHALL be used instead of the normal set point temperature whilst the BOOST state is active.
+     * @param targetPercentage  If the tank supports the TankPercent feature, this field indicates the amount of water
+     *                          that SHALL be heated by this Boost command before the heater is switched off.
+     * @param targetReheat  If the tank supports the TankPercent feature, and the heating by this Boost command has ceased
+     *                      because the TargetPercentage of the water in the tank has been heated to the set point (or
+     *                      TemporarySetpoint if included), this field indicates the percentage to which the hot water in
+     *                      the tank SHALL be allowed to fall before again beginning to reheat it.
      *
      * @return  Success if the boost command is accepted; otherwise the command SHALL be rejected with appropriate error.
      */
@@ -104,57 +107,6 @@ class WaterHeaterManagementDelegate : public WaterHeaterManagement::Delegate, pu
     void SetTankPercentage(Percent tankPercentage);
     void SetBoostState(BoostStateEnum boostState);
 
-    /*********************************************************************************
-     *
-     * Methods implementing the ModeBase::Delegate interface
-     *
-     *********************************************************************************/
-
-    CHIP_ERROR Init() override;
-
-    /**
-     * Handle application logic when the mode is changing.
-     *
-     * @param mode The new mode that the device is requested to transition to.
-     * @param response A reference to a response that will be sent to the client. The contents of which con be modified by the
-     * application.
-     */
-    void HandleChangeToMode(uint8_t mode, ModeBase::Commands::ChangeToModeResponse::Type & response) override;
-
-    /**
-     * Get the mode label of the Nth mode in the list of modes.
-     *
-     * @param modeIndex The index of the mode to be returned. It is assumed that modes are indexable from 0 and with no gaps.
-     * @param label A reference to the mutable char span which will be mutated to receive the label on success. Use
-     * CopyCharSpanToMutableCharSpan to copy into the MutableCharSpan.
-     *
-     * @return Returns a CHIP_NO_ERROR if there was no error and the label was returned successfully.
-     * CHIP_ERROR_PROVIDER_LIST_EXHAUSTED if the modeIndex in beyond the list of available labels.
-     */
-    CHIP_ERROR GetModeLabelByIndex(uint8_t modeIndex, MutableCharSpan & label) override;
-
-    /**
-     * Get the mode value of the Nth mode in the list of modes.
-     *
-     * @param modeIndex The index of the mode to be returned. It is assumed that modes are indexable from 0 and with no gaps.
-     * @param value a reference to the uint8_t variable that is to contain the mode value.
-     *
-     * @return Returns a CHIP_NO_ERROR if there was no error and the value was returned successfully.
-     * CHIP_ERROR_PROVIDER_LIST_EXHAUSTED if the modeIndex in beyond the list of available values.
-     */
-    CHIP_ERROR GetModeValueByIndex(uint8_t modeIndex, uint8_t & value) override;
-
-    /**
-     * Get the mode tags of the Nth mode in the list of modes.
-     * @param modeIndex The index of the mode to be returned. It is assumed that modes are indexable from 0 and with no gaps.
-     * @param tags a reference to an existing and initialised buffer that is to contain the mode tags. std::copy can be used
-     * to copy into the buffer.
-     *
-     * @return Returns a CHIP_NO_ERROR if there was no error and the mode tags were returned successfully.
-     * CHIP_ERROR_PROVIDER_LIST_EXHAUSTED if the modeIndex in beyond the list of available mode tags.
-     */
-    CHIP_ERROR GetModeTagsByIndex(uint8_t modeIndex, DataModel::List<ModeTagStructType> & tags) override;
-
     /*********************************************************************************
      *
      * WaterHeaterManagementDelegate specific methods
@@ -293,9 +245,6 @@ class WaterHeaterManagementDelegate : public WaterHeaterManagement::Delegate, pu
      *
      *********************************************************************************/
 
-    // Access to the Water Heater Mode instance
-    ModeBase::Instance mWaterHeaterModeInstance;
-
     // This attribute SHALL indicate the methods to call for heat that the controller supports. If a bit is set then the controller
     // supports the corresponding method.
     BitMask<WaterHeaterTypeBitmap> mHeaterTypes;
@@ -319,28 +268,6 @@ class WaterHeaterManagementDelegate : public WaterHeaterManagement::Delegate, pu
 
     // This attribute SHALL indicate if the BOOST state, as triggered by a Boost command, is currently active.
     BoostStateEnum mBoostState;
-
-    /*********************************************************************************
-     *
-     * Member variables implementing the ModeBase::Delegate interface
-     *
-     *********************************************************************************/
-
-    ModeTagStructType modeTagsOff[1]    = { { .value = to_underlying(WaterHeaterMode::ModeTag::kOff) } };
-    ModeTagStructType modeTagsManual[1] = { { .value = to_underlying(WaterHeaterMode::ModeTag::kManual) } };
-    ModeTagStructType modeTagsTimed[1]  = { { .value = to_underlying(WaterHeaterMode::ModeTag::kTimed) } };
-
-    const detail::Structs::ModeOptionStruct::Type kModeOptions[3] = {
-        detail::Structs::ModeOptionStruct::Type{ .label    = CharSpan::fromCharString("Off"),
-                                                 .mode     = ModeOff,
-                                                 .modeTags = DataModel::List<const ModeTagStructType>(modeTagsOff) },
-        detail::Structs::ModeOptionStruct::Type{ .label    = CharSpan::fromCharString("Manual"),
-                                                 .mode     = ModeManual,
-                                                 .modeTags = DataModel::List<const ModeTagStructType>(modeTagsManual) },
-        detail::Structs::ModeOptionStruct::Type{ .label    = CharSpan::fromCharString("Timed"),
-                                                 .mode     = ModeTimed,
-                                                 .modeTags = DataModel::List<const ModeTagStructType>(modeTagsTimed) }
-    };
 };
 
 } // namespace WaterHeaterManagement

examples/all-clusters-app/all-clusters-common/include/water-heater-mode.h

@@ -0,0 +1,73 @@
+/*
+ *
+ *    Copyright (c) 2023 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#pragma once
+
+#include <app/clusters/mode-base-server/mode-base-server.h>
+#include <app/util/config.h>
+#include <cstring>
+#include <utility>
+
+namespace chip {
+namespace app {
+namespace Clusters {
+
+namespace WaterHeaterMode {
+
+constexpr uint8_t ModeOff    = 0;
+constexpr uint8_t ModeManual = 1;
+constexpr uint8_t ModeTimed  = 2;
+
+/// This is an application level delegate to handle WaterHeaterMode commands according to the specific business logic.
+class ExampleWaterHeaterModeDelegate : public ModeBase::Delegate
+{
+private:
+    using ModeTagStructType             = detail::Structs::ModeTagStruct::Type;
+    ModeTagStructType modeTagsOff[1]    = { { .value = to_underlying(ModeTag::kOff) } };
+    ModeTagStructType modeTagsManual[1] = { { .value = to_underlying(ModeTag::kManual) } };
+    ModeTagStructType modeTagsTimed[1]  = { { .value = to_underlying(ModeTag::kTimed) } };
+
+    const detail::Structs::ModeOptionStruct::Type kModeOptions[3] = {
+        detail::Structs::ModeOptionStruct::Type{
+            .label = "Off"_span, .mode = ModeOff, .modeTags = DataModel::List<const ModeTagStructType>(modeTagsOff) },
+        detail::Structs::ModeOptionStruct::Type{
+            .label = "Manual"_span, .mode = ModeManual, .modeTags = DataModel::List<const ModeTagStructType>(modeTagsManual) },
+        detail::Structs::ModeOptionStruct::Type{
+            .label = "Timed"_span, .mode = ModeTimed, .modeTags = DataModel::List<const ModeTagStructType>(modeTagsTimed) }
+    };
+
+    CHIP_ERROR Init() override;
+    void HandleChangeToMode(uint8_t mode, ModeBase::Commands::ChangeToModeResponse::Type & response) override;
+
+    CHIP_ERROR GetModeLabelByIndex(uint8_t modeIndex, MutableCharSpan & label) override;
+    CHIP_ERROR GetModeValueByIndex(uint8_t modeIndex, uint8_t & value) override;
+    CHIP_ERROR GetModeTagsByIndex(uint8_t modeIndex, DataModel::List<ModeTagStructType> & tags) override;
+
+public:
+    ~ExampleWaterHeaterModeDelegate() override = default;
+};
+
+ModeBase::Instance * Instance();
+
+void Shutdown();
+
+} // namespace WaterHeaterMode
+
+} // namespace Clusters
+} // namespace app
+} // namespace chip

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

@@ -17,8 +17,10 @@
  */
 #include <app/clusters/water-heater-management-server/water-heater-management-server.h>
 
+
 #include <WhmDelegate.h>
 #include <WhmManufacturer.h>
+#include <water-heater-mode.h>
 
 using namespace chip;
 using namespace chip::app;
@@ -29,11 +31,9 @@ using Protocols::InteractionModel::Status;
 
 WaterHeaterManagementDelegate::WaterHeaterManagementDelegate(EndpointId clustersEndpoint) :
     mpWhmInstance(nullptr), mpWhmManufacturer(nullptr), mBoostTargetTemperatureReached(false),
-    mWaterHeaterModeInstance(this, clustersEndpoint, WaterHeaterMode::Id, 0), mTankVolume(0), mEstimatedHeatRequired(0),
+    mTankVolume(0), mEstimatedHeatRequired(0),
     mTankPercentage(0), mBoostState(BoostStateEnum::kInactive)
 {
-    // Initialise the WaterHeaterMode instance
-    mWaterHeaterModeInstance.Init();
 }
 
 void WaterHeaterManagementDelegate::SetWaterHeaterManagementInstance(WaterHeaterManagement::Instance & instance)
@@ -287,56 +287,6 @@ Status WaterHeaterManagementDelegate::HandleCancelBoost()
  *
  *********************************************************************************/
 
-CHIP_ERROR WaterHeaterManagementDelegate::Init()
-{
-    return CHIP_NO_ERROR;
-}
-
-void WaterHeaterManagementDelegate::HandleChangeToMode(uint8_t NewMode, ModeBase::Commands::ChangeToModeResponse::Type & response)
-{
-    response.status = to_underlying(ModeBase::StatusCode::kSuccess);
-}
-
-CHIP_ERROR WaterHeaterManagementDelegate::GetModeLabelByIndex(uint8_t modeIndex, chip::MutableCharSpan & label)
-{
-    if (modeIndex >= ArraySize(kModeOptions))
-    {
-        return CHIP_ERROR_PROVIDER_LIST_EXHAUSTED;
-    }
-
-    return chip::CopyCharSpanToMutableCharSpan(kModeOptions[modeIndex].label, label);
-}
-
-CHIP_ERROR WaterHeaterManagementDelegate::GetModeValueByIndex(uint8_t modeIndex, uint8_t & value)
-{
-    if (modeIndex >= ArraySize(kModeOptions))
-    {
-        return CHIP_ERROR_PROVIDER_LIST_EXHAUSTED;
-    }
-
-    value = kModeOptions[modeIndex].mode;
-
-    return CHIP_NO_ERROR;
-}
-
-CHIP_ERROR WaterHeaterManagementDelegate::GetModeTagsByIndex(uint8_t modeIndex, DataModel::List<ModeTagStructType> & tags)
-{
-    if (modeIndex >= ArraySize(kModeOptions))
-    {
-        return CHIP_ERROR_PROVIDER_LIST_EXHAUSTED;
-    }
-
-    if (tags.size() < kModeOptions[modeIndex].modeTags.size())
-    {
-        return CHIP_ERROR_INVALID_ARGUMENT;
-    }
-
-    std::copy(kModeOptions[modeIndex].modeTags.begin(), kModeOptions[modeIndex].modeTags.end(), tags.begin());
-    tags.reduce_size(kModeOptions[modeIndex].modeTags.size());
-
-    return CHIP_NO_ERROR;
-}
-
 /*********************************************************************************
  *
  * WaterHeaterManagementDelegate specific methods
@@ -431,9 +381,9 @@ Status WaterHeaterManagementDelegate::CheckIfHeatNeedsToBeTurnedOnOrOff()
 
     if (!HasWaterTemperatureReachedTarget())
     {
-        uint8_t mode = mInstance->GetCurrentMode();
+        uint8_t mode = WaterHeaterMode::Instance()->GetCurrentMode();
 
-        // The water in the tank is not at the target temperature. See if we heating is currently off
+        // The water in the tank is not at the target temperature. See if heating is currently off
         if (mHeatDemand.Raw() == 0)
         {
             // Need to track whether the water temperature has reached the target temperature for the boost
@@ -524,13 +474,13 @@ Status WaterHeaterManagementDelegate::CheckIfHeatNeedsToBeTurnedOnOrOff()
 
 void WaterHeaterManagementDelegate::SetWaterHeaterMode(uint8_t modeValue)
 {
-    if (!mInstance->IsSupportedMode(modeValue))
+    if (!WaterHeaterMode::Instance()->IsSupportedMode(modeValue))
     {
         ChipLogError(AppServer, "SetWaterHeaterMode bad mode");
         return;
     }
 
-    Status status = mInstance->UpdateCurrentMode(modeValue);
+    Status status = WaterHeaterMode::Instance()->UpdateCurrentMode(modeValue);
     if (status != Status::Success)
     {
         ChipLogError(AppServer, "SetWaterHeaterMode updateMode failed");

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

@@ -74,8 +74,7 @@ CHIP_ERROR WhmInit()
     /* Manufacturer may optionally not support all features, commands & attributes */
     gWhmInstance = std::make_unique<WaterHeaterManagementInstance>(
         EndpointId(WHM_ENDPOINT), *gWhmDelegate,
-        BitMask<Feature>(Feature::kEnergyManagement, Feature::kTankPercent)); // GetFeatureMapFromCmdLine());
-
+        BitMask<Feature>(Feature::kEnergyManagement, Feature::kTankPercent));
     if (!gWhmInstance)
     {
         ChipLogError(AppServer, "Failed to allocate memory for WaterHeaterManagementInstance");

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

@@ -25,6 +25,7 @@
 #include <app-common/zap-generated/attributes/Accessors.h>
 
 using namespace chip;
+using namespace chip::app::Clusters::WaterHeaterManagement;
 
 using Protocols::InteractionModel::Status;
 
@@ -44,6 +45,10 @@ CHIP_ERROR WhmManufacturer::Init()
 
     mBoostActive = false;
 
+    dg->SetHeaterTypes(BitMask<WaterHeaterTypeBitmap>(WaterHeaterTypeBitmap::kImmersionElement1));
+    dg->SetHeatDemand(BitMask<WaterHeaterDemandBitmap>(WaterHeaterDemandBitmap::kImmersionElement1));
+    dg->SetEstimatedHeatRequired(10000);
+
     return CHIP_NO_ERROR;
 }
 
@@ -72,11 +77,11 @@ BitMask<WaterHeaterDemandBitmap> WhmManufacturer::DetermineHeatingSources()
 
     // The corresponding list of valid headerDemands
     uint8_t waterHeaterDemandValues[] = {
-        static_cast<uint8_t>(WaterHeaterTypeBitmap::kImmersionElement1),
-        static_cast<uint8_t>(WaterHeaterTypeBitmap::kImmersionElement2),
-        static_cast<uint8_t>(WaterHeaterTypeBitmap::kHeatPump),
-        static_cast<uint8_t>(WaterHeaterTypeBitmap::kBoiler),
-        static_cast<uint8_t>(WaterHeaterTypeBitmap::kOther),
+        static_cast<uint8_t>(WaterHeaterDemandBitmap::kImmersionElement1),
+        static_cast<uint8_t>(WaterHeaterDemandBitmap::kImmersionElement2),
+        static_cast<uint8_t>(WaterHeaterDemandBitmap::kHeatPump),
+        static_cast<uint8_t>(WaterHeaterDemandBitmap::kBoiler),
+        static_cast<uint8_t>(WaterHeaterDemandBitmap::kOther),
     };
 
     // Iterate across the valid waterHeaterTypes seeing which heating sources are available based on heaterTypes.