这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@ccutrer
Copy link
Contributor

@ccutrer ccutrer commented Nov 29, 2022

closes #10096

also add absolute color temp channel. and remove level channel when we already have a color channel (you can link a DimmerItem to a color channel if your light doesn't happen to have color or you want direct control of brightness only for some reason)

Note that this is a breaking change (brightness channel is no longer available on RGB or RGB+CCT bulbs in favor of linking Dimmer items to the color channel). My understanding is that this PR goes as is, then a separate PR is made to the -distro repo calling out the breaking changes? Do I need to handle anything of automatically pruning the extra channel from things that were created before this code existed?

@ccutrer ccutrer requested a review from Skinah as a code owner November 29, 2022 00:00
@Skinah
Copy link
Contributor

Skinah commented Nov 29, 2022

Thank you for the contribution it will be great to have this feature. I think it would be best to wait until AFTER the STABLE build is released in 2-3 weeks before merging any breaking changes so they can be trialed in Milestone builds first. So I will make just a few quick comments now.

Yes a separate PR is made to the -distro repo calling out the breaking changes and you mark it with a label AWAITIN|G OTHER PR and give a brief description + link that it is waiting on this PR to be merged first. But please read below as it may not be needed to make a breaking change.

"Do I need to handle anything of automatically pruning the extra channel from things that were created before this code existed?"
No, but this is what you need to mention in the alert for the breaking change. In the file there will be other bindings that alert that you need to delete and re-create the THINGS so copy their text if you wish. For people that do texual config there is nothing that needs to be done (other then adjust rules that may use the missing channel), but for GUI users the thing must have a summery of the channels that it held back when the thing was created, so it is necessary to delete and re-add. You can get some very strange behavior that people will blame on the binding having a bug.

As for the breaking changes, do we really need to remove the level channel? What do you think of making it marked as ADVANCED so it is not presented to users and leaving it in place? There is also another way of marking a channel as hidden from memory (not well documented that this is there so I would have to hunt it down and confirm its not my bad memory). In this latter option it still works but cannot be seen, even if the advanced box is ticked. Two alternative ways forward which I would lean towards the advanced method.

I do believe I like/d the separate level channel, as I also went to remove it once in the past and then had to put it back in. The reason was based on the color channel only sends a RGB/HSB command and this was causing the code to drop back to using the RGB leds when you wanted to stay on the WHITE leds. Your recent PR that is merged does solve this to some degree, but it is an OPTIONAL feature and not the only way to use the binding.

This is why my gut feeling is to leave the channel in place as advanced so there is no breaking change. If you disagree, I can do some tests to see how it all works as this is just a few minutes after taking a quick look at your code and not having really done any work on the binding and doing tests in over 2 years.

Let's discuss this point and then look to have it merged in time for 3.5 milestone 1.

@Skinah
Copy link
Contributor

Skinah commented Nov 30, 2022

If removing the level channel depending on result of discussion of the above, the readme will need to be updated most likely it will be listed and described in there.

Is there any other breaking changes? Does the slider change the colour temp the same way/direction, so rules and controls are not affected by the changes?

@jlaur jlaur added enhancement An enhancement or new feature for an existing add-on (potentially) not backward compatible labels Dec 1, 2022
@jlaur
Copy link
Contributor

jlaur commented Dec 1, 2022

@Skinah, didn't read the code yet, but from your comments, this might be relevant (or not): #11668

@Skinah
Copy link
Contributor

Skinah commented Dec 3, 2022

Thanks for posting @jlaur was good to read that for my knowledge. I am hoping that the channel is not removed, as I suspect that it is still needed but I am open to be wrong as I often am. It has been a long time since I looked into this binding in depth and done tests. Also, I took a quick look and it appears to only be for a THING that you can prevent it from being listed in the UI, not sure if it is supported for channels to use listed="false".
https://www.openhab.org/docs/developer/bindings/thing-xml.html#things

@ccutrer
Copy link
Contributor Author

ccutrer commented Dec 21, 2022

I do believe I like/d the separate level channel, as I also went to remove it once in the past and then had to put it back in. The reason was based on the color channel only sends a RGB/HSB command

No, it is completely unnecessary to have a dedicated level channel. To begin with, it is perfectly permissible to send a PercentType command to an HSBType item/channel. And in fact, the handler for this binding already treats them identically:

            case CHANNEL_COLOUR:
            case CHANNEL_LEVEL:
                handleLevelColour(command);
                break;

It's also perfectly permissible to link a DimmerItem to an HSBType channel (analogous to linking a SwitchItem to a PercentType channel. Or even an HSBType channel). Also note that at least in BasicUI, the "up" and "down" buttons next to the color-picker already send PercentType.HUNDRED and PercentType.ZERO commands. If you want to explicitly treat it as the level, you would either link a DimmerItem to the color channel, or in your sitemap explicitly select a Slider control rather than the default or an explicit Colorpicker. Even if you leave it as a Colorpicker, the slider for brightness when drilling into the color will still send only PercentType commands, not full HSBType commands. Allowing you to "stay on white" if you're just wanting to dim the current white color temperature.

@Skinah
Copy link
Contributor

Skinah commented Dec 22, 2022

I have read the changes and am happy to have them merged as is, just please make changes to the readme to reflect the changes to the channels, and also we need to put the Alert in place for the breaking change/s as already mentioned. Lastly it may be necessary to re-run the mvn command to generate what is needed for doing translations, not sure if it is needed when they are system channels or the removed channels mean it is needed.

https://www.openhab.org/docs/developer/utils/i18n.html#generating-i18n-properties-file

Regarding your last post, I agree and know that the color channel can be used for PercentType and slider controls. I do not know where/why I had it in my head it was needed, but I can see no reason to keep it by reading the code there would be no difference.

lolodomo
lolodomo previously approved these changes Dec 31, 2022
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 lolodomo self-requested a review December 31, 2022 10:31
@lolodomo lolodomo dismissed their stale review December 31, 2022 10:31

Approved too fast

@lolodomo
Copy link
Contributor

Code LGTM but as @Skinah said, it remains to adjust README and i18n default translations.

ccutrer added a commit to ccutrer/openhab-distro that referenced this pull request Jan 15, 2023
See openhab/openhab-addons#13801

Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer ccutrer force-pushed the mqtt-espmilighthub-system-channel-types branch from 544a65d to 6ee0b9a Compare January 15, 2023 21:20
@ccutrer
Copy link
Contributor Author

ccutrer commented Jan 15, 2023

sorry it took me so long to get back to this. it was very low priority for me, and I had lots of other things I wanted to fix first. I updated the readme, and removed the now-unused translations. The breaking-change alert is pending in distro (openhab/openhab-distro#1463). I rebased to resolve a trivial merge conflict in the README; no code changes through any of this.

@wborn wborn changed the title [mqtt.espmilighthub] use system channel types for color temp and brightness [mqtt.espmilighthub] Use system channel types for color temp and brightness Mar 17, 2023
@jlaur
Copy link
Contributor

jlaur commented Mar 22, 2023

@ccutrer thanks for the PR. Can you provide channel update instructions - see for example #14587? I will then have another look and help to get this PR merged.

ccutrer added 4 commits July 17, 2023 15:44
…htness

closes openhab#10096

also add absolute color temp channel. and remove level channel when we
already have a color channel (you can link a DimmerItem to a color
channel if your light doesn't happen to have color or you want direct
control of brightness only for some reason)

Signed-off-by: Cody Cutrer <cody@cutrer.us>
Signed-off-by: Cody Cutrer <cody@cutrer.us>
…ystem types

the system types provide the translations

Signed-off-by: Cody Cutrer <cody@cutrer.us>
Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer ccutrer force-pushed the mqtt-espmilighthub-system-channel-types branch from c7692d0 to d0206c0 Compare July 17, 2023 21:44
@ccutrer
Copy link
Contributor Author

ccutrer commented Jul 17, 2023

I just did a simple rebase with no merge conflicts to resolve CI failing on the copyright year line

Signed-off-by: Cody Cutrer <cody@cutrer.us>
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Thanks for the update. I have added a few new comments.

ccutrer and others added 2 commits July 18, 2023 14:19
Co-authored-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Cody Cutrer <cody@cutrer.us>
Signed-off-by: Cody Cutrer <cody@cutrer.us>
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Sorry, found some more issues.

<instruction-set targetVersion="1">
<remove-channel id="level"/>
<update-channel id="colorTemperature">
<type>system.color-temperature</type>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<type>system.color-temperature</type>
<type>system:color-temperature</type>

Please check others. It might be worth verifying the file against the XSD after fixing these. I was checking because I was unsure if update-channel/add-channel can be freely mixed or had to be in specific order (add/update/remove) and found these issues.

I'm wondering if the upgrade actually worked despite these issues?

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 corrected the XML, the type references, ran the XML through a schema validator, and also recreated my things with the current main, then added with this PR, and confirmed my channels properly changed type on existing things.

@jlaur jlaur added the awaiting feedback Awaiting feedback from the pull request author label Aug 31, 2023
@lolodomo
Copy link
Contributor

@ccutrer : it looks like there are not a lot of things to do to finish this PR. Could you take a look to @jlaur last comments ?

@lolodomo
Copy link
Contributor

@ccutrer : maybe it's a shame not to finish this PR ? You have just two comments from @jlaur to consider.

@ccutrer
Copy link
Contributor Author

ccutrer commented Dec 14, 2023

It's not super important, so I'm okay with it slipping past 4.1. I was planning getting to it during the holiday break.

Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer
Copy link
Contributor Author

ccutrer commented Dec 15, 2023

I had a few minutes this morning, so this is ready now.

@lolodomo lolodomo removed the awaiting feedback Awaiting feedback from the pull request author label Dec 15, 2023
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 lolodomo merged commit de418fe into openhab:main Dec 16, 2023
@lolodomo lolodomo added this to the 4.1 milestone Dec 16, 2023
ccutrer added a commit to ccutrer/openhab-distro that referenced this pull request Dec 16, 2023
See openhab/openhab-addons#13801

Signed-off-by: Cody Cutrer <cody@cutrer.us>
ccutrer added a commit to ccutrer/openhab-addons that referenced this pull request Dec 27, 2023
since openhab#13801, it's not been possible to add new milight things,
because the thing XML already has the new channel types, but not
the thingTypeVersion property set, so it would try to apply the
update instructions and error about duplicate channels

Signed-off-by: Cody Cutrer <cody@cutrer.us>
jlaur pushed a commit that referenced this pull request Dec 27, 2023
since #13801, it's not been possible to add new milight things,
because the thing XML already has the new channel types, but not
the thingTypeVersion property set, so it would try to apply the
update instructions and error about duplicate channels

Signed-off-by: Cody Cutrer <cody@cutrer.us>
jlaur pushed a commit that referenced this pull request Dec 27, 2023
since #13801, it's not been possible to add new milight things,
because the thing XML already has the new channel types, but not
the thingTypeVersion property set, so it would try to apply the
update instructions and error about duplicate channels

Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer ccutrer deleted the mqtt-espmilighthub-system-channel-types branch January 2, 2024 21:55
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
…htness (openhab#13801)

* [mqtt.espmilighthub] use system channel types for color temp and brightness

closes openhab#10096

also add absolute color temp channel. and remove level channel when we
already have a color channel (you can link a DimmerItem to a color
channel if your light doesn't happen to have color or you want direct
control of brightness only for some reason)

---------

Signed-off-by: Cody Cutrer <cody@cutrer.us>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
since openhab#13801, it's not been possible to add new milight things,
because the thing XML already has the new channel types, but not
the thingTypeVersion property set, so it would try to apply the
update instructions and error about duplicate channels

Signed-off-by: Cody Cutrer <cody@cutrer.us>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
…htness (openhab#13801)

* [mqtt.espmilighthub] use system channel types for color temp and brightness

closes openhab#10096

also add absolute color temp channel. and remove level channel when we
already have a color channel (you can link a DimmerItem to a color
channel if your light doesn't happen to have color or you want direct
control of brightness only for some reason)

---------

Signed-off-by: Cody Cutrer <cody@cutrer.us>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
since openhab#13801, it's not been possible to add new milight things,
because the thing XML already has the new channel types, but not
the thingTypeVersion property set, so it would try to apply the
update instructions and error about duplicate channels

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

enhancement An enhancement or new feature for an existing add-on (potentially) not backward compatible

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[mqtt.espmilighthub] expose color temperature as Number in mireds

4 participants