[shelly] Fix Gen1 initialization when thing is defined in .things #17009
Conversation
…ings Signed-off-by: Markus Michels <[email protected]>
markus7017
added
the
bug
jlaur
added
critical
regression
jlaur
changed the title
| blu = ShellyDeviceProfile.isBluSeries(thingType); | ||
| gen2 = "2".equals(gen) || "3".equals(gen) || blu || ShellyDeviceProfile.isGeneration2(thingType); |
There was a problem hiding this comment.
Can thing type be ambiguous, so that you cannot reliably determine generation from it?
If it can, I guess this will still be broken for users having manually created things?
It it cannot, why check the property at all to determine generation? Would this work:
gen2 = blu || ShellyDeviceProfile.isGeneration2(thingType);?
Or going further:
blu = ShellyDeviceProfile.isBluSeries(thingType);
gen2 = ShellyDeviceProfile.isGeneration2(thingType);
if (blu) {
this.api = new ShellyBluApi(thingName, thingTable, this);
} else if (gen2) {
this.api = new Shelly2ApiRpc(thingName, thingTable, this);
} else {
this.api = new Shelly1HttpApi(thingName, this);
}See also #17006 (comment)
There was a problem hiding this comment.
That is exactly one of my questions in previous @J-N-K PR.
There was a problem hiding this comment.
It depends on whether this property is only informative or really required to initialize the handler. If required, it should be a configuration setting.
There was a problem hiding this comment.
I know, but since the property is still checked in this PR, I wanted to ask it again and even more explicitly to fully understand. I can see only one reason for checking the property at all, and that would be some corner case where thing type is not sufficient. In that case, even this fix would not work for all scenarios.
There was a problem hiding this comment.
Here are the thing type-based generation checks:
I tried to apply this logic the all thing types:
| Thing Type ID | Label | Result |
|---|---|---|
| shellyblubutton | Shelly BLU Button | blu |
| shellybludw | Shelly BLU Door/Window | blu |
| shellyblumotion | Shelly BLU Motion | blu |
| shellybluht | Shelly BLU H&T | blu |
| shellyblugw | Shelly BLU Gateway | gen1 |
| shellybulb | Shelly Bulb | gen1 |
| shellybulbduo | Shelly Duo | gen1 |
| shellycolorbulb | Shelly Color Bulb | gen1 |
| shellyvintage | Shelly Vintage | gen1 |
| shellyrgbw2-color | Shelly RGBW2 Color Mode | gen1 |
| shellyrgbw2-white | Shelly RGBW2 White Mode | gen1 |
| shelly1 | Shelly 1 | gen1 |
| shelly1l | Shelly 1L | gen1 |
| shelly1pm | Shelly 1PM | gen1 |
| shellyem | Shelly EM | gen1 |
| shellyem3 | Shelly EM3 | gen1 |
| shelly2-relay | Shelly 2 Relay | gen1 |
| shelly2-roller | Shelly 2 Roller | gen1 |
| shelly25-relay | Shelly 2.5 Relay | gen1 |
| shelly25-roller | Shelly 2.5 Roller | gen1 |
| shelly4pro | Shelly 4 Pro Relay | gen1 |
| shellyplug | Shelly Plug | gen1 |
| shellyplugs | Shelly Plug-S | gen1 |
| shellyplugu1 | Shelly Plug US | gen1 |
| shellyuni | Shelly UNI | gen1 |
| shellydimmer | Shelly Dimmer | gen1 |
| shellydimmer2 | Shelly Dimmer 2 | gen1 |
| shellyix3 | Shelly ix3 | gen1 |
| shellyht | Shelly H&T | gen1 |
| shellysmoke | Shelly Smoke | gen1 |
| shellygas | Shelly Gas | gen1 |
| shellyflood | Shelly Flood | gen1 |
| shellydw | Shelly Door/Window | gen1 |
| shellydw2 | Shelly Door/Window | gen1 |
| shellysense | Shelly Sense | gen1 |
| shellybutton1 | Shelly Button 1 | gen1 |
| shellybutton2 | Shelly Button 2 | gen1 |
| shellymotion | Shelly Motion | gen1 |
| shellytrv | Shelly TRV | gen1 |
| shellyplus1 | ShellyPlus 1 | gen2 |
| shellyplus1pm | ShellyPlus 1PM | gen2 |
| shellyplus2pm-relay | ShellyPlus 2 Relay | gen2 |
| shellyplus2pm-roller | ShellyPlus 2PM Roller | gen2 |
| shellyplusplug | ShellyPlus Plug | gen2 |
| shellyplusi4 | ShellyPlus i4 | gen2 |
| shellyplusi4dc | ShellyPlus i4DC | gen2 |
| shelly1mini | ShellyPlus 1 Mini | gen2 |
| shellypmmini | ShellyPlus PM Mini | gen2 |
| shelly1pmmini | ShellyPlus 1PM Mini | gen2 |
| shellypro1 | ShellyPro 1 | gen2 |
| shellypro1pm | ShellyPro 1PM | gen2 |
| shellypro2-relay | ShellyPro 2 Relay | gen2 |
| shellypro2pm-relay | ShellyPro 2PM Relay | gen2 |
| shellypro2pm-roller | ShellyPro 2PM Roller | gen2 |
| shellypro3 | ShellyPro 3 | gen2 |
| shellypro3em | Shelly Pro 3EM | gen2 |
| shellyproem50 | Shelly Pro EM-50 | gen2 |
| shellypro4pm | ShellyPro 4PM | gen2 |
| shellypluswdus | Shelly Plus Dimmer US | gen2 |
| shellyplus10v | Shelly Plus Dimmer 10V | gen2 |
| shellyhtg3 | ShellyPlus H&T Gen 3 | gen2 |
| shellyplussmoke | Shelly Plus Smoke | gen2 |
| shellywalldisplay | Shelly Wall Display | gen1 |
| shellydevice | Shelly Device | gen1 |
| shellyunknown | Unknown Shelly Device | gen1 |
Does it look good to you?
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/openhab-4-2-milestone-discussion/154316/247 |
|
The device reports the device generation as part of the mDNS discovery, that's the source of truth and has to have priority in determining this information, which is then used to select Gen1 or Gen2 API format. When the thing is manually defined in .things (without proper information) the binding checks the thing type to get the information:
Therefore the logic is a proper solution. |
|
In that case, I understand the property has to be replaced by a configuration parameter so that it can be set either in a thing file or in UI when creating a thing. It will be optional. The problem is not specific to file configuration, the problem is if discovery is not used. |
No, strongly disagree. A config parameter is selectable by the use. A Gen1 device has to use a Gen1 API and Gen 2+3 the Gen 2 API. It doesn't make any sense that the use can select Gen 2 API for Gen 1 device and vice versa. |
|
I give up |
You cannot rely on mDNS for this, this will break manually created things. I think I understand everything you've written, but you still haven't really answered my questions here:
I would really appreciate if you would, because otherwise I'm afraid we'll continue running in circles. I will ask them again here, slightly differently, can you please answer each question directly so we can avoid further misunderstandings/miscommunication? First, discovery determines generation from mDNS. It then creates a thing with a specific
|
|
I'm jumping on this without a full understanding, so I apologise beforehand. However, it seems to me that the solution should be:
|
|
Re 4) yes, the list is correct until Allterca changes their naming schema As I said, this will work unless Allterco brings a new Shelly device with different naming schema, but for now the logic will work and I could change it. I refuse to implement a config option, a Gen1 device will never work with a Gen2 API and vice versa. Do we agree on 2) and see what happens for users upgrading to the new version? |
|
I verified the change with
|
|
PR #17011 has been created with this change |
I don't understand the correlation here, can you explain? I had a look at this thing type mapping: Once a thing type id is determined and the thing is created, one specific API will be used for that thing type based on the mentioned logic. If vendor changes naming scheme, I suppose the mapping would need to be updated to determine the correct thing types to create from discovery, correct? That would not break existing things, only things created from discovery. And the work-around would be to create the thing manually, so that the correct corresponding API would be used?
If you agree that the proposed approach is reliable, yes - then I agree we should remove any dependency upon properties for determining the API revision to use. |
…ings (openhab#17009) Signed-off-by: Markus Michels <[email protected]>
…ings (openhab#17009) Signed-off-by: Markus Michels <[email protected]> Signed-off-by: Patrik Gfeller <[email protected]>
…ings (openhab#17009) Signed-off-by: Markus Michels <[email protected]>
…ings (openhab#17009) Signed-off-by: Markus Michels <[email protected]>
…ings (openhab#17009) Signed-off-by: Markus Michels <[email protected]> Signed-off-by: Ciprian Pascu <[email protected]>
Fixes Gen1 initialization when thing is defined in .things (discovered and added thing via UI worked fine, Gen2+3 also)
See also #17006