[shelly] Fix Shelly Pro 4PM discovery, thing init for unknown thing

[markus7017]

This PR fixes some discovery / initialization issues

• Shelly Pro 4PM was no longer discovered
• Shelly Pro 2 was discovered, but initialization failed
• Unknown device types will be added using type shellyunknown. Now the initialization of those things is blocked.

[markus7017]

@jlaur During testing I recognized that Pro 2 was discovered, but not initialized.
reason:
The thing-id still has the suffix "-relay". In this case device.mode is required to be = "relay". However, due to the fact that the device is not dual mode (relay / roller) it does not include the relay mode in /shelly

For now I fixed it by initializing device.mode = "relay" and kept the "shellypro2-relay" thing id.

Option b) would be to remove the "-relay" from the thing id, in fact there is no need for anymore. However, consequently we would have to rename thing id shellypro2-relay to shellypro2, but this breaks backward compatibility.

Option c) we implement b), but keep thing definition for shellypro2-relay. New discovered things map to shellypro2, whereas old setups continue to use shellypro2-relay

What do you think?

[jlaur]

The thing-id still has the suffix "-relay". In this case device.mode is required to be = "relay". However, due to the fact that the device is not dual mode (relay / roller) it does not include the relay mode in /shelly

I need a bit of additional context: Is it related to discovery, initialization or both? With #18882 the device type mapping should now work correctly, so that e.g. SPSW-002XE16EU with mode = "" will correctly map to thing type shellypro2-relay. With mode being "relay", this mapping would not work. The service name mapping was however not changed, so shellypro2-relay is still expected (so I guess will not work, thus we depend on device type).

Without manually setting mode to "relay", where will the problem occur - can you point me to the code line?

[markus7017]

See ShellyBaseHandler:343

        // Validate device mode
        String reqMode = thingType.contains("-") ? substringAfter(thingType, "-") : "";
        if (!reqMode.isEmpty() && !mode.equals(reqMode)) {
            setThingOfflineAndDisconnect(ThingStatusDetail.CONFIGURATION_ERROR, "offline.conf-error-wrong-mode", mode,
                    reqMode);
            return false;
        }

reqMode is relay and mode ""

[jlaur]

I think there is also a problem here:

openhab-addons/bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/api2/Shelly2ApiRpc.java

