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

[amazonechocontrol] Support QuantityType Color Temperature command #17919

Merged
merged 5 commits into from
Dec 21, 2024
Merged

[amazonechocontrol] Support QuantityType Color Temperature command #17919

merged 5 commits into from
Dec 21, 2024

Conversation

andrewfg
Copy link
Contributor

@andrewfg andrewfg commented Dec 17, 2024 •
edited
Loading

Resolves #17917

Signed-off-by: Andrew Fiddian-Green [email protected]

@andrewfg andrewfg added enhancement An enhancement or new feature for an existing add-on work in progress A PR that is not yet ready to be merged labels Dec 17, 2024
@andrewfg andrewfg requested a review from lolodomo December 17, 2024 16:28
@andrewfg andrewfg self-assigned this Dec 17, 2024
@andrewfg
cosmetics
533db73 
Signed-off-by: Andrew Fiddian-Green <[email protected]>
@andrewfg andrewfg added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Dec 17, 2024
@andrewfg
add shelly
31f9ef7 
Signed-off-by: Andrew Fiddian-Green <[email protected]>
@andrewfg andrewfg requested a review from markus7017 as a code owner December 17, 2024 17:30
@andrewfg andrewfg removed the work in progress A PR that is not yet ready to be merged label Dec 17, 2024
@andrewfg andrewfg mentioned this pull request Dec 18, 2024
Closed
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-4-3-zigbee2mqtt-color-temperature-in-mired-not-working/160911/4

@andrewfg

This comment was marked as resolved.

Sign in to view
@lsiepel lsiepel marked this pull request as draft December 18, 2024 17:32
@lsiepel
Copy link
Contributor

lsiepel commented Dec 18, 2024

Changed to draft to prevent unwanted merge. Please revert when mqtt channels are fixed.

@jlaur jlaur added bug An unexpected problem or unintended behavior of an add-on and removed enhancement An enhancement or new feature for an existing add-on labels Dec 18, 2024
@andrewfg
Copy link
Contributor Author

@ccutrer do you think anything needs to be done in the MQTT bindings in order for them to accept QuantityType inputs. AFAICT they do already accept such inputs and a) if the Channel has a ‘unit’ field then UoM conversions are applied, or b) if the Channel has no ‘unit’ field then it takes the raw decimal value of the given QuantityType (without conversion).

@ccutrer
Copy link
Contributor

ccutrer commented Dec 20, 2024

