openhab / openhab-core

Core framework of openHAB
https://www.openhab.org/
Eclipse Public License 2.0
912 stars 421 forks source link

[UoM] IAE on parsing value "0.41 µ(km/h)" #644

Closed cweitkamp closed 5 years ago

cweitkamp commented 5 years ago

I tried to use the new unit for rainfall MILLIMETRE_PER_HOUR (introduced in #588) which results in an IAE:

2019-03-11 09:35:19.506 [DEBUG] [nWeatherMapWeatherAndForecastHandler] - Update channel 'rain' of group 'current' with new state '0.41 µ(km/h)'.
2019-03-11 09:35:19.507 [ERROR] [me.core.internal.events.EventHandler] - Creation of ESH-Event failed, because one of the registered event factories has thrown an exception: Error invoking #valueOf(String) on class 'org.eclipse.smarthome.core.library.types.QuantityType' with value '0.41 µ(km/h)'.
java.lang.IllegalStateException: Error invoking #valueOf(String) on class 'org.eclipse.smarthome.core.library.types.QuantityType' with value '0.41 µ(km/h)'.
    at org.eclipse.smarthome.core.items.events.ItemEventFactory.parseSimpleClassName(ItemEventFactory.java:190) ~[?:?]
    at org.eclipse.smarthome.core.items.events.ItemEventFactory.parseType(ItemEventFactory.java:158) ~[?:?]
    at org.eclipse.smarthome.core.items.events.ItemEventFactory.getState(ItemEventFactory.java:136) ~[?:?]
    at org.eclipse.smarthome.core.items.events.ItemEventFactory.createStateEvent(ItemEventFactory.java:116) ~[?:?]
    at org.eclipse.smarthome.core.items.events.ItemEventFactory.createEventByType(ItemEventFactory.java:80) ~[?:?]
    at org.eclipse.smarthome.core.events.AbstractEventFactory.createEvent(AbstractEventFactory.java:51) ~[?:?]
    at org.eclipse.smarthome.core.internal.events.EventHandler.createESHEvent(EventHandler.java:121) ~[?:?]
    at org.eclipse.smarthome.core.internal.events.EventHandler.handleEvent(EventHandler.java:95) ~[?:?]
    at org.eclipse.smarthome.core.internal.events.EventHandler.handleEvent(EventHandler.java:72) ~[?:?]
    at org.eclipse.smarthome.core.internal.events.ThreadedEventHandler.lambda$0(ThreadedEventHandler.java:67) ~[?:?]
    at java.lang.Thread.run(Thread.java:748) [?:?]
Caused by: java.lang.reflect.InvocationTargetException
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:?]
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[?:?]
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:?]
    at java.lang.reflect.Method.invoke(Method.java:498) ~[?:?]
    at org.eclipse.smarthome.core.items.events.ItemEventFactory.parseSimpleClassName(ItemEventFactory.java:181) ~[?:?]
    ... 10 more
Caused by: java.lang.IllegalArgumentException: µ not recognized (in 0.41 µ(km/h) at index 5)
    at tec.uom.se.quantity.Quantities.getQuantity(Quantities.java:80) ~[?:?]
    at org.eclipse.smarthome.core.library.types.QuantityType.<init>(QuantityType.java:96) ~[?:?]
    at org.eclipse.smarthome.core.library.types.QuantityType.valueOf(QuantityType.java:139) ~[?:?]
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:?]
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[?:?]
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:?]
    at java.lang.reflect.Method.invoke(Method.java:498) ~[?:?]
    at org.eclipse.smarthome.core.items.events.ItemEventFactory.parseSimpleClassName(ItemEventFactory.java:181) ~[?:?]
    ... 10 more

What I did in the binding is similar to this - which is working in a unit tests but not when running the binding.

    @Test
    public void testSpeedSucceeds() {
        QuantityType<?> value = new QuantityType<>(0.41, SmartHomeUnits.MILLIMETRE_PER_HOUR);
    }

This test fails - I entered the µ by clicking on AltGr + M.

    @Test
    public void testSpeedFails() {
        QuantityType<?> value = QuantityType.valueOf("0.41 µ(km/h)");
    }
