QuantityType<Time> incorrectly formats durations
jpg0 opened this issue · 14 comments
Currently QuantityType<Time> is hardcoded to format the held Quantity<Time> as a UTC ZonedDateTime when not provided a format string.
Expected Behavior
Format as a Duration when the scale is Scale.RELATIVE, and a ZonedDateTime when the scale is Scale.ABSOLUTE.
Current Behavior
Always formats to a UTC ZDT. This fails for me when I use QuantityType<Time> to represent a duration. For example, I am using it to represent the period of time that my irrigation needs to run, which is then sent to the controller over MQTT. It was failing to send the duration required, but instead sending a formatted ZonedDateTime measured at the duration (in seconds) past the epoch. Currently my outgoing format logic is required to parse the ZDT then determine how many seconds past the epoch it is, convert this back into a QuantityType<Time>, then reformat with a format string. Note that other quantities appear to have no such formatting logic hardcoded into the QuantityType class.
Possible Solution
A small solution, localised to my specific problem, would be to allow specification of format strings to the MQTT binding. It seems that this is currently not supported and always passes a null format string. However, this would not fix the underlying problem, that Quantity<Time> representing a duration (i.e. with a relative scale) is incorrectly formatted by default. Indeed, looking at QuantityType it doesn't really appear to handle the scale parameter at all, and just hardcodes it to Scale.RELATIVE.
I think more details need to be provided because unless you've set a state description pattern on the Item and you are sending the displayState I don't see how you would ever get a ZDT.
A Quantity<Time> should always represent a duration. By default, unless the unit metadata is set on the Item, that will get rendered as N s for N seconds.
In the state description, it's possible to break this number of seconds up into DD:HH:MM:SS (for example) by using date time string formatting. I suspect you are somehow accessing this latter capability. But the default value should be <number> <unit> just like every other QuantityType.
But as far as I can tell, it shouldn't even be possible to access this ZDT formatted String on the QuantityType<Time> from the MQTT binding. That's what's confusing me.
To test things out I configured a Number Channel on an MQTT Thing leaving pretty much everything as the default.
UID: mqtt:topic:broker:TimeTest
label: TimeTest
thingTypeUID: mqtt:topic
configuration: {}
bridgeUID: mqtt:broker:broker
channels:
- id: NumberChan
channelTypeUID: mqtt:number
label: NumberChan
description: ""
configuration:
commandTopic: timetest
I linked this to a Number:Time Item and sent "1234 s" as a command. The Item shows "1234 s" as expected and it published 1234 as expected to the topic.
So there must be something different in how you've set it up to get the result you are seeing. Maybe you are using a DateTime Channel on the MQTT Thing?
Overall, I don't like the way QuantityType<Time> cheats to allow the formatting of the duration so assuming this is a configuration issue, I would keep this open as an enhancement issue. Using date time formatting to split the seconds into DD:HH:MM:SS has significant limitations: for example it can't support durations over 31 days.
That is strange. I wonder if you have some default formatting pattern somehow that I do not? I know that the MQTT code passes no format pattern (well, null) to the QuantityType when rendering it; it's hardcoded this way. I traced it through the code from the MQTT binding and when it gets to the format the QuantityType, this is the area of code responsible for formatting as a ZDT:
It certainly looks like it will always output a ZDT if no format pattern is passed (unitPlaceholder is always null with a null format pattern).
Maybe a little OT, but does MQTT always just take whatever unit was originally provided to the QuantityType? So seconds in your case, but if you instead send it 2 minutes, it would send the number 2? I noticed that I do have unit: s in my channel configuration (not sure where I saw to add that), in an attempt to have the duration always formatting into seconds when going out to MQTT.
I wonder if you have some default formatting pattern somehow that I do not?
There shouldn't be. I just created the number and didn't set anything, not even the unit.
label: NumberChan
type: Number:Time
category: ""
groupNames: []
tags:
- Point
And there is no metadata.
The MQTT binding doesn't push state description patterns behind the scenes (like some bindings do) I'm pretty sure so there should be no hidden pattern applying here unless there is a default pattern that gets applied. Have you by any chance overridden the state description pattern for this Item, or set the outgoing value format parameter in the MQTT Channel config?
Maybe a little OT, but does MQTT always just take whatever unit was originally provided to the QuantityType?
I think so.
So seconds in your case, but if you instead send it 2 minutes, it would send the number 2?
No, if I send it 2 min, that will be converted to 120 s before the Item is updated and 120 would be published. The value sent to the Item will always be converted to the unit of the Item. If the value doesn't have a unit, the unit of the Item is assumed. So if I just send it 2 then the Item will become 2 s.
I noticed that I do have unit: s in my channel configuration (not sure where I saw to add that), in an attempt to have the duration always formatting into seconds when going out to MQTT.
The MQTT binding will append that unit to the values it sends to the Item linked to the Channel. If this is a command only Channel that property doesn't really do anything.
For Channels that subscribe for values, it's good to set this unit to what ever unit the value coming in is. Then you can set the unit metadata on the Item to what you want it to be and the value will automatically be converted.
All Number:<Dimension> Items now have a default unit when the unit metadata is not set. For Number:Time that default is s for seconds. So anything you send to this Item as an update will be converted to seconds, or assumed to be seconds. But that's all handled independently of the binding.
It's still really unclear to me why/how you are hitting that block of code though. All I can think of is:
- the Item has a state description pattern that doesn't include a unit (maybe the pattern is set to ""?)
- the Channel's outgoing value format is set to something other than
%s - there is a recent regression that is causing the default
%soutgoing value format to trigger that block of code. I'm on the milestones for 4.2, are you perhaps on the snapshot?
Ok, I've reproduced this. My definitions are as follows:
The item
label: FrontGardenWaterTEST Valve irrigation_target
type: Number:Time
category: ""
groupNames:
- gPersist
- gHourlyStats
tags:
- Point_Measurement
The channel
- id: irrigation_target
channelTypeUID: mqtt:number
label: irrigation_target
description: null
configuration:
commandTopic: zigbee2mqtt/0xa4c138b108bd9895TEST/set
unit: s
transformationPatternOut: KOTLININLINE:de514cc8-e124-49e0-b2c5-7a4253032d29
stateTopic: zigbee2mqtt/0xa4c138b108bd9895TEST
transformationPattern: JSONPath:$.irrigation_target
Which produces an MQTT message of:
zigbee2mqtt/0xa4c138b108bd9895TEST/set {"irrigation_target":1970-01-01T00:00:21Z}
I'm aware of some weirdness in the definitions! The item looks mostly ok, the channel has transformationPatternOut which is inline kotlin code (hence the prefix), which simply does the reverse of the JSONPath transformationPattern by making it a JSON object (you can see this in the MQTT message). I can certainly take this out, but I know it's not the culprit. I do have the unit of 's' in there too, which I would expect to improve the situation if anything, rather than break it.
I do have the unit of 's' in there too, which I would expect to improve the situation if anything, rather than break it.
Nah, that's a noop here. It only applies for incoming data and you don't have a stateTopic defined.
The item looks mostly ok, the channel has transformationPatternOut which is inline kotlin code (hence the prefix), which simply does the reverse of the JSONPath transformationPattern by making it a JSON object (you can see this in the MQTT message).
This looks like a UID or encoded somehow. What does the actual code look like? Is there any way to log from the transform to see if the date time formatting is occurring before it gets to the transform or inside the transform (I'm wondering if the way the Item state is pulled is triggering the problem).
The only time that code in the QuantityType should be invoked at all is when rendering the "displayState" as seen in the UIs. The only reason it should be invoked in the MQTT binding is if the Outgoing Value Format filed is set to something, unless the binding just blindly invokes the state formatting whether or not the field is set.
Ultimately, eliminating the DateTime formatting for Number:Time is not going to be allowed as it would be a pretty significant breaking change. But I do think that the way the code determines whether to apply the DateTime formatting can be improved. Instead of simply treating a lack of a unit as the signal to apply the DateTime formatting, actually look for a positive sign that a date time formatting is desired, maybe by looking for one or more $t in the format string. I don't know if that's feasible but it's the first idea that comes to mind.
A less universal solution would be to add special logic to the MQTT (and HTTP?) bindings to handle a Number:Time state specially, or to not pass the Item's state through the outgoing value format when it's the default (i.e. %s). I suspect it just passes the value through the formatting because%s should be a noop. You should get the same thing as when calling toString() on the QuantityType.
Temporary work arounds would be to not use a Number:Time and instead just use a Number.
The kotlin code is pretty simple: fun jsonPropNumber(attr: String) = fn { "{\"${attr}\":${it}}" } (yes it's a UUID as the functions are stored in a map by UUID for lookup at runtime.) Anyway, I tried just taking that out (verified in the yaml) and setting the item to 24 s and the MQTT message I get is:
zigbee2mqtt/0xa4c138b108bd9895TEST/set 1970-01-01T00:00:24Z
(e.g. no JSON, just the ZDT).
unless the binding just blindly invokes the state formatting whether or not the field is set
I took a look at the code and the MQTT binding passes a format string of '%s' to format number types (in method NumberValue.getMQTTPublishValue). There is no QuantityValue class, which may also be possible fix in the MQTT case. This does mean that this can effectively be reduced to a very simple test case:
QuantityType(23, Units.SECOND).format("%s")
-> should produce: 23 s (or just 23)
-> but in fact produces: 1970-01-01T00:00:23Z
I still believe that the correct solution is to handle the scale of the quantity (relative v. absolute), but I don't know how much work that is.
As I mentioned, I do have a solution so it's not a huge problem for me, but I'd be surprised if others' don't hit this. Unfortunately my solution is more inline kotlin code so won't be suitable for others:
fun timeAsUnitDuration(unit: javax.measure.Unit<Time>, attr: String) = fn {
val seconds = ZonedDateTime.parse(it).toEpochSecond()
val converted = Units.SECOND.getConverterTo(unit).convert(seconds).toString()
"{\"${attr}\":${converted}}"
}
OH has a dedicated type (DateTimeType) to represent absolute date and time. I therefore think it makes a lot of sense to use a relative scale for QuantityType and I believed that is the common use.
I agree that the formatting fails when one is provided that is different from something that would represent DD:HH:MM:SS. As the MQTT binding uses %s for formatting, the code you reference indeed formats as an absolute datetime because of the conversion to a ZonedDateTime before formatting.
My suggestion would be to replace this conversion to ZonedDateTime by a conversion to Duration and put in specific formatting for that duration. That would make it always relative. Unfortunately, there is no formatting available out of the box for Duration, but we could try to mimic the DateTime formatting patterns in specific treatment and fall through to standard conversion if not available.
While this is a breaking change, I think it would not be visible in typical use cases. I am OK trying something. WDYT?
Good point re: the existing DateTimeType. Also if you look at the actual QuantityType code, other than the odd bit of handling for PERCENT types, there is nothing specific to any particular unit of measurement, except this code in the format method which converts to ZDT. Certainly smells wrong to me. TBH you could probably just remove the time-specific handling and then you'd get some level of reasonable formatting (just the value, no units, as you'd get with any other quantity).
IMO formatting Number:Time as ZonedDateTime is completely wrong. The Number:Time should represent a duration like weeks or seconds (similar to an amount of X for other dimensions). We have a DateTimeType to store points in time.
@cweitkamp @kaikreuzer @wborn #1470 was merged before my time, can you comment on why this has been introduced? I would vote for removing that.
I would vote for removing that.
Removing it entirely will break some UI's. E.g. I use QuantityType in the upnpcontrol binding for song duration and position. I want to format that as HH:MM:SS, not just seconds or minutes. Without a matching format pattern in a state description, one would have to create a transformation or rule for this. That's why I went out of the way in PR #4169 to try to mimic the DateTime format patterns and apply them to a duration.
While this is a breaking change, I think it would not be visible in typical use cases. I am OK trying something. WDYT?
If you put in some sort of check to only go down the datetime or duration formatting when it's explicitly called for by the pattern would it be a breaking change? I would cause the current behavior a bug. Getting a date time string from %s is definitely unexpected to say the least. I would expect that anyone who would get caught by this would have already added either the unit or date time formatting already because "1970-01-01T00:00:23Z" makes no sense.
But I really like the idea of getting real duration formatting instead of using the date time which is limited to support for durations of 31 days. It's also limited in that you can't do just hours over 24 hours (e.g. if you want to see something like 46h25m instead of 1d22h25m).
TBH you could probably just remove the time-specific handling and then you'd get some level of reasonable formatting (just the value, no units, as you'd get with any other quantity).
@cweitkamp @kaikreuzer @wborn #1470 was merged before my time, can you comment on why this has been introduced? I would vote for removing that.
That definitely would be a breaking change for end users too. Many use the date time formatting for Number:Time Items today beyond add-ons in their state description patterns and Item labels instead of a script transform to split them up.
But I really like the idea of getting real duration formatting instead of using the date time which is limited to support for durations of 31 days. It's also limited in that you can't do just hours over 24 hours (e.g. if you want to see something like 46h25m instead of 1d22h25m).
#4169 does just that, thereby abusing the meaning of the day, hour, minute and second identifiers to go over the limit if the higher level is not in the format string, e.g. if there is no day format identifier in the pattern, hours will happily go over 24.