To answer here: the handleCommand and downstream calls are all set up to handle K or mirek in MQTT, but due to another issue core was only sending decimal commands. PRs are open (openhab/openhab-core#4507 and #17929) to fix that.

@andrewfg andrewfg marked this pull request as ready for review December 20, 2024 08:07
@andrewfg
Copy link
Contributor Author

@lolodomo the case of MQTT is covered by @ccutrer 's two PRs, so this one is ready to go. We should probably cherry pick it back into OH 4.3..

@lolodomo
Copy link
Contributor

I take a look when I am back at home.
Wouldn't it better to have a PR by binding to sort what can be backported without risk ?
@lsiepel @jlaur WDYT ?

@andrewfg
Copy link
Contributor Author

have a PR by binding

No problem. I will split it.

lolodomo
lolodomo approved these changes Dec 21, 2024
View reviewed changes
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@lolodomo
Copy link
Contributor

No problem. I will split it.

Wait, I am not 100% sure it is necessary and I don't want to make you loose your time!
@jlaur @lsiepel : is it ok for you to backport this PR in 4.3.x even if it impacts several bindings ?

@andrewfg
Copy link
Contributor Author

It will only take about 30 minutes to do the split

@andrewfg
split into multiple PRs
bc6f5e0 
Signed-off-by: Andrew Fiddian-Green <[email protected]>
@andrewfg andrewfg changed the title [various] Handle color temperature quantity type commands [amazonechocontrol] Handle color temperature quantity type commands Dec 21, 2024
This was referenced Dec 21, 2024
Merged
Merged
Merged
Merged
Merged
Merged
@andrewfg andrewfg changed the title [amazonechocontrol] Handle color temperature quantity type commands [amazonechocontrol] Support quantity type commands Dec 21, 2024
@andrewfg andrewfg changed the title [amazonechocontrol] Support quantity type commands [amazonechocontrol] Support QuantityType Color Temperature command Dec 21, 2024
lolodomo
lolodomo approved these changes Dec 21, 2024
View reviewed changes
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@lolodomo
Copy link
Contributor

Can you please adjust the content of your first message ?

@andrewfg andrewfg requested a review from lolodomo December 21, 2024 11:27
@jlaur
Copy link
Contributor

jlaur commented Dec 21, 2024 •
edited
Loading

is it ok for you to backport this PR in 4.3.x even if it impacts several bindings ?

If I get the context right, what is left here in this PR is a fix for a regression introduced by #17754 - and likewise for all the others (i.e. #17942 fixes #17777 etc.). So I think all the PR's should be backported to 4.3.x.

Given a clean commit history (one commit per binding) we could have merged just one PR without squash-merging the commits. We used that approach here: #17725 (comment)

But I like this split approach as well, since there is a clean 1:1 relation for each PR to a PR that introduced the issue - and also all bindings will be mentioned in the release notes. 👍

@lolodomo lolodomo merged commit 6acfeb6 into openhab:main Dec 21, 2024
3 checks passed
@lolodomo lolodomo added this to the 5.0 milestone Dec 21, 2024
@lolodomo
Copy link
Contributor

To be backported in branch 4.3.x.

@lolodomo
Copy link
Contributor

lolodomo commented Dec 21, 2024 •
edited
Loading

I will do the different backporting, I am just searching again the proper git commands... to not make a mistake!

lolodomo pushed a commit to lolodomo/openhab-addons that referenced this pull request Dec 21, 2024
@andrewfg @lolodomo
[amazonechocontrol] Support QuantityType Color Temperature command (o…
83924e8 
…penhab#17919)

* [various] process color temperature quantity type commands

Signed-off-by: Andrew Fiddian-Green <[email protected]>
@lolodomo lolodomo added the patch A PR that has been cherry-picked to a patch release branch label Dec 21, 2024
@jlaur
Copy link
Contributor

jlaur commented Dec 21, 2024

I will do the different backporting, I am just searching again the proper git commands... to not make a mistake!

For reference: #15329 (comment)

@lolodomo
Copy link
Contributor

The 5 PRs are now backported to branch 4.3.x.

@ccutrer ccutrer mentioned this pull request Dec 23, 2024
Closed
cipianpascu pushed a commit to cipianpascu/openhab-addons that referenced this pull request Jan 2, 2025
@andrewfg
[amazonechocontrol] Support QuantityType Color Temperature command (o…
1c26aea 
…penhab#17919)

* [various] process color temperature quantity type commands

Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Ciprian Pascu <[email protected]>
@andrewfg andrewfg mentioned this pull request Jan 17, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hilbrand Hilbrand Awaiting requested review from Hilbrand

@wborn wborn Awaiting requested review from wborn

@cweitkamp cweitkamp Awaiting requested review from cweitkamp

@J-N-K J-N-K Awaiting requested review from J-N-K

@mgeramb mgeramb Awaiting requested review from mgeramb mgeramb is a code owner

@markus7017 markus7017 Awaiting requested review from markus7017

@lolodomo lolodomo Awaiting requested review from lolodomo

Assignees

@andrewfg andrewfg

Labels
bug An unexpected problem or unintended behavior of an add-on patch A PR that has been cherry-picked to a patch release branch
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.

Are we sure that all bindings accept properly a value in Kelvin and a value in mirek as command?
6 participants
@andrewfg @openhab-bot @lsiepel @ccutrer @lolodomo @jlaur