[shelly] Various channel types changed to system-defined ones, WD fix, BLU support optimized

[markus7017]

This PR adds

• Various channels changed to system-defined channel types
• Auto-upgrade of existing channels (not already linked items)
• Fix for Wall Display sensor updates
• Some more infos on WebSocket errors
• BLU support optimized

Closing

• #15325
• #15414
• #15960

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/shelly-binding/56862/3561

[markus7017]

@wborn

I removed the dependency to org.eclipse.jetty.websocket.websocket-server in the last PR.

However, on a 4.1 IDE system I see the following exception in the log

2023-11-30 23:07:53.766 [DEBUG] [elly.internal.api.ShellyEventServlet] - bundle org.openhab.binding.shelly:4.1.0.202311302018 (173)[org.openhab.binding.shelly.internal.api.ShellyEventServlet(303)] : Changed state from satisfied to active
2023-11-30 23:07:53.769 [WARN ] [/                                   ] - unavailable
java.lang.RuntimeException: Unable to load org.eclipse.jetty.websocket.server.WebSocketServerFactory
	at org.eclipse.jetty.websocket.servlet.WebSocketServletFactory$Loader.load(WebSocketServletFactory.java:54) ~[?:?]
	at org.eclipse.jetty.websocket.servlet.WebSocketServlet.init(WebSocketServlet.java:140) ~[?:?]
	at javax.servlet.GenericServlet.init(GenericServlet.java:158) ~[org.apache.felix.http.servlet-api-1.2.0.jar:?]
	at org.ops4j.pax.web.service.spi.servlet.OsgiInitializedServlet.init(OsgiInitializedServlet.java:68) ~[pax-web-spi-8.0.22.jar:?]
	at org.eclipse.jetty.servlet.ServletHolder$Wrapper.init(ServletHolder.java:1345) ~[jetty-servlet-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.servlet.ServletHolder.initServlet(ServletHolder.java:632) ~[jetty-servlet-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.servlet.ServletHolder.getServlet(ServletHolder.java:486) ~[jetty-servlet-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.servlet.ServletHolder.prepare(ServletHolder.java:759) ~[jetty-servlet-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.ops4j.pax.web.service.jetty.internal.PaxWebServletHolder.prepare(PaxWebServletHolder.java:295) ~[pax-web-jetty-8.0.22.jar:?]
	at org.ops4j.pax.web.service.jetty.internal.PaxWebServletHandler.doHandle(PaxWebServletHandler.java:316) ~[pax-web-jetty-8.0.22.jar:?]
	

which disappears when I re-add the exception. If I understood you corrected this artifact should be already part of the distribution, but why does the load fails? Do I need to add something to feature.xml instead?

The WebSocket servlet is used to receive device events and is therefore essential for the binding.

[wborn]

If I understood you corrected this artifact should be already part of the distribution, but why does the load fails? Do I need to add something to feature.xml instead?

Karaf features are not used when debugging in Eclipse. So you need to manually add any missing bundles to app.bndrun. I added the WebSocket API bundle in https://github.com/openhab/openhab-distro/pull/1611 which also adds your missing bundle.

[markus7017]

If I understood you corrected this artifact should be already part of the distribution, but why does the load fails? Do I need to add something to feature.xml instead?

Karaf features are not used when debugging in Eclipse. So you need to manually add any missing bundles to app.bndrun. I added the WebSocket API bundle in openhab/openhab-distro#1611 which also adds your missing bundle.

ok, thanks for double checking

[markus7017]

@lolodomo Do you know the timeline for the 4.1 release? I would be good get to get this PR in to resolve issues with UOM and improve backward compatibility

[kaikreuzer]

@markus7017 See here.

[markus7017]

I think we could get this done

[markus7017]

@lolodomo changes applied (+2 fixes for MiniPM and WallDisplay)

[markus7017]

@lolodomo Iapplied some changes and comments for the open topics

Question: How could I add a breaking change information. There are some channels, which get upgraded, but the user needs to remove and re-add the item linkage to fix UOM issues?

[markus7017]

@lolodomo @kaikreuzer @J-N-K @jlaur

Obviously this PR doesn't make it into the 4.1 release, which is sad from my point of view, but I also understand that you priority is now on finalizing "the next big thing" and I fully respect this.

Nevertheless, could you please provide feedback on the open topics. I already have 2 more PRs I would like to submit and step-by-step merge this. I'm not the Git expert, but again I'm running into the situation that I messed up the 2nd PR, because it's hard for me to work on 3 PRs at the same time and finally it creates merge effort etc. We be great if you find the time to provide comments for the above topics before the weekend so I could work on that.

Re channel upgrades: I really validated the upgrade feature, but my conclusion was that this is not feasable for this binding, because I would need to duplicate channel related information for each thing type (around 20 depending per device type), which would create a ton of duplicate definitions for nothing than the potential for inconsistency. Maybe the code is crap, then I appreciate some help to get a cleaner implementation. Or even better the upgrade information can be define for multiple thing types using wildcard. In my case a particular channel definition is always the same and used by multiple devices. This means "define it once and use * for thing type" would be the best, fits into the concept and avoid redundant definitions.

End part is using pre-defined system types. I use than for multiple channels even the pre-defined label/description might be misleading. However, the binding creates channels using those types and specifying label and description so the channel gets created with proper type and meaningful descriptions on the UI level. Again, why should I add duplicates of the basic descriptions only because the label is different. Adding those binding specific definitions as system types to the core also doesn't make sense even that I'm missing some types, which are from my point of view commonly used in the real world.

If this PR doesn't make it into the 4.1 release we carry over the UOM issues, which causes channel updates to fail and user rules etc. not working, because they rely on those updates.

[lolodomo]

One first thing you say that I don't understand is that the changes about UoM in OH4 broke this binding. There was no UoM change in OH4 requiring an update in bindings. So that is the first point to clarify. Maybe you can provide an example ?

Second point is that if the user uses a Number item to link a channel while a Number with dimension is expected by your binding, or vice versa, this is just a bad usage by the user, not something you should try to fix in a binding.

Last point is that if you change a channel switching from Number to Number with dimension, this is a breaking change for users and it has to be announced.

I will try to read again your different messages but I should admit that you do things that looks to me outside the openHAB standards and/or beyond my skills, that is the reason why @kaikreuzer / @J-N-K I asked help for this review.

[markus7017]

Even this PR doesn't made it to the 4.1 release, we continue. I think somehow we got stuck in the discussion, so I would suggest to separate the topics

facts

• OH 4.0 introduced stricter channel updates when using UOM. The binding had some bugs in correct channel definitions and a bunch of custom channel types could/should be replaced by those defined by the framework. The binding included in OH 4.10 will continue show update errors, e.g. writing an Amp value into a Watt channel. #15414 gives an overview on channel type changes.
• An upgrade of definitions should be done mostly automatically to make it easy for the user. Using the core feature to do those channel upgrades is not feasible unless thing-type=* can be used in those definitions.The current implementation would require highly redundant definitions. The PR here integrates a custom upgrade routine. -The change could be considered a breaking change. Users might have to adjust existing rules or fix the type of linked items.

Question 1: @lolodomo: Could with resolve the issues beside the upgrade topic first to close the comments above? Question 2: @lolodomo Are you fine with @J-N-K's comment on using system defined channel types and adding custom label/description Question 3: @J-N-K What's your view on the custom upgrade routine in a binding? Question 4: If this is ok: The above code might not be perfect, but it works. How can the implementation code be improved.

Question 5: How could I add information on breaking changes?

[lolodomo]

Question 1: @lolodomo: Could with resolve the issues beside the upgrade topic first to close the comments above?

@markus7017 : I marked as resolved all my comments that are now fine. Only remains:

• one question about your hardcoded IP 169.254.x.x
• all my comments about your switch to system channel types (using same channel type for different channels immediately triggers a warning for me) and the way you implement this change of channel types which is probably over my skills, reason why I asked someone else to review this part.

[markus7017]

Question 1: @lolodomo: Could with resolve the issues beside the upgrade topic first to close the comments above?

@markus7017 : I marked as resolved all my comments that are now fine. Only remains:

• one question about your hardcoded IP 169.254.x.x

see above, this is just a check when something with the network setup is wrong (OH shouldn't even bind to an interface with 169.254.x.x/16).

all my comments about your switch to system channel types (using same channel type for different channels immediately triggers a warning for me)

this has been answered by @J-N-K and he is fine with it. Otherwise, I need to add and maintain dummy channel types.

the way you implement this change of channel types which is probably over my skills, reason why I asked someone else to review this part.

@J-N-K @kaikreuzer Any proposal here?

[markus7017]

@lolodomo Re: UOM changes See what @kaikreuzer posted for the 4.0 announcement: https://www.openhab.org/blog/2023-07-23-openhab-4-0-release.html#units-of-measurement-uom This resulted in the following issues with the binding

• Channel types are checked stronger, there were some mismatch is channel type definitions and channel updates in the code (e.g. update with A for a channel defined with Watt)
• Percentage values show up as "One" instead of 100% therefore I clean up the code and changed channel types to system: types were applicable

@lolodomo @J-N-K @kaikreuzer So, how do we move forward? I see various reported bugs in the back log, I don't want to start working on, because I already have 3 pending PRs to manage.

@lolodomo As you mentioned this change should be marked as a breaking change Maybe someone could have a look to the upgrade code, I'm sure that can be done smarter (I'm not a Java expert)

[markus7017]

any uodate?

[lolodomo]

In case it would not be enough clear, I prefer another maintainer review this PR. Please don't wait for me.

[markus7017]

@openhab/add-ons-maintainers Could anybody support here?

[jimtng]

Regarding the CHANNEL_METER_LASTMIN1 i.e. lastPower1 channel - it is confusing that in the README.md it says

| | lastPower1 | Number | yes | Energy consumption for a round minute, 1 minute ago |

It should say Power instead of Energy consumption.

Also what does "for a round minute" mean? It is vague and needs clarification.

I would suggest it to say "The average power for the previous minute".

The README.md should also include the dimensions, so instead of Number it should say Number:Power for clarity and this applies to all the QuantityTypes.

[markus7017]

@lolodomo @lsiepel @J-N-K @jlaur @kaikreuzer

How do we proceed here?

As you might saw I moved all changes not related to new channel types and upgrade routine to other PRs to reduce the scope here to a minimum, thanks @lsiepel for supporting this.

open questions

• Do you accept changing channel types, some of them are fixes, some improve using system defined ones vs. custom - @J-N-K was fine here, @lolodomo had concerns
• I would also like to include the upgrade routine. As discussed, the integrated upgrade mechanism is no option, because it would required a ton of redundant definitions for all channels and all device types. @J-N-K any advise from your side?
• If yes, could the above code be optimized before merging?

Thanks for patience dealing with this PR.

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/shelly-binding/56862/3720

[lsiepel]

@markus7017 we have only a few weeks before 4.2 release, i guess we are waiting for @J-N-K his advice.

[lsiepel]

Short of time, but would be really nice to see this move forward before 4.2. Are the questions from @markus7017 answered?

[lsiepel]

@markus7017 seen you fixing several Shelly issues, how to proceed with this PR?

[J-N-K]

@openhab/add-ons-maintainers @markus7017 What is the current state here?

[lsiepel]

Have not heard from @markus7017 for a while, hope he is ok. Obviously the merge conflict needs to be fixed, but maybe more important this discussion seems to be waiting for input from you @J-N-K :-) https://github.com/openhab/openhab-addons/pull/15980/files#diff-24c9a26ffad9af868b07f97aa587b13b6370b32e95985e210f24c41a3eefbff2

This PR holds fixes for several issues, one blocking other confirmed issues to be merged. For future PR's it would be better to have them seperate. I cannot asses if it is possible to extract them or not.