fix(thermostat-server): notify subscribers when ActivePresetHandle changes#43583
Open
lboue wants to merge 2 commits intoproject-chip:masterfrom
Open
fix(thermostat-server): notify subscribers when ActivePresetHandle changes#43583lboue wants to merge 2 commits intoproject-chip:masterfrom
lboue wants to merge 2 commits intoproject-chip:masterfrom
Conversation
…anges SetActivePreset() was calling delegate->SetActivePresetHandle() without notifying the reporting engine, so active subscriptions never received a Report Data message after a SetActivePresetRequest command. The new value was silently applied on the device side (confirmed via a direct read) but was invisible to subscribed controllers. Add a call to MatterReportingAttributeChangeCallback() after a successful SetActivePresetHandle() to mark the attribute dirty and trigger a subscription report to all subscribers.
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses a bug where changes to the ActivePresetHandle attribute in the Thermostat cluster were not being reported to subscribers. The fix correctly adds a call to MatterReportingAttributeChangeCallback after the attribute's value is updated via the delegate. This ensures that subscribers will receive notifications when the active preset changes. The change is correct and well-contained.
21 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR ensures Thermostat preset updates trigger subscription reporting by explicitly marking the ActivePresetHandle attribute as changed when it’s updated via the server’s preset-handling path.
Changes:
- Include the reporting API header needed to notify attribute changes.
- Call
MatterReportingAttributeChangeCallback()after successfully settingActivePresetHandleso subscribers receive an updated report.
|
PR #43583: Size comparison from 70b86c8 to 7c1d6d6 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43583 +/- ##
==========================================
- Coverage 54.12% 54.12% -0.01%
==========================================
Files 1550 1550
Lines 106947 106948 +1
Branches 13312 13312
==========================================
Hits 57888 57888
- Misses 49059 49060 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
I created a task to add a Python test for the certification. This test must have been overlooked because the certification was approved even though some code was missing: |
|
PR #43583: Size comparison from 708056c to d3f579d Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Contributor
Author
|
The Darwin test fails due to a memory leak (#43594). That has nothing to do with this request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SetActivePreset()was updating theActivePresetHandleattribute via the delegate but not callingMatterReportingAttributeChangeCallback()afterwards. As a result, active subscriptions never received a Report Data message when the active preset changed - the new value was silently applied on the device side but never pushed to subscribers.Problem
When a controller sends a
SetActivePresetRequestcommand:ActivePresetHandleinternally ✅read active-preset-handlereturns the new value correctly ✅This means any controller relying on subscriptions (the standard Matter
approach) will never learn about the change without explicitly polling the
attribute.
Confirmed against:
examples/thermostat/linux)Root Cause
In
thermostat-server-presets.cpp,SetActivePreset()was missing a call toMatterReportingAttributeChangeCallback()after a successfulSetActivePresetHandle():Related issues
Fixes: #43582
Testing
I am not a member of the CSA, so I do not have permission to update the Python test file. I don't know what step number to assign to a new test.
Tested with thermostat/linux example:
https://github.com/project-chip/connectedhomeip/tree/master/examples/thermostat/linux
Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines