-
-
Notifications
You must be signed in to change notification settings - Fork 429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add defaultUnit metadata for NumberItem #3248
Conversation
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/uom-default-units-and-consequences/142360/53 |
Thanks for your quick implementation. I made some other suggestions in the issue which I currently do not see addressed in this PR. |
I decided to use metadata instead of your approach because it requires no changes to other components. Metadata is also already available in the REST API. |
I get that it's easier to implement with metadata but this brings lots of other metadata related issues (see some of the issues I've created concerning metadata). This is more a strategic decision: |
In general this configuration should not be needed at all. The state description is used for display purposes and values send to an UoM item should have a unit. This is merely for bindings that should be updated to provide good units. Bindings with a generic approach (like MQTT or HTTP) allow configuring units in the channel configuration. |
There are also devices which change the Units based on its configuration. For the binding maintainer it's impossible to provide a good unit because the device internal configuration might be different per user and the device internal default might be different per country. Also what you are describing is a unit transformation "on the channel".
I disagree - this is actually a very important feature and without it the UoM doesn't make (fully) sense at all.
While from a pure openHAB internal perspective there isn't much difference but it changes when you interact with the outside world, e.g.:
There it makes sense to supply the value in a predefined defined unit and this unit very often does not align with the system default. And the supplied value is the item state. Example: Instead of adding the countless transformations it would be much more elegant if I could tell openHAB in which unit the value should be represented. The system default might often be a good fit but just as often it is not. Since the users know best they should not be patronized and be free to chose the appropriate representation (as in item state representation). |
Internally a We have four different cases:
All of this has nothing to do with the representation in the UI or external APIs:
Persistence should (and actually is) storing and restoring the value in the unit provided by the item's I fail to see how we patronize users here: they can do as they like, stick with what we offer or set their own. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/uom-default-units-and-consequences/142360/72 |
I'm satisfied with this. It provides my core goal - allowing a unit to be defined for all conversion use cases we've discussed, separately from the state description which is the default way UIs will present an item. As for retaining state description as a source for the item's unit - you're right that it would be cleanest to not. It would be a bit of a breaking change to remove it. I guess I don't have strong feelings either way. I know users tend to dislike breaking changes where they have to take action, but as a developer it's often worth the energy to make the breaking change instead of having to maintain cruft in perpetuity. It just depends on how much cruft it is; this seems like not much. There also the link to #3132, where one could argue that because state descriptions can come from bindings, they're not just for presentation purposes. @spacemanspiff2007: can you point to some of the issues you've had with metadata in the past? I know I've had issues with things stored in metadata (like state description!) persisting after the item has been removed, but those all seem to be sorted out now, and well understood. It's pretty widely used now by several features, and most areas (MainUI, .items files, JSR223 scripting languages) are able to present metadata as if it were just part of the item. It's simply an implementation detail that it's stored completely separately. |
We still talk about different things and your response doesn't fit to my question. What I suggest:
This would allow me as a user to normalize values. Currently the value would change from "999 m" to "1 km" and external systems often just ignore the unit (see the |
Oh yikes, I didn't even know it did that. 95% of the time I work with unit conversions between systems. This definitely does seem like an issue for persistence. But it's a separate issue, not to be addressed by this PR. |
Yes, but there is no convincing point in why a user would want to be doing that. Offering that capability to users would be a very bad idea.
Why would it ? A counter in some binding is associated with a base unit, and that will not change. 999+1 is 1000 not 1 so you will get 1000 m. |
It might avoid confusion to use "unitless" instead of dimensionless since we have an actual unit called "Dimensionless".
Maybe now I'm confused. Is this not the
|
I think a lot of misunderstanding is about the question what a "unit" is. |
Yes - there is. And that is interaction with other systems.
I strongly disagree. And a nice side effect to a normalization field would be that for everyone it's very clear how e.g. this value would have to be persisted, passed around through the rest api, etc.. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/uom-default-units-and-consequences/142360/115 |
Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Jan N. Klug <[email protected]>
47350d3
to
2b1eb7b
Compare
Signed-off-by: Jan N. Klug <[email protected]>
There is very surprising code in |
Oooh, this might explain how the change to color-temperature-abs's channel type to a Number:Temperature caused bindings to receive QuantityType, even though the Item was still just a Number. |
@ccutrer I must admit that I was not aware of that, too. And I think it's also very surprising and unnecessary. If a binding knows how to handle a value without unit, it can implement code for |
Agreed. Or the user can add a profile that can do conversions. CommunicationManager shouldn't be adding extra logic here (besides instantiating and applying profiles). |
I've given this issue some more thoughts and I still think we should not go with metadata but rather with a field directly on the item. 1. With metadata we use two places where store the same information
With metadata Nicer would be:
or if the user doesn't care about unit and normalization:
Then there is one way/value only which defines what happens. 2. With a field on the item there is no lifecycle issueDefine You have to keep both values in sync, which can lead to confusion and hard to track user and code issues . Additionally metadata is not loaded together with items, so it might be that the item is already loaded and the metadata is still missing for example on startup (see my other issue to provide the item metadata together with the 3. Units and normalization can be directly validatedThis can directly be validated by the *.items file parser
While through the GUI it's possible to present the user a dropdown menu for normalization metadata, it's still possible that with the RestAPI/Rules/etc an unsupported or invalid unit can be set. Or defining a String item with @J-N-K : There has been this pr and other issues. Would you want me to do a write up in a fresh issue how I have envisioned this feature so it's complete in one place. Or do you still think this should be more of a quick fix? |
I din't think that's the way to go and also very confusing. Items have a type. That type is String item's don't use the That aside: We don't support (and have never supported) changing item types, for managed things they need to be removed and re-added, and the same happens internally when the type is changed in the file. Additionally, if you delete a managed item, associated metadata is also removed, so the described scenario cannot happen. If someone wants to make everything more complicated than necessary and use the file-based item method, he should take care of removing metadata himself. Regarding lifecycle: |
I think that's a misunderstanding:
It should rather be rejected with http status 422 at least for the RestAPI It still think the "Number:Length" syntax in the item type was a mistake and should have never happened because it's not a different type but as you say yourself an extension of the Number Item. But if I follow the argumentation that
I have a testcase in HABApp which does exactly that:
Was this introduced in 3.4? The last time I checked (I think it was 3.3 but I am not sure) metadata was kept. |
I'd think another field for scaling ain't necessary and like J-N-K I think it would be very confusing. |
You are not disagreeing but affirming my argument. If the user has to define unit and scale prefix in different locations it's very confusing and inconsistent. |
If we go that way, then we should remove different item-types completely and only have a metadata |
Err, no. I'm against introducing another field.
Today a user cannot explicitly define the scale via field, the only means to explicitly set scale today is by setting
I agree with that statement in itself, but totally disagree with that plan. It would mean to needlessly destroy & rebuild a 5yr old established mechanism. You will create pain with and upset a lot of users. |
That would only be consistent and fine with me. 👍
You wrote that the internal state representation and scale doesn't matter for the end user. Based on your comment it seems you've not yet understood what the goal and the issue actually is. Please read the whole issue and the linked issues. |
That just applies as long as there is no means for the user to affect it |
@rkoshak I would like your opinion here. Would you agree that it would be beneficial to have only „Number“ items and make them handle units if a |
Just pitching in an additional point here: "scale" isn't always a simple multiplication or division. My example is mired/mirek, often used in lightbulb color temperatures (more often in calculations and bindings, than user facing). It's an "inverse mega-kelvin", or 1/MK, or MK^-1. I don't have any strong recommendations here, besides that it's natural for a user to want to seamlessly convert between mired and kelvin the same way they would want to seamlessly convert between Celsius and Fahrenheit. As of now, this is implemented as QuantityType.toInvertedUnit(), since the dimension as actually changing (from Temperature to Temperature^-1 or vice versa) and toUnit() cannot change dimension. I'm not sure if this sort of conversion should be handled more generally, but wanted to keep it in people's minds as they're discussing things. |
@ccutrer That would not be affected by any of these proposals. IMO mired/K is a special case (I know of no other dimension with such units) and already handled well. |
I kind of like this idea. It certainly simplifies the whole units concepts and configuration. There will be some significant impacts to the UI and we'd have to determine the impacts on the bindings. But I like this. It's elegant. But are we then saying here that if the binding sends a value and we've not defined And/or, should the bindings be allowed to set the But then the This feels consistent and gives the end user more control over the whole situation. However, I suspect a bunch of users will say "forget that, I ain't using UoM, it's complicated" if the default becomes no units. |
Yes - the unit should be stripped. Or to rephrase:
No. The
Just like today where they just can use "Number". |
Bindings can provide state descriptions, they should not interfere with item properties. |
Closed, will be superseded by a PR depending on the outcome of #3282. |
Closes #3247
Before merging we should discuss if we remove the "derive unit from state description" code. Looking at discussion here, it seems that the majority would prefer to keep item-unit and representation (state description) completely separate. I'm fine with that, but it might be breaking for existing installations.
Signed-off-by: Jan N. Klug [email protected]