[matter] Bridge dimmer fixes - #21402
Conversation
Signed-off-by: Dan Cunningham <dan@digitaldan.com>
Signed-off-by: Dan Cunningham <dan@digitaldan.com>
There was a problem hiding this comment.
Pull request overview
Fixes Matter bridge dimmer ramping and cluster feature configuration.
Changes:
- Tracks and reports dimmer levels during ramps.
- Adds level, undefined-state, and activation tests.
- Enables required Matter Lighting and OnOff features.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
DimmableLightDevice.java |
Revises level and OnOff synchronization. |
DimmableLightDeviceTest.java |
Expands dimmer behavior tests. |
DimmableDeviceType.ts |
Enables required dimmable-light features. |
ColorDeviceType.ts |
Enables required color-light features. |
OnOffLightDeviceType.ts |
Restores the Lighting feature. |
OnOffPlugInDeviceType.ts |
Restores the Lighting feature. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
Additional AI review:
The cluster feature restoration and ramp-state handling otherwise look consistent with matter.js 0.17.4, and the current build/static-analysis results are clean. One Level Control edge case still needs to be addressed before merge: ExecuteIfOff allows a without-OnOff level command while the endpoint is off, but the new unconditional Java level handling turns the openHAB dimmer on.
| if (lastOnOffState == OnOffType.ON) { | ||
| updateLevel(ValueUtils.levelToPercent(((Double) data).intValue())); | ||
| } | ||
| updateLevel(ValueUtils.levelToPercent(((Double) data).intValue())); |
There was a problem hiding this comment.
Enabling LevelControl.Feature.OnOff handles the normal off-state case, but ExecuteIfOff is still an exception. In matter.js 0.17.4, the Options semantics explicitly allow a without-OnOff command to execute while OnOff=false when ExecuteIfOff is set, and #optionsAllowExecution implements exactly that. A regular MoveToLevel still calls moveToLevelLogic(..., false, ...), so the OnOff state is not coupled.
CustomLevelControlServer forwards that level to openHAB before the base logic, and this unconditional updateLevel() turns a nonzero level into a PercentType command for the dimmer, turning the physical light on even though this is a without-OnOff command. Please preserve the off state for the ExecuteIfOff path while still allowing MoveToLevelWithOnOff to turn the device on.
There was a problem hiding this comment.
Co-pilot found this line and responded that same, i will repeat what i commented there
#optionsAllowExecution (LevelControlServer.ts:596) already enforces this with LevelControl.Feature.OnOff
There was a problem hiding this comment.
I double-checked this, as I also saw the Copilot comment while reviewing. I think we may be looking at two slightly different cases here.
#optionsAllowExecution() does handle the normal case where ExecuteIfOff is false. The edge case the AI is pointing to is when ExecuteIfOff is true. In that case #optionsAllowExecution allows a regular MoveToLevel to run while the light is off. MoveToLevel then calls moveToLevelLogic(..., false, ...), so the OnOff state should remain unchanged. This also seems to match the documented ExecuteIfOff semantics.
The concern is that CustomLevelControlServer forwards the new level to openHAB regardless of withOnOff, so that distinction may get lost on the bridge side.
Perhaps a regression test can be added for this specific case to settle the question and make sure the implementation keeps working as expected?
There was a problem hiding this comment.
Fair point, the bridge (javascript side) is the one part of the binding that lacks test coverage, so i may need to open another PR for that (but i think thats a good idea in any case). What i can do however, is modify this to work the same way as the LevelControlServer, which really should of been done in the first place as that will cover color lights as well. I'll have that up shortly.
|
Here's another AI review... Thanks for working on this! Adding the missing cluster features ( Here are a few findings and suggestions to review: 1.
|
|
@robnielsen please don't just fire off random AI generated reviews on PR's that are not your own. Half of that review is just plain wrong, the rest nit picky, with one point being something @wborn already mentioned in his review. Its not helpful, and is just wasting the time we have to volunteer here. |
@digitaldan. If its wrong, then respond and explain why. BTW, I don't appreciate the tone of your comments, including the comments here. |
|
@robnielsen if i was harsh i apologize, but i spent a very, very long time creating this binding, and while i welcome contributions, i am extremely skeptical of completely AI generate PR's where the authors are not reviewing the changes, or have a understanding of how the underlying code works, that last PR changed a shared utility class use by many others, which should of been a big red flag when reviewing (i'm surprised your ai did not flag it). It costs little of ones time to ask AI to generate code or a review and push that, its costs significantly more for someone to have to read that, understand it, and validate its claims. its not fair in my opinion to push this burden on others without that person also investing in understanding what they are asking, knowing how the code works, reviewing what they are posting. I will not spend time defending something you spent little to no time generating. |
|
@robnielsen you are right , i was too harsh on that last review upon re-reading, i apologize for that, you were trying to help and i was rude in my response. Thats a lesson for me to relax a little bit around here and be more civil. |
|
@digitaldan, thank you for apology. If you are interested, I can give you a more detailed response. I should of originally asked antigravity to look at the comments on the PR. |
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
The current HEAD still has the ExecuteIfOff issue discussed above. Since an update for that is already planned, there is no need to duplicate the inline comment here.
There is also one behavior worth preserving when updating this: an OnOffType.ON should not overwrite the previously remembered dimmer level with 100%. The previous implementation kept lastLevel and reported that level again when turning on. The new generic state.as(PercentType.class) handling converts OnOffType.ON to 100%, so lastLevel becomes 254.
The test testOnAfterOffReportsFullBrightness() currently appears inconsistent with its comment: the comment says that turning the light back on should return to the previous brightness, but after starting at 50% the test expects currentLevel = 254.
The Matter feature changes otherwise look correct, and CI/static analysis are clean.
This review was AI-assisted.
|
Feel free to ignore if you want... @wborn's review regarding the The only additional finding from that review not mentioned is in |
|
Yes it should just have said that it is worth looking into and why as a normal maintainer would do instead of presenting it as a new finding. I will try to make it a bit smarter. It's being trained on the job and still has a bit of learning to do. 😉 |
|
I built and am running the latest version locally and tried with Google Home. The dimmer is at 100% 
Then I slide the slider to 0% and get an error: Also, if I set the value to 1% it becomes 2%. 2% or higher stays at correct value. Here are logs for 100% -> 0% -> 1%: And 100% -> 1% -> 2%: |
|
And hopefully the analysis from antigravity is helpful: Log Analysis: Light Turning Off & Setting Level from 1%Log Source: 1. Summary of IssuesWhen controlling bridged dimmable lights (such as
2. Detailed Breakdown & Log EvidenceA. Setting Level to 1% Fails (
|
|
Thanks Rob, although again ai analysis like this is really just noise thrown into this conversation between humans, the first post with the logs was perfect and all that is needed, we all have AI tools that do our own analysis. So first off your logs show a transaction dead lock, so thats not good. The second , and this is something i've struggled with matter, Lights can be 1-100, but never zero. What i misunderstood until i started working this issue, is that 1% means off, which makes no sense to me, but I'm sure the Matter folks have their reasons. Whats worse, is that other "Levels" that are not Dimmable lights do allow for 0%. So we can not use the same helper function for everything (which was what was changed in the first PR to try and fix this) as we need different helper functions for this. I'll have a fix shortly |
and a move to the lowest level is reported as off instead of a brightness. Signed-off-by: Dan Cunningham <dan@digitaldan.com>
So this is a good lesson that if something does not smell right , to push back on AI and not just roll with it. Turns out this is not right, and when i pressed claude for references about this, it realized it was using logic in the matter.js source to come to that conclusion, which was actually not quite right either (thats another story). Fortunately, this realization did not change the logic here much....but there you go. I have a fix up that i think should hopefully do it 🤞 . tl;dr So fun fact, when i decided i would introduce the bridge as part of the 1.0 of this binding, the very first feature was this cluster. I originally thought dimmable lights would be the most used device, and also be the simplest one to implement (besides ON/OFF) . I mean, like openHAB its just 0-100, right? Turns out i was half right, and half very wrong. It is the most used cluster, but thats also why LevelControl cluster might be one of the most complicated of all the ~100 clusters in the matter standard. There is so much diversity and legacy out there, likely also from its zigbee inheritance, its remarkably complex to implement. |
This fixes a long standing issue where we would not report correctly on Dimmable lights who ramp up and down when changed (dim to off)