Make test cluster code an "manufacturer extension cluster" #19288
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
pullapprove
bot
requested review from
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13,
harimau-qirex,
hawk248,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs and
jmartinez-silabs
|
PR #19288: Size comparison from e57476d to 305715f Increases (6 builds for cyw30739, efr32, telink)
Full report (13 builds for cyw30739, efr32, k32w, mbed, telink)
|
… codes. I am unsure why mfgCode was used to have a cluster code, but it seems to behave like that
andy31415
commented
Jun 7, 2022
|
PR #19288: Size comparison from e57476d to 8c5ddeb Increases above 0.2%:
Increases (12 builds for cc13x2_26x2, efr32, esp32, linux, mbed)
Decreases (41 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Full report (41 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
bzbarsky-apple
approved these changes
Jun 8, 2022
examples/tv-casting-app/tv-casting-common/tv-casting-app.matter
Outdated
Show resolved
Hide resolved
|
This seems to have uncovered bugs (at least in our tests, but likely in our code). I see TestServerCommandDispatch, TestEventChunkig and TestRead failing (and maybe more). Need to investigate why. |
|
|
PR #19288: Size comparison from b8e9ab1 to 12a8805 Increases (14 builds for cc13x2_26x2, cyw30739, k32w, linux, nrfconnect, p6, telink)
Decreases (3 builds for cc13x2_26x2)
Full report (23 builds for cc13x2_26x2, cyw30739, k32w, linux, mbed, nrfconnect, p6, telink)
|
…: "onSuccessWasCalled && !onFailureWasCalled"" is not as useful
|
Failures seems to be due to the cluster validity check: This seems to say that vendor extensions have a limit range of cluster ids. Will re-read the spec to understand if my change picked a wrong ID or if the test is wrong. |
…estrictions .. even more restricted it seems
…estrictions .. even more restricted it seems
|
PR #19288: Size comparison from b8e9ab1 to c2b4d85 Increases (17 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, p6, telink)
Decreases (4 builds for cc13x2_26x2, esp32)
Full report (30 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
…r the test cluster
…the id is too large to be acccepted by protoc
tcarmelveilleux
approved these changes
Jun 9, 2022
mkardous-silabs
pushed a commit
to mkardous-silabs/connectedhomeip
that referenced
this pull request
Jun 10, 2022
…hip#19288) * Move 0x050F to 0xFFF1050F for test cluster * Update test cluster code in zap files * Ran zap regen * For purposes of naming, switch data sizes to uint32_t to support full codes. I am unsure why mfgCode was used to have a cluster code, but it seems to behave like that * Remove MFG code from println code: it does not seem used * Restyle * Single extra error log for failure since the log of "assertion failed: "onSuccessWasCalled && !onFailureWasCalled"" is not as useful * Switch cluster ID from 0xFFF1050F to 0xFFF1F50F to match spec range restrictions * Switch cluster ID from 0xFFF1F50F to 0xFFF1FC05 to match spec range restrictions .. even more restricted it seems * Switch cluster ID from 0xFFF1F50F to 0xFFF1FC05 to match spec range restrictions .. even more restricted it seems * Remove unused argument * Update unit test getting of cluster info to use the new cluster id for the test cluster * Update protobuf for pigwed with the updated cluster id for the test cluster * Update tvapp constant for test cluster. Why does TV app use a test cluster? * Use ChipClusterId data type * Isolate the protobuf definition in an enum for the test cluster id - the id is too large to be acccepted by protoc * Define the constant after all, with a big explanation about why it is likely bad
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.
Problem
Spec does not define a test cluster, however we need it to test things. Existing code makes it look like a "standard cluster",
#16886 asks for manufacturer specific clusters - this makes the test cluster one of those.
Change overview
Change code 0x050F to 0xFFF1050F to match a "test MC 0xFFF1 prefix" according to chip spec.
Testing
CI will validate that test cluster tests still pass.