-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add new cluster washer-control-cluster #26649
Conversation
PR #26649: Size comparison from ca83f16 to c135fb0 Increases (4 builds for bl602, psoc6)
Decreases (2 builds for cyw30739, psoc6)
Full report (40 builds for bl602, bl702, cc32xx, cyw30739, efr32, k32w, linux, mbed, nrfconnect, psoc6, qpg)
|
PR #26649: Size comparison from ca83f16 to d1ed5d9 Increases above 0.2%:
Increases (36 builds for bl602, bl702, cc32xx, cyw30739, efr32, esp32, k32w, linux, nrfconnect, psoc6, qpg, telink)
Decreases (4 builds for bl702, esp32, telink)
Full report (57 builds for bl602, bl702, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
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 XML seems mostly unrelated to the Washer Controls spec as it currently exists. I checked for in-flight spec PRs that would affect that, and don't see any.
Please carefully go through the XML and make it match the spec, then request review.
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.
Again, please leave implementation out until the XML has been reviewed and merged. That means: don't add this file, don't add things to zap_cluster_list.json, don't add example app bits.
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 have updated it. Please review. Thank you
<configurator> | ||
<domain name="CHIP"/> | ||
|
||
<enum name="WasherControlsEnum" type="ENUM8"> |
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 type does not exist in the spec.
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.
It's fixed. Please review again. Thank you.
<item name="Error" value="0x03"/> | ||
</enum> | ||
|
||
<enum name="ErrorStateEnum" type="ENUM8"> |
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.
What does this have to do with Washer Controls?
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.
It's fixed. Please review again. Thank you.
<item name="WasherControlsID" type="WasherControlsEnum" optional="false"/> | ||
<item name="WasherControlsLabel" type="CHAR_STRING" optional="false"/> |
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.
These field names do not match the spec.
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.
It's fixed. Please review again. Thank you.
<cluster> | ||
<domain>General</domain> | ||
<name>Washer Controls</name> | ||
<code>0x0061</code> |
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 cluster id does not match the spec.
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.
It's fixed. Please review. Thank you
PR #26649: Size comparison from ca83f16 to 3be2448 Increases above 0.2%:
Increases (34 builds for bl602, bl702, cc32xx, cyw30739, efr32, k32w, linux, nrfconnect, psoc6, qpg, telink)
Decreases (5 builds for psoc6, telink)
Full report (55 builds for bl602, bl702, cc32xx, cyw30739, efr32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
PR #26649: Size comparison from 21147a5 to 982ab68 Increases (12 builds for bl602, cc32xx, psoc6, telink)
Decreases (7 builds for bl702, efr32, k32w, qpg, telink)
Full report (55 builds for bl602, bl702, cc32xx, cyw30739, efr32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
||
<globalAttribute side="server" code="0xFFFD" value="1" /> | ||
|
||
<attribute side="server" code="0x0000" define="SPIN_SPEEDS" type="ARRAY" entryType="SpinSpeedIndex" writable="false" isNullable="false" optional="false">SpinSpeeds</attribute> |
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.
<attribute side="server" code="0x0000" define="SPIN_SPEEDS" type="ARRAY" entryType="SpinSpeedIndex" writable="false" isNullable="false" optional="false">SpinSpeeds</attribute> | |
<attribute side="server" code="0x0000" define="SPIN_SPEEDS" type="ARRAY" entryType="SpinSpeedIndex" writable="false" isNullable="false" optional="true">SpinSpeeds</attribute> |
It's feature-dependent, so optional.
<globalAttribute side="server" code="0xFFFD" value="1" /> | ||
|
||
<attribute side="server" code="0x0000" define="SPIN_SPEEDS" type="ARRAY" entryType="SpinSpeedIndex" writable="false" isNullable="false" optional="false">SpinSpeeds</attribute> | ||
<attribute side="server" code="0x0001" define="SPIN_SPEED_CURRENT" type="INT8U" min="0x00" max="0x1F" writable="true" isNullable="true" optional="true">SpinSpeedCurrent</attribute> |
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.
<attribute side="server" code="0x0001" define="SPIN_SPEED_CURRENT" type="INT8U" min="0x00" max="0x1F" writable="true" isNullable="true" optional="true">SpinSpeedCurrent</attribute> | |
<attribute side="server" code="0x0001" define="SPIN_SPEED_CURRENT" type="INT8U" min="0x00" max="0x0F" writable="true" isNullable="true" optional="true">SpinSpeedCurrent</attribute> |
Spec says "0 to 15", no?
|
||
<attribute side="server" code="0x0000" define="SPIN_SPEEDS" type="ARRAY" entryType="SpinSpeedIndex" writable="false" isNullable="false" optional="false">SpinSpeeds</attribute> | ||
<attribute side="server" code="0x0001" define="SPIN_SPEED_CURRENT" type="INT8U" min="0x00" max="0x1F" writable="true" isNullable="true" optional="true">SpinSpeedCurrent</attribute> | ||
<attribute side="server" code="0x0002" define="NUMBER_OF_RINSES" type="INT8U" min="0x00" max="0x003" writable="true" isNullable="true" optional="true">NumberOfRinses</attribute> |
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 did this max="0x003"
come from?
<attribute side="server" code="0x0000" define="SPIN_SPEEDS" type="ARRAY" entryType="SpinSpeedIndex" writable="false" isNullable="false" optional="false">SpinSpeeds</attribute> | ||
<attribute side="server" code="0x0001" define="SPIN_SPEED_CURRENT" type="INT8U" min="0x00" max="0x1F" writable="true" isNullable="true" optional="true">SpinSpeedCurrent</attribute> | ||
<attribute side="server" code="0x0002" define="NUMBER_OF_RINSES" type="INT8U" min="0x00" max="0x003" writable="true" isNullable="true" optional="true">NumberOfRinses</attribute> | ||
<attribute side="server" code="0x0003" define="MAXRINSES" type="INT8U" writable="true" optional="false">MaxRinses</attribute> |
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 not writable, but is optional, in the spec, no?
<configurator> | ||
<domain name="CHIP"/> | ||
|
||
<bitmap name="WasherControlsFeatures" type="BITMAP8"> |
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.
<bitmap name="WasherControlsFeatures" type="BITMAP8"> | |
<bitmap name="Feature" type="BITMAP8"> |
<item name="Spin" value="0x00"/> | ||
<item name="Rinse" value="0x01"/> |
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 don't think those will work right. Please compare to other bitmaps....
Signed-off-by: Chin-Ran Lo <chin-ran.lo@nxp.com>
PR #26649: Size comparison from cb4dec6 to e143fa6 Increases (13 builds for bl602, bl702, cc32xx, k32w, telink)
Decreases (6 builds for efr32, k32w, telink)
Full report (37 builds for bl602, bl702, cc32xx, efr32, esp32, k32w, linux, mbed, nrfconnect, qpg, telink)
|
No description provided.