Implement ZCL compliant thermostat, fan and thermostat ui attributes #358
Conversation
This commit adds ZCL compliant thermostat, fan, and thermostat UI attributes Changes are based on similar ncs implementations. Code was tested with ZCL CLI and simple sample code. Signed-off-by: Adam Ćwiek <adam.cwiek@lerta.energy>
lohisynths
force-pushed
the
lerta_nrfxlib
branch
from
15df29f to
2aedd97
Compare
|
Is it possible to retrigger Jenkins build? |
There was a problem hiding this comment.
Hello @lohisynths!
Thank you for your work and submitting the PR!
The Jenkins build has failed due to the commit author validation check.
I am glad that you have modified only those files that are not updated through the ZBOSS release process, so no additional merges in the future will be needed.
I've left some comments after checking your definitions against the ZCL specification.
Additionally, in order to run the CI, the second branch on the sdk-nrf repository is needed, updating the west.yml file, so it points to the PR in nrfxlib. I can make it for you, if you like.
| typedef struct { | ||
| uint8_t temperature_display_mode; | ||
| uint8_t keypad_lockout; | ||
| uint8_t schedule_programming_visibility; |
There was a problem hiding this comment.
The ScheduleProgrammingVisibility attribute is optional, thus it is not required by the ZB_ZCL_DECLARE_THERMOSTAT_UI_CONFIG_ATTRIB_LIST macro.
Generally, the full list of attributes should have attrs_ext_t suffix and be passed to the macro with ATTRIB_LIST_EXT suffix.
Do you use the schedule_programming_visibility field? If so, could you provide macro definitions for the extended variant?
ZB_ZCL_DECLARE_THERMOSTAT_UI_CONFIG_ATTRIB_LIST_EXTZB_SET_ATTR_DESCR_WITH_ZB_ZCL_ATTR_THERMOSTAT_UI_CONFIG_SCHEDULE_PROGRAMMING_VISIBILITY_ID
If not, please simply remove this field or rename the type definition 🙂
| zb_int16_t abs_max_heat_setpoint_limit; | ||
| zb_int16_t abs_min_cool_setpoint_limit; | ||
| zb_int16_t abs_max_cool_setpoint_limit; | ||
| zb_int16_t PI_cooling_demand; |
There was a problem hiding this comment.
The PICoolingDemand field has an uint8 type.
| zb_int16_t abs_min_cool_setpoint_limit; | ||
| zb_int16_t abs_max_cool_setpoint_limit; | ||
| zb_int16_t PI_cooling_demand; | ||
| zb_int16_t PI_heating_demand; |
There was a problem hiding this comment.
The PIHeatingDemand field has an uint8 type.
| zb_int16_t PI_cooling_demand; | ||
| zb_int16_t PI_heating_demand; | ||
| zb_uint8_t HVAC_system_type_configuration; | ||
| } zb_zcl_thermostat_info_attrs_t; |
There was a problem hiding this comment.
This list goes to the ATTRS_LIST_EXT macro, thus should have the attrs_ext_t suffix.
| zb_uint8_t system_mode; | ||
| zb_uint8_t alarm_mask; | ||
| zb_uint8_t thermostat_running_mode; | ||
| } zb_zcl_thermostat_settings_attrs_t; |
There was a problem hiding this comment.
This list goes to the ATTRS_LIST_EXT macro, thus should have the attrs_ext_t suffix.
| zb_uint16_t temperature_setpoint_hold_ouration; | ||
| zb_uint8_t thermostat_programming_operation_mode; | ||
| zb_int16_t thermostat_running_state; | ||
| } zb_zcl_thermostat_schedule_and_HVAC_attrs_t; |
There was a problem hiding this comment.
This list goes to the ATTRS_LIST_EXT macro, thus should have the attrs_ext_t suffix.
|
|
||
| /** @} */ | ||
|
|
||
| #endif /* ZB_ZCL_THERMOSTAT_ADDONS_H__ */ |
There was a problem hiding this comment.
General: I do agree with you. Even though the ZB_ZCL_DECLARE_THERMOSTAT_ATTRIB_LIST_EXT macro does not require all fields to be present, it is better to provide a complete list in type definitions.
|
Hello @lohisynths! When it comes to the comments, I propose to resolve them in this PR. Please leave your feedback, if you disagree with my notes. |
|
This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 7 days. Note, that you can always re-open a closed pull request at any time. |
|
none Note: This comment is automatically posted and updated by the Contribs GitHub Action. |
This commit adds ZCL compliant thermostat, fan, and thermostat UI attributes
Changes are based on similar ncs implementations.
Code was tested with ZCL CLI and simple sample code.