From 01b03ce691b2641a88f81dd0af64b8381f837ff1 Mon Sep 17 00:00:00 2001 From: Oliver Fan Date: Tue, 16 Jul 2024 19:20:40 -0700 Subject: [PATCH] Apply suggestions from code review * Avoid mixing code-backed and Attribute-store-backed attributes * Avoid global singletons to maintain state * Initialize the cluster from the application * Report features based on code, not on ZAP-configured values Signed-off-by: Oliver Fan --- .../src/occupancy-sensing-stub.cpp | 61 ++++++ examples/all-clusters-app/linux/BUILD.gn | 1 + .../occupancy-sensor-server.cpp | 175 ++++++++++-------- .../occupancy-sensor-server.h | 68 ++----- 4 files changed, 172 insertions(+), 133 deletions(-) create mode 100644 examples/all-clusters-app/all-clusters-common/src/occupancy-sensing-stub.cpp diff --git a/examples/all-clusters-app/all-clusters-common/src/occupancy-sensing-stub.cpp b/examples/all-clusters-app/all-clusters-common/src/occupancy-sensing-stub.cpp new file mode 100644 index 00000000000000..f1652ecf7839c4 --- /dev/null +++ b/examples/all-clusters-app/all-clusters-common/src/occupancy-sensing-stub.cpp @@ -0,0 +1,61 @@ +/* + * + * Copyright (c) 2024 Project CHIP Authors + * + * 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. + */ + +#include +#include +#include +#include + + +using namespace chip; +using namespace chip::app::Clusters; +using namespace chip::app::Clusters::OccupancySensing; +using namespace chip::app::Clusters::OccupancySensing::Structs; +using namespace chip::DeviceLayer; + +using chip::Protocols::InteractionModel::Status; + +static std::unique_ptr gAttrAccess[MATTER_DM_OCCUPANCY_SENSING_CLUSTER_SERVER_ENDPOINT_COUNT + CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT]; + + +void emberAfOccupancySensingClusterInitCallback(chip::EndpointId endpointId) +{ + + VerifyOrDie(!gAttrAccess[endpointId]); + + gAttrAccess[endpointId] = std::make_unique( + BitMask(OccupancySensing::Feature::kOther)); + + + OccupancySensing::Structs::HoldTimeLimitsStruct::Type holdTimeLimits = { + .holdTimeMin = 1, + .holdTimeMax = 300, + .holdTimeDefault = 10, + }; + + uint16_t holdTime = 10; + + if (gAttrAccess[endpointId]) + { + gAttrAccess[endpointId]->Init(); + + SetHoldTimeLimits(endpointId, holdTimeLimits); + + SetHoldTime(endpointId, holdTime); + } + +} diff --git a/examples/all-clusters-app/linux/BUILD.gn b/examples/all-clusters-app/linux/BUILD.gn index 3d52ef748de90d..7b0c70122217aa 100644 --- a/examples/all-clusters-app/linux/BUILD.gn +++ b/examples/all-clusters-app/linux/BUILD.gn @@ -46,6 +46,7 @@ source_set("chip-all-clusters-common") { "${chip_root}/examples/all-clusters-app/all-clusters-common/src/laundry-washer-controls-delegate-impl.cpp", "${chip_root}/examples/all-clusters-app/all-clusters-common/src/laundry-washer-mode.cpp", "${chip_root}/examples/all-clusters-app/all-clusters-common/src/microwave-oven-mode.cpp", + "${chip_root}/examples/all-clusters-app/all-clusters-common/src/occupancy-sensing-stub.cpp", "${chip_root}/examples/all-clusters-app/all-clusters-common/src/operational-state-delegate-impl.cpp", "${chip_root}/examples/all-clusters-app/all-clusters-common/src/oven-modes.cpp", "${chip_root}/examples/all-clusters-app/all-clusters-common/src/oven-operational-state-delegate.cpp", diff --git a/src/app/clusters/occupancy-sensor-server/occupancy-sensor-server.cpp b/src/app/clusters/occupancy-sensor-server/occupancy-sensor-server.cpp index 8596853323479f..293232a46b5fa6 100644 --- a/src/app/clusters/occupancy-sensor-server/occupancy-sensor-server.cpp +++ b/src/app/clusters/occupancy-sensor-server/occupancy-sensor-server.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -31,117 +32,133 @@ namespace app { namespace Clusters { namespace OccupancySensing { -Structs::HoldTimeLimitsStruct::Type * HoldTimeLimitsManager::HoldTimeLimits::GetHoldTimeLimitsStruct(EndpointId endpoint) -{ - size_t endpointIndex = 0; - Structs::HoldTimeLimitsStruct::Type * holdTimeLimitsStruct = nullptr; - CHIP_ERROR status = FindHoldTimeLimitsIndex(endpoint, endpointIndex); - if (CHIP_NO_ERROR == status) - { - holdTimeLimitsStruct = &mHoldTimeLimitsStructs[endpointIndex]; - } - return holdTimeLimitsStruct; -} - -CHIP_ERROR -HoldTimeLimitsManager::HoldTimeLimits::SetHoldTimeLimitsStruct(EndpointId endpoint, - Structs::HoldTimeLimitsStruct::Type & holdTimeLimitsStruct) -{ - VerifyOrReturnError(kInvalidEndpointId != endpoint, CHIP_ERROR_INVALID_ARGUMENT); - - size_t endpointIndex = 0; - ReturnErrorOnFailure(FindHoldTimeLimitsIndex(endpoint, endpointIndex)); +Structs::HoldTimeLimitsStruct::Type mHoldTimeLimitsStructs[MATTER_DM_OCCUPANCY_SENSING_CLUSTER_SERVER_ENDPOINT_COUNT + CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT]; - mHoldTimeLimitsStructs[endpointIndex] = holdTimeLimitsStruct; +uint16_t mHoldTime[MATTER_DM_OCCUPANCY_SENSING_CLUSTER_SERVER_ENDPOINT_COUNT + CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT]; +CHIP_ERROR OccupancySensingAttrAccess::Init() +{ + VerifyOrReturnError(registerAttributeAccessOverride(this), CHIP_ERROR_INCORRECT_STATE); return CHIP_NO_ERROR; } -/// @brief Returns the index of the HoldTimeLimits associated to an endpoint -/// @param[in] endpoint target endpoint -/// @param[out] endpointIndex index of the corresponding HoldTimeLimits for an endpoint -/// @return CHIP_NO_ERROR or CHIP_ERROR_NOT_FOUND, CHIP_ERROR_INVALID_ARGUMENT if invalid endpoint -CHIP_ERROR HoldTimeLimitsManager::HoldTimeLimits::FindHoldTimeLimitsIndex(EndpointId endpoint, size_t & endpointIndex) +void OccupancySensingAttrAccess::Shutdown() { - VerifyOrReturnError(kInvalidEndpointId != endpoint, CHIP_ERROR_INVALID_ARGUMENT); + unregisterAttributeAccessOverride(this); +} - uint16_t index = emberAfGetClusterServerEndpointIndex(endpoint, OccupancySensing::Id, - MATTER_DM_OCCUPANCY_SENSING_CLUSTER_SERVER_ENDPOINT_COUNT); - if (index < ArraySize(mHoldTimeLimitsStructs)) +CHIP_ERROR OccupancySensingAttrAccess::Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) +{ + VerifyOrDie(aPath.mClusterId == app::Clusters::OccupancySensing::Id); + + switch (aPath.mAttributeId) { - endpointIndex = index; + case Attributes::FeatureMap::Id: + ReturnErrorOnFailure(aEncoder.Encode(mFeature)); + break; + case Attributes::HoldTime::Id: { + + uint16_t * holdTime = GetHoldTimeForEndpoint(aPath.mEndpointId); + + if (holdTime == nullptr) + { + return CHIP_ERROR_NOT_FOUND; + } + + return aEncoder.Encode(*holdTime); + } + case Attributes::HoldTimeLimits::Id: { + + Structs::HoldTimeLimitsStruct::Type * holdTimeLimitsStruct = GetHoldTimeLimitsForEndpoint(aPath.mEndpointId); + + if (holdTimeLimitsStruct == nullptr) + { + return CHIP_ERROR_NOT_FOUND; + } + + return aEncoder.Encode(*holdTimeLimitsStruct); + } + default: return CHIP_NO_ERROR; } - return CHIP_ERROR_NOT_FOUND; + + return CHIP_NO_ERROR; } -HoldTimeLimitsManager HoldTimeLimitsManager::mInstance; - -HoldTimeLimitsManager & HoldTimeLimitsManager::Instance() +bool OccupancySensingAttrAccess::HasFeature(Feature aFeature) const { - return mInstance; + return mFeature.Has(aFeature); } -CHIP_ERROR HoldTimeLimitsManager::Init() +Structs::HoldTimeLimitsStruct::Type * GetHoldTimeLimitsForEndpoint(EndpointId endpoint) { - // Prevents re-initializing - VerifyOrReturnError(!mIsInitialized, CHIP_ERROR_INCORRECT_STATE); + auto index = emberAfGetClusterServerEndpointIndex(endpoint, app::Clusters::OccupancySensing::Id, + MATTER_DM_OCCUPANCY_SENSING_CLUSTER_SERVER_ENDPOINT_COUNT); + + if (index == kEmberInvalidEndpointIndex) + { + return nullptr; + } - for (size_t i = 0; i <= kOccupancySensingServerMaxEndpointCount; i++) + if (index >= ArraySize(mHoldTimeLimitsStructs)) { - if (emberAfContainsServer(EndpointId(i), OccupancySensing::Id)) - { - Structs::HoldTimeLimitsStruct::Type holdTimeLimitsInit; + ChipLogError(NotSpecified, "Internal error: invalid/unexpected hold time limits index."); + return nullptr; + } + return &mHoldTimeLimitsStructs[index]; +} - // Set up some sane initial values for hold time limits structures - holdTimeLimitsInit.holdTimeMin = 1; - holdTimeLimitsInit.holdTimeMax = 300; - holdTimeLimitsInit.holdTimeDefault = 10; +CHIP_ERROR SetHoldTimeLimits(EndpointId endpointId, const Structs::HoldTimeLimitsStruct::Type & holdTimeLimits) +{ - HoldTimeLimitsManager::Instance().SetHoldTimeLimitsStruct(EndpointId(i), holdTimeLimitsInit); - } - } + VerifyOrReturnError(kInvalidEndpointId != endpointId, CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError(registerAttributeAccessOverride(this), CHIP_ERROR_INCORRECT_STATE); + Structs::HoldTimeLimitsStruct::Type * holdTimeLimitsForEndpoint = GetHoldTimeLimitsForEndpoint(endpointId); + VerifyOrReturnError(holdTimeLimitsForEndpoint != nullptr, CHIP_ERROR_INVALID_ARGUMENT); + + holdTimeLimitsForEndpoint->holdTimeMin = holdTimeLimits.holdTimeMin; + holdTimeLimitsForEndpoint->holdTimeMax = holdTimeLimits.holdTimeMax; + holdTimeLimitsForEndpoint->holdTimeDefault = holdTimeLimits.holdTimeDefault; + + MatterReportingAttributeChangeCallback(endpointId, OccupancySensing::Id, Attributes::HoldTimeLimits::Id); - mIsInitialized = true; return CHIP_NO_ERROR; } -// AttributeAccessInterface -CHIP_ERROR HoldTimeLimitsManager::Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) +uint16_t * GetHoldTimeForEndpoint(EndpointId endpoint) { - switch (aPath.mAttributeId) + auto index = emberAfGetClusterServerEndpointIndex(endpoint, app::Clusters::OccupancySensing::Id, + MATTER_DM_OCCUPANCY_SENSING_CLUSTER_SERVER_ENDPOINT_COUNT); + + if (index == kEmberInvalidEndpointIndex) { - case Attributes::HoldTimeLimits::Id: { - - Structs::HoldTimeLimitsStruct::Type * holdTimeLimitsStruct = - HoldTimeLimitsManager::Instance().mHoldTimeLimits.GetHoldTimeLimitsStruct(aPath.mEndpointId); - Structs::HoldTimeLimitsStruct::Type res; - res.holdTimeMin = holdTimeLimitsStruct->holdTimeMin; - res.holdTimeMax = holdTimeLimitsStruct->holdTimeMax; - res.holdTimeDefault = holdTimeLimitsStruct->holdTimeDefault; - return aEncoder.Encode(res); + return nullptr; } - default: - return CHIP_NO_ERROR; + + if (index >= ArraySize(mHoldTimeLimitsStructs)) + { + ChipLogError(NotSpecified, "Internal error: invalid/unexpected hold time index."); + return nullptr; } + return &mHoldTime[index]; } -Structs::HoldTimeLimitsStruct::Type * HoldTimeLimitsManager::GetHoldTimeLimitsStruct(EndpointId endpoint) +CHIP_ERROR SetHoldTime(EndpointId endpointId, const uint16_t & holdTime) { - Structs::HoldTimeLimitsStruct::Type * holdTimeLimitsStruct = mHoldTimeLimits.GetHoldTimeLimitsStruct(endpoint); - return holdTimeLimitsStruct; -} + VerifyOrReturnError(kInvalidEndpointId != endpointId, CHIP_ERROR_INVALID_ARGUMENT); + + uint16_t * holdTimeForEndpoint = GetHoldTimeForEndpoint(endpointId); + VerifyOrReturnError(holdTimeForEndpoint != nullptr, CHIP_ERROR_INVALID_ARGUMENT); + + *holdTimeForEndpoint = holdTime; + + MatterReportingAttributeChangeCallback(endpointId, OccupancySensing::Id, Attributes::HoldTime::Id); -CHIP_ERROR HoldTimeLimitsManager::SetHoldTimeLimitsStruct(EndpointId endpoint, - Structs::HoldTimeLimitsStruct::Type & holdTimeLimitsStruct) -{ - ReturnErrorOnFailure(mHoldTimeLimits.SetHoldTimeLimitsStruct(endpoint, holdTimeLimitsStruct)); return CHIP_NO_ERROR; } + } // namespace OccupancySensing } // namespace Clusters } // namespace app @@ -213,11 +230,7 @@ HalOccupancySensorType __attribute__((weak)) halOccupancyGetSensorType(EndpointI return HAL_OCCUPANCY_SENSOR_TYPE_PIR; } + void MatterOccupancySensingPluginServerInitCallback() -{ - CHIP_ERROR err = HoldTimeLimitsManager::Instance().Init(); - if (err != CHIP_NO_ERROR) - { - ChipLogError(Zcl, "HoldTimeLimitsManager::Instance().Init() error: %" CHIP_ERROR_FORMAT, err.Format()); - } -} +{} + diff --git a/src/app/clusters/occupancy-sensor-server/occupancy-sensor-server.h b/src/app/clusters/occupancy-sensor-server/occupancy-sensor-server.h index 8acc57108b3414..9d2ac08a7f736a 100644 --- a/src/app/clusters/occupancy-sensor-server/occupancy-sensor-server.h +++ b/src/app/clusters/occupancy-sensor-server/occupancy-sensor-server.h @@ -30,73 +30,37 @@ namespace app { namespace Clusters { namespace OccupancySensing { -/** - * Interface to help manage the Hold Time Limits of the Occupancy Sensing Cluster. - */ -class HoldTimeLimitsManager : public AttributeAccessInterface + +class OccupancySensingAttrAccess : public AttributeAccessInterface { public: - static constexpr size_t kOccupancySensingServerMaxEndpointCount = - MATTER_DM_OCCUPANCY_SENSING_CLUSTER_SERVER_ENDPOINT_COUNT + CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT; - static_assert(kOccupancySensingServerMaxEndpointCount <= kInvalidEndpointId, "Occupancy Sensing endpoint count error"); - - // HoldTimeLimits - class HoldTimeLimits - { - public: - Structs::HoldTimeLimitsStruct::Type * GetHoldTimeLimitsStruct(EndpointId endpoint); - CHIP_ERROR SetHoldTimeLimitsStruct(EndpointId endpoint, Structs::HoldTimeLimitsStruct::Type & holdTimeLimitsStruct); + OccupancySensingAttrAccess(BitMask aFeature) : + app::AttributeAccessInterface(Optional::Missing(), app::Clusters::OccupancySensing::Id), mFeature(aFeature) + {} + + ~OccupancySensingAttrAccess() { Shutdown(); } - private: - /// @brief Returns the index of the HoldTimeLimits associated to an endpoint - /// @param[in] endpoint target endpoint - /// @param[out] endpointIndex index of the corresponding HoldTimeLimits for an endpoint - /// @return CHIP_NO_ERROR or CHIP_ERROR_NOT_FOUND, CHIP_ERROR_INVALID_ARGUMENT if invalid endpoint - CHIP_ERROR FindHoldTimeLimitsIndex(EndpointId endpoint, size_t & endpointIndex); - - Structs::HoldTimeLimitsStruct::Type mHoldTimeLimitsStructs[kOccupancySensingServerMaxEndpointCount]; - }; - - static HoldTimeLimitsManager & Instance(); CHIP_ERROR Init(); + void Shutdown(); - // AttributeAccessInterface CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override; - // HoldTimeLimitsStruct Accessors - Structs::HoldTimeLimitsStruct::Type * GetHoldTimeLimitsStruct(EndpointId endpoint); - CHIP_ERROR SetHoldTimeLimitsStruct(EndpointId endpoint, Structs::HoldTimeLimitsStruct::Type & holdTimeLimitsStruct); + bool HasFeature(Feature aFeature) const; private: - HoldTimeLimitsManager() : AttributeAccessInterface(Optional(), Id) {} - ~HoldTimeLimitsManager() {} + BitMask mFeature; + +}; - bool mIsInitialized = false; +CHIP_ERROR SetHoldTimeLimits(EndpointId endpointId, const Structs::HoldTimeLimitsStruct::Type & holdTimeLimits); - // HoldTimeLimits - HoldTimeLimits mHoldTimeLimits; +CHIP_ERROR SetHoldTime(EndpointId endpointId, const uint16_t & holdTime); - // Instance - static HoldTimeLimitsManager mInstance; -}; +Structs::HoldTimeLimitsStruct::Type * GetHoldTimeLimitsForEndpoint(EndpointId endpoint); -inline bool HasFeature(EndpointId ep, Feature feature) -{ - uint32_t map; - bool success = (Attributes::FeatureMap::Get(ep, &map) == Protocols::InteractionModel::Status::Success); - return success ? (map & to_underlying(feature)) : false; -} +uint16_t * GetHoldTimeForEndpoint(EndpointId endpoint); -/** @brief Occupancy Cluster Server Post Init - * - * Following resolution of the Occupancy state at startup for this endpoint, - * perform any additional initialization needed; e.g., synchronize hardware - * state. - * - * @param endpoint Endpoint that is being initialized Ver.: always - */ -void emberAfPluginOccupancyClusterServerPostInitCallback(chip::EndpointId endpoint); } // namespace OccupancySensing } // namespace Clusters