[oppo] Add discrete thing types & other improvements - #21445
Conversation
Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR enhances the Oppo binding by introducing discrete Thing types per player model so channel availability, remote-button options, and polling behavior can be tailored to each device’s capabilities, while keeping the existing generic player Thing type available (now deprecated).
Changes:
- Added dedicated Thing type definitions for BDP-83, BDP-93/95, BDP-103/103D, BDP-105/105D, UDP-203, and UDP-205.
- Refactored handler/discovery logic to support the new Thing types (model inference, model-specific query command sets, improved source handling/debouncing, updated discovery classification).
- Updated binding documentation and i18n resources to reflect the new Thing types and options.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| bundles/org.openhab.binding.oppo/src/main/resources/OH-INF/thing/udp205.xml | Adds a UDP-205-specific Thing type with a tailored channel list. |
| bundles/org.openhab.binding.oppo/src/main/resources/OH-INF/thing/udp203.xml | Adds a UDP-203-specific Thing type and model-specific remote button channel-type definition. |
| bundles/org.openhab.binding.oppo/src/main/resources/OH-INF/thing/bdp93.xml | Adds a BDP-93/95-specific Thing type with a tailored channel list. |
| bundles/org.openhab.binding.oppo/src/main/resources/OH-INF/thing/bdp83.xml | Adds a BDP-83-specific Thing type and model-specific remote button channel-type definition. |
| bundles/org.openhab.binding.oppo/src/main/resources/OH-INF/thing/bdp105.xml | Adds a BDP-105/105D-specific Thing type referencing model-appropriate remote button options. |
| bundles/org.openhab.binding.oppo/src/main/resources/OH-INF/thing/bdp103.xml | Adds a BDP-103/103D-specific Thing type and model-specific remote button channel-type definition. |
| bundles/org.openhab.binding.oppo/src/main/resources/OH-INF/thing/channels.xml | Marks the generic player Thing type as deprecated (comment) and splits the legacy model option into 83 vs 93/95. |
| bundles/org.openhab.binding.oppo/src/main/resources/OH-INF/i18n/oppo.properties | Adds labels/descriptions for the new Thing types, new config keys, and remote-button option i18n. |
| bundles/org.openhab.binding.oppo/src/main/resources/OH-INF/config/config.xml | Introduces a reusable config description (thing-type:oppo:oppoconfig) referenced by the new Thing types. |
| bundles/org.openhab.binding.oppo/src/main/java/org/openhab/binding/oppo/internal/OppoHandlerFactory.java | Extends supported Thing types to include the new per-model ThingTypeUIDs. |
| bundles/org.openhab.binding.oppo/src/main/java/org/openhab/binding/oppo/internal/OppoBindingConstants.java | Adds ThingTypeUID constants for the new Thing types and a new MODEL93 constant. |
| bundles/org.openhab.binding.oppo/src/main/java/org/openhab/binding/oppo/internal/handler/OppoHandler.java | Infers model from Thing type (non-legacy), uses model-specific query sets, improves source handling for BDP direct IP and source “bounce” prevention. |
| bundles/org.openhab.binding.oppo/src/main/java/org/openhab/binding/oppo/internal/discovery/OppoDiscoveryService.java | Updates discovery to produce per-model Thing types and improves related documentation/comments. |
| bundles/org.openhab.binding.oppo/src/main/java/org/openhab/binding/oppo/internal/communication/OppoCommand.java | Splits polling query command sets by model family (83/9x vs 10x vs 20x). |
| bundles/org.openhab.binding.oppo/README.md | Documents new Thing types, updated configuration guidance, and refreshed remote-button notes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jlaur
left a comment
There was a problem hiding this comment.
Thanks, this looks really nice! I had a quick look and found only some nitpicking in addition to Copilot's findings. Looking forward to try it out in the weekend. 🙂
| } | ||
| model = config.model; | ||
| } else { | ||
| model = Integer.parseInt(thing.getThingTypeUID().getAsString().replaceAll("[^\\d.]", "")); |
There was a problem hiding this comment.
Although not as clever, perhaps it would be more robust (and faster) to provide explicit mapping from ThingTypeUID to model?
There was a problem hiding this comment.
What would you suggest? A thing property (modelId) perhaps? Otherwise an if-else block is all that comes to mind.
There was a problem hiding this comment.
How about adding this to OppoBindingConstants right after the model constants:
public static final Map<ThingTypeUID, Integer> THING_TYPE_TO_MODEL = Map.of( //
THING_TYPE_BDP83, MODEL83, //
THING_TYPE_BDP93, MODEL93, //
THING_TYPE_BDP103, MODEL103, //
THING_TYPE_BDP105, MODEL105, //
THING_TYPE_UDP203, MODEL203, //
THING_TYPE_UDP205, MODEL205);and then something like this to look it up:
model = THING_TYPE_TO_MODEL.getOrDefault(thing.getThingTypeUID(), 0);?
I considered validing that all thing types have models defined somewhere in OppoHandlerFactory and throw so that the binding refuses to start if missing. But since no new models will be released, I guess the risk of adding new thing types and forgetting to add corresponding model is theoretical.
Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
| <description>Mute or unmute the volume on the player</description> | ||
| </channel> | ||
| <channel id="source" typeId="source"/> | ||
| <channel id="play_mode" typeId="play_mode"/> |
There was a problem hiding this comment.
Fully optional, just a thought: Now that new Things are being introduced, this is the only chance to rename channels to comply with the current naming convention as a non-breaking change:
| <channel id="play_mode" typeId="play_mode"/> | |
| <channel id="play-mode" typeId="play_mode"/> |
WDYT? The player Thing should of course keep the existing naming then. We can rename all channel types as well as a non-breaking change for player, this only requires update instructions for the player Thing.
There was a problem hiding this comment.
That is a good idea, I will work on this tomorrow. All other comments should be addressed now.
| <parameter name="serialPort" type="text" required="false"> | ||
| <context>serial-port</context> | ||
| <limitToOptions>false</limitToOptions> | ||
| <label>Serial Port</label> | ||
| <description>Serial port to use for connecting to the Oppo player.</description> | ||
| </parameter> |
There was a problem hiding this comment.
Does all models have serial ports? Perhaps we could provide two different configurations depending on interface (serial/Ethernet) availability. This would simplify the configuration for each Thing type by removing optional/not applicable parameters.
There was a problem hiding this comment.
Yes all models have serial ports and it is the preferred way to talk to them with the exception of the 20x models.
| xsi:schemaLocation="https://openhab.org/schemas/thing-description/v1.0.0 https://openhab.org/schemas/thing-description-1.0.0.xsd"> | ||
|
|
||
| <!-- Oppo Blu-ray player Thing --> | ||
| <!-- Generic Oppo Blu-ray player Thing (deprecated) --> |
There was a problem hiding this comment.
Perhaps this could be extracted to player.xml as well, leaving only the channel type definitions in this file?
Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
This PR adds discrete thing types for each model. By doing so the available channels, remote button options, polling commands, etc. are tailored to the capabilities of each model. The original thing type id
playerremains as before but is now deprecated.Additional fixes include: