eclipse-archived / smarthome

Eclipse SmartHome™ project
https://www.eclipse.org/smarthome/
Eclipse Public License 2.0
862 stars 783 forks source link

[Astro] Failure to render values with units in Basic & Classic UI #6055

Closed kaikreuzer closed 6 years ago

kaikreuzer commented 6 years ago

I just tried the latest ESH code on the openHAB demo setup with items

Number Sun_Elevation            "Sun Elevation"                 <sun>       { channel = "astro:sun:home:position#elevation" }
Number Sun_Azimuth              "Sun Azimuth"                   <sun>       { channel = "astro:sun:home:position#azimuth" }
Number Moon_Elevation           "Moon Elevation"                <moon>      { channel = "astro:moon:home:position#elevation" }
Number Moon_Azimuth             "Moon Azimuth"                  <moon>      { channel = "astro:moon:home:position#azimuth" }

When accessing those items through Basic UI or Classic UI, they fail to render the values and the log shows these exceptions:

2018-08-16 10:01:57.392 [DEBUG] [.s.u.b.i.servlet.WebAppServlet:176  ] - reading sitemap demo
2018-08-16 10:01:57.393 [WARN ] [s.u.i.items.ItemUIRegistryImpl:402  ] - Exception while formatting value '36.03152296519132' of item Sun_Elevation with format '%.2f %unit%': Conversion = 'u'
2018-08-16 10:01:57.393 [WARN ] [s.u.i.items.ItemUIRegistryImpl:402  ] - Exception while formatting value '117.59132770165894' of item Sun_Azimuth with format '%.2f %unit%': Conversion = 'u'
2018-08-16 10:01:57.395 [WARN ] [s.u.i.items.ItemUIRegistryImpl:402  ] - Exception while formatting value '-18.790852871495495' of item Moon_Elevation with format '%.2f %unit%': Conversion = 'u'
2018-08-16 10:01:57.395 [WARN ] [s.u.i.items.ItemUIRegistryImpl:402  ] - Exception while formatting value '77.0354987191384' of item Moon_Azimuth with format '%.2f %unit%': Conversion = 'u'

The ESH code from Aug 1 still worked as expected.

kaikreuzer commented 6 years ago

Ok, I identified the issue to be in the Astro binding - while it declares the channels to be UoM-enabled, it still sends states that are pure DecimalTypes and not QuantityTypes.

@cweitkamp This bug has been introduced with https://github.com/eclipse/smarthome/pull/6013 - checking the code, it seems weird that e.g. Position.getAzimuth() (which is supposed to provide the QuantityType state) is nowhere called from the code. Could you have a look, please?

kaikreuzer commented 6 years ago

@lolodomo & @htreu Despite the bug in the Astro binding, I think the ItemUIRegistry does not behave nicely here either - the warning is not really understandable for the user. The item is defined without a dimension, so the framework indeed does not care about the unit. In such a case, I would expect that the "%unit%" part of the formatting is simply ignored.

htreu commented 6 years ago

The thing is: the state description NumberItem provides makes no sense with %unit% in case no dimension and no QuantityType is given. Maybe NumberItem should modify the state description in this case, so following services (like ItemUIRegistry, REST interface, ...) do not have to deal with this issue.

cweitkamp commented 6 years ago

I cannot confirm a bug in the Astro binding. The root cause must be something else. I have seen this warning reported in the community for other bindings too (see https://community.openhab.org/t/solved-netatmo-binding-openhab-shows-error/48912).

it seems weird that e.g. Position.getAzimuth() (which is supposed to provide the QuantityType state) is nowhere called from the code

The Astro binding tries to guess the method to invoke for retrieving the expected values based on the ChannelUID:

https://github.com/eclipse/smarthome/blob/f63951c52761bbf2a1fbe65e7130e7832c16066e/extensions/binding/org.eclipse.smarthome.binding.astro/src/main/java/org/eclipse/smarthome/binding/astro/internal/util/PropertyUtils.java#L95-L103

kaikreuzer commented 6 years ago

Ah, it is doing reflection, that's why I didn't find any reference :-)

So I agree, the Astro binding seems to work ok and there is a simple solution to the problem: https://github.com/openhab/openhab-distro/pull/747/files

Nonetheless this shows us that the UoM feature is not as backward compatible as we had planned for - it should not have had any impact on existing installations (with plain Number items), if a binding is changed to support UoM.

So if we implement @htreu's suggestion for the ItemUIRegistry, it should be "almost" compatible again (only thing is that we lose the unit, which was previously defined by the state description of the channel, which it now does no longer do).

cweitkamp commented 6 years ago

I second @htreu . The warning seem to disappear if you either change the item type to Number:... or add a different pattern to the item label.

kaikreuzer commented 6 years ago

Maybe NumberItem should modify the state description

Hm, I didn't read it in detail - how do you imagine this to work technically? Postprocessing in getStateDescription? I'd rather think it should be tackled in ItemUIRegistry.

htreu commented 6 years ago

I didn't thought this through all the way. But there are other places the state description is used. Its exposed over the REST interface and Paper UI tries to format a unit for %unit%, too.

ghys commented 6 years ago

May I simply ask you to bug me if https://demo.openhab.org:8443/habpanel/index.html#/view/weather-astro doesn't work well in relation to this? ;) In all theory it should work for items with an unit, that is, it will replace %unit% when using the state's description pattern by something like state.split(' ')[1] (ref: https://github.com/openhab/org.openhab.ui.habpanel/pull/310)...

kaikreuzer commented 6 years ago

In all theory it should work for items with an unit

Just checked and it does. With https://github.com/openhab/openhab-distro/pull/747 in place, the HABPanel page correctly displays the values.

htreu commented 6 years ago

Thats why I suggested a central fix, so other consumers wouldn't even notice the special formatting when no unit is provided with the value.