dynamic endpoints: add automatic attr storage, instantiation from ZAP templates #28372
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. |
|
PR #28372: Size comparison from dc52d9d to fd5b66c Increases above 0.2%:
Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Oct 30, 2024
andy31415
reviewed
Oct 30, 2024
plan44
force-pushed
the
PR/better-dynamic-ep-support
branch
from
87a8f51 to
8350c4d
Compare
|
Rebased again in hope to get past (IMHO completely unrelated) check failures |
andy31415
reviewed
Nov 13, 2024
andy31415
approved these changes
Nov 13, 2024
|
PR #28372: Size comparison from 45a75ba to 8350c4d Full report (5 builds for cc32xx, stm32, tizen)
|
|
PR #28372: Size comparison from 45a75ba to 7c4c25c Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
I cannot relate the failing checks to the PR itself. Please let me know in case this is not correct and there's something I need to change in my PR to make it pass. |
|
@plan44 these tests are generally quite stable and the same tests seems to fail on several platforms. It is the PR itself somehow, however unsure how. This generally needs debugging. I would try with |
|
@plan44 I would maybe consider to not have |
Would be technically easy, the ifdefs are only there to avoid the extra 8 bytes per cluster in the non-dynamic case. But I guess these wasted bytes would be hard to swallow for all non-bridge-apps.
That would mean there are tests/builds where |
…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.
...to allow differentiating server and client sides of the same cluster type
to allow checking that it has sufficient size
more readable attributeLocation calculation, remove unneeded ugly C cast
- af-types needs platform/CHIPDeviceConfig.h for CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT - reflect this in BUILD.gn by adding the platform_config_header sourceset as dependency - remove unneeded comment lines in agreement with original author
- check passed in endpointType is explicitly "empty" (zeroed out) - use CanCastTo instead of literally testing against 255 - return appropriate errors - only return completely set-up or untouched endpointType - no const cast needed
… code Turns out that while this works well on current compilers, it does not with many embedded toolchains. - idea was that an if testing an always false constexpr bool should skip *compiling* its body. - we need that because that body refers to a struct field that is non-existing in the case when the if condition is constexpr false - note that we can't use a real >=C++17 constexpr if, because in the CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT>0 case, the condition cannot be a constexpr Bottom line: there seems no way around the uglier #ifdef around the `else if` clause. We can avoid an #ifdef in the middle of a condition expression.
… int size result on some platforms - took the chance to add safe error exit on excessive cumulative cluster storage size. - using `typeof()` and `CanCastTo` throughout to make code independent of struct field sizes.
Co-authored-by: Andrei Litvin <[email protected]>
…R review process In "[attribute storage] apply suggestion from PR review" the somewhat hard-to-read conditional expression was refactored to an if/else, but unfortunately left out adding the actual attribute offset. This ruined the attribute storage pretty badly (all attributes written to the same memory location), but still passed surprisingly many tests.
plan44
force-pushed
the
PR/better-dynamic-ep-support
branch
from
7c4c25c to
6cd2a7e
Compare
|
PR #28372: Size comparison from 403d595 to 6cd2a7e Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Found a bad bug that slipped in with the conditional expression refactoring. Fixed now, see latest commit. Much more green, but I have no idea yet what causes the remaining tests to fail. |
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.