[shelly] Fix support for Shelly Plug US #18775

markus7017 · 2025-06-07T20:01:50Z

Fix Shelly Plug US: This is a Gen2 device, but the service name is missing the "plus" - shellyplugus instead of shellyplusplugus. As a result the thing uses the Gen1 API, which does not work.

Signed-off-by: Markus Michels markus7017@gmail.com

missing the "plus" - shellyplugus instead of shellyplusplugus. As a result the thing uses the Gen1 API, which does not work. Signed-off-by: Markus Michels <markus7017@gmail.com>

Signed-off-by: Markus Michels <markus7017@gmail.com>

lsiepel · 2025-06-08T08:56:36Z

Are you testing this yourself @markus7017 or if you need others, providing a jar and/or link to a community thread, testing would be made easier and the review/merge proces shorter.

jlaur

Thanks for the fix. I'm on holiday reviewing on my phone, so could have missed some details. However, two minor things noted.

jlaur · 2025-06-08T20:51:36Z

...g.shelly/src/main/java/org/openhab/binding/shelly/internal/discovery/ShellyThingCreator.java

    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";


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

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.

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.

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

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'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.

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

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.

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.

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.

bundles/org.openhab.binding.shelly/src/main/resources/OH-INF/i18n/shelly.properties

jlaur · 2025-06-08T21:22:01Z

@markus7017 - since thing type shellyplusplugus was missing, I'm confused by the issue title "does not work anymore". Did it ever work? Is it a regression from a recent PR?

Signed-off-by: Markus Michels <markus7017@gmail.com>

markus7017 · 2025-06-09T14:14:13Z

@jlaur review changes applied

markus7017 · 2025-06-10T01:43:33Z

@markus7017 - since thing type shellyplusplugus was missing, I'm confused by the issue title "does not work anymore". Did it ever work? Is it a regression from a recent PR?

Yes it did.
A longer time ago the device generation was fetched from a mDNS value and written as a thing property, which then gets process by the thing handler to select between API v1 and v2 format. Due to some issues, the code was changed to detect v2 based on the serviceName prefix shellyplusxxx, shellyproxxx, shellyxxxmini etc. Thus "shellyplugus" is interpreted as Gen1 device even it's a Gen2+ device (using API v2). Now thing type is shellyplusplugus, which means it starts with prefix shellyplus and therefore initializes API v2 access.

jlaur · 2025-06-10T10:51:03Z

@markus7017 - since thing type shellyplusplugus was missing, I'm confused by the issue title "does not work anymore". Did it ever work? Is it a regression from a recent PR?

Yes it did. A longer time ago the device generation was fetched from a mDNS value and written as a thing property, which then gets process by the thing handler to select between API v1 and v2 format. Due to some issues, the code was changed to detect v2 based on the serviceName prefix shellyplusxxx, shellyproxxx, shellyxxxmini etc. Thus "shellyplugus" is interpreted as Gen1 device even it's a Gen2+ device (using API v2). Now thing type is shellyplusplugus, which means it starts with prefix shellyplus and therefore initializes API v2 access.

Thanks for the explanation. I guess when it last worked, it was using the same thing type as the non-US version then, but now it has its own in order to have a different label/description?

markus7017 · 2025-06-10T18:42:03Z

Thanks for the explanation. I guess when it last worked, it was using the same thing type as the non-US version then, but now it has its own in order to have a different label/description?

It was integrated a long time ago, when the Gen flag of the mDNS discovery was processed. Then we changed that handling and as a side effect broke support for this device. I'm in the US only from time to time. This time I recognized that it's broken and fixed it here on site. I think there is no user in the US using the binding and having this device. Therefore...

jlaur · 2025-06-10T20:07:12Z

Thanks for the explanation. I guess when it last worked, it was using the same thing type as the non-US version then, but now it has its own in order to have a different label/description?

It was integrated a long time ago, when the Gen flag of the mDNS discovery was processed. Then we changed that handling and as a side effect broke support for this device. I'm in the US only from time to time. This time I recognized that it's broken and fixed it here on site. I think there is no user in the US using the binding and having this device. Therefore...

I meant to simply ask for confirmation that the reason a new thing type was introduced here (which was never there before), is to have a different label and description? Otherwise they are totally the same, correct?

markus7017 · 2025-06-11T04:28:58Z

I need the "plus" in the thingType name to ensure using V2 API

jlaur · 2025-06-11T06:25:15Z

I need the "plus" in the thingType name to ensure using V2 API

Ah, right, I forgot this is based on thing type id. So otherwise this method would need to be tweaked:

openhab-addons/bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/api/ShellyDeviceProfile.java

Lines 412 to 417 in 057dedf

    
           public static boolean isGeneration2(String thingType) { 
        
               return thingType.startsWith("shellyplus") || thingType.startsWith("shellypro") || thingType.contains("mini") 
        
                       || thingType.startsWith("shellywall") 
        
                       || (thingType.startsWith("shelly") && (thingType.contains("g3") || thingType.contains("g4"))) 
        
                       || isBluSeries(thingType) || thingType.startsWith(THING_TYPE_SHELLYBLUGW_STR); 
        
           }

