[wled] Fix discovery inbox entry matching existing things#20345
[wled] Fix discovery inbox entry matching existing things#20345t2000 wants to merge 4 commits intoopenhab:mainfrom
Conversation
It also uses the new context parameter from openhab#20294 Fixes openhab#20344 Signed-off-by: Stefan Triller <github@stefantriller.de>
...enhab.binding.wled/src/main/java/org/openhab/binding/wled/internal/WLedDiscoveryService.java
Outdated
Show resolved
Hide resolved
|
I would also expect the mac-address to be the representation property, did you consider adding this property as representation property? |
There was a problem hiding this comment.
Pull request overview
This PR aims to improve WLED discovery so inbox entries correctly match already-existing Things by using MAC address as the representation key (and leveraging the newly introduced mac-address config context).
Changes:
- Add a
macAddressbridge configuration parameter withmac-addresscontext validation. - Format the discovered MAC address into colon-delimited form when storing
Thing.PROPERTY_MAC_ADDRESSin discovery results.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
bundles/org.openhab.binding.wled/src/main/resources/OH-INF/thing/thing-types.xml |
Adds a macAddress bridge config parameter (with mac-address context) intended to support consistent identification/matching. |
bundles/org.openhab.binding.wled/src/main/java/org/openhab/binding/wled/internal/WLedDiscoveryService.java |
Changes discovery result MAC property to a colon-delimited format for representation-based matching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bundles/org.openhab.binding.wled/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.wled/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
...enhab.binding.wled/src/main/java/org/openhab/binding/wled/internal/WLedDiscoveryService.java
Show resolved
Hide resolved
That is the reason why I have created this PR and the corresponding bug ticket ;-) It already exists in https://github.com/openhab/openhab-addons/blob/main/bundles/org.openhab.binding.wled/src/main/java/org/openhab/binding/wled/internal/WLedDiscoveryService.java#L105 but it is missing as a property on the thing. |
Signed-off-by: Stefan Triller <github@stefantriller.de>
|
Thanks for the reviews, I have addressed your comments. |
...enhab.binding.wled/src/main/java/org/openhab/binding/wled/internal/WLedDiscoveryService.java
Outdated
Show resolved
Hide resolved
|
It looks like you make a confusion between a thing configuration parameter and a thing property. |
Signed-off-by: Stefan Triller <github@stefantriller.de>
Valid point. Usually the MAC address is not changeable. I have moved it from the configuration section to the properties section of the thing-type. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bundles/org.openhab.binding.wled/src/main/resources/OH-INF/thing/thing-types.xml
Show resolved
Hide resolved
| String firmware = WLedHelper.getValue(response, "\"ver\":\"", "\""); | ||
| ThingUID thingUID = new ThingUID(THING_TYPE_JSON, macAddress); | ||
| Map<String, Object> properties = Map.of(Thing.PROPERTY_MAC_ADDRESS, macAddress, | ||
| Map<String, Object> properties = Map.of(Thing.PROPERTY_MAC_ADDRESS, macAddressProperty, |
There was a problem hiding this comment.
Can you also retrieve the macadddress and set it as property for things that are unmanaged (file based) and therefor not added by the discovery class?
There was a problem hiding this comment.
Yes, properties have to be retrieved and set by the thing handler, generally during its initialisation.
Signed-off-by: Stefan Triller <github@stefantriller.de>
|
Almost there, this comment needs to be addressed and then we have a full fix: #20345 (comment) |
It also uses the new context parameter from #20294
Fixes #20344