-
-
Notifications
You must be signed in to change notification settings - Fork 428
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
[uom] Add unit
metadata for NumberItem
#3481
Conversation
ed5001c
to
494a7db
Compare
Do I understand this correctly that this will keep e.g.
Wasn't it |
Yes, Regarding the naming: it was always One other thing that could be implemented (independently) is changing the system default unit (which is currently hard-coded, depending on dimension and system of measurement). |
So if I select
Depends on how it works. |
Regarding |
What would be the syntax in items file? |
@lolodomo Like any other metadata. For using
|
If nothing is configured, the system default (in my case |
But if it carries a unit (e,g,
Imho you're mixing two things - how the GUI presents the setting and how it actually is configured. However there is no need for a Also if I as a user don't explicitly set the unit it might lead to the impression that the unit will change when I e.g. change my system locale which is something we explicitly don't want to do. We neither want to change the normalization value automatically for existing items nor do we want to create the impression that openHAB will behave that way. |
As I said earlier: if it's If the user creates the item from a thing channel with "Add link to item -> Create new item" then the item type is automatically set from the accepted item type of the channel. Nothing that can go wrong there. If the user doesn't want anything special and the system default unit is fine, then nothing else needs to be configured. If he wants to use a different unit, he can configure the unit metadata to use another unit. In this case UI can make suggestions (a list of common units for this dimension) and verify the compatibility of the selected unit. It's debatable if the unit metadata should be automatically added when the item is created and initialized with the system default unit. I would be open for that, this would also solve the "change system of measurement". But this is IMO a rare case and we'll probably have a 50/50 chance that someone wants to change the item units in that case or not. |
I think I understand what you are describing.
But why doesn't the channel type say
I think explicit is better than implicit and in this case I'd stick to that. It takes a very short time to configure it but it's such an important field that the user should be aware of it. So from the current perspective I'd vote against that. |
Strictly typed item-types (liked it is now) have some advantages. The channel defines This also applies to textual configuration, if you define an incompatible unit, the item will still work and provide values. Breaking that requires deliberately changing the item-type, not just a typo in the unit (e.g. I don't think there is a benefit of using |
But would that fail quickly - e.g. if I change the unit from Also if I am not super fluent in English I'll quickly get confused with
I think this is a misunderstanding. My suggestion is that the item only defines the unit and scale and the channel defines the item type (e.g. String/Number) and optionally an UoM (in case of Number). There should be one way to define the uom and one way only. It just makes no sense to say: "This item should have the temperature in °C and yes - I'm really sure it's a temperature". You're also not saying "Today it'll be 15°C and what I just told you was a temperature". Everyone know that already from the °C. |
Just one remark as I see that some of you want to remove the number dimension: we cannot break the setup of 100% of users by making the item syntax not backward compatible... |
For managed things #3268 would solve the issue. This will also be needed for the approach taken here. Textual configuration will in any case require manual action, but that is exactly what those users want. |
We had a huge breaking changes from OH1 to OH2 and from OH2 to OH3 and people figured it out, too. |
494a7db
to
ce70a39
Compare
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/openhab-4-0-milestone-discussion/145133/158 |
ce70a39
to
1c3dd0a
Compare
@J-N-K what are your current thoughts on dropping e.g. If you're not sold can you share your doubts so I can better understand them. |
We have discussed that before (see the attached issue) and there have been concerns regarding clarity what the item actually stores. That is why I decided to go on with this PR. |
I see a big advantage to @J-N-K 's solution, it is (almost fully) backward compatible. @J-N-K : please correct me if I am wrong. Even if the user does not update their items definition in config files, it will continue to work as before. In case the user was not using the default system unit for an item, the only impacts would be the values displayed in charts. No change in UIs. Not sure if rules could be impacted ? |
@lolodomo The breaking change is that currently we use the state description for determining the unit and if that is not set the system default unit. With this PR the state description is not considered, but the So if someone currently uses I don't see any effect on UIs or rules, bindings might be affected depending on my last question in #3282. |
Yes, I clearly understand that point but I have the feeling this change will be almost fully transparent for the user ? The values displayed in UIs will be the same as before. As you say his rules will not be impacted, the only impact I can guess for the user is that now in charts the item values will be in °C rather than in Kelvin before.
Not sure what question you are referring. In case we have to update all the bindings, this might be a critical problem ! |
IMO bindings that can be linked to That is why I proposed a profile that strips units if necessary (effectively converting |
No - the change will not be transparent. To specify what values you want to display you have to either set a unit in the state description (e.g That's why I am so against keeping |
Should I add |
That would mean that item state passed to icon request is the internal state. That is probable. |
As for the Android app, sliders currently don't follow the unit metadata, but TBH, I am not sure what the correct algorithm for value formatting is supposed to be.
This already is interesting. The item state has unit Even more interesting is the sitemap response. Relevant excerpt:
So the item state matches the item endpoint (as expected), but the widget state is (Consequently, the Android app interprets everything as As much as I like the UoM handling in general, I feel we (as in: app people) need some more pieces of information mapped into the sitemap response here:
With that, the widget format string could override the state description format string, which would lead to the correct behavior for the test case, I believe. |
What version did you use for that test? |
Snapshot 3389. I'll update to a newer one and test again. |
That's very old (the last snapshot is 3479) and I believe way before this PR was merged, can you please try with a recent snapshot? |
Just did just that, but AFAICT, the sitemap response is the same as with 3389:
... so the points from above (particularly the missing widget format string) still apply. |
IMO the response is fully valid, the state is
(and that overrides the state description, which is also correct). Also the label is formatted accordingly, as well as the state. So I guess the only thing that is missing is the unit. @lolodomo WDYT? |
Well, the state is
As I said, it's all there, it just needs to be put in the sitemap widget response (and SSE event, in case of the state) ;-) |
Default unit for length is meters, not centimeters. |
@lolodomo Correct. That's interesting. The item endpoint reports:
Which looks fine. I added a sitemap:
and the REST API returns
which also looks reasonable to me. The state is correctly set to |
This should be discussed in a new issue. |
'Result above' as in: mine? I'll retry with a new install (as opposed to unpacking the zip over the old install) tomorrow.
Will open a new one for that. |
Unitless values should fall back to the defined |
Regarding min/max/step values, there is am open issue to add unit. Currently, the default unit for these attributes is the display unit if one is defined. |
I have not read everything in ultra details but I see nothing wrong in the events. I agree that it could be better to explain the units for min/max(step or even to be able to provide an unit. |
Yes, but that doesn't help for formatting min/max etc. We can continue that discussion in #3624. |
…hab#14966) Changes in openhab/openhab-core#3481 broke this service due to an breaking API change in NumberItem class (constructor). Related to openhab/openhab-core#3600 Signed-off-by: Laurent Garnier <[email protected]> Signed-off-by: Thomas Burri <[email protected]>
* Add defaultUnit metadata for NumberItem Signed-off-by: Jan N. Klug <[email protected]> GitOrigin-RevId: 9ef076d
…hab#14966) Changes in openhab/openhab-core#3481 broke this service due to an breaking API change in NumberItem class (constructor). Related to openhab/openhab-core#3600 Signed-off-by: Laurent Garnier <[email protected]> Signed-off-by: Matt Myers <[email protected]>
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/blockly-quantity-type-gets-lost-when-using-persistence/150470/6 |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/modbus-thermostat-stepper-dont-work/149836/7 |
…2126) Related to openhab/openhab-core#3481. Closes #2108. - Ensures that the internal unit is set when an Item is created, so that in case the system unit changes (i.e. measurement system changes) persisted data does not get corrupted. By default, the unit metadata is set to the system default unit, however the user can easily change that on Item creation. The unit can also be changed later as it is just normal metadata, however this can corrupt persisted data. - Adds the ability to set state description pattern when creating a UoM Item. - Shows the group type for groups, e.g. `Group (Number:Temperature)` instead of just the Item type. This applies to both the Items and the model page. --------- Signed-off-by: Florian Hotze <[email protected]>
…hab#14966) Changes in openhab/openhab-core#3481 broke this service due to an breaking API change in NumberItem class (constructor). Related to openhab/openhab-core#3600 Signed-off-by: Laurent Garnier <[email protected]> Signed-off-by: Jørgen Austvik <[email protected]>
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/modbus-thermostat-stepper-dont-work/149836/20 |
This seems to have broken profile that converts unit-aware-number commands to plain numbers Is this new behaviour desired? https://community.openhab.org/t/modbus-thermostat-stepper-dont-work/149836/20?u=ssalonen |
This is a new approach to solving #3282. It keeps the dimension when the item is created but it separates the state description from the internal unit.
With this PR:
UnitProvider
, depends on dimension and system of measurement) or the unit configured in the newdefaultUnit
metadata (takes precedence if present)Number
items units are stripped,DecimalType
updates to dimension items are considered to be in the configured unit, incompatible units are rejected.QuantityType
to a plain item is stripped, the item unit is added toDecimalType
if there is a dimension, incompatible units are rejected. In contrast to state updates, the unit for commands is NOT converted.CommunicationManager
strips the unit and sendsDecimalType
if the channel has the accepted item-typeNumber
, it adds the item-unit if the dimension of the item and the channel are the same. Commands with incompatible dimension are rejected. "Guessing" a unit based on the channel type when the item is dimensionless has been removed as it is confusing.DecimalType
value, and aQuantityType
value is converted to that unit, if no unit is configured, the unit is stripped from aQuantityType
value.To fully solve #3282 we also need to merge #3141 and check where
ItemStateEvent
was used before.