dynamic endpoints: add automatic attr storage, instantiation from ZAP templates #28372
+198
−15
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
|
PR #28372: Size comparison from c1d26c0 to 2e48bd5 Increases above 0.2%:
Increases (10 builds for bl602, bl702, bl702l, linux, mbed, qpg)
Decreases (6 builds for bl602, bl702, nrfconnect, qpg)
Full report (13 builds for bl602, bl702, bl702l, linux, mbed, nrfconnect, qpg)
|
plan44
force-pushed
the
PR/better-dynamic-ep-support
branch
from
2e48bd5
to
3865e89
Compare
|
PR #28372: Size comparison from a406232 to 3865e89 Increases above 0.2%:
Increases (37 builds for bl602, bl702, bl702l, linux, mbed, qpg, telink)
Decreases (24 builds for bl602, bl702, bl702l, linux, nrfconnect, qpg, telink)
Full report (42 builds for bl602, bl702, bl702l, linux, mbed, nrfconnect, qpg, telink)
|
plan44
force-pushed
the
PR/better-dynamic-ep-support
branch
2 times, most recently
from
35b81e5
to
4cea18b
Compare
|
PR #28372: Size comparison from 367a0c6 to 4cea18b Increases (44 builds for bl602, bl702, bl702l, esp32, linux, mbed, nrfconnect, qpg, telink)
Decreases (23 builds for bl602, bl702, bl702l, esp32, linux, nrfconnect, telink)
Full report (44 builds for bl602, bl702, bl702l, esp32, linux, mbed, nrfconnect, qpg, telink)
|
plan44
force-pushed
the
PR/better-dynamic-ep-support
branch
from
4cea18b
to
f8c1117
Compare
|
PR #28372: Size comparison from bf0b45a to f8c1117 Increases (24 builds for bl702, bl702l, esp32, linux, telink)
Decreases (18 builds for bl602, bl702l, linux, telink)
Full report (44 builds for bl602, bl702, bl702l, esp32, linux, mbed, nrfconnect, qpg, telink)
|
|
PR #28372: Size comparison from bf0b45a to b597042 Increases above 0.2%:
Increases (14 builds for bl602, bl702, bl702l, cc32xx, k32w, mbed, nrfconnect, qpg)
Decreases (2 builds for nrfconnect)
Full report (15 builds for bl602, bl702, bl702l, cc32xx, k32w, mbed, nrfconnect, qpg)
|
plan44
force-pushed
the
PR/better-dynamic-ep-support
branch
from
b597042
to
8526882
Compare
|
PR #28372: Size comparison from b0b0d58 to 8526882 Increases (27 builds for bl702, bl702l, cc32xx, esp32, linux, nrfconnect, psoc6, telink)
Decreases (28 builds for bl602, bl702, bl702l, cc32xx, efr32, linux, psoc6, telink)
Full report (60 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
There is a strange problem in the "Darwin (no-ble-asan-clang)" test, see test run here: That statement accesses a struct member conditionally defined in af-types.h:221: attribute-storage.cpp:313: This can only fail when af-struct.h gets included before As this works in all other builds, only fails in Darwin, I assume there must be something odd in the way CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT gets (re?)defined >0 somehow after I'll make this an issue after trying another run with the PR rebased to see if that changes anything (and to get around the Open IoT SDK permafail that exists at bf0b45a) |
plan44
force-pushed
the
PR/better-dynamic-ep-support
branch
from
8526882
to
4eb71f5
Compare
|
PR #28372: Size comparison from b0b0d58 to 4eb71f5 Increases above 0.2%:
Increases (28 builds for bl702, bl702l, cc32xx, esp32, linux, nrfconnect, psoc6, telink)
Decreases (23 builds for bl702, bl702l, cc32xx, efr32, linux, psoc6, telink)
Full report (60 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
@bzbarsky-apple any idea why |
|
@plan44
|
|
That said, I tried doing this: |
plan44
force-pushed
the
PR/better-dynamic-ep-support
branch
from
4eb71f5
to
ff6c957
Compare
...which must be something else than just building chip-tool on Darwin directly as recommended in its README.md (
Thanks to your analysis above, I think I found the problem: In I now moved the inclusion of This now makes chip-tool build again and passes global "check" (on Darwin). CI will reveal if it breaks other things… |
|
PR #28372: Size comparison from 96c6c7a to ff6c957 Increases above 0.2%:
Increases (24 builds for bl702, bl702l, cc32xx, esp32, linux, psoc6, telink)
Decreases (11 builds for bl702, bl702l, cc32xx, efr32, psoc6)
Full report (62 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
@bzbarsky-apple Uh-oh. Now clang-format insists on sorting the includes alphabetically, so it simply reverts my attempt to get platform stuff included first 😟 Now what? I understand that order of includes should not matter, but to really archieve this, every header file in the whole framework is required to include all of its potential dependencies itself. When platforms are allowed to tweak globals like CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT, that means every header would need to include I have no idea how to resolve this conflict. |
Isn't the right fix for af-types.h to include config headers if it depends on config, instead of doing weird header ordering things? |
plan44
force-pushed
the
PR/better-dynamic-ep-support
branch
from
ff6c957
to
3e2d0cb
Compare
|
PR #28372: Size comparison from 454d7d9 to 3e2d0cb Increases above 0.2%:
Increases (25 builds for bl702, bl702l, cc32xx, esp32, linux, psoc6, telink)
Decreases (11 builds for bl702, bl702l, cc32xx, efr32, psoc6)
Full report (62 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
Yes, indeed. |
|
@plan44 this needs then merging to master so I can take a look. Right now the diff shows unrelated files. |
|
For what its worth, large changes that use https://github.com/project-chip/connectedhomeip/blob/master/src/app/data-model-provider/Provider.h#L45 are already merged so one could potentially build a fully dynamic data model. However that is somewhat blocked on flash increases as well as difficulty from having the root endpoint populated in a fully code driven fashion. |
plan44
force-pushed
the
PR/better-dynamic-ep-support
branch
from
c410a81
to
423b4fd
Compare
Done.
Oh, I see the previous version was based on |
…ints Dynamically defined endpoints can now have their own block of storage (to be provided by the caller of emberAfSetDynamicEndpoint()). If no storage is set for a dynamic endpoint, it behaves exactly as before, which is treating all attributes as "external", regardless of the respective bit in the attribute's metadata. With a storage provided, attributes behave the same way as static endpoint's do, that is, only those marked "external" need to be handled programmatically in the respective callbacks, other attributes' storage is automatic.
Instead of re-creating dynamic endpoint's cluster declarations using the DECLARE_DYNAMIC_xxx macros, the new `setupDynamicEndpointDeclaration()` function allows setting up a dynamic endpoint by using clusters defined in another endpoint as templates. Usually that is a disabled endpoint created in ZAP with all possibly dynamically used clusters assigned. In combination with dynamic endpoint attribute storage, this greatly simplifies implementing dynamic endpoints and allows configuring them using the ZAP tool to ensure specs conformance, much the same way as with statically defined endpoints.
plan44
force-pushed
the
PR/better-dynamic-ep-support
branch
from
423b4fd
to
87259f0
Compare
|
PR #28372: Size comparison from d2d06b3 to 87259f0 Full report (82 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Oct 11, 2024
| @@ -24,6 +24,8 @@ | |||
| */ | |||
|
|
|||
| #include "att-storage.h" | |||
| #include <platform/CHIPDeviceConfig.h> // For CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT | |||
There was a problem hiding this comment.
please include an extra dependency on src/platform:platform_buildconfig in https://github.com/project-chip/connectedhomeip/blob/master/src/app/util/BUILD.gn#L58 (so that the dependency is direct rather than indirect)
There was a problem hiding this comment.
tried, but failed:
ERROR at //src/app/util/BUILD.gn:56:1: Dependency not allowed.
source_set("af-types") {
^-----------------------
The item //src/app/util:af-types
can not depend on //src/platform:platform_buildconfig
because it is not in //src/platform:platform_buildconfig's visibility list: [
//src/platform:platform_config_header
//src/ble:ble_config_header
//src/system:system_config_header
]
I don't see enough through the gn complexity to see what would be needed to make it work
There was a problem hiding this comment.
https://github.com/project-chip/connectedhomeip/blob/master/src/platform/BUILD.gn#L431
It seems you need to use platform_config_header
andy31415
reviewed
Oct 11, 2024
andy31415
reviewed
Oct 11, 2024
| { | ||
| // allocate cluster list | ||
| endpointType.clusterCount = static_cast<uint8_t>(templateClusterIds.size()); | ||
| endpointType.cluster = new EmberAfCluster[endpointType.clusterCount]; |
There was a problem hiding this comment.
where is the corresponding delete[] for this new? Please add a comment about lifetime and ownership of these.
There was a problem hiding this comment.
Also wondering if heap or PlatformNew is the correct path here. Ideally there would be a way to have some std::unique_ptr for auto memory management if we find a way to do that.
There was a problem hiding this comment.
There was no matching delete[]. Now I added emberAfResetDynamicEndpointDeclaration as a proper counterpart to emberAfSetupDynamicEndpointDeclaration.
I omitted that initially, because I see no real use case, and the implications of actually getting rid of a dynamic endpoint in the rest of the code seemed so far reaching that it did not seem worthwhile to consider at all.
andy31415
reviewed
Oct 11, 2024
src/app/util/attribute-storage.cpp
Outdated
| const Span<const ClusterId> & templateClusterIds) | ||
| { | ||
| // allocate cluster list | ||
| endpointType.clusterCount = static_cast<uint8_t>(templateClusterIds.size()); |
There was a problem hiding this comment.
Please add a check that template is smaller than 256 entries?
andy31415
reviewed
Oct 11, 2024
src/app/util/attribute-storage.cpp
Outdated
| return nullptr; | ||
| } | ||
|
|
||
| CHIP_ERROR setupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, EndpointId templateEndpointId, |
There was a problem hiding this comment.
This does not seem to ever return anything except CHIP_NO_ERROR (it verifies or dies). Should we change this to return instead of VerifyOrDie? Or alternatively we should make this void and document that it crashes on invalid data or internal failures.
There was a problem hiding this comment.
Thanks for pointing out. I inititially thought of handling this at runtime, so prepared for returning an error. But later I realized trying to instantiate non-existing clusters means that the app is severely broken and there's no good runtime action one could take.
So I stayed with VerifyOrDie and void and documented that.
andy31415
reviewed
Oct 11, 2024
src/app/util/attribute-storage.cpp
Outdated
| // get the actual cluster pointers and sum up memory size | ||
| for (size_t i = 0; i < templateClusterIds.size(); i++) | ||
| { | ||
| auto cluster = getClusterTypeDefinition(templateEndpointId, templateClusterIds.data()[i], 0); |
There was a problem hiding this comment.
Does just operator[] work?
Suggested change
| auto cluster = getClusterTypeDefinition(templateEndpointId, templateClusterIds.data()[i], 0); | |
| auto cluster = getClusterTypeDefinition(templateEndpointId, templateClusterIds[i], 0); |
There was a problem hiding this comment.
Needs a comment of what 0 is. If we always pass in the same value to the method call, maybe the parameter should not exist.
There was a problem hiding this comment.
- []: yes, corrected.
- the
maskparameter is an option for subselecting clusters in the underlyingemberAfFindClusterInType, so I kept that option in the new utility function, altough there's no immediate need for having it now.
andy31415
reviewed
Oct 11, 2024
src/app/util/attribute-storage.h
Outdated
| // metadata including all attributes already exists and can be re-used this way, | ||
| // without error prone manual duplicating with DECLARE_DYNAMIC_* | ||
| // | ||
| CHIP_ERROR setupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, chip::EndpointId templateEndpointId, |
There was a problem hiding this comment.
public methods should be emberAf prefixed for easy identification (since we are in C land rather than C++ namespaces)
andy31415
reviewed
Oct 11, 2024
andy31415
reviewed
Oct 11, 2024
| // instead of duplicating them here once for every instance. | ||
| memcpy((void *) &endpointType.cluster[i], cluster, sizeof(EmberAfCluster)); | ||
| // sum up the needed storage | ||
| endpointType.endpointSize = (uint16_t) (endpointType.endpointSize + cluster->clusterSize); |
There was a problem hiding this comment.
Suggested change
| endpointType.endpointSize = (uint16_t) (endpointType.endpointSize + cluster->clusterSize); | |
| endpointType.endpointSize += cluster->clusterSize; |
There was a problem hiding this comment.
Seems not changed. If you cannot use operator+= please at least use C++ static_cast here instead of C-style cast.
|
PR #28372: Size comparison from d2d06b3 to 40c48f2 Increases above 0.2%:
Full report (82 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Oct 15, 2024
andy31415
reviewed
Oct 15, 2024
andy31415
reviewed
Oct 15, 2024
| @@ -261,9 +261,62 @@ uint16_t emberAfGetDynamicIndexFromEndpoint(EndpointId id) | |||
| return kEmberInvalidEndpointIndex; | |||
| } | |||
|
|
|||
| namespace { | |||
|
|
|||
| const EmberAfCluster * getClusterTypeDefinition(EndpointId endpointId, ClusterId clusterId, EmberAfClusterMask mask) | |||
There was a problem hiding this comment.
This is called in a single place with mask 0.
Please apply YAGNI here and drop the mask parameter, assume mask is always 0 since this is the usage. We should also explain why mask 0 is used here given the above ... if clusters can be both server and client, we seem to not be able to differentiate between them.
There was a problem hiding this comment.
I am not sure that structure of clusters will be identical between client and server (i.e. attributes may differ).
andy31415
reviewed
Oct 15, 2024
| // | ||
| // Note: function may allocate memory for the endpoint declaration. | ||
| // Use emberAfResetEndpointDeclaration to properly dispose of an endpoint declaration. | ||
| void emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, chip::EndpointId templateEndpointId, |
There was a problem hiding this comment.
should we make endpointType be verifiable empty so that we avoid memory leaks if I do emberAfSetupDynamicEndpointDeclaration twice on the same endpoint without a reset?
There was a problem hiding this comment.
How could that be done? We'd need a flag differentiating dynamically set up endpoint types from those generated via ZAP/codegen. And as long as EmberAfEndpointType is a simple struct, any members, including a potential flag, are undefined before the endpoint is passed to a initializer function such as emberAfSetupDynamicEndpointDeclaration.
andy31415
reviewed
Oct 15, 2024
andy31415
reviewed
Oct 15, 2024
| if (!isDynamicEndpoint) | ||
| // and dynamic ones with dynamicAttributeStorage assigned. | ||
| if (!isDynamicEndpoint | ||
| #if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 |
There was a problem hiding this comment.
how about having hasDynamicAttributeStorage always exist and have a single ifdef (and if the count is 0, then it will always be false? ifdefs inside if clauses are hard to maintain.
There was a problem hiding this comment.
My goal was not to cause any code or storage size increase for the (majority) of non-dynamic use cases of this code.
We can of course drop that goal, and remove all the CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT ifdefs. But that would mean even static-only builds would have the dynamicAttributeStorage field (now a span, 2size_t) so a RAM increase of 2size_t*MAX_ENDPOINT_COUNT.
...to allow differentiating server and client sides of the same cluster type
to allow checking that it has sufficient size
more readable attributeLocation calculation
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.
This PR originates from the discussion in #28166 and solves two problems when working with dynamic endpoints, as is the case in bridge-type apps.
There is no change in behaviour unless dynamic endpoint storage is explicitly assigned by an app. The
DECLARE_DYNAMIC_xxxmacros can still be used to define dynamic endpoints,(but cannot use automatic attribute storage because the macros force all attributes to be "external").
automatic attribute storage for dynamic endpoints
Dynamically defined endpoints can now have their own block of storage
(to be provided by the caller of emberAfSetDynamicEndpoint()).
If no storage is set for a dynamic endpoint, it behaves exactly as before,
which is treating all attributes as "external", regardless of the respective
bit in the attribute's metadata.
With a storage block provided, attributes behave the same way as static endpoint's
do, that is, only those marked "external" need to be handled programmatically
in the respective callbacks, other attributes' storage is automatic.
creating dynamic endpoints from ZAP templates instead via macros
Instead of re-creating dynamic endpoint's cluster declarations
using the DECLARE_DYNAMIC_xxx macros, the new
setupDynamicEndpointDeclaration()function allows settingup a dynamic endpoint by using clusters defined in another
endpoint as templates. Usually that endpoint is a disabled endpoint created
in ZAP with all clusters possibly to be used dynamically assigned.
In combination with dynamic endpoint attribute storage, this
greatly simplifies implementing dynamic endpoints and allows
configuring them using the ZAP tool to ensure specs conformance,
much the same way as with statically defined endpoints.