Skip to content
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

Pump Control and Configuration ControlMode and OperationMode should have min/max specified in XML #17112

Closed
bzbarsky-apple opened this issue Apr 6, 2022 · 5 comments · Fixed by #17181
Labels
app-clusters Application cluster work spec Mismatch between spec and implementation

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

We don't do range checks on sets to these attributes.

Proposed Solution

We should be doing range checks, but that requires the valid range to be specified in the XML.

@tlykkeberg-grundfos @mgarb1

@bzbarsky-apple bzbarsky-apple added spec Mismatch between spec and implementation app-clusters Application cluster work labels Apr 6, 2022
@tlykkeberg-grundfos
Copy link
Contributor

Hi @bzbarsky-apple I have been looking into this on ToT and I find the following in the pump-configuration-and-control.cluster.xml file:

    <attribute side="server" code="0x0020" define="OPERATION_MODE" type="ENUM8" min="0x00" max="0xFE" writable="true" default="0x00" optional="false">OperationMode</attribute>
    <attribute side="server" code="0x0021" define="CONTROL_MODE" type="ENUM8" min="0x00" max="0xFE" writable="true" default="0x00" optional="true">ControlMode</attribute>

I'm not quite sure what you mean in this context? The min and max values are specified in the attribute definition in the XML.

@bzbarsky-apple
Copy link
Contributor Author

Yes, but they are specified for the full (nullable) uint8 range, not the actual valid range for the relevant enums.

@tlykkeberg-grundfos
Copy link
Contributor

Fixed in #17181

@tlykkeberg-grundfos
Copy link
Contributor

Hi @bzbarsky-apple since the pr #17181 I have introduced this into these two attributes in the XML:

<attribute side="server" code="0x0020" define="OPERATION_MODE" type="PumpOperationMode" min="0x00" max="0x03" writable="true" default="0x00" optional="false">
      <description>OperationMode</description>
      <access op="read" privilege="view"/>
      <access op="write" privilege="manage"/>
    </attribute>
    <attribute side="server" code="0x0021" define="CONTROL_MODE" type="PumpControlMode" min="0x00" max="0x07" writable="true" default="0x00" optional="true">
      <description>ControlMode</description>
      <access op="read" privilege="view"/>
      <access op="write" privilege="manage"/>
    </attribute>

Here there are min/max values and the exact enum type (instead of ENUM8). Would that be sufficient or do you see any further implementation needed for this issue to be closed?

@bzbarsky-apple
Copy link
Contributor Author

#17181 fixes this, yes. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app-clusters Application cluster work spec Mismatch between spec and implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants