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.

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

[mqtt.generic] Send ON/OFF for dimmer channels when so configured #15929

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

ccutrer
Copy link
Contributor

@ccutrer ccutrer commented Nov 19, 2023

Similar to how UP/DOWN are processed for rollershutters.

Fixes #7991

@ccutrer ccutrer requested review from a team and antroids as code owners November 19, 2023 18:36
@ccutrer ccutrer force-pushed the mqtt-generic-dimmer-on-off branch from 0ece641 to d740042 Compare November 19, 2023 19:01
@J-N-K
Copy link
Member

J-N-K commented Nov 19, 2023

As a general question: Wouldn't it make sense to use the converters introduced in openhab/openhab-core#3355?

@ccutrer
Copy link
Contributor Author

ccutrer commented Nov 19, 2023

I was not aware of their existence. Yes, it probably would make sense to use those, but I would prefer to get this PR in as is, and then do a separate (much larger!) PR converting to those. It will need to consider how all the "downstream" MQTT bindings rely on their conversions, and possibly add more capabilities to core. In particular, some of the recent Home Assistant components I've added rely on non-round-trippable conversions. I.e. Cover/Rollershutter you send OPEN, but the state is open (or fully configurable command and state values). I have a WIP I haven't opened a PR for yet for Lock where you send LOCK, UNLOCK, or OPEN, but the states are LOCKED, UNLOCKED, LOCKING, UNLOCKING, JAMMED.

@ccutrer ccutrer marked this pull request as draft November 19, 2023 20:15
@ccutrer
Copy link
Contributor Author

ccutrer commented Nov 19, 2023

Note that this conflicts with #15925. I'd rather that one merge first. Once that's done, I'll rebase this one.

@ccutrer ccutrer force-pushed the mqtt-generic-dimmer-on-off branch from d740042 to 1fc3ff7 Compare January 30, 2024 17:14
@ccutrer ccutrer marked this pull request as ready for review January 30, 2024 20:50
@ccutrer
Copy link
Contributor Author

ccutrer commented Jan 30, 2024

I just tested this, and it's ready for review

@ccutrer
Copy link
Contributor Author

ccutrer commented Feb 7, 2024

ping @antroids @jlaur @lolodomo

@ccutrer
Copy link
Contributor Author

ccutrer commented Mar 21, 2024

Ping @antroids @jlaur @lolodomo

@lsiepel
Copy link
Contributor

lsiepel commented Apr 30, 2024

Please rebase.

@lsiepel
Copy link
Contributor

lsiepel commented Jul 16, 2024

ping @ccutrer

Similar to how UP/DOWN are processed for rollershutters.

Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer ccutrer force-pushed the mqtt-generic-dimmer-on-off branch from c426d36 to e679080 Compare July 16, 2024 20:28
@ccutrer
Copy link
Contributor Author

ccutrer commented Jul 16, 2024

rebased, conflicts resolved, which then required some changes to tests, and squashed it all together

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

LGTM.

One remaining question. The current documentation in readme.md does not show the posiblity of assigning a number to the on or off parameters. But the parameters itself do. As that functionality has been removed by this PR and now strictly needs a specific string as on or off, i wonder if this 1. is considered a breaking change and/or 2. needs a notice in the openhab update procedure, @ccutrer wdyt?

@ccutrer
Copy link
Contributor Author

ccutrer commented Jul 23, 2024

Looking through this again, there's nothing that prevents you from still entering a number for the special ON/OFF values. They'll just be treated as a string --- exactly as before. The only difference is that on/off no longer have a default (of 1/0). Which isn't actually a difference, because ON/OFF (on sending a command) will first be translated to PercentType.HUNDRED/PercentType.ZERO, and then scaled to the min/max, ending up at 100/0 or 1/0 as appropriate. On return back from MQTT with a current state, this commit will actually fix a bug if your min/max is 0-100, but you leave the on string as the default of "1", "1" would inadvertently get treated as 100%, not 1%.

@lsiepel lsiepel added the bug An unexpected problem or unintended behavior of an add-on label Jul 23, 2024
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@lsiepel lsiepel merged commit 0156612 into openhab:main Jul 23, 2024
5 checks passed
@lsiepel lsiepel added this to the 4.3 milestone Jul 23, 2024
@ccutrer ccutrer deleted the mqtt-generic-dimmer-on-off branch July 23, 2024 21:41
digitaldan pushed a commit to digitaldan/openhab-addons that referenced this pull request Aug 29, 2024
…enhab#15929)

Similar to how UP/DOWN are processed for rollershutters.

Signed-off-by: Cody Cutrer <cody@cutrer.us>
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
…enhab#15929)

Similar to how UP/DOWN are processed for rollershutters.

Signed-off-by: Cody Cutrer <cody@cutrer.us>
Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
…enhab#15929)

Similar to how UP/DOWN are processed for rollershutters.

Signed-off-by: Cody Cutrer <cody@cutrer.us>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
…enhab#15929)

Similar to how UP/DOWN are processed for rollershutters.

Signed-off-by: Cody Cutrer <cody@cutrer.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MQTT] Dimmer channel on/off properties not sending "ON" or "OFF"
4 participants