-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.homeassistant] Fix newStyleChannels #17491
[mqtt.homeassistant] Fix newStyleChannels #17491
Conversation
b37f60b
to
80f6ad7
Compare
While this commit changes channel IDs, it shouldn't be considered breaking because it just further refines another breaking change to channel IDs introduced in 4.3 |
To what PR do you relate this comment? |
Thanks, that PR was not considered breaking due to: Before i look at the code i wonder if that statement is still true. Not looking for an alltime backwards compatbility here, but just to be clear on the user impact. Ant to determine if an upgrade notification would be usefull. |
Right, I guess that is a bit confusing. So that PR is not-breaking to existing installations. New things created will get the new style channels. This PR "breaks" compatibility with those new style channels. But because that PR has not gone out in a proper release yet, it shouldn't be considered actually breaking. Especially since I finally tracked down why I sometimes have channels missing, and it's due to that PR (i.e. new style channels never fully worked). |
I don't consider it breaking compared to the previous PR as that is still the development branch, but just wanted to be sure that exising (openHAB 4.2.2 or before) installations are not affected. |
... and this can't be merged yet, either. I just found a bug that some components/channels don't send their updates on properly |
@lsiepel this should be ready now. I'll continue testing over the next few days anyway, but things seem pretty happy and I've moved on to other fixes and enhancements. And I labeled it as a bug - this really needs to get in before 4.3 goes final, otherwise more complex Home Assistant things will be intermittently broken. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small comments. As you are doing a ton of work to this binding, it would be nice if you could also try to reduce the compiler warnings while doing so. Not asking you to do it all in once, but just keep it in mind.
.../java/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandler.java
Outdated
Show resolved
Hide resolved
.../main/java/org/openhab/binding/mqtt/homeassistant/internal/component/DefaultSchemaLight.java
Outdated
Show resolved
Hide resolved
.../main/java/org/openhab/binding/mqtt/homeassistant/internal/component/DefaultSchemaLight.java
Outdated
Show resolved
Hide resolved
itest seems to fail. |
This fixes a couple of problems. The biggest is that if multiple components were single channel OR no-name, they would all get mapped to a "groupId" of "", and the next time the Thing started, all but one of them would disappear unless new discovery information was received. While fixing that, I fixed two other issues: * Multi-channel components with a blank name were getting all their channels dumped into the top level of no group. In the newStyleChannels world, we don't care about the component's name anymore - multi-channel components get a group, single-channel components do not. * Instead of statically saying that certain components are only ever single or multi-channel components, determine at runtime if a component only added a single channel, and if so re-classify it as a single-channel component, moving its channel out of the group (and just naming it as the group was named) Signed-off-by: Cody Cutrer <cody@cutrer.us>
don't include the component type in the channel ID unless it's necessary to resolve a conflicting object ID from another component. this is now possible since we don't have to rely on the channel ID matching the component ID to get its channel state Signed-off-by: Cody Cutrer <cody@cutrer.us>
…D changed Signed-off-by: Cody Cutrer <cody@cutrer.us>
…g handlers Only need to adjust for channels that could be renamed due to being single channel components Signed-off-by: Cody Cutrer <cody@cutrer.us>
Co-authored-by: lsiepel <leosiepel@gmail.com> Signed-off-by: Cody Cutrer <cody@cutrer.us>
Signed-off-by: Cody Cutrer <cody@cutrer.us>
Signed-off-by: Cody Cutrer <cody@cutrer.us>
9e6cb7b
to
10a3259
Compare
fixed (I did have to rebase so that itests compiled correctly; presumably due to #17509 |
Thanks, LGTM |
* [mqtt.homeassistant] fix newStyleChannels * further simplify channel IDs Signed-off-by: Cody Cutrer <cody@cutrer.us>
* [mqtt.homeassistant] fix newStyleChannels * further simplify channel IDs Signed-off-by: Cody Cutrer <cody@cutrer.us>
This fixes a couple of problems. The biggest is that if multiple components were single channel OR no-name, they would all get mapped to a "groupId" of "", and the next time the Thing started, all but one of them would disappear unless new discovery information was received. While fixing that, I fixed two other issues:
As part of this, I needed to keep a separate mapping of Channel ID to Channel State, instead of relying on the component ID matching the channel ID. Doing this enabled further simplifying of channel IDs by not including the component type at all in the channel ID, unless it's necessary to resolve a conflict from a duplicate object id of a different component type (which should be rare to never). In practice, this simplifies lots of channels like
light_dimmer
,switch_switch
,cover_cover
,lock_lock
to justdimmer
,switch
,cover
, andlock
.