From 1083086f9285ba09bbc5f10cf460d6e856052d20 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs <112982107+lpbeliveau-silabs@users.noreply.github.com> Date: Fri, 17 Nov 2023 10:35:19 -0500 Subject: [PATCH] [Scenes] SceneFabricInfo nitpicks (#30532) * Addressed comment in previous SceneFabricInfo PR * Apply suggestions from code review Co-authored-by: mkardous-silabs <84793247+mkardous-silabs@users.noreply.github.com> * Fixed end of sentence, removed extra 'the' from the suggestion and clarified what capacity we are talking about --------- Co-authored-by: mkardous-silabs <84793247+mkardous-silabs@users.noreply.github.com> --- .../clusters/scenes-server/scenes-server.cpp | 22 ++++++++++--------- .../clusters/scenes-server/scenes-server.h | 14 ++++++++++++ .../CHIP/templates/availability.yaml | 5 +++-- 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/app/clusters/scenes-server/scenes-server.cpp b/src/app/clusters/scenes-server/scenes-server.cpp index 1b319399719f4c..cac001cdc2c49d 100644 --- a/src/app/clusters/scenes-server/scenes-server.cpp +++ b/src/app/clusters/scenes-server/scenes-server.cpp @@ -164,14 +164,14 @@ CHIP_ERROR UpdateFabricSceneInfo(EndpointId endpoint, FabricIndex fabric, Option Span ScenesServer::FabricSceneInfo::GetFabricSceneInfo(EndpointId endpoint) { size_t endpointIndex = 0; - Span FabricSceneInfoSpan; + Span fabricSceneInfoSpan; CHIP_ERROR status = FindFabricSceneInfoIndex(endpoint, endpointIndex); if (CHIP_NO_ERROR == status) { - FabricSceneInfoSpan = + fabricSceneInfoSpan = Span(&mSceneInfoStructs[endpointIndex][0], mSceneInfoStructsCount[endpointIndex]); } - return FabricSceneInfoSpan; + return fabricSceneInfoSpan; } /// @brief Gets the SceneInfoStruct for a specific fabric for a specific endpoint @@ -273,8 +273,10 @@ CHIP_ERROR ScenesServer::FabricSceneInfo::FindFabricSceneInfoIndex(EndpointId en /// @param[in] fabric target fabric index /// @param[in] endpointIndex index of the corresponding FabricSceneInfo for an endpoint, corresponds to a row in the /// mSceneInfoStructs array -/// @param[out] index index of the corresponding SceneInfoStruct if found, mFabricSceneInfo[endpoint] out of bounds index otherwise -/// @return CHIP_NO_ERROR or CHIP_ERROR_NOT_FOUND, CHIP_ERROR_INVALID_ARGUMENT if invalid fabric or endpoint +/// @param[out] index index of the corresponding SceneInfoStruct if found, otherwise the index value will be invalid and +/// should not be used. This is safe to store in a uint8_t because the index is guaranteed to be smaller than +/// CHIP_CONFIG_MAX_FABRICS. +/// @return CHIP_NO_ERROR or CHIP_ERROR_NOT_FOUND, CHIP_ERROR_INVALID_ARGUMENT if invalid fabric or endpointIndex are provided CHIP_ERROR ScenesServer::FabricSceneInfo::FindSceneInfoStructIndex(FabricIndex fabric, size_t endpointIndex, uint8_t & index) { VerifyOrReturnError(endpointIndex < ArraySize(mSceneInfoStructs), CHIP_ERROR_INVALID_ARGUMENT); @@ -541,7 +543,7 @@ void ViewSceneParse(HandlerContext & ctx, const CommandData & req, GroupDataProv CHIP_ERROR StoreSceneParse(const FabricIndex & fabricIdx, const EndpointId & endpointID, const GroupId & groupID, const SceneId & sceneID, GroupDataProvider * groupProvider) { - // Make current fabric's SceneValid false before store scenes + // Make the current fabric's SceneValid false before storing a scene ScenesServer::Instance().MakeSceneInvalid(endpointID, fabricIdx); uint16_t endpointTableSize = 0; @@ -602,7 +604,7 @@ CHIP_ERROR RecallSceneParse(const FabricIndex & fabricIdx, const EndpointId & en const SceneId & sceneID, const Optional> & transitionTime, GroupDataProvider * groupProvider) { - // Make SceneValid false for all fabrics before recall scenes + // Make SceneValid false for all fabrics before recalling a scene ScenesServer::Instance().MakeSceneInvalidForAllFabrics(endpointID); uint16_t endpointTableSize = 0; @@ -699,8 +701,8 @@ CHIP_ERROR ScenesServer::Read(const ConcreteReadAttributePath & aPath, Attribute { case Attributes::FabricSceneInfo::Id: { return aEncoder.EncodeList([&](const auto & encoder) -> CHIP_ERROR { - Span FabricSceneInfoSpan = mFabricSceneInfo.GetFabricSceneInfo(aPath.mEndpointId); - for (auto & info : FabricSceneInfoSpan) + Span fabricSceneInfoSpan = mFabricSceneInfo.GetFabricSceneInfo(aPath.mEndpointId); + for (auto & info : fabricSceneInfoSpan) { ReturnErrorOnFailure(encoder.Encode(info)); } @@ -1163,7 +1165,7 @@ void emberAfScenesClusterServerInitCallback(EndpointId endpoint) ChipLogDetail(Zcl, "ERR: setting LastConfiguredBy on Endpoint %hu Status: %x", endpoint, status); } - // Initialize the FabricSceneInfo list from data in Flash + // Initialize the FabricSceneInfo by getting the number of scenes and the remaining capacity for storing fabric scene data for (auto & info : chip::Server::GetInstance().GetFabricTable()) { auto fabric = info.GetFabricIndex(); diff --git a/src/app/clusters/scenes-server/scenes-server.h b/src/app/clusters/scenes-server/scenes-server.h index d8501318eabfe2..49bd1537de8734 100644 --- a/src/app/clusters/scenes-server/scenes-server.h +++ b/src/app/clusters/scenes-server/scenes-server.h @@ -49,7 +49,21 @@ class ScenesServer : public CommandHandlerInterface, public AttributeAccessInter void ClearSceneInfoStruct(EndpointId endpoint, FabricIndex fabric); private: + /// @brief Returns the index of the FabricSceneInfo associated to an endpoint + /// @param[in] endpoint target endpoint + /// @param[out] endpointIndex index of the corresponding FabricSceneInfo for an endpoint, corresponds to a row in the + /// mSceneInfoStructs array, + /// @return CHIP_NO_ERROR or CHIP_ERROR_NOT_FOUND, CHIP_ERROR_INVALID_ARGUMENT if invalid endpoint CHIP_ERROR FindFabricSceneInfoIndex(EndpointId endpoint, size_t & endpointIndex); + + /// @brief Returns the SceneInfoStruct associated to a fabric + /// @param[in] fabric target fabric index + /// @param[in] endpointIndex index of the corresponding FabricSceneInfo for an endpoint, corresponds to a row in the + /// mSceneInfoStructs array + /// @param[out] index index of the corresponding SceneInfoStruct if found, otherwise the index value will be invalid and + /// should not be used. This is safe to store in a uint8_t because the index is guaranteed to be smaller than + /// CHIP_CONFIG_MAX_FABRICS. + /// @return CHIP_NO_ERROR or CHIP_ERROR_NOT_FOUND, CHIP_ERROR_INVALID_ARGUMENT if invalid fabric or endpoint CHIP_ERROR FindSceneInfoStructIndex(FabricIndex fabric, size_t endpointIndex, uint8_t & index); Structs::SceneInfoStruct::Type mSceneInfoStructs[kScenesServerMaxEndpointCount][kScenesServerMaxFabricCount]; diff --git a/src/darwin/Framework/CHIP/templates/availability.yaml b/src/darwin/Framework/CHIP/templates/availability.yaml index 34bf6964e34395..2ad0831f77792f 100644 --- a/src/darwin/Framework/CHIP/templates/availability.yaml +++ b/src/darwin/Framework/CHIP/templates/availability.yaml @@ -7819,12 +7819,13 @@ - CoupleColorTempToLevel attributes: Scenes: - - FabricSceneInfo # Targeting Spring 2024 Matter release + - FabricSceneInfo structs: Scenes: - - SceneInfoStruct # Targeting Spring 2024 Matter release + - SceneInfoStruct + deprecated: event fields: WiFiNetworkDiagnostics: