[shelly] Add support for Shelly Pro RGBWW PM - #19227
Conversation
markus7017
added
enhancement
markus7017
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR enhances support for Shelly Plus RGBW devices and adds support for the new Shelly Pro RGBW PM device. The changes improve device compatibility and provide better channel control for RGBW lighting devices.
Key changes include:
- Added new Shelly Pro RGBW PM device support with proper thing type definitions
- Enhanced existing Shelly Plus RGBW PM configuration with additional channel groups
- Refactored light status handling to extract reusable components for better maintainability
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ShellyThingCreatorTest.java | Added test cases for new Pro RGBW PM device type |
| shellyGen2_lights.xml | Enhanced Plus RGBW PM configuration and added Pro RGBW PM thing type definition |
| shelly.properties | Updated device descriptions for Plus RGBW PM |
| ShellyLightHandler.java | Refactored light update logic to use extracted component method |
| ShellyComponents.java | Added reusable updateLights method extracted from handler |
| Shelly2ApiRpc.java | Enhanced light status handling for multiple light channels |
| Shelly2ApiJsonDTO.java | Extended data structures to support additional light configuration options |
| Shelly2ApiClient.java | Added light settings and status handling for new device types |
| ShellyDevices.java | Added device type constants and mappings for Pro RGBW PM |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@markus7017 any news about this ? |
|
Another shelly PR with binding core refactoring is waiting to be merged. Once done, i guess this is rebased and we can do a final review. |
markus7017
changed the title
Review actions takenAll Copilot inline comments addressed:
Additional hardening:
455 tests pass, 0 failures. |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
@markus7017 see my comment #19227 (comment) above. In my own PR #21286 I have -- in anticipation of your PR here being merged -- already written the code to manage the different profiles PS if you want to look at my PR the main themes are threefold..
|
|
I'll give it a try to see what happens! |
|
@andrewfg it thought long and deep to come to this conclusion: The component modeling in this PR looks like something #21286 should build on rather than replace. The #21286 already creates one In particular, a composite RGB+CCT So there doesn't seem to be a reason for #19227 to duplicate or anticipate the Core |
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
The latest changes address several of the previous issues. All five inputs are now included in the Pro RGBWW PM status/config processing, component-specific ct_range values are retained and used by the normal light-handler path, and RPC operations now translate the flat light index to the correct component-local ID.
Two functional issues still remain in the hybrid/CCT handling.
Commands to secondary lightN groups in rgbcct and rgbx2light are mapped back to the wrong settings.lights index, so they can be handled as the wrong Shelly component.
WebSocket/push CCT status updates also still use the profile-wide temperature range instead of the component-specific ct_range, which produces incorrect colorTemp percentages for customized ranges.
Requesting changes until those two remaining paths use the correct component mapping and temperature-range logic.
This review was AI-assisted.
Crikey! I shall have to think long and deep to figure out what that means! |
|
I guess that we really only need to agree on the hierarchy of device/component/channel-group/channel for each product variant. My working hypothesis is shown below. I think @markus7017 if you deliver the "architecture/framework" so the correct things are discovered and created with the correct channel-groups and channels, then I shall deliver the "logic" to produce, consume, and synchronize actual data values within those channels.. BULB
DUO
VINTAGE (PS perhaps you currently have this wrong..)
RGBW2 COLOR MODE
RGBW2 WHITE MODE
RGBWW in RGB profile
RGBWW in RGBW profile
RGBWW in RGBCCT profile
RGBWW in RGBX2LIGHT profile
RGBWW in LIGHT profile
RGBWW in CCTX2 profile
|
|
Shelly PR’s have the strange ability to grow and grow and have a hard time to get to the finish. That being said, the lightmodel discussion is best to move to the dedicated issue at #21259 |
@lsiepel I gave my feedback because @markus7017 asked for it. I see no point in rushing through a potentially wrong architecture and regretting it later. |
There was a problem hiding this comment.
I have a few comments, summarised as follows:
- in RGBWW PM if the first component is a color component it is RGB:n and not RGBW:n i.e. no white channel.
- in RGBWW PM any CCT:n component is a
WHITEgroup with cool-warm, and brightness (plus either implicit, or explicit on-off, and auto-on/off, etc.) - in RGBWW PM any LIGHT:n component is a
LIGHTgroup with brightness (plus either implicit, or explicit on-off, and auto-on/off, etc.)
See this https://shelly-api-docs.shelly.cloud/gen2/Devices/Gen2/ShellyProRGBWWPM
EDIT: it may be worth looking forward to #20909 and keeping in mind the following:
- The docs say that Duo Gen 3 supports
cct:0profile. So it should probably be modelled the same ascct:nhere. - The docs say that Color Bulb Gen 3 supports
rgbcct:0profile. Note that it isrgbcct:0and notrgbwcct:0. Also this looks superficially the same as thergbcct:0profile in this PR. HOWEVER in the RGBWWPM thergb:0andcct:0are independent components that can each be operated stand alone. WHEREAS in the Color Bulb Gen 3 they are mutually exclusive components where either thergbor thecctmode is selected via the operating mode switch.
| | | red | Dimmer | r/w | Red brightness: 0..100% (control only the red channel) | | ||
| | | green | Dimmer | r/w | Green brightness: 0..100% (control only the green channel) | | ||
| | | blue | Dimmer | r/w | Blue brightness: 0..100% (control only the blue channel) | | ||
| | | white | Dimmer | r/w | White brightness: 0..100% (control only the white channel) | |
There was a problem hiding this comment.
As described here https://shelly-api-docs.shelly.cloud/gen2/Devices/Gen2/ShellyProRGBWWPM the device has 5 outputs which can be used in various combinations:
- 5x individual light --
LIGHTprofile - 1x RGB group, plus 2x individual light --
RGBX2LIGHTprofile - 1x RGB group, plus 1x cool-warm pair --
RGBXCCTprofile - 2x cool-warm pair --
CCTX2profile
Note that the first group seems always to be RGB and not RGBW.
Therefore I would assume that the RGB mode will NOT expose a White component. i.e. RGB -only..
See my comments here #19227 (comment)
There was a problem hiding this comment.
There is still a concrete RGB-vs-RGBW issue here. colorSettingsRGB reuses the colorFull channel type, which still offers white as an option.
The Pro RGBWW PM exposes an RGB component, not an RGBW component. Selecting white therefore makes handleFullColor() produce RGBW = 0,0,0,255, but the RGB RPC path only sends the RGB values and ignores the white value. The device consequently receives RGB = 0,0,0.
An RGB-only component should therefore not expose the White option, unless White is explicitly translated to a valid RGB value.
There was a problem hiding this comment.
the RGB RPC path only sends the RGB values and ignores the white value. The device consequently receives RGB = 0,0,0.
This is fixed by my PR
|
|
||
| `Note`: | ||
| `rgbcct` and `rgbx2light` combine the RGB component above with additional CCT (`rgbcct`) or Light | ||
| (`rgbx2light`) components. Each additional component is exposed as its own `light1`/`light2` group |
There was a problem hiding this comment.
Each additional component is exposed as its own
light1/light2group
This applies only to LIGHT or RGBX2LIGHT and not to RGBCCT or CCTX2
.. also (nit) in OH markdown convention every sentence must be on a new line.
| (`rgbx2light`) components. Each additional component is exposed as its own `light1`/`light2` group | ||
| (same layout as the `light` profile below) with its own independent meter (`meter2`/`meter3`). | ||
| Since every profile has more than one meter, the device also gets the aggregated `device#accumulatedPower`/`device#totalEnergy` channels described in the general notes on channels above. | ||
|
|
There was a problem hiding this comment.
Probably you should add a whole new table of channels for RGBCCT with its own comments etc.
| @@ -12,16 +12,16 @@ | |||
| <channel-groups> | |||
There was a problem hiding this comment.
I think in your prior code you had used typeId rgbwpmChannel and now you are proposing to rename it as rgbwChannel.
Probably these group-types are wrong for two reasons:
-
xyzChannelshould really bexyzComponent -
It is hard to tell from the diff, but it looks like you do not have a group for all RGBWW profiles. What you need is the following:
- RGB component (NOTE: excluding W) .. typeId
rgbComponent - CCT cool-warm component .. typeId
cctComponent - LIGHT brightness-only component .. typeId
lightComponent
- RGB component (NOTE: excluding W) .. typeId
See my comments in the ReadMe above.
And also here #19227 (comment)
| // RGBW2 devices | ||
| public static final Set<ThingTypeUID> GROUP_RGBW2_THING_TYPES = Set.of( // | ||
| THING_TYPE_SHELLYRGBW2_COLOR, THING_TYPE_SHELLYRGBW2_WHITE, THING_TYPE_SHELLYPLUSRGBWPM); | ||
| THING_TYPE_SHELLYRGBW2_COLOR, THING_TYPE_SHELLYRGBW2_WHITE, THING_TYPE_SHELLYPLUSRGBWPM, |
There was a problem hiding this comment.
I think RGBWWPM is not strictly an RGBW thing type since it has no W component.
See other comments above and below
| if (lights != null) { | ||
| ShellySettingsRgbwLight light = lights.get(idx); | ||
| String whiteGroup = profile.isRGBW2 && !profile.inColor ? group : CHANNEL_GROUP_WHITE_CONTROL; | ||
| String whiteGroup = profile.isRGBW2 && !profile.hasColorTag(idx) ? group : CHANNEL_GROUP_WHITE_CONTROL; |
There was a problem hiding this comment.
See comments about difference between RGB and RGBW
| .add(new ShellyChannel(m, CHGR_LIGHTCH, CHANNEL_TIMER_ACTIVE, "timerActive", ITEMT_SWITCH)) | ||
| // RGBW2-white / RGBW PM-white | ||
| .add(new ShellyChannel(m, CHGR_LIGHT_IDX, CHANNEL_BRIGHTNESS, "whiteBrightness", ITEMT_DIMMER)) | ||
| .add(new ShellyChannel(m, CHGR_LIGHT_IDX, CHANNEL_COLOR_TEMP, "whiteTemp", ITEMT_DIMMER)) |
There was a problem hiding this comment.
I think not. I think it is as follows..
- LIGHT components have a
LIGHTgroup which never has a color temperature channel. It only has explicit brightness (and implicit on-off). - CCT components have
WHITEgroup which has brightness, cool-warm color temperature, .. and perhaps either a) aCONTROLgroup, or b) includes implicit on-off, and auto-on/off etc.
| @@ -1 +1 @@ | |||
| /* | |||
There was a problem hiding this comment.
Don't worry too much about the logic here. My PR almost certainly re-writes it..
|
I agree with @lsiepel, we should focus on fixes here, create a new pr to complete the basics and then integrate light control and finally the Duo bulb |
On rgbcct/rgbx2light profiles, the indexed light1..n channel groups start after the leading color component slot, so the group number alone was one short of the flat settings.lights index. Add back profile.getColorComponentCount() when reversing the group name to an index, mirroring getControlGroup()'s forward mapping. Signed-off-by: Markus Michels <markus7017@gmail.com>
setAutoTimer built config.name from the flat settings.lights index, but on RGBW2 hybrid profiles that index doesn't match the on-device component id used elsewhere in the same call, so the name was frequently wrong. This request only ever changes the auto-timer, so just leave the name unset instead of sending an incorrect one. Also drops the now-unused displayName field from LightRpcMethods. Signed-off-by: Markus Michels <markus7017@gmail.com>
updateLightMode() converted light.temp with profile.minTemp/maxTemp instead of the per-component profile.getMinTemp(i)/getMaxTemp(i), reporting the wrong CCT percentage for custom-ranged components pushed over the WebSocket status path. The polling path already used the per-component range. Signed-off-by: Markus Michels <markus7017@gmail.com>
|
@wborn Pushed fixes for the two remaining issues from your last review:
Also found and fixed a related issue while in there: Full build (spotless, tests, SAT) passes; added regression tests for both hybrid profiles. Re: the channel/group modeling discussion — agreed with @lsiepel above to keep this PR scoped to fixes and move that to a follow-up PR. |
The existing updateLightMode hybrid-profile test only asserted any() for the CHANNEL_COLOR_TEMP update, so it would not have caught a regression in the per-component ct_range conversion. Add a test that sets a custom minTemp/maxTemp on the CCT component and asserts the resulting percentage, which fails against the pre-fix profile-wide range. Signed-off-by: Markus Michels <markus7017@gmail.com>
@markus7017 please do not open another PR yourself. Instead I will include any necessary changes or adaptions in my own PR. Otherwise we have too many independently moving parts. |
|
this one is already big, I think we should have a step in between, but up to you Anything else for this one? |
No.
I am trying to parallel track most of what you are doing here and in the Gen 3 light PR, but juggling a 3 way merge is hard; and a 4 way merge would be harder yet. So let's finish this and the Gen 3 lights asap, and then I have a stable base to work from. |
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
The previous review findings have been addressed.
One functional issue remains: the Pro RGBWW PM exposes an RGB component in the rgbcct and rgbx2light profiles, but the Thing definition still exposes RGBW controls, including white. The RPC implementation does not support white for an RGB component, so that advertised control cannot work.
The Pro color component should expose RGB-only controls, with the README updated accordingly.
This review was AI-assisted.
|
uff, the is a multi dimensional feature matrix 🤪 |
ok, let's merge this one, then I'll rebase & review the Duo PR and you take it fron there |
@markus7017 This error here is about removing the excess W channel from the group. My PR is (so far) focussed on the channel command/update process assuming that the actual list of groups and channels were correct from your side. Therefore I think we could split this removal process into two parts:
So part 2. is already solved in my scope. The only question is if a) you want to remove the excess W channel from thing-type xml and from |
The rgb:0 component on the Pro RGBWW PM has no physical white output, but the color channel group reused the RGBW variant's channel-group type, exposing a white channel that always read/wrote 0%. Signed-off-by: Markus Michels <markus7017@gmail.com>
|
Fixed the excess white channel: Pro RGBWW's |
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
One additional functional issue remains: brightness commands for secondary CCT/Light components can update the reported state of the independent RGB component instead of the component that was commanded.
The existing RGB-vs-RGBW feedback also still applies. In particular, the RGB-only color group still exposes the shared colorFull White option, although that value cannot be represented by the device's RGB component.
The previously reported issues around component IDs, CCT ranges, input count, metering, timers, and the explicit White channel appear fixed in the current HEAD.
This review was AI-assisted.
| @@ -130,7 +131,7 @@ public boolean handleDeviceCommand(ChannelUID channelUID, Command command) throw | |||
| col.setGain(setColor(lightId, SHELLY_COLOR_GAIN, command, SHELLY_MIN_GAIN, SHELLY_MAX_GAIN)); | |||
There was a problem hiding this comment.
The command is sent to the correct secondary CCT/Light component, but the optimistic state update afterward targets the wrong openHAB channel.
For example, in the rgbcct profile:
control = RGB component
light1 = CCT component
A command such as light1#brightness = 50 is correctly sent to the CCT component, but this code then updates control#power to ON. That incorrectly changes the reported state of the independent RGB component.
The ON/OFF brightness path has a similar issue because it updates the hard-coded white#brightness group instead of the current lightN group.
Please derive the optimistic update target from the current component/group and add a regression test for brightness commands on a secondary component of a hybrid profile.
That AI statement is wrong!! RGB 255,255,255 is white!! |
The rgb:0 component has no white parameter, so selecting "white" on the colorFull channel sent rgb=0,0,0 with a dropped white value, switching the light off instead of producing white. Mix full RGB (255,255,255) instead for this component, and recognize that combination when reporting the current colorFull state back. Signed-off-by: Markus Michels <markus7017@gmail.com>
|
On the colorFull white debate: confirmed against the RGB.Set/GetStatus API spec — RGB component has only |
Description
Adds support for the Shelly Pro RGBWW PM, building on top of the Plus RGBW PM fixes in #21256 (merged).
Feature: Support for Shelly Pro RGBWW PM, with all four of its firmware profiles:
rgbcct(RGB + CCT),cctx2(dual CCT),rgbx2light(RGB + two white channels), andlight(five independent white channels).The device is handled by the existing RGBW thing group. Power metering channels are created for all profiles.
Fix: Color-temperature (
ct) is now propagated end-to-end for thecctx2profile — both CCT channels expose acolorTempchannel alongside their dimmer, on both the RPC-poll and WebSocket-push/periodic-status paths.Fix: Component-level power/energy readings (
apower/aenergy/voltage/current) now reach themeterchannel group for every component on every profile:rgbcctandrgbx2lightmeter their RGB component (meter) plus their secondary CCT/Light component(s) (meter2/meter3),cctx2meters both CCT channels independently (meter1/meter2),lightmeters all five channels independently.Fix: The device-level aggregated
accumulatedPower/totalEnergychannels are now only created for the Pro RGBWW PM hybrid profiles (which have multiple independently metered components), not for the single-meter Plus RGBW PM.Rebased onto current
main(which now includes #21256) — ready for review.Testing
README documents the full channel layout for every profile of the device.
rgbcctmode: RGB channel + CCT channel work independentlycctx2mode: dual CCT channels work, no error on status pollrgbx2lightmode: RGB + 2 white channels worklightmode: 5 white channels workAcceptance criteria
Closing