Chef: Add APIs to provide deviceType info on Endpoints. Fix composition initialisation (full-family vs tree) in ember. #38142
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
…on initialization in attribute storage.
|
PR #38142: Size comparison from 4616f83 to dc03344 Full report (86 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #38142: Size comparison from 4616f83 to e528154 Increases above 0.2%:
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #38142: Size comparison from 79b53e9 to 8426fb2 Full report (3 builds for cc32xx, stm32)
|
|
PR #38142: Size comparison from 79b53e9 to 27faf55 Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Mar 28, 2025
| @@ -195,7 +195,7 @@ enum class EmberAfEndpointOptions : uint8_t | |||
| { | |||
| isEnabled = 0x1, | |||
| isFlatComposition = 0x2, | |||
| isTreeComposition = 0x3, | |||
| isTreeComposition = 0x4, | |||
There was a problem hiding this comment.
why is this a bitmap now? I would have imagined flat & tree would be mutually exclusive.
Please add comments explaining usage and meaning. What does isEnabled mean here?
There was a problem hiding this comment.
This was always a bitmap. It's the flags that are runtime-kept by Ember.
andy31415
reviewed
Mar 28, 2025
andy31415
reviewed
Mar 28, 2025
| @@ -21,32 +21,41 @@ | |||
| #include <app/clusters/temperature-control-server/supported-temperature-levels-manager.h> | |||
| #include <app/util/config.h> | |||
|
|
|||
| namespace ChefTemperatureControl { | |||
| struct EndpointPair | |||
There was a problem hiding this comment.
please add comments: is this a pair of what (given that it has 3 members, not 2). What does mSize represent? What is the lifetime of mTemperatureLEvels?
| DataModel::ListBuilder<EndpointId> DataModelUtils::GetAllEndpointsHavingDeviceType(DeviceTypeId devieType) | ||
| { | ||
| DataModel::ListBuilder<DataModel::EndpointEntry> endpointsList; | ||
| InteractionModelEngine::GetInstance()->GetDataModelProvider()->Endpoints(endpointsList); |
There was a problem hiding this comment.
Is there a better way to get the data model provider, like via Server::Instance ?
| @@ -21,32 +21,41 @@ | |||
| #include <app/clusters/temperature-control-server/supported-temperature-levels-manager.h> | |||
| #include <app/util/config.h> | |||
|
|
|||
| namespace ChefTemperatureControl { | |||
There was a problem hiding this comment.
Why not have chef::TemperatureControl rather than Smurf-Naming "ChefTemperatureControl" ?
There was a problem hiding this comment.
Also, it's not clear why this wouldn't just be along with other chef::Configuration. Suggest using that name for all the bits and pieces related to chef endpoint/cluster configuration.
| struct EndpointPair | ||
| { | ||
| chip::EndpointId mEndpointId; | ||
| const chip::CharSpan * mTemperatureLevels; |
There was a problem hiding this comment.
Why a pointer to charspans? Charspans are already non-owning
There was a problem hiding this comment.
mTemperatureLevels is an array of Charspans.
Comment on lines
76
to
82
| ChipLogDetail(DeviceLayer, "Initializing TemperatureControl cluster for Endpoint: %d", endpoint); | ||
| uint16_t epIndex = emberAfGetClusterServerEndpointIndex(endpoint, TemperatureControl::Id, | ||
| MATTER_DM_TEMPERATURE_CONTROL_CLUSTER_SERVER_ENDPOINT_COUNT); | ||
| sAppSupportedTemperatureLevelsDelegate.SetSupportedEndpointPair( | ||
| epIndex, | ||
| ChefTemperatureControl::EndpointPair(endpoint /* endpointId */, ChefTemperatureControl::temperatureLevelOptions, | ||
| MATTER_ARRAY_SIZE(ChefTemperatureControl::temperatureLevelOptions))); |
There was a problem hiding this comment.
We can't use the same temperature control for every instance --> some need TL, some TN. For refrigerators and oven, TN is the default. For refrigerator, there may be a TL. This has to be example per example.
For now, please remove the TL stuff until we use it.
There was a problem hiding this comment.
The server cluster code decides to use this based on feature map (TL vs TN).
We can add a check here to initialise only if TL is set in feature map for the endpoint. But there is no harm in just initialising for all.
This file can be a generic implementation for temperature levels and can be used by any chef example using TLs in one or more clusters.
|
PR #38142: Size comparison from 97ffa50 to faaee88 Full report (7 builds for cc32xx, qpg, stm32, tizen)
|
|
PR #38142: Size comparison from 97ffa50 to fbed38a Full report (86 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #38142: Size comparison from 97ffa50 to b6fbf5e Full report (3 builds for cc32xx, stm32)
|
|
PR #38142: Size comparison from 97ffa50 to 9a688f7 Full report (26 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, qpg, stm32, telink, tizen)
|
|
PR #38142: Size comparison from 97ffa50 to fa062ed Full report (43 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, psoc6, qpg, stm32, telink, tizen)
|
|
PR #38142: Size comparison from 97ffa50 to f731b94 Full report (86 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
requested changes
Apr 1, 2025
Comment on lines
+181
to
+185
| if ((deviceType.deviceTypeId == kRootnodeId) || (deviceType.deviceTypeId == kAggregatorId)) | ||
| { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
This is wrong. Someone could define a vendor-prefixed device type that is full-family.
This information should be in the device type XML, and then should be in endpoint_config.h and used from there in emberAfEndpointConfigure as needed, for fixed endpoints.
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.
Summary -
EmberAfEndpointOptionsenum values.Testing
Tested with oven device from PR: #38108
src/python_testing/TC_DeviceBasicComposition.py on oven device.