From a0c4e72ac696da1b96048894c0a014c7c81835c6 Mon Sep 17 00:00:00 2001 From: William Date: Fri, 1 Sep 2023 15:27:34 +0100 Subject: [PATCH] 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 * AirQuality: Moved the check for the valid enum value in the switch statement. * Restyled by clang-format --------- Co-authored-by: Restyled.io Co-authored-by: Boris Zbarsky --- .../include/air-quality-instance.h | 2 ++ .../src/air-quality-instance.cpp | 7 ++++- .../linux/AllClustersCommandDelegate.cpp | 26 +++++++++++++++++++ .../linux/AllClustersCommandDelegate.h | 5 ++++ .../air-quality-server/air-quality-server.cpp | 23 +++++++++------- .../air-quality-server/air-quality-server.h | 6 ++--- 6 files changed, 56 insertions(+), 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 index eb2b7c89288da4..8480fd61509ab8 100644 --- 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 index eb5f4dc5607dc3..c78f34e9a8f31f 100644 --- 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 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 index 19c8635a6b665e..39e3516ada4c96 100644 --- a/examples/all-clusters-app/linux/AllClustersCommandDelegate.cpp +++ b/examples/all-clusters-app/linux/AllClustersCommandDelegate.cpp @@ -27,6 +27,7 @@ #include #include +#include #include #include #include @@ -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(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(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 index 20b2ab731b75d3..7998cd15581a74 100644 --- 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 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 index 938c158e49ca56..875bd4c4dfa9bb 100644 --- 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 aFeature) : AttributeAccessInterface(Optional(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 index 90abf3b17e290a..d14d4e52a351a0 100644 --- 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 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 mFeature; // AttributeAccessInterface CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override;