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

Fixes: [OPSTATE] specific phase in the list should not null #31176

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ class GenericOperationalStateDelegateImpl : public Delegate
/**
* Fills in the provided MutableCharSpan with the phase at index `index` if there is one,
* or returns CHIP_ERROR_NOT_FOUND if the index is out of range for the list of phases.
* If fills in the provided MutableCharSpan with the phase at index `0` and returns CHIP_ERROR_NOT_FOUND,
* it represents PhaseList attribute is an empty list, the SDK will set PhaseList attribute value to null.
* Note: This is used by the SDK to populate the phase list attribute. If the contents of this list changes, the
* device SHALL call the Instance's ReportPhaseListChange method to report that this attribute has changed.
* @param index The index of the phase, with 0 representing the first phase.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ class GenericOperationalStateDelegateImpl : public Delegate
/**
* Fills in the provided MutableCharSpan with the phase at index `index` if there is one,
* or returns CHIP_ERROR_NOT_FOUND if the index is out of range for the list of phases.
* If fills in the provided MutableCharSpan with the phase at index `0` and returns CHIP_ERROR_NOT_FOUND,
* it represents PhaseList attribute is an empty list, the SDK will set PhaseList attribute value to null.
* Note: This is used by the SDK to populate the phase list attribute. If the contents of this list changes, the
* device SHALL call the Instance's ReportPhaseListChange method to report that this attribute has changed.
* @param index The index of the phase, with 0 representing the first phase.
Expand Down
2 changes: 2 additions & 0 deletions examples/chef/common/chef-rvc-operational-state-delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ class GenericOperationalStateDelegateImpl : public Delegate
/**
* Fills in the provided MutableCharSpan with the phase at index `index` if there is one,
* or returns CHIP_ERROR_NOT_FOUND if the index is out of range for the list of phases.
* If fills in the provided MutableCharSpan with the phase at index `0` and returns CHIP_ERROR_NOT_FOUND,
* it represents PhaseList attribute is an empty list, the SDK will set PhaseList attribute value to null.
* Note: This is used by the SDK to populate the phase list attribute. If the contents of this list changes, the
* device SHALL call the Instance's ReportPhaseListChange method to report that this attribute has changed.
* @param index The index of the phase, with 0 representing the first phase.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ class OperationalStateDelegate : public Delegate
* Get the list of supported operational phases.
* Fills in the provided MutableCharSpan with the phase at index `index` if there is one,
* or returns CHIP_ERROR_NOT_FOUND if the index is out of range for the list of phases.
* If fills in the provided MutableCharSpan with the phase at index `0` and returns CHIP_ERROR_NOT_FOUND,
* it represents PhaseList attribute is an empty list, the SDK will set PhaseList attribute value to null.
* @param index The index of the phase, with 0 representing the first phase.
* @param operationalPhase The MutableCharSpan is filled.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ class ExampleMicrowaveOvenDevice : public MicrowaveOvenControl::Delegate,
/**
* Fills in the provided MutableCharSpan with the phase at index `index` if there is one,
* or returns CHIP_ERROR_NOT_FOUND if the index is out of range for the list of phases.
* If fills in the provided MutableCharSpan with the phase at index `0` and returns CHIP_ERROR_NOT_FOUND,
* it represents PhaseList attribute is an empty list, the SDK will set PhaseList attribute value to null.
* Note: This is used by the SDK to populate the phase list attribute. If the contents of this list changes, the
* device SHALL call the Instance's ReportPhaseListChange method to report that this attribute has changed.
* @param index The index of the phase, with 0 representing the first phase.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ class RvcOperationalStateDelegate : public OperationalState::Delegate
/**
* Fills in the provided MutableCharSpan with the phase at index `index` if there is one,
* or returns CHIP_ERROR_NOT_FOUND if the index is out of range for the list of phases.
* If fills in the provided MutableCharSpan with the phase at index `0` and returns CHIP_ERROR_NOT_FOUND,
* it represents PhaseList attribute is an empty list, the SDK will set PhaseList attribute value to null.
* Note: This is used by the SDK to populate the phase list attribute. If the contents of this list changes, the
* device SHALL call the Instance's ReportPhaseListChange method to report that this attribute has changed.
* @param index The index of the phase, with 0 representing the first phase.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,8 @@ class Delegate
/**
* Fills in the provided MutableCharSpan with the phase at index `index` if there is one,
mideayanghui marked this conversation as resolved.
Show resolved Hide resolved
* or returns CHIP_ERROR_NOT_FOUND if the index is out of range for the list of phases.
* If fills in the provided MutableCharSpan with the phase at index `0` and returns CHIP_ERROR_NOT_FOUND,
* it represents PhaseList attribute is an empty list, the SDK will set PhaseList attribute value to null.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

Suggested change
* If fills in the provided MutableCharSpan with the phase at index `0` and returns CHIP_ERROR_NOT_FOUND,
* it represents PhaseList attribute is an empty list, the SDK will set PhaseList attribute value to null.
*
* If CHIP_ERROR_NOT_FOUND is returned for index 0, that indicates that the PhaseList attribute is null
* (there are no phases defined at all).
*

and the same in all the other places where this got copy/pasted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably more of a spec question than an SDK question, but why have a nullable list to begin with? Per 17.18.1

Composite data types that have a length (i.e. octet string and list), and derived types that have those as the base type, SHALL NOT differentiate semantically between the null value and the empty (zero length) value. 

This seems like extra wrappers for no good reason. Or is this just for backwards compatibility?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec could have gone either way here.... There are various opinions about which is better (e.g. null is one less byte on the wire than an empty list is). But for purposes of the SDK, this is what the spec says, and yes, this has already shipped as nullable, so at this point we can't really change the spec.

* Note: This is used by the SDK to populate the phase list attribute. If the contents of this list changes, the
* device SHALL call the Instance's ReportPhaseListChange method to report that this attribute has changed.
* @param index The index of the phase, with 0 representing the first phase.
Expand Down
Loading