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.

Sign up for GitHub

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

Jump to bottom

Delete notification_types_node when there are no supported notification types #25

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Delete notification_types_node when there are no supported notification types #25

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5d588c
Delete notification_types_node when there are no supported notificati…
monikasoni Nov 29, 2023
0e31493
Merge branch 'SiliconLabs:main' into mrathor/no-notification-types
monikasoni Jan 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 18 additions & 1 deletion applications/zpc/components/zwave_command_classes/src/zwave_command_class_notification.cpp
View file
Open in desktop
Expand Up @@ -354,6 +354,23 @@ static sl_status_t
= attribute_store_get_first_child_by_type(
ep_node,
ATTRIBUTE(SUPPORTED_NOTIFICATION_TYPES));
// Delete notification_types_node when there are no supported notification types.
Copy link
Collaborator

Choose a reason for hiding this comment

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

just curious, wouldnt it be possible to avoid creation of node instead of remove it ?

Copy link
Author

Choose a reason for hiding this comment

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

Node is created away ahead in the inclusion process. I am not sure if it is feasible to avoid creation.

Copy link

Choose a reason for hiding this comment

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

The issue was more that the variable length bitmask was not processed correctly, and therefore the supported type was not populated correctly.
It was fixed in 1.5.0.

Note that this solution does not feel right, you are "removing the device functionality" by erasing the supported types... and there is still a risk that this SUPPORTED_NOTIFICATION_TYPES attribute get re-spawned at each system restart by zwave_command_class_notification_on_version_attribute_update()

I'd suggest to close this PR.

if (number_of_supported_notification_types == 0) {
sl_status_t status
= attribute_store_delete_node(supported_notification_types_node);
if (status != SL_STATUS_OK) {
sl_log_debug(LOG_TAG,
"Failed to remove node %u",
supported_notification_types_node);
}
return SL_STATUS_OK;
}
if (supported_notification_types_node == ATTRIBUTE_STORE_INVALID_NODE) {
sl_log_info(LOG_TAG,
"Failed to find node %u",
supported_notification_types_node);
return SL_STATUS_FAIL;
}
attribute_store_set_node_attribute_value(
supported_notification_types_node,
REPORTED_ATTRIBUTE,
Expand Down Expand Up @@ -794,4 +811,4 @@ sl_status_t zwave_command_class_notification_init()
zwave_command_handler_register_handler(handler);

return SL_STATUS_OK;
}
}