[Scenes] FabricSceneInfo #30038
Merged
[Scenes] FabricSceneInfo #30038
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a80f215 to
50dc945
Compare
|
PR #30038: Size comparison from eec1318 to 6a24597 Increases above 0.2%:
Increases (32 builds for bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, psoc6, telink)
Decreases (12 builds for cc13x4_26x4, esp32, linux, nrfconnect, psoc6)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
6a24597 to
3bf92f4
Compare
|
PR #30038: Size comparison from 05f985a to 3bf92f4 Increases above 0.2%:
Increases (22 builds for cc13x4_26x4, cyw30739, efr32, esp32, linux, nrfconnect, psoc6, telink)
Decreases (11 builds for cc13x4_26x4, esp32, linux, nrfconnect, psoc6)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
PR #30038: Size comparison from 05f985a to 3f1349d Increases above 0.2%:
Increases (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, nrfconnect, psoc6, qpg, telink)
Decreases (23 builds for cc13x4_26x4, esp32, linux, nrfconnect, psoc6, qpg)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
3f1349d to
cc2a3f6
Compare
|
PR #30038: Size comparison from c76fcda to cc2a3f6 Increases above 0.2%:
Increases (22 builds for cc13x4_26x4, cyw30739, efr32, esp32, linux, nrfconnect, psoc6, telink)
Decreases (13 builds for cc13x4_26x4, esp32, linux, nrfconnect, psoc6)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
woody-apple
approved these changes
Nov 16, 2023
cc2a3f6 to
f181481
Compare
|
PR #30038: Size comparison from 835b58f to f181481 Increases above 0.2%:
Increases (22 builds for cc13x4_26x4, cyw30739, efr32, esp32, linux, nrfconnect, psoc6, telink)
Decreases (13 builds for cc13x4_26x4, esp32, linux, nrfconnect, psoc6)
Full report (63 builds for cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
…support in scenes-server
…still needs Multi fabric testing
f181481 to
64726b1
Compare
|
PR #30038: Size comparison from d7c7f8e to 64726b1 Increases above 0.2%:
Increases (19 builds for cc13x4_26x4, cyw30739, efr32, esp32, linux, nrfconnect, psoc6)
Decreases (13 builds for cc13x4_26x4, esp32, linux, nrfconnect, psoc6)
Full report (56 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg)
|
|
PR #30038: Size comparison from bb2d05e to c00bc5e Increases above 0.2%:
Increases (22 builds for cc13x4_26x4, cyw30739, efr32, esp32, linux, nrfconnect, psoc6, telink)
Decreases (13 builds for cc13x4_26x4, esp32, linux, nrfconnect, psoc6)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
| Span<Structs::SceneInfoStruct::Type> ScenesServer::FabricSceneInfo::GetFabricSceneInfo(EndpointId endpoint) | ||
| { | ||
| size_t endpointIndex = 0; | ||
| Span<Structs::SceneInfoStruct::Type> FabricSceneInfoSpan; |
There was a problem hiding this comment.
Local variables should have names starting with lowercase letters.
| /// @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 |
There was a problem hiding this comment.
How about just saying that if not found the value of this outparam should not be assumed to be anything in particular and callers must not use it? Or do they in fact use it?
| @@ -402,6 +602,9 @@ CHIP_ERROR RecallSceneParse(const FabricIndex & fabricIdx, const EndpointId & en | |||
| const SceneId & sceneID, const Optional<DataModel::Nullable<uint16_t>> & transitionTime, | |||
| GroupDataProvider * groupProvider) | |||
| { | |||
| // Make SceneValid false for all fabrics before recall scenes | |||
There was a problem hiding this comment.
"before recalling a scene"
| @@ -341,6 +541,9 @@ 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 | |||
There was a problem hiding this comment.
"before storing a scene"
| @@ -935,6 +1162,13 @@ void emberAfScenesClusterServerInitCallback(EndpointId endpoint) | |||
| { | |||
| ChipLogDetail(Zcl, "ERR: setting LastConfiguredBy on Endpoint %hu Status: %x", endpoint, status); | |||
| } | |||
|
|
|||
| // Initialize the FabricSceneInfo list from data in Flash | |||
There was a problem hiding this comment.
This comment is still here and is still confusing...
| return aEncoder.Encode(value); | ||
| case Attributes::FabricSceneInfo::Id: { | ||
| return aEncoder.EncodeList([&](const auto & encoder) -> CHIP_ERROR { | ||
| Span<Structs::SceneInfoStruct::Type> FabricSceneInfoSpan = mFabricSceneInfo.GetFabricSceneInfo(aPath.mEndpointId); |
There was a problem hiding this comment.
fabricSceneInfoSpan for the variable name, please.
|
|
||
| private: | ||
| CHIP_ERROR FindFabricSceneInfoIndex(EndpointId endpoint, size_t & endpointIndex); | ||
| CHIP_ERROR FindSceneInfoStructIndex(FabricIndex fabric, size_t endpointIndex, uint8_t & index); |
There was a problem hiding this comment.
So this index is safe to be uint8_t because it's no more than CHIP_CONFIG_MAX_FABRICS? Would be good to have that documented.
There was a problem hiding this comment.
Added to the parameter description and copied in the .h file
| attributes: | ||
| Scenes: | ||
| - FabricSceneInfo | ||
| # Targeting Spring 2024 Matter release |
There was a problem hiding this comment.
The comments should be above the thing being commented, not below...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Modified Attribute 7 from scene cluster to SceneInfoStruct and added support in scenes-server, zap, yaml, etc.
fixes #28957
fixes #28956
fixes #26900
fixes #28221