Skip to content

[shelly] Fix support for Shelly Plug US #18775

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jun 12, 2025
Merged

[shelly] Fix support for Shelly Plug US #18775

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
81fd47a
Fix Shelly Plug US: This is a Gen2 device, but the service name is
Jun 7, 2025
432b329
cleanup
Jun 7, 2025
769aa6f
README updated
Jun 7, 2025
0ead38f
README updated
Jun 7, 2025
057dedf
apply review changes
Jun 9, 2025
23be848
Add test coverage
jlaur Jun 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bundles/org.openhab.binding.shelly/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1226,7 +1226,7 @@ If the Shelly Add-On is installed:
The roller positioning calibration has to be performed using the Shelly Web UI or App before the position can be set in percent.
Refer to [Smartify Roller Shutters with openHAB and Shelly](doc/UseCaseSmartRoller.md) for more information on roller integration.

### Shelly Plus Plug-S/IT/UK/US (thing-type: shellyplusplug)
### Shelly Plus Plug-S/IT/UK/US (thing-type: shellyplusplug, shellyplusplugus)

| Group | Channel | Type | read-only | Description |
| ----- | ------------ | -------- | --------- | --------------------------------------------------------------------------------- |
Expand Down
4 changes: 3 additions & 1 deletion ...helly/src/main/java/org/openhab/binding/shelly/internal/discovery/ShellyThingCreator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ public class ShellyThingCreator {
public static final String THING_TYPE_SHELLYPLUSSMOKE_STR = "shellyplussmoke";
public static final String THING_TYPE_SHELLYPLUSUNI_STR = "shellyplusuni";
public static final String THING_TYPE_SHELLYPLUSPLUGS_STR = "shellyplusplug";
public static final String SVC_TYPE_SHELLYPLUSPLUGUS_STR = "shellyplugus";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is SVC for? All other seem to have THING_TYPE as prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used a constant to handle this special case. The serviceName from mDNS discovery is shellyplugus whereas the thing type is shellyplusplug. With the missing plus (shellyplusplugus) the detection of a Gen2+ device fails.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is similar to e.g. THING_TYPE_SHELLYBLUGWG3_STR, so you could use:

Suggested change
public static final String SVC_TYPE_SHELLYPLUSPLUGUS_STR = "shellyplugus";
public static final String THING_TYPE_SHELLYPLUGUS_STR = "shellyplugus";

for consistency, or refactor all "unused"/alias thing types into a group with similar naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, because SVC_TYPE_SHELLYPLUSPLUGUS_STR is the serviceName reported by the device and
THING_TYPE_SHELLYPLUGUS_STR is the thing type.

I'm wondering, because my code shows

public static final String SVC_TYPE_SHELLYPLUSPLUGUS_STR = "shellyplugus";
public static final String THING_TYPE_SHELLYPLUSPLUGUS_STR = "shellyplusplugus";

See the difference between shellyplugus (serviceName) and shellyplusplugus (thingType).
That's the mapping, which is required
if serviceName == shellyplugus map it to thingType shellyplusplugus, which then initializes the V2 API

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware of that. As I said, it's the same as for example THING_TYPE_SHELLYBLUGWG3_STR, so I don't see any reason to use different naming here? And what does SVC mean?

This is related to #18729 (comment), you might want to have a look and possibly comment if I didn't understand it correctly. We could refactor it further, but probably not in context of this PR here. I would appreciate any input for the direction. Currently the THING_TYPE_*_STR constants represent id/serviceName which has almost a 1:1 relationship with thing type id's. However, a few of them are mapped differently in THING_TYPE_MAPPING.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware of that. As I said, it's the same as for example THING_TYPE_SHELLYBLUGWG3_STR, so I don't see any reason to use different naming here? And what does SVC mean?

No, it's not.
In case of the BLU GW it's mapping shellyblugw as well as shellyblugwg3 to shellyblugw. shellyblu is recognized as a prefix to enforce APIv2 initialization. shellyblugwg3 is the device's service name announced via mDNS.

In this case here shellyplugus would initialize APIv1
SVC_ stands for Service. SVC_TYPE_SHELLYPLUSPLUGUS_STR is the device's service name announced via mDNS. This is not the thingType. It is just used to do the mapping into the thing type name.
We could also use a different prefix as you prefer, but I need the 2 different values and don't want to hard-code them.

Copy link
Contributor Author

@markus7017 markus7017 Jun 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware of that. As I said, it's the same as for example THING_TYPE_SHELLYBLUGWG3_STR, so I don't see any reason to use different naming here? And what does SVC mean?

This is related to #18729 (comment), you might want to have a look and possibly comment if I didn't understand it correctly. We could refactor it further, but probably not in context of this PR here. I would appreciate any input for the direction. Currently the THING_TYPE_*_STR constants represent id/serviceName which has almost a 1:1 relationship with thing type id's. However, a few of them are mapped differently in THING_TYPE_MAPPING.

I commented the questions in the other comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

THING_TYPE_SHELLYBLUGWG3_STR = "shellyblugwg3" => creates thing type shellyblugw (thing type shellyblugwg3 doesn't exist).

So let's try the other way around:

In case of the BLU GW it's mapping shellyblugw as well as shellyblugwg3 to shellyblugw. shellyblu is recognized as a prefix to enforce APIv2 initialization. shellyblugwg3 is the device's service name announced via mDNS.

In case of the Shelly Plug US it's mapping shellyplusplugus as well as shellyplugus to shellyplusplugus. shellyplus is recognized as a prefix to enforce APIv2 initialization. shellyplugus is the device's service name announced via mDNS.

I'm not insisting on any specific name, only to keep things consistent, since it's already a bit confusing and error-prone.

Following the BLU GW example (as well as other similar ones), I already made the suggestion here: #18775 (comment). I agree that the THING_TYPE prefix is confusing when it doesn't actually represent a thing type, however that would be consistent with how things are now.

If you don't want to touch this right now in this PR, I can offer to propose some additional rework, since I don't think we should leave this inconsistency.

Copy link
Contributor Author

@markus7017 markus7017 Jun 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the BLU GW example

That's a different case, because the device reports the correct serviceName > thingType
Gen2: shellyblugw -> shellyblugw
Gen3: shellyblugwg3 > shellyblugw

If you don't want to touch this right now in this PR

I prefer to leave it as is. Any change here could result in significant regression testing
We should merge as is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a different case, because the device reports the correct serviceName > thingType
Gen2: shellyblugw -> shellyblugw
Gen3: shellyblugwg3 > shellyblugw

It seems we are still not understanding each other. Maybe it reports "correct" service name, but it's still not mapped directly to a thing type id. shellyblugwg3 is mapped to shellyblugw. So here shellyblugwg3 is the service name, but we don't have a corresponding thing type id. Just like shellyplus is not directly mapped to shellyplugus, but rather shellyplusplugus. You could say that it doesn't have a correct service name, but it really makes no difference from code perspective. It has the service name it has. In either case, we are mapping one service name to a (different) thing type id.

I can merge in the evening, I'll just add one additional line for testing the gen2 property: #18775 (comment). I will then try to propose something to make this more robust. I may start out creating an issue outlining the problem and possible improvements.

public static final String THING_TYPE_SHELLYPLUSPLUGUS_STR = "shellyplusplugus";
public static final String THING_TYPE_SHELLYPLUSDIMMERUS_STR = "shellypluswdus";
public static final String THING_TYPE_SHELLYPLUSDIMMER10V_STR = "shellyplus10v";
Expand Down Expand Up @@ -515,6 +516,7 @@ public class ShellyThingCreator {
Map.entry(THING_TYPE_SHELLYPLUS1PMG4_STR, THING_TYPE_SHELLYPLUS1PM_STR),
Map.entry(THING_TYPE_SHELLYPLUS2PM_RELAY_STR, THING_TYPE_SHELLYPLUS2PM_RELAY_STR),
Map.entry(THING_TYPE_SHELLYPLUS2PM_ROLLER_STR, THING_TYPE_SHELLYPLUS2PM_ROLLER_STR),
Map.entry(SVC_TYPE_SHELLYPLUSPLUGUS_STR, THING_TYPE_SHELLYPLUSPLUGUS_STR),
Map.entry(THING_TYPE_SHELLYPLUSPLUGS_STR, THING_TYPE_SHELLYPLUSPLUGS_STR),
Map.entry(THING_TYPE_SHELLYPLUSPLUGUS_STR, THING_TYPE_SHELLYPLUSPLUGUS_STR),
Map.entry(THING_TYPE_SHELLYPLUSI4_STR, THING_TYPE_SHELLYPLUSI4_STR),
Expand Down Expand Up @@ -604,7 +606,7 @@ private static String getThingTypeID(String serviceName, String deviceType, Stri
if (serviceNameLowerCase.startsWith(THING_TYPE_SHELLY2_PREFIX)) { // Shelly v2
return getRelayOrRollerType(THING_TYPE_SHELLY2_RELAY_STR, THING_TYPE_SHELLY2_ROLLER_STR, mode);
}
if (serviceNameLowerCase.startsWith(THING_TYPE_SHELLYPLUG_STR)) {
if (serviceNameLowerCase.startsWith(THING_TYPE_SHELLYPLUG_STR) && !serviceNameLowerCase.contains("plugus")) {
// shellyplug-s needs to be mapped to shellyplugs to follow the schema
// for the thing types: <thing type>-<mode>
if (serviceNameLowerCase.startsWith(THING_TYPE_SHELLYPLUGS_STR) || serviceNameLowerCase.contains("-s")) {
Expand Down
3 changes: 2 additions & 1 deletion bundles/org.openhab.binding.shelly/src/main/resources/OH-INF/i18n/shelly.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ thing-type.shelly.shellyplus1.description = Shelly Plus 1 (Single Relay Switch)
thing-type.shelly.shellyplus1pm.description = Shelly Plus 1PM - Single Relay Switch with Power Meter
thing-type.shelly.shellyplus2-relay.description = Shelly Plus 2PM - Dual Relay Switch with Power Meter
thing-type.shelly.shellyplus2pm-roller.description = Shelly Plus 2PM - Roller Control with Power Meter
thing-type.shelly.shellyplusplug.description = Shelly Plus Plug S/IT/UK/US . Outlet with Power Meter
thing-type.shelly.shellyplusplug.description = Shelly Plus Plug S/IT/UK. Outlet with Power Meter
thing-type.shelly.shellyplusplugus.description = Shelly Plus Plug US . Outlet with Power Meter
thing-type.shelly.shellyplusi4.description = Shelly Plus i4 - 4xInput Device
thing-type.shelly.shellyplusi4dc.description = Shelly Plus i4DC - 4xDC Input Device
thing-type.shelly.shellyplusuni.description = Shelly Plus UNI - Universal Module
Expand Down
14 changes: 14 additions & 0 deletions bundles/org.openhab.binding.shelly/src/main/resources/OH-INF/thing/shellyGen2_relay.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,20 @@
<config-description-ref uri="thing-type:shelly:relay-gen2"/>
</thing-type>

<thing-type id="shellyplusplugus">
<label>ShellyPlus Plug US</label>
<description>@text/thing-type.shelly.shellyplusplugus.description</description>
<category>PowerOutlet</category>
<semantic-equipment-tag>PowerOutlet</semantic-equipment-tag>
<channel-groups>
<channel-group id="relay" typeId="relayChannelPlug"/>
<channel-group id="meter" typeId="meter"/>
<channel-group id="device" typeId="deviceStatus"/>
</channel-groups>

<representation-property>serviceName</representation-property>
<config-description-ref uri="thing-type:shelly:relay-gen2"/>
</thing-type>

<thing-type id="shellyplusi4">
<label>ShellyPlus i4</label>
Expand Down
1 change: 1 addition & 0 deletions ...y/src/test/java/org/openhab/binding/shelly/internal/discovery/ShellyThingCreatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ private static Stream<Arguments> provideTestCasesForGetThingUIDReturnsThingUidAc
Arguments.of("shellyplug-su1-" + DEVICE_ID, "", "", THING_TYPE_SHELLYPLUGS_STR), //
Arguments.of("shellyplugu1-" + DEVICE_ID, "", "", THING_TYPE_SHELLYPLUGU1_STR), //
Arguments.of("shellyplugu12-" + DEVICE_ID, "", "", THING_TYPE_SHELLYPLUGU1_STR), //
Arguments.of("shellyplugus-" + DEVICE_ID, "", "", THING_TYPE_SHELLYPLUSPLUGUS_STR), //
Arguments.of("shellyrgbw2-" + DEVICE_ID, "", "color", THING_TYPE_SHELLYRGBW2_COLOR_STR), //
Arguments.of("shellyrgbw2-" + DEVICE_ID, "", "", THING_TYPE_SHELLYRGBW2_WHITE_STR), //
Arguments.of("shellyrgbw2-" + DEVICE_ID, "", "colour", THING_TYPE_SHELLYRGBW2_WHITE_STR), //
Expand Down