[shelly] Implement OH Core LightModel - #21286
Conversation
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
|
First look seems good. There are some TODO’s left. Let’s see what @markus7017 thinks of it |
Yes. Indeed I have added a whole lot more such annotations. And I need to add a test suite. |
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@markus7017 thanks for the info. I half understand it. But I am still confused why all the different flavors are used in same |
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
There was a problem hiding this comment.
Pull request overview
This PR migrates the Shelly binding’s light handling toward the openHAB Core LightModel, introducing a Shelly-specific ShellyLightModel wrapper and exposing “primary” standard light channels in thing definitions to align with the openHAB lighting model.
Changes:
- Added
ShellyLightModel(extends CoreLightModel) plus a new unit test suite. - Refactored
ShellyLightHandlerand related update paths (CoIoT/CoAP/components) to use the new model instead ofShellyColorUtils(which is removed). - Added “primary” channel groups and i18n labels/descriptions for standard light control channels across Gen1/Gen2 light thing types.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| bundles/org.openhab.binding.shelly/src/test/java/org/openhab/binding/shelly/internal/handler/ShellyLightModelTest.java | New unit tests for the Shelly-specific LightModel behavior and dirty-flag logic |
| bundles/org.openhab.binding.shelly/src/main/resources/OH-INF/thing/shellyGen2_lights.xml | Adds primary channel-group to Gen2 light thing definitions |
| bundles/org.openhab.binding.shelly/src/main/resources/OH-INF/thing/shellyGen1_lights.xml | Adds primary channel-group to Gen1 light thing definitions |
| bundles/org.openhab.binding.shelly/src/main/resources/OH-INF/thing/primary_light_group.xml | New channel-group-type definitions for standard (“primary”) light controls |
| bundles/org.openhab.binding.shelly/src/main/resources/OH-INF/i18n/shelly.properties | Adds i18n keys for the new primary-* channel-group-types |
| bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/ShellyBindingConstants.java | Adds constants for primary group + channels |
| bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/handler/ShellyThingInterface.java | Adds default getLightModel(int) hook for handlers |
| bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/handler/ShellyLightModel.java | New Shelly wrapper around Core LightModel with Shelly-specific helpers/caching/locking |
| bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/handler/ShellyLightHandler.java | Refactors light command handling + status updates to be model-driven and updates channels incl. primary group |
| bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/handler/ShellyComponents.java | Updates RGBW status update helper to use ShellyLightModel |
| bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/handler/ShellyColorUtils.java | Removes the legacy color utility class |
| bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/api1/Shelly1CoIoTVersion2.java | Adjusts CoIoT handler signature due to removal of ShellyColorUtils plumbing |
| bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/api1/Shelly1CoIoTVersion1.java | Updates CoIoT handling to use ShellyLightModel for color temperature updates |
| bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/api1/Shelly1CoIoTProtocol.java | Reworks CoIoT parsing for RGBW/gain/power updates using the new model |
| bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/api1/Shelly1CoIoTInterface.java | Signature change to remove ShellyColorUtils argument |
| bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/api1/Shelly1CoapHandler.java | Updates CoAP update flow to use ShellyLightModel instead of a local ShellyColorUtils |
Suppressed comments (4)
bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/handler/ShellyLightHandler.java:316
- updateRemoteDeviceFromLightModel assumes model.getRGBX() always has a white component (rgbw[3]). For RGB-only profiles (e.g. SHELLY2_PROFILE_RGB / RGB_NO_BRIGHTNESS), getRGBX() will be length 3 and this will throw ArrayIndexOutOfBoundsException when COLOR is dirty.
if (model.isColorDirty()) {
int[] rgbw = model.getRGBX();
parms.put(SHELLY_COLOR_RED, String.valueOf(rgbw[0]));
parms.put(SHELLY_COLOR_GREEN, String.valueOf(rgbw[1]));
parms.put(SHELLY_COLOR_BLUE, String.valueOf(rgbw[2]));
bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/handler/ShellyLightHandler.java:296
- apiCommandSent is initialized to true, which causes requestUpdates(1, false) to run even when no API command was sent (e.g., no dirty fields / no parms). This adds unnecessary network traffic and update churn.
boolean apiCommandSent = true;
// MODE:
if (profile.isBulb && model.isModeDirty()) {
api.setLightMode(Mode.COLOR == model.getMode() ? SHELLY_MODE_COLOR : SHELLY_MODE_WHITE);
bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/handler/ShellyLightHandler.java:396
- updateLightModelFromLightStatus() uses ShellyLightModel#setBrightness(int) and #setColorTemp(...), both of which change shellyMode. During status refresh this can overwrite the mode derived from the device (and then propagate incorrect CHANNEL_LIGHT_COLOR_MODE updates / spurious mode changes). Use LightModel setters (brightness/mirek) here so updating values does not change mode.
// WHITE:
if ((!profile.inColor && (!profile.isGen2 || profile.isRGBW2)) || profile.isBulb) {
model.setBrightness(getInteger(light.brightness));
}
bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/api1/Shelly1CoIoTProtocol.java:274
- updatePower() now always calls thingHandler.getLightModel(...) for the (profile.isLight || profile.isDimmer) path. For dimmers this will typically throw UnsupportedOperationException (default interface implementation), breaking CoIoT updates. Also, casting brightness to int and calling ShellyLightModel#setBrightness(int) can unintentionally change the model mode; use LightModel#setBrightness(double) here instead.
if (thingHandler.getLightModel(id - 1) instanceof ShellyLightModel model) {
try {
model.lock(this.getClass(), allUpdates);
// TODO check logic
if (brightness != -1) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Replace the String-based settings.lights[i].apiComponent tag
("rgb"/"rgbw"/"cct"/"light") with a ShellyLightApiComponent enum. An
invalid tag is now a compile-time impossibility instead of a silent
runtime fallback to the LIGHT RPC methods in
Shelly2ApiRpc.lightRpcMethods(). The field stays transient and is set
only in Shelly2ApiClient.createRgbwLightSetting() (Gen2-only, never
Gson-(de)serialized), so this is not a breaking change.
Also:
- Rename ShellyDeviceProfile.isColorComponent(int) to hasColorTag(int)
to resolve a name collision with the unrelated, differently-scoped
ShellyApiLightUtil.isColorComponent(ShellyLightApiComponent).
- Restore the setLightParms() fail-fast guard for non-RGBW2 profiles
and unify its RGB/RGBW/CCT/Light dispatch onto the same
lightRpcMethods() lookup already used elsewhere in this class.
- Document on ShellyLightApiComponent that each settings.lights index
is an independently addressed physical component, not a sub-channel
of one combined light - relevant for PR openhab#21286's OH Core LightModel
work, whose Pro RGBWW PM branch currently assumes one model per
whole-device profile string.
Signed-off-by: Markus Michels <markus7017@gmail.com>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
|
@markus7017 I now have this PR working and tested for my RGBW2 device. It does not (yet) integrate the devices you will add in #19227 and #20909 however it is ready for your first functional review. Please don't waste time on AI fine tuning yet, as at this stage I am just looking for basic functional feedback only. |
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
…Util with type-safe enum
Replace scattered profile.device.profile string-equality checks
(SHELLY2_PROFILE_CCTX2.equals(...) etc.) and RPC-method-name
comparisons with static isColorComponent/isRgbComponent/
isRgbwComponent/isCctComponent/isLightComponent(tag) helpers backed
by the settings.lights[i].apiComponent tag rather than the whole-
profile string. Add profile.isProRgbwwPm to replace the local
isProRgbwwPmProfile() duplicate, and add profile.isCctComponent(idx)
alongside the existing isColorComponent(idx). Also collapse
ShellyUtils.buildControlGroupName/buildWhiteGroupName into
profile.getControlGroup(), removing the now-redundant duplicate group
resolution logic.
The tag itself moves from a raw String ("rgb"/"rgbw"/"cct"/"light")
to a ShellyLightApiComponent enum, making an invalid tag a
compile-time impossibility instead of a silent runtime fallback to
the LIGHT RPC methods in Shelly2ApiRpc.lightRpcMethods(). The field
stays transient and is set only in
Shelly2ApiClient.createRgbwLightSetting() (Gen2-only, never
Gson-(de)serialized), so this is not a breaking change.
Static logic doesn't belong on a DTO class, so the helpers land in a
new ShellyApiLightUtil in the api package rather than on the
component tag holder itself. While at it, fold in the other
light-specific group/id helpers (getLightIdFromGroup,
lightChannelGroupPrefix, buildWhiteGroupName) that were sitting in
the generic ShellyUtils for the same reason, and add missing coverage
for the tag-lookup helpers and lightChannelGroupPrefix.
Also:
- Rename ShellyDeviceProfile.isColorComponent(int) to hasColorTag(int)
to resolve a name collision with the unrelated, differently-scoped
ShellyApiLightUtil.isColorComponent(ShellyLightApiComponent).
- Restore the setLightParms() fail-fast guard for non-RGBW2 profiles
and unify its RGB/RGBW/CCT/Light dispatch onto the same
lightRpcMethods() lookup already used elsewhere in this class.
- Document on ShellyLightApiComponent that each settings.lights index
is an independently addressed physical component, not a sub-channel
of one combined light - relevant for PR openhab#21286's OH Core LightModel
work, whose Pro RGBWW PM branch currently assumes one model per
whole-device profile string.
Signed-off-by: Markus Michels <markus7017@gmail.com>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Resolves #21259
Resolves #18215
Migrates all Shelly light handling (Bulb, Duo, Vintage, RGBW2 color/white, and generation 2 and 3) onto the new OH Core
LightModelbase class, replacing the binding-localShellyColorUtils. This unifies on/off, brightness, RGB(W), gain, effect, and color-temperature handling behind a single shared model instead of three generations of ad-hoc, per-device-family logic, and adds a standard "Primary" channel group (system.color/system.brightness/system.color-temperature) so these lights work out of the box with OH's generic light widgets.Depends on #19227
Depends on #20909
Testing
Signed-off-by: Andrew Fiddian-Green software@whitebear.ch