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

Issues found in BooleanStateConfiguration cluster implementation #32377

Open
7 tasks
tcarmelveilleux opened this issue Feb 29, 2024 · 0 comments
Open
7 tasks
Labels
app-clusters Application cluster work

Comments

@tcarmelveilleux
Copy link
Contributor

While reviewing implementation of Boolean State Configuration cluster it was found that there are several parts that could be improved to make the cluster more easily reusable/testable.

Issues:

  • Initial value of sensitivity is not valid until a first write, and it always starts at "N-1", irrespective of whether this is what the product wants. This requires products calling CHIP_ERROR SetCurrentSensitivityLevel(EndpointId ep, uint8_t level); at least one time, even though it's not possible to tell easily if this was done before.
  • When sensitivity changes via a write, the only path to see the change is via MatterPostAttributeChangeCallback (global). The delegate exists but cannot see the value being set.
  • Reads of current sensitivity always hit a flash read and are not cached.
  • There is no document indicating that the entire public interface of the Boolean State Configuration cluster needs to be called from Matter context (i.e. not safe to call directly from application if running in different context).
  • The public APIs or cluster interface behaviors are not documented. This makes it harder to understand that only MatterPostAttributeChangeCallback is what will be called for all state mutations.
  • Number of SupportedSensitivityLevels is always read from attribute store using SupportedSensitivityLevels::Get() which forces the default value to always be included in ZAP config. This makes it harder to test (not unit testable), and also prevents a delegate-based interface to have code-backed configuration.
  • When any Alarm mutator (e.g. SetAlarmsActive) is called, an event is always generated, even if nothing changed in the set of alarms, which does not match the intent of the cluster (can cause a large number of events generated if SetAlarmsActive(same_value) is called many times
@tcarmelveilleux tcarmelveilleux added the app-clusters Application cluster work label Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app-clusters Application cluster work
Projects
Status: Todo
Development

No branches or pull requests

1 participant