Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[mqtt.generic] Percentage range fix #10587

Merged
merged 1 commit into from
Jun 21, 2021
Merged

[mqtt.generic] Percentage range fix #10587

merged 1 commit into from
Jun 21, 2021

Conversation

rogierhofboer
Copy link
Contributor

Signed-off-by: Rogier Hofboer rogier@hofboer.nl

Configure mqtt.generic percentage channel value as 0..100

Fixes #10586

@rogierhofboer
[mqtt.generic] Percentage range fix openhab#10586
c1b63e3 
Signed-off-by: Rogier Hofboer <rogier@hofboer.nl>
@rogierhofboer
Copy link
Contributor Author

Note, the reported step should probably be stepPercent, but this didn't work when just trying (inoperable slider without scale).

It should also be clear what step is specified, the dimmer steps (in the range 0..100) or the steps on the mqtt channel, which might have a different range. When stepping multiple times down, the stepper runs into a an infinite loop now.
To prevent numbers with many decimals, maybe allow to provide step values for the mqtt channel and for the percentage value separately and convert. Also we might not want to calculate a value, but make sure it is a valid step.
So 0..254 with step 2 only results in 0,2,4 etc on the mqtt channel.

@wborn wborn added bug An unexpected problem or unintended behavior of an add-on rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Apr 28, 2021
jochen314
jochen314 reviewed May 2, 2021
View reviewed changes
...ding.mqtt.generic/src/main/java/org/openhab/binding/mqtt/generic/values/PercentageValue.java
@@ -154,7 +154,7 @@ public String getMQTTpublishValue(@Nullable String pattern) {

@Override
public StateDescriptionFragmentBuilder createStateDescription(boolean readOnly) {
return super.createStateDescription(readOnly).withMaximum(max).withMinimum(min).withStep(step)
return super.createStateDescription(readOnly).withMaximum(HUNDRED).withMinimum(BigDecimal.ZERO).withStep(step)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The step now seems wrong to me.
When the we have new PercentageValue (0,1000, 100, null, null), then we do not want a step of 100 here, right?
I think, the step here should be stepPercent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right, but it didn't work for me at first try.

I think we'll need to define / document how the percentage type is expected to behave and what the fields are defining.And then review / adapt all the code to get this behaviour.

On a side note: when doing a custom transformation with JavaScript, I first needed to have 0..1 as range for the input (to get 0..254 as output range), which I could not explain, but then after a restart it was 0..100 like expected, so for me there are still some strange things / things I don't understand going on with the percentage type...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also a lot of things unclear to me going from the Value towards the UI.
I would want to cleanup the values in the sense, that we separate the changes from the UI from those comming from the device.
Currently a change from the UI is going through ChannelState.publishValue, which then calls update on the value.
A change from the device is going through ChannelState.processMessage', which then also calls updateon the value. Therefor in update` there is no way to know, from where the change originated. So setting an absolute value can cause a problem, because the base [min...max] or [0...100] is not clear.

@wborn wborn changed the title [mqtt.generic] Percentage range fix #10586 [mqtt.generic] Percentage range fix May 12, 2021
kaikreuzer
kaikreuzer approved these changes Jun 21, 2021
View reviewed changes
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent of what's the right strategy for the step, this fix is worthwhile on it's own, so let us merge it. Many thanks, @rogierhofboer!

@kaikreuzer kaikreuzer merged commit ea2cb21 into openhab:main Jun 21, 2021
@kaikreuzer kaikreuzer added this to the 3.1 milestone Jun 21, 2021
@kaikreuzer kaikreuzer mentioned this pull request Jun 21, 2021
Closed
computergeek1507 pushed a commit to computergeek1507/openhab-addons that referenced this pull request Jul 13, 2021
@rogierhofboer @computergeek1507
[mqtt.generic] Percentage range fix openhab#10586 (openhab#10587)
dd43de8 
Signed-off-by: Rogier Hofboer <rogier@hofboer.nl>
lucacalcaterra pushed a commit to lucacalcaterra/openhab-addons that referenced this pull request Jul 26, 2021
@rogierhofboer @lucacalcaterra
[mqtt.generic] Percentage range fix openhab#10586 (openhab#10587)
addb5c3 
Signed-off-by: Rogier Hofboer <rogier@hofboer.nl>
Signed-off-by: Luca Calcaterra <calcaterra.luca@gmail.com>
lucacalcaterra pushed a commit to lucacalcaterra/openhab-addons that referenced this pull request Jul 26, 2021
@rogierhofboer @lucacalcaterra
[mqtt.generic] Percentage range fix openhab#10586 (openhab#10587)
3ac9b26 
Signed-off-by: Rogier Hofboer <rogier@hofboer.nl>
Signed-off-by: Luca Calcaterra <calcaterra.luca@gmail.com>
lucacalcaterra pushed a commit to lucacalcaterra/openhab-addons that referenced this pull request Aug 3, 2021
@rogierhofboer @lucacalcaterra
[mqtt.generic] Percentage range fix openhab#10586 (openhab#10587)
e0886d1 
Signed-off-by: Rogier Hofboer <rogier@hofboer.nl>
Signed-off-by: Luca Calcaterra <calcaterra.luca@gmail.com>
frederictobiasc pushed a commit to frederictobiasc/openhab-addons that referenced this pull request Oct 26, 2021
@rogierhofboer @frederictobiasc
[mqtt.generic] Percentage range fix openhab#10586 (openhab#10587)
b7b6afa 
Signed-off-by: Rogier Hofboer <rogier@hofboer.nl>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
@rogierhofboer @thinkingstone
[mqtt.generic] Percentage range fix openhab#10586 (openhab#10587)
0398495 
Signed-off-by: Rogier Hofboer <rogier@hofboer.nl>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
@rogierhofboer
[mqtt.generic] Percentage range fix openhab#10586 (openhab#10587)
8546f6f 
Signed-off-by: Rogier Hofboer <rogier@hofboer.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jochen314 jochen314 jochen314 left review comments

@kaikreuzer kaikreuzer kaikreuzer approved these changes

@davidgraeff davidgraeff Awaiting requested review from davidgraeff

Assignees
No one assigned
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Milestone
3.1
Development

Successfully merging this pull request may close these issues.

[mqtt.generic] Percentage value not configured as 0..100 if min and max use a different range
4 participants
@rogierhofboer @kaikreuzer @jochen314 @wborn