Could you also add a testcase for the new thing type id around here to make sure it will keep working? 😉

openhab-addons/bundles/org.openhab.binding.shelly/src/test/java/org/openhab/binding/shelly/internal/api/ShellyDeviceProfileTest.java

Lines 70 to 72 in 057dedf

    
           Arguments.of(THING_TYPE_SHELLYPLUG_STR, false, false), // 
        
           Arguments.of(THING_TYPE_SHELLYPLUGS_STR, false, false), // 
        
           Arguments.of(THING_TYPE_SHELLYPLUGU1_STR, false, false), //

markus7017 · 2025-06-11T16:29:53Z

Ah, right, I forgot this is based on thing type id. So otherwise this method would need to be tweaked:

I'm thinking to re-implement the handling based on the gen flag from mDNS announcement. This is a clear indicator, everything around names might has gaps, because as Plug US shows even single devices do not implement consistent naming. On the other this gen detected based on mDNS does not cover the scenario when the user adds a thing manually.

markus7017 · 2025-06-11T16:42:10Z

Could you also add a testcase for the new thing type id around here to make sure it will keep working? 😉

I added some more and will come back to that on the Gen4 PR. I try to cover all devices.

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>

jlaur

LGTM

* Fix Shelly Plug US: This is a Gen2 device, but the service name is missing the "plus" - shellyplugus instead of shellyplusplugus. As a result the thing uses the Gen1 API, which does not work. Signed-off-by: Markus Michels <markus7017@gmail.com>

* Fix Shelly Plug US: This is a Gen2 device, but the service name is missing the "plus" - shellyplugus instead of shellyplusplugus. As a result the thing uses the Gen1 API, which does not work. Signed-off-by: Markus Michels <markus7017@gmail.com> Signed-off-by: Paul Smedley <paul@smedley.id.au>

Fix Shelly Plug US: This is a Gen2 device, but the service name is

81fd47a

missing the "plus" - shellyplugus instead of shellyplusplugus. As a result the thing uses the Gen1 API, which does not work. Signed-off-by: Markus Michels <markus7017@gmail.com>

markus7017 self-assigned this Jun 7, 2025

markus7017 added bug An unexpected problem or unintended behavior of an add-on additional testing preferred The change works for the pull request author. A test from someone else is preferred though. labels Jun 7, 2025

markus7017 added 3 commits June 7, 2025 16:02

cleanup

432b329

Signed-off-by: Markus Michels <markus7017@gmail.com>

README updated

769aa6f

Signed-off-by: Markus Michels <markus7017@gmail.com>

README updated

0ead38f

Signed-off-by: Markus Michels <markus7017@gmail.com>

markus7017 removed the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Jun 8, 2025

markus7017 requested a review from jlaur June 8, 2025 20:19

jlaur reviewed Jun 8, 2025

View reviewed changes

jlaur linked an issue Jun 8, 2025 that may be closed by this pull request

[shelly] Plus Plug US does not work anymore #18783

Closed

jlaur mentioned this pull request Jun 8, 2025

[shelly] Add support for Gen4 devices #18782

Merged

apply review changes

057dedf

Signed-off-by: Markus Michels <markus7017@gmail.com>

Add test coverage

23be848

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>

jlaur approved these changes Jun 12, 2025

View reviewed changes

jlaur merged commit b3893bf into openhab:main Jun 12, 2025
1 of 2 checks passed

jlaur added this to the 5.0 milestone Jun 12, 2025

jlaur mentioned this pull request Jun 12, 2025

[shelly] Refactor thing type handling #18796

Merged

markus7017 deleted the shelly_fixplugus branch June 14, 2025 17:34

This was referenced Jul 13, 2025

[shelly] Use thing type shellyplusht for Shelly Plus HT Gen3 #18943

Merged

[shelly] Add initial support for Shelly Dimmer Gen3 #18686

Merged

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

Uh oh!

[shelly] Fix support for Shelly Plug US #18775

[shelly] Fix support for Shelly Plug US #18775

Uh oh!

Conversation

markus7017 commented Jun 7, 2025

Uh oh!

lsiepel commented Jun 8, 2025

Uh oh!

jlaur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markus7017 Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markus7017 Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jlaur commented Jun 8, 2025

Uh oh!

markus7017 commented Jun 9, 2025

Uh oh!

markus7017 commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jlaur commented Jun 10, 2025

Uh oh!

markus7017 commented Jun 10, 2025

Uh oh!

jlaur commented Jun 10, 2025

Uh oh!

markus7017 commented Jun 11, 2025

Uh oh!

jlaur commented Jun 11, 2025

Uh oh!

markus7017 commented Jun 11, 2025

Uh oh!

markus7017 commented Jun 11, 2025

Uh oh!

jlaur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

markus7017 Jun 11, 2025 •

edited

Loading

markus7017 Jun 11, 2025 •

edited

Loading

markus7017 commented Jun 10, 2025 •

edited

Loading