mhilbush commented 5 years ago

Ugh. I was just getting ready to use this in my Ambient Weather binding. 😦

So in your binding code you had something like this?

new QuantityType<>(0.41, SmartHomeUnits.MILLIMETRE_PER_HOUR)

And at execution time, it threw the exception about the micro symbol?

cweitkamp commented 5 years ago

Yes, exactly. The exception is thrown at runtime.

Funny thing is that it is working with MICROGRAM_PER_CUBICMETRE without a problem. The unit contains a µ too ("µg/m³").

hakan42 commented 5 years ago

You might be running into the "different encodings for the µ sign" issue I had with my sensebox binding:

https://github.com/eclipse/smarthome/pull/6818

https://github.com/openhab/openhab2-addons/pull/4507#discussion_r245507046

I fixed that issue in my binding by explicitly sanitizing input data so I can accept both encodings:

https://github.com/openhab/openhab2-addons/pull/4507/files#diff-bb93b9e8706382c920525c3f8e66ff0fR100

cweitkamp commented 5 years ago

@hakan42 Thanks for the hint. But I am afraid it will not help in my situation. As @mhilbush already noted I do neither use the µ sign in my code nor the API of the weather provider returns it. My guess is that we loose it somewhere on the track towards the item. What we can do - to be save - but I think its a "not so nice" workaround - is to move your suggested sanitizing into the constructor of the QuantityType class.

https://github.com/openhab/openhab2-addons/blob/4e397a7f324127713e41de09fea20e2284e64fef/addons/binding/org.openhab.binding.sensebox/src/main/java/org/openhab/binding/sensebox/internal/SenseBoxAPIConnection.java#L101-L105

maggu2810 commented 5 years ago

Well, I assume a common place for the replacement of certain unit characters could make sense.

Isn't there are a workaround for °C vs. (https://www.fileformat.info/info/unicode/char/2103/index.htm ) somewhere in our code base?

cweitkamp commented 5 years ago

I guess you are talking about https://github.com/eclipse/smarthome/pull/5721?

mhilbush commented 5 years ago

@cweitkamp I was looking into this issue to try to figure out what was wrong so that I could implement hourly rainfall rate in my Ambient Weather binding.

Oddly, I've not been able to reproduce the IAE.

My code is doing this, which I think is the same thing you were doing?

double rainfallRate = 22.5;
QuantityType quantity = new QuantityType<>(rainfallRate, SmartHomeUnits.MILLIMETRE_PER_HOUR);
updateState("hourlyRainfallRate", quantity);

The item is defined like this.

Number:Speed                RainfallRate
                            "Rainfall Rate [%.2f %unit%]"
                            { channel="ambientweather:ws1400ip:ws1400ip:weatherDataWs1400ip#rainHourlyRate" }

And in my sitemap, I put this to test it.

                Text item=RainfallRate   label="Rainfall Rate [%.2f mm/h]"
                Text item=RainfallRate   label="Rainfall Rate [%.2f in/h]"

I can't figure out what might've changed to make this work.

cweitkamp commented 5 years ago

@mhilbush Thanks for digging into this. Yes, you did the same like I did. I tested it again and I can see the same error like reported in the opening post. I used Paper UI for my tests and "Simple Mode" for "Item Linking". Did you check what happens if you add the Text element to your sitemap without the different label or with "%unit%"?

mhilbush commented 5 years ago

Did you check what happens if you add the Text element to your sitemap without the different label or with "%unit%"?

No, but I can try this.

When exactly does the IAE occur. Is it on the call to updateState with the newly created QuantityType object? I can't quite tell from the stack trace.

One thing that different between our systems... My system config uses Imperial units, yours likely is SI. I'll also change my test system to use SI and see what happens.

cweitkamp commented 5 years ago

Yes, both assumptions are correct. The IAE occurred during updateState . The item remains NULL and does not show any value. Another difference are the regional settings, but I cannot imagine that a "µ" is handled different in US and DE.

Another test which you could give a try is to call QuantityType.valueOf("0.41 µ(km/h)") directly in your code. This always failed for me. I tried to add it to the itests of QuantityType too.

mhilbush commented 5 years ago

