[Silabs] Fix Test Event Trigger processing #36981
Draft
+411
−119
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
|
Review changes with |
|
PR #36981: Size comparison from e7f6d0e to 8e2664c Full report (70 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
mkardous-silabs
force-pushed
the
fix/test_event_trigger
branch
from
d98fd60 to
168cd50
Compare
jmartinez-silabs
approved these changes
Jan 9, 2025
jmartinez-silabs
added
the
changing-submodules-on-purpose
|
PR #36981: Size comparison from f044060 to 9fb1053 Full report (70 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
lpbeliveau-silabs
approved these changes
Jan 13, 2025
| @@ -236,9 +236,12 @@ CHIP_ERROR SilabsMatterConfig::InitMatter(const char * appName) | |||
|
|
|||
| // Provision Manager | |||
| Silabs::Provision::Manager & provision = Silabs::Provision::Manager::GetInstance(); | |||
| Silabs::Provision::Storage & storage = provision.GetStorage(); | |||
There was a problem hiding this comment.
I'm never a big fan of creating intermittent variables to store reference. This just results in 4 extra bytes being pushed on top of the stack with no code size reduction whatsoever, actually the opposite since we added one extra line. So bigger code size, bigger stack usage.
Is this really worth the clarity gain? was &provision.GetStorage() event unclear to begin with?
| @@ -662,12 +666,7 @@ CHIP_ERROR Storage::SetOtaTlvEncryptionKey(const ByteSpan & value) | |||
| } | |||
| #endif // OTA_ENCRYPTION_ENABLE | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Maybe we want to keep/update the comment?
|
|
||
| // If mProvider is equal to nullptr, we still continue in the function to check if the requested enableKey matches the zero | ||
| // key. | ||
| if (mProvider != nullptr) |
There was a problem hiding this comment.
I think I understood the logic, which I agree with. However, I was wondering, would it be possible to instead reserve an invalid key or un-initialized key value (such as an array filled with 0xFF) so that we do not need to split the checks into 2 steps? That way, a failure would not results in a 0 bytespan, which would solve the issue of the zero bytespan match.
It would simplify this function and require less reading to understand the logic.
feasel0
reviewed
Jan 17, 2025
| #include <app/TestEventTriggerDelegate.h> | ||
| #include <lib/core/CHIPError.h> | ||
| #include <lib/support/CodeUtils.h> | ||
| #include <lib/support/Span.h> | ||
| #include <platform/silabs/provision/ProvisionedDataProvider.h> | ||
| #include <stdint.h> |
There was a problem hiding this comment.
Standard library includes usually come before project includes.
feasel0
reviewed
Jan 17, 2025
| tests = [] | ||
|
|
||
| if (sl_build_unit_tests) { | ||
| tests += [ "${chip_root}/examples/platform/silabs/tests:examples_tests" ] |
There was a problem hiding this comment.
Will it work without the path?
tests += [ ":examples_tests" ]
feasel0
reviewed
Jan 17, 2025
| #include <pw_unit_test/framework.h> | ||
|
|
||
| #include <SilabsTestEventTriggerDelegate.h> | ||
| #include <lib/support/Span.h> |
There was a problem hiding this comment.
Could add #include <lib/core/StringBuilderAdapters.h> here since you have string-builder-adapters in the public_deps.
feasel0
approved these changes
Jan 17, 2025
mkardous-silabs
force-pushed
the
fix/test_event_trigger
branch
from
b4d2a1a to
b7db67e
Compare
|
PR #36981: Size comparison from f7226c1 to 380aa60 Full report (21 builds for bl602, bl702, bl702l, cc13x4_26x4, esp32, nrfconnect, nxp, stm32)
|
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.
Description
When the SDK check if the test event trigger is enabled, it checks if the test event trigger enableKey is equal to a zero Bytespan. If the enableKey matches, returns false that the feature is not enabled.
In the current implementation, if we fail to read the enable key, we immediatly return that the key does not match. If the key we are trying to match was the zero bytespan, this triggers the stack to think the feature is enabled since the platform enable key does not match the zero bytepspan which is incorrect.
The other issue was that we return that the enableKeys don't match if !enableKeySpan.empty() . When checking with the zero bytespan, this returns false immediatly without checking the platform enable key,
The PR changes the logic to always check if the data matches or not which, trough the bytespan function, covers all the edge cases.
PR also does a small cleanup of the provisioning test event trigger related code.
PR adds the silabs examples tests directory to the list of test directories
Testing