Lines 270 to 271 in b7a64a0

	if (thingType.equals(THING_TYPE_SHELLYPRO2_RELAY_STR)) {
	profile.numMeters = 0;

openhab-addons/bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/discovery/ShellyThingCreator.java

Line 223 in b7a64a0

public static final String THING_TYPE_SHELLYPRO2_RELAY_STR = "shellypro2-relay";

If the thingType parameter is actually a thing type id, then this should be okay. But as analyzed in #18802, this parameter sometimes contain a service name rather than a thing type id. The service name in this case would be "shellypro2" rather than "shellypro2-relay", I presume?

[jlaur]

See ShellyBaseHandler:343

So we rely on two things there:

1. Thing type id must end with "-relay" (otherwise reqMode will be empty).
2. ShellyDeviceProfile.device.mode is initialized with "relay" (I need to look more into how).

Is that correct?

I'll need to think some more about this. I'm trying to figure out how such things are usually determined, but I'm currently a bit at a loss, so will probably need to try again with a fresh mind. 🙂

[markus7017]

if we change the naming those would be ok, but you're right, in this case the old thing type wouldn't work and needs special handling.

So I suggest to go with the current version.

[jlaur]

I have added a few additional comments, otherwise LGTM. There is also a test to be fixed.

[markus7017]

While testing with Shelly Shutter, I tried to omit the -relay/-roller suffix and got it working, I'll commit tomorrow

[markus7017]

@jlaur I fixed the build error/unit test
anything else?

[jlaur]

anything else?

Only one question:

While testing with Shelly Shutter, I tried to omit the -relay/-roller suffix and got it working, I'll commit tomorrow

Did you mean you'd commit something in relation to these test results?

[jlaur]

There is also this comment unanswered: #18902 (comment)

We compare directly by "shellypro2-relay" inside getDeviceProfile. But this call doesn't provide thing type id, it provides service name, and even makes sure not to include anything after "-":

openhab-addons/bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/discovery/ShellyBasicDiscoveryService.java

Lines 143 to 144 in 551bb5d

	thingType = substringBeforeLast(name, "-");
	profile = api.getDeviceProfile(thingType, devInfo);

So even if service name was "shellypro2-relay" (which I guess it isn't), it would still call getDeviceProfile with thingType = "shellypro2". So the special case here is not handled:

openhab-addons/bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/api2/Shelly2ApiRpc.java

Lines 266 to 280 in 551bb5d

	// handle special cases, because there is no indicator for a meter in GetConfig
	// Pro 3EM has 3 meters
	// Pro 2 has 2 relays, but no meters
	// Mini PM has 1 meter, but no relay
	if (thingType.equals(THING_TYPE_SHELLYPRO2_RELAY_STR)) {
	profile.numMeters = 0;
	} else if (thingType.equals(THING_TYPE_SHELLYPRO3EM_STR)) {
	profile.numMeters = 3;
	} else if (dc.pm10 != null) {
	profile.numMeters = 1;
	} else if (dc.em0 != null) {
	profile.numMeters = 3;
	} else if (dc.em10 != null) {
	profile.numMeters = 2;
	}

[markus7017]

@jlaur while testing the implementation I ran into issues that default init mode="relay" if device doesn't provide the mode (relay/roller) impacts discovery of other devices, which is a no go. On the other side keeping the shellypro2-roller naming would be the only exception, also doesn't make sense.

Therefor I decided to implement a breaking change in renaming thing shellypro2-relay into shellypro2. This makes is consistent naming schema, but also omits special handlings.

Implicitly this also solves the issue above, because now service name and thing name are both shellypro2. Works for the moment, but I agree we should remove this mix. However, this is currently used to handle the case that the Pro2 has 2 relays, but no meters, therefor numMeters have to be 0. I already through how to avoid dependency to device types when determining the number of meters, but couldn't find a way so far. The problem is that the device doesn't return the number of meters in the device settings nor elements like em0/em10/em11/pm0, which could be used. This is included in the status report when meter data changes. However, I already need it at initialization time. I want to check if I could implement an auto-allocation/initialization on status updates instead of initialization, but that needs good testing and might create side effects.

Long story short: Could you please review the current changes while I do some additional testing, esp. with detection/initialization of other devices before we merge. Sorry for the effort, but changes to that part of the code always has the risk for nice side effects :-), appreciate your help 👍

[jlaur]

Implicitly this also solves the issue above, because now service name and thing name are both shellypro2. Works for the moment, but I agree we should remove this mix.

I already briefly started some draft refactoring regarding to this issue - in relation to #18802. I plan to run through my refactoring steps once again and provide distinct commits per step so you can follow the thinking. I'll then provide a draft PR to discuss. The main objective is to remove all dependencies between service name and thing type id.

Long story short: Could you please review the current changes

I already reviewed up until current state. 👍 Will await the next changes.

[markus7017]

I try to do testing tonight
What's the closing window for 5.0

• We need to get in this one
• Gen4 device support should be in
• support for some new devices would be nice, but not essential
• I have another PR for various Plus/Pro dimmers, but that needs further development. I think it will not make it into 5.0

[jlaur]

I try to do testing tonight

👍

What's the closing window for 5.0

Feature freeze is planned three days from now. After that additional five days until code freeze, i.e. important bugfixes can still get in. So there is not much time left. Can you also have a look at other Shelly PR's with pending reviews?

[markus7017]

looks good from a first check. I was able to discover 75 out of 82 devices on the network. The remaining 7 are device types, which are not integrated with this PR level like Pro Dimmer 1PM/2PM, DALI Dimmer etc.

How could I document a breaking change (thing type shellypro2-relay was renamed to shellypro2 and things need to be re-discovered).

I do some more testing tomorrow and check a bunch of devices that all channels get created correctly.

[jlaur]

How could I document a breaking change (thing type shellypro2-relay was renamed to shellypro2 and things need to be re-discovered).

You can create a PR towards the openhab-distro repo adding a line around here:
https://github.com/openhab/openhab-distro/blob/183912ff484f5b0e9d4b8777babc04ece4c1aec2/distributions/openhab/src/main/resources/bin/update.lst#L162-L173

[markus7017]

I did additional testing and it LGTM, please go ahead and merge

[lsiepel]

@jlaur i'll leave it to you to merge. @markus7017 could you both update the start post with a small description of the breaking change so that users have some guidance when looking at the release notes. And please also create a PR with the breaking change notices here: https://github.com/openhab/openhab-distro/pulls

[markus7017]

@lsiepel done, PR #1765 created

[jlaur]

LGTM