[shelly] Fix thing type descriptions for Plus Mini series

[jlaur]

See #17013 (comment)

Before:

After:

Related to #17013

[lsiepel]

LGTM, waiting for Markus to confirm.

[jlaur]

@markus7017 - while you are currently active on #17011, would you care to review this one? 🙂

[markus7017]

I was just working on similar cleanup in the other PR.

README changes are fine
.properties changes are fine

I would like to keep MINI as a class of devices so

THING_TYPE_SHELLYMINI1
THING_TYPE_SHELLYMINIPM
THING_TYPE_SHELLYMINI1PM

rather than

THING_TYPE_SHELLY1MINI
THING_TYPE_SHELLYPMMINI
THING_TYPE_SHELLY1PMMINI

or even better

THING_TYPE_SHELLYMINI_1
THING_TYPE_SHELLYMINI_PM
THING_TYPE_SHELLYMINI_1PM

to make it more readable. The pending PR for Mini Gen3 will bring also a G3 suffix.

I would suggest to keep the current notation and just fix the descriptions, which also reduces the size of the PR and change the name schema with the next PR including Gen3 names.

Nevertheless, I'm also fine to merge it as is and take it from there with the next PR

[markus7017]

let's keep it as is and clean-up with the next PR bringing also Mini G3

[jlaur]

Thing type id is shelly1mini, so it seems inconsistent to use a different name here when the sole purpose of this constant is to reference the thing type. IMHO this is part of inconsistency which paved the way for this bug.

[falkena]

Just to give my 5 cents to this. I think, the confusion comes from device naming: They called really Shelly Mini 1 and so on. Binding choose another naming convention. I fixed this issue long time ago in my fork exactly oppisit way trying to take my Shellies to operation: 9749749 Commit is isolated so far and shall be cherry-pickable easy way. For Gen1 devices rest api call may return empty string. Means, the check in ShellyBaseHandler.java in line 159 shall check empty string. Some additional information can be found under https://community.openhab.org/t/shelly-binding/56862/3890

[jlaur]

Just to give my 5 cents to this. I think, the confusion comes from device naming: They called really Shelly Mini 1 and so on. Binding choose another naming convention.

Thanks for chipping in. I just unboxed my first Shelly (PM Mini) this evening and got it online, so I now have my first hands-on experience. 😄 This already made me realize the implications behind this comment: #17009 (comment) regarding naming schema changes. The mDNS name of my device starts with "shellypmmini-" and I then found the logic for determining the thing type:

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

Lines 547 to 552 in c0d2638

	// Check general mapping
	if (!deviceType.isEmpty()) {
	String res = THING_TYPE_MAPPING.get(deviceType); // by device type
	if (res != null) {
	return res;
	}

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

Line 477 in c0d2638

THING_TYPE_MAPPING.put(THING_TYPE_SHELLYMINIPM_STR, THING_TYPE_SHELLYMINIPM_STR);

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

Line 190 in c0d2638

public static final String THING_TYPE_SHELLYMINIPM_STR = "shellypmmini";

So the mapping between mDNS name and thing type here is a bit coupled (same constant used for key/value). OTOH, it seems mostly about naming (prefix THING_TYPE_), so this could be easily mitigated going foward.

Anyway, to conclude and reiterate the scope of this PR:

• Fix thing type descriptions since they are currently shown as I18N keys (see description).
• For consistency, rename corresponding constants, since this mixup is likely the reason why this bug occurred in the first place.

For improving and fixing determination of the API revision to use for specific devices/things, there is already #17011.

[markus7017]

The official device naming is
Shelly Plus PM Mini, Shelly Plus 1 Mini, Shelly Plus 1PM Mini and
Shelly Plus PM Mini Gen3, Shelly Plus 1 Mini Gen3, Shelly Plus 1M Mini Gen3
are currently shipping and will be part of PR #17012
this results into service names reported by the device e.g. shelly1mini, shellypmmini
so they left out the Plus for length reasons (discussed that with the Allterco Dev team)
Nevertheless, those case the values, which doesn't mean that naming of constants needs to be exactly the same. I used the same naming for the thing types and I18 keys to avoid even more overhead.

One or the other, @jlaur I'm fine with your changes, please trigger a merge soI could finish #17012 to integrate support for Gen3 devices.

[jlaur]

The official device naming is
Shelly Plus PM Mini, Shelly Plus 1 Mini, Shelly Plus 1PM Mini and
Shelly Plus PM Mini Gen3, Shelly Plus 1 Mini Gen3, Shelly Plus 1M Mini Gen3
are currently shipping and will be part of PR #17012
this results into service names reported by the device e.g. shelly1mini, shellypmmini
so they left out the Plus for length reasons (discussed that with the Allterco Dev team)

Sorry, but I still don't understand your point:

• Product name: Shelly Plus PM Mini
• Service name: shellypmmini
• Thing type id: shellypmmini

So why would you prefer to keep THING_TYPE_SHELLYMINIPM as name of the constant referring to thing type id shellypmmini?

I can understand that you may be annoyed by this:

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

Line 407 in 7755681

return thingType.startsWith("shellyplus") || thingType.startsWith("shellypro") || thingType.contains("mini")

i.e. thingType.contains("mini") rather than thingType.startsWith("shellymini"). But this is an entirely different topic which relates to the way thing types are already named.

One or the other, @jlaur I'm fine with your changes, please trigger a merge soI could finish #17012 to integrate support for Gen3 devices.

In that case, please approve the PR, and @lsiepel can merge it.

[jlaur]

One or the other, @jlaur I'm fine with your changes, please trigger a merge soI could finish #17012 to integrate support for Gen3 devices.

In that case, please approve the PR, and @lsiepel can merge it.

@lsiepel - I suspect @markus7017 is not receiving messages/notifications when tagged, so I think you can merge based on the "written approval" above.

[lsiepel]

I suspect htis should also be backported?

[jlaur]

I suspect htis should also be backported?

Yes, done. 🙂 Thanks!