Sure enough, I was able to get an IAE using QuantityType.valueOf.

To make sure I was using the right symbol, I tried it both ways.

// Greek small letter MU (should fail)
QuantityType.valueOf("0.41 \u03bc(km/h)");
// Micro symbol (should work)
QuantityType.valueOf("0.41 \u00b5(km/h)");
mhilbush commented 5 years ago

And, using the two flavors of the constructor.

This works.

new QuantityType(0.41, SmartHomeUnits.MILLIMETRE_PER_HOUR)

But this throws an IAE.

new QuantityType("0.41 " + SmartHomeUnits.MILLIMETRE_PER_HOUR)
cweitkamp commented 5 years ago

Alright, I am not sure if we can find a solution this way. Let's have a look at the issue from a different point of view: the modeling side.

Currently MILLIMETRE_PER_HOUR is modeled based an KILOMETRE_PER_HOUR like this:

https://github.com/openhab/openhab-core/blob/4b7eddf16e3c4da657030518b66e92e5e45736f0/bundles/org.openhab.core/src/main/java/org/eclipse/smarthome/core/library/unit/SmartHomeUnits.java#L146-L147

It should be similar to MetricPrefix.MICRO(Units.KILOMETRE_PER_HOUR). Is it? The underlying library uses this formula as internal representation and calculation. What if we change the base to Units.METRE_PER_SECOND? It could avoid using "µ" intenally. The result should be the same. Do not know, just a guess...

... = new TransformedUnit<>(MetricPrefix.MILLI(Units.METRE_PER_SECOND), new RationalConvertor(1l, 3600l));

Did I miss something?

mhilbush commented 5 years ago

Did I miss something?

Nope. I was starting to think the same thing.

Since this doesn't work.

new QuantityType("0.41 µ(km/h)")

But this does.

new QuantityType("0.41 mm/h")

Just avoid SmartHomeUnits.MILLIMETRE_PER_HOUR being represented as "µ(km/h)". :wink:

BTW, I did see this change in the most recent version of the uom-se library. But I'm not necessarily sure if it fixes the problem. So, your suggestion above seems like the right move.

cweitkamp commented 5 years ago

Does a version bump hurt? I prepared everythig https://github.com/openhab/openhab-core/compare/master...cweitkamp:bugfix-644-uom-1.0.10.

Otherwise we will try the modeling change.

mhilbush commented 5 years ago

Does a version bump hurt?

Absolutely not! Definitely worth a try as those string comparisons in SimpleUnitFormat were not right.

mhilbush commented 5 years ago

Also, it looked like they put in an alias for the MU symbol, so it might also handle cases where MU is used mistakenly in place of the micro symbol.

cweitkamp commented 5 years ago

:+1:

Looks great. Updating the tec.uom library did the trick. The IAE is gone. Unfortunately there will be no rain in my area for the next few days thus I only see 0 ... :wink:

@mhilbush thanks for your support.

grafik

grafik

mhilbush commented 5 years ago

Great news. Thanks for pulling together the fix!

mhilbush commented 5 years ago

@cweitkamp My only beef with the UoM implementation (having nothing to do with your recent change), is that the item's default unit (i.e. %unit%) on a sitemap ends up being km/h (if SI) or mph (if Imperial), even though you specify SmartHomeUnits.MILLI_METRE_PER_HOUR or SmartHomeUnits.INCHES_PER_HOUR when creating the QuantityType. It makes the use of the default unit impractical.

To get it to display properly on the sitemap, you need to specifically define the label in your item or sitemap to use mm/h or in/h if you want the number to be formatted to be easily readable.

So, this

Frame {
    Text item=WS1400IP_RainHourlyRate                   label="Rate using default %unit% [%.8f %unit%]"
    Text item=WS1400IP_RainHourlyRate                   label="Rate using mm/h [%.1f mm/h]"
    Text item=WS1400IP_RainHourlyRate                   label="Rate using in/h [%.2f in/h]"
}

ends up looking like this.

Ambient Weather 2019-07-15 07-55-45

Am I being too nit-picky about this?

cweitkamp commented 5 years ago

Yes, I saw that too. Not very satisfying but I do not know how to change it in a different way except the label.