-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dynamic endpoints: add automatic attr storage, instantiation from ZAP templates #28372
base: master
Are you sure you want to change the base?
Conversation
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)
|
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)
|
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)
|
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)
|
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) |
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:
|
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? |
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. |
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.
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)
|
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/project-chip/connectedhomeip/blob/master/src/platform/BUILD.gn#L431
It seems you need to use platform_config_header
{ | ||
// allocate cluster list | ||
endpointType.clusterCount = static_cast<uint8_t>(templateClusterIds.size()); | ||
endpointType.cluster = new EmberAfCluster[endpointType.clusterCount]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a check that template is smaller than 256 entries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/app/util/attribute-storage.cpp
Outdated
return nullptr; | ||
} | ||
|
||
CHIP_ERROR setupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, EndpointId templateEndpointId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does just operator[] work?
auto cluster = getClusterTypeDefinition(templateEndpointId, templateClusterIds.data()[i], 0); | |
auto cluster = getClusterTypeDefinition(templateEndpointId, templateClusterIds[i], 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- []: yes, corrected.
- the
mask
parameter 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public methods should be emberAf
prefixed for easy identification (since we are in C land rather than C++ namespaces)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endpointType.endpointSize = (uint16_t) (endpointType.endpointSize + cluster->clusterSize); | |
endpointType.endpointSize += cluster->clusterSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
|
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure that structure of clusters will be identical between client and server (i.e. attributes may differ).
// | ||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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_xxx
macros 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.