[un-revert] Occupancy sensing updates for Matter 1.4 #34593
Merged
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
…(all-clusters-app) implementation to Rev 5 (Matter 1.4) (project-chip#34293) * [occupancy-sensing]Updated occupancy sensing cluster SDK & Sample app(all-clusters-app) implementation to Rev 5 (Matter 1.4) Signed-off-by: Oliver Fan <[email protected]> * Restyled by whitespace * Restyled by clang-format * Apply suggestions from code review Update Copyright suggestions Co-authored-by: Boris Zbarsky <[email protected]> * Apply suggestions from code review, remove some dead code. Signed-off-by: Oliver Fan <[email protected]> * Restyled by clang-format * 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 <[email protected]> * Restyled by whitespace * Restyled by clang-format * Restyled by clang-format * Restyled by clang-format * Restyled by clang-format * Update examples/all-clusters-app/all-clusters-common/src/occupancy-sensing-stub.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Apply suggestions from code review change mHoldTimeLimitsStructs and mHoldTime to sHoldTimeLimitsStructs and sHoldTime put above two variables in an anonymous namespace Signed-off-by: Oliver Fan <[email protected]> * Restyled by clang-format --------- Signed-off-by: Oliver Fan <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
|
Review changes with SemanticDiff. Analyzed 1 of 9 files.
|
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
axelnxp,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chapongatien,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
doru91,
fessehaeve,
harimau-qirex,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy and
joonhaengHeo
pullapprove
bot
requested review from
tecimovic,
tehampson,
tima-q and
tobiasgraf
pullapprove
bot
requested review from
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
|
PR #34593: Size comparison from 3d9ada2 to 6971f61 Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
…witch app. This app does not actually use occupancy-sensor-server, which means that ZAP-provided information the cluster server implementation needs is not available. And light-switch-app does not use occupancy sensor in any way.
bzbarsky-apple
approved these changes
Jul 29, 2024
|
Fast-tracking, since assuming CI passes this is just a clone of #34293 (which had approvals), with a CI fix. |
cecille
approved these changes
Jul 29, 2024
fessehaeve
approved these changes
Jul 29, 2024
|
PR #34593: Size comparison from 3d9ada2 to d534299 Increases above 0.2%:
Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #34593: Size comparison from 3d9ada2 to 4770b9f Increases above 0.2%:
Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Comment on lines
+31
to
+33
| static std::unique_ptr<OccupancySensingAttrAccess> | ||
| gAttrAccess[MATTER_DM_OCCUPANCY_SENSING_CLUSTER_SERVER_ENDPOINT_COUNT + CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT]; | ||
|
|
There was a problem hiding this comment.
@OliverFan1 This should be mapped to the EndpointId and not the array index. If I have OccupancySensingCluster in endpoints 1, 2, and 5, the code will crash on endpoint 5.
There was a problem hiding this comment.
This will be fixed in the follow-up PR #34658
|
|
||
| void emberAfOccupancySensingClusterInitCallback(EndpointId endpointId) | ||
| { | ||
| VerifyOrDie(!gAttrAccess[endpointId]); |
There was a problem hiding this comment.
It's crashing for ESP over here because the array is mapped by the index and not by endpointId.
j-ororke
pushed a commit
to j-ororke/connectedhomeip
that referenced
this pull request
Jul 31, 2024
) * [occupancy-sensing]Updated occupancy sensing cluster SDK & Sample app(all-clusters-app) implementation to Rev 5 (Matter 1.4) (project-chip#34293) * [occupancy-sensing]Updated occupancy sensing cluster SDK & Sample app(all-clusters-app) implementation to Rev 5 (Matter 1.4) Signed-off-by: Oliver Fan <[email protected]> * Restyled by whitespace * Restyled by clang-format * Apply suggestions from code review Update Copyright suggestions Co-authored-by: Boris Zbarsky <[email protected]> * Apply suggestions from code review, remove some dead code. Signed-off-by: Oliver Fan <[email protected]> * Restyled by clang-format * 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 <[email protected]> * Restyled by whitespace * Restyled by clang-format * Restyled by clang-format * Restyled by clang-format * Restyled by clang-format * Update examples/all-clusters-app/all-clusters-common/src/occupancy-sensing-stub.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Apply suggestions from code review change mHoldTimeLimitsStructs and mHoldTime to sHoldTimeLimitsStructs and sHoldTime put above two variables in an anonymous namespace Signed-off-by: Oliver Fan <[email protected]> * Restyled by clang-format --------- Signed-off-by: Oliver Fan <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]> * Don't try to built occupancy-sensor-server as part of the ESP light-switch app. This app does not actually use occupancy-sensor-server, which means that ZAP-provided information the cluster server implementation needs is not available. And light-switch-app does not use occupancy sensor in any way. * Don't try to built occupancy-sensor-server as part of the ESP energy-management-app. --------- Signed-off-by: Oliver Fan <[email protected]> Co-authored-by: Oliver FAN <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
rochaferraz
pushed a commit
to rochaferraz/connectedhomeip
that referenced
this pull request
Jul 31, 2024
) * [occupancy-sensing]Updated occupancy sensing cluster SDK & Sample app(all-clusters-app) implementation to Rev 5 (Matter 1.4) (project-chip#34293) * [occupancy-sensing]Updated occupancy sensing cluster SDK & Sample app(all-clusters-app) implementation to Rev 5 (Matter 1.4) Signed-off-by: Oliver Fan <[email protected]> * Restyled by whitespace * Restyled by clang-format * Apply suggestions from code review Update Copyright suggestions Co-authored-by: Boris Zbarsky <[email protected]> * Apply suggestions from code review, remove some dead code. Signed-off-by: Oliver Fan <[email protected]> * Restyled by clang-format * 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 <[email protected]> * Restyled by whitespace * Restyled by clang-format * Restyled by clang-format * Restyled by clang-format * Restyled by clang-format * Update examples/all-clusters-app/all-clusters-common/src/occupancy-sensing-stub.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Apply suggestions from code review change mHoldTimeLimitsStructs and mHoldTime to sHoldTimeLimitsStructs and sHoldTime put above two variables in an anonymous namespace Signed-off-by: Oliver Fan <[email protected]> * Restyled by clang-format --------- Signed-off-by: Oliver Fan <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]> * Don't try to built occupancy-sensor-server as part of the ESP light-switch app. This app does not actually use occupancy-sensor-server, which means that ZAP-provided information the cluster server implementation needs is not available. And light-switch-app does not use occupancy sensor in any way. * Don't try to built occupancy-sensor-server as part of the ESP energy-management-app. --------- Signed-off-by: Oliver Fan <[email protected]> Co-authored-by: Oliver FAN <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
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.
PR #34293 was merged by mistake with ESP32 break. This PR brings back the code to fix and land.
Updated occupancy sensing cluster SDK implementation based on XML changes(PR#34163) for Occupancy Sensing rev 5
Updated occupancy sensing cluster sample app(all-clusters-app) implementation to Rev 5 in the zap and matter file of all-clusters-app
Remove "occupancy-sensor-server" from "ClientDirectories" in zap_cluster_list.json to resolve a building issue found from rebuilding the chip-tool