Fix/post pr 28707 comments (#28993)

* AitQuality: Changed the feature attribute type to a BitMask of the Feature type. Fixed bug in the UpdateAirQuality method where checks where made against an incorrect enum. * Added an out-of-band API to set the AirQuality attribute in the linux all-clusters-app. * Added a check to ensure that the value given to UpdateAirQuality is not beyond the list of acceptable enums. * Restyled by clang-format * AirQuality: Renamed the out-of-band message's key Co-authored-by: Boris Zbarsky <[email protected]> * AirQuality: Moved the check for the valid enum value in the switch statement. * Restyled by clang-format --------- Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Jan 18, 2024 · a0c4e72 · a0c4e72
1 parent 9e8f667
commit a0c4e72
Show file tree

Hide file tree

Showing 6 changed files with 56 additions and 13 deletions.
diff --git a/examples/all-clusters-app/all-clusters-common/include/air-quality-instance.h b/examples/all-clusters-app/all-clusters-common/include/air-quality-instance.h
@@ -7,6 +7,8 @@ namespace app {
 namespace Clusters {
 namespace AirQuality {
 
+Instance * GetInstance();
+
 void Shutdown();
 
 } // namespace AirQuality

diff --git a/examples/all-clusters-app/all-clusters-common/src/air-quality-instance.cpp b/examples/all-clusters-app/all-clusters-common/src/air-quality-instance.cpp
@@ -5,6 +5,11 @@ using namespace chip::app::Clusters::AirQuality;
 
 Instance * gAirQualityCluster = nullptr;
 
+Instance * AirQuality::GetInstance()
+{
+    return gAirQualityCluster;
+}
+
 void AirQuality::Shutdown()
 {
     if (gAirQualityCluster != nullptr)
@@ -20,6 +25,6 @@ void emberAfAirQualityClusterInitCallback(chip::EndpointId endpointId)
     VerifyOrDie(gAirQualityCluster == nullptr);
     chip::BitMask<Feature, uint32_t> airQualityFeatures(Feature::kModerate, Feature::kFair, Feature::kVeryPoor,
                                                         Feature::kExtremelyPoor);
-    gAirQualityCluster = new Instance(1, airQualityFeatures.Raw());
+    gAirQualityCluster = new Instance(1, airQualityFeatures);
     gAirQualityCluster->Init();
 }
diff --git a/examples/all-clusters-app/linux/AllClustersCommandDelegate.cpp b/examples/all-clusters-app/linux/AllClustersCommandDelegate.cpp
@@ -27,6 +27,7 @@
 #include <app/server/Server.h>
 #include <platform/PlatformManager.h>
 
+#include <air-quality-instance.h>
 #include <dishwasher-mode.h>
 #include <laundry-washer-mode.h>
 #include <rvc-modes.h>
@@ -164,6 +165,19 @@ void AllClustersAppCommandHandler::HandleCommand(intptr_t context)
         }
         self->OnModeChangeHandler(device, type, mode);
     }
+    else if (name == "SetAirQuality")
+    {
+        Json::Value jsonAirQualityEnum = self->mJsonValue["NewValue"];
+
+        if (jsonAirQualityEnum.isNull())
+        {
+            ChipLogError(NotSpecified, "The SetAirQuality command requires the NewValue key.");
+        }
+        else
+        {
+            self->OnAirQualityChange(static_cast<uint32_t>(jsonAirQualityEnum.asUInt()));
+        }
+    }
     else
     {
         ChipLogError(NotSpecified, "Unhandled command: Should never happens");
@@ -414,6 +428,18 @@ void AllClustersAppCommandHandler::OnModeChangeHandler(std::string device, std::
     }
 }
 
+void AllClustersAppCommandHandler::OnAirQualityChange(uint32_t aNewValue)
+{
+    AirQuality::Instance * airQualityInstance = AirQuality::GetInstance();
+    Protocols::InteractionModel::Status status =
+        airQualityInstance->UpdateAirQuality(static_cast<AirQuality::AirQualityEnum>(aNewValue));
+
+    if (status != Protocols::InteractionModel::Status::Success)
+    {
+        ChipLogDetail(NotSpecified, "Invalid value: %u", aNewValue);
+    }
+}
+
 void AllClustersCommandDelegate::OnEventCommandReceived(const char * json)
 {
     auto handler = AllClustersAppCommandHandler::FromJSON(json);

diff --git a/examples/all-clusters-app/linux/AllClustersCommandDelegate.h b/examples/all-clusters-app/linux/AllClustersCommandDelegate.h
@@ -93,6 +93,11 @@ class AllClustersAppCommandHandler
      * Should be called when it is necessary to change the mode to manual operation.
      */
     void OnModeChangeHandler(std::string device, std::string type, chip::app::DataModel::Nullable<uint8_t> mode);
+
+    /**
+     * Should be called when it is necessary to change the air quality attribute.
+     */
+    void OnAirQualityChange(uint32_t aEnum);
 };
 
 class AllClustersCommandDelegate : public NamedPipeCommandDelegate

diff --git a/src/app/clusters/air-quality-server/air-quality-server.cpp b/src/app/clusters/air-quality-server/air-quality-server.cpp
@@ -32,7 +32,7 @@ namespace app {
 namespace Clusters {
 namespace AirQuality {
 
-Instance::Instance(EndpointId aEndpointId, uint32_t aFeature) :
+Instance::Instance(EndpointId aEndpointId, BitMask<Feature> aFeature) :
     AttributeAccessInterface(Optional<EndpointId>(aEndpointId), Id), mEndpointId(aEndpointId), mFeature(aFeature)
 {}
 
@@ -51,9 +51,9 @@ CHIP_ERROR Instance::Init()
     return CHIP_NO_ERROR;
 }
 
-bool Instance::HasFeature(AirQualityEnum aFeature) const
+bool Instance::HasFeature(Feature aFeature) const
 {
-    return mFeature & to_underlying(aFeature);
+    return mFeature.Has(aFeature);
 }
 
 Protocols::InteractionModel::Status Instance::UpdateAirQuality(AirQualityEnum aNewAirQuality)
@@ -62,35 +62,40 @@ Protocols::InteractionModel::Status Instance::UpdateAirQuality(AirQualityEnum aN
     switch (aNewAirQuality)
     {
     case AirQualityEnum::kFair: {
-        if (!HasFeature(AirQualityEnum::kFair))
+        if (!HasFeature(Feature::kFair))
         {
             return Protocols::InteractionModel::Status::ConstraintError;
         }
     }
     break;
     case AirQualityEnum::kModerate: {
-        if (!HasFeature(AirQualityEnum::kModerate))
+        if (!HasFeature(Feature::kModerate))
         {
             return Protocols::InteractionModel::Status::ConstraintError;
         }
     }
     break;
-    case AirQualityEnum::kPoor: {
-        if (!HasFeature(AirQualityEnum::kPoor))
+    case AirQualityEnum::kVeryPoor: {
+        if (!HasFeature(Feature::kVeryPoor))
         {
             return Protocols::InteractionModel::Status::ConstraintError;
         }
     }
     break;
     case AirQualityEnum::kExtremelyPoor: {
-        if (!HasFeature(AirQualityEnum::kExtremelyPoor))
+        if (!HasFeature(Feature::kExtremelyPoor))
         {
             return Protocols::InteractionModel::Status::ConstraintError;
         }
     }
     break;
-    default:
+    case AirQualityEnum::kUnknown:
+    case AirQualityEnum::kGood:
+    case AirQualityEnum::kPoor:
         break;
+    default: {
+        return Protocols::InteractionModel::Status::InvalidValue;
+    }
     }
 
     mAirQuality = aNewAirQuality;

diff --git a/src/app/clusters/air-quality-server/air-quality-server.h b/src/app/clusters/air-quality-server/air-quality-server.h
@@ -34,7 +34,7 @@ class Instance : public AttributeAccessInterface
      * @param aEndpointId The endpoint on which this cluster exists. This must match the zap configuration.
      * @param aFeature The bitmask value that identifies which features are supported by this instance.
      */
-    Instance(EndpointId aEndpointId, uint32_t eFeature);
+    Instance(EndpointId aEndpointId, BitMask<Feature> aFeature);
 
     ~Instance() override;
 
@@ -49,7 +49,7 @@ class Instance : public AttributeAccessInterface
      * Returns true if the feature is supported.
      * @param feature the feature to check.
      */
-    bool HasFeature(AirQualityEnum) const;
+    bool HasFeature(Feature aFeature) const;
 
     /**
      * Sets the AirQuality attribute.
@@ -66,7 +66,7 @@ class Instance : public AttributeAccessInterface
 private:
     EndpointId mEndpointId;
     AirQualityEnum mAirQuality = AirQualityEnum::kUnknown;
-    uint32_t mFeature;
+    BitMask<Feature> mFeature;
 
     // AttributeAccessInterface
     CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override;