[linkplay] Initial Contribution

[digitaldan]

This integrates audio devices based on the LinkPlay platform. LinkPlay is a popular Wi-Fi audio module used by many manufacturers. This binding uses both UPnP communication for realtime events, meta data, etc.. and the LinkPlay HTTP API for control, group management, etc...

This is dependent on openhab/openhab-core#5095 which was just merged.

[digitaldan]

Yes, there is another Linkplay binding on the marketplace...... And no, i am not trying to make a habit of creating competing bindings out there ...... I promise 🤞

In this case i did try reaching out to Michael, but did not get a response. The existing binding has been untouched for a long time, and the author's github accounts looks like they have moved to another home automation system, so i don't think its a problem.

This version is fundamentally different in that is was built around UPnP for realtime updates, then i layered on the HTTP api when needed, so there's no polling and and it behaves very similar to the Linkplay/WiiM apps themselves. Also it has strong group support tailored for easy use in our UIs.

I have 4 WiiM streamers that have replaced my 4 aging Sonos Connect boxes i am using this with.

[Copilot]

Pull Request Overview

This PR introduces a new binding for LinkPlay-based Wi-Fi audio devices. LinkPlay is a popular audio module platform used by various manufacturers. The binding enables control of playback, volume, input sources, equalizer settings, and multi-room audio grouping through a combination of UPnP and HTTP API communications.

Key changes:

• Adds complete LinkPlay binding with UPnP discovery support
• Implements multi-room audio group management functionality
• Provides AudioSink integration for audio streaming

Reviewed Changes

Copilot reviewed 52 out of 53 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
bundles/pom.xml	Adds linkplay module to build
bundles/org.openhab.binding.linkplay/pom.xml	Maven configuration for new binding
bundles/org.openhab.binding.linkplay/src/main/resources/OH-INF/thing/thing-types.xml	Channel and thing type definitions
bundles/org.openhab.binding.linkplay/src/main/resources/OH-INF/i18n/linkplay.properties	Internationalization strings
bundles/org.openhab.binding.linkplay/src/main/java/.../LinkPlayHandlerFactory.java	Factory for creating handlers and managing UPnP device registry
bundles/org.openhab.binding.linkplay/src/main/java/.../group/*.java	Multi-room group management service and interfaces
bundles/org.openhab.binding.linkplay/src/main/java/.../client/http/*.java	HTTP API client and DTOs for LinkPlay devices
bundles/org.openhab.binding.linkplay/src/main/java/.../client/upnp/*.java	UPnP communication, XML parsing, and command execution
bundles/org.openhab.binding.linkplay/src/main/java/.../discovery/*.java	UPnP discovery participant implementation
bundles/org.openhab.binding.linkplay/README.md	Complete binding documentation with examples
bom/openhab-addons/pom.xml	Adds binding to BOM dependencies
CODEOWNERS	Registers code owner for the binding

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull Request Overview

Copilot reviewed 52 out of 53 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull Request Overview

Copilot reviewed 54 out of 55 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[lsiepel]

Thanks for adding this binding. Looked at 42/55 files, mainly dto's and metadata files. Handlers will be next.

[digitaldan]

Thanks @lsiepel , hopefully those comments are addressed.

[lsiepel]

Thanks @lsiepel , hopefully those comments are addressed.

Currently reviewing the unifi binding, will come back to this PR when that first round is done.

[lsiepel]

Looked at 49 out of 55 files. Vital parts are left, didn't want to hold back on these.

[lsiepel]

Suggested change

	data[i / 2] = (byte) ((Character.digit(hex.charAt(i), 16) << 4)
	+ Character.digit(hex.charAt(i + 1), 16));
	int hi = Character.digit(s.charAt(i), 16);
	int lo = Character.digit(s.charAt(i + 1), 16);
	// If any nibble is invalid, treat whole input as non-hex and return original
	if (hi < 0 || lo < 0) {
	return hex;
	}
	data[j] = (byte) ((hi << 4) + lo);

[lsiepel]

When interrupted, the method should restate the flag and exit immediate and gracefully

[digitaldan]

Hmm, not sure i'm following, what flag?

[Nadahar]

The interrupted flag - each thread has one. Just call Thread.currentThread().interrupt() after catching InterrutedExceptions (more or less always), because the flag is reset when you catch it - which means that if the code enters other blocking calls before execution is terminated, it will wait there instead of throwing a new InterruptedException. It's one of those Java peculiarities: https://www.baeldung.com/java-interrupted-exception

Many people get this wrong, which leads to problems with threads - as interruption is the control mechanism that is used to terminate threads in Java.

Once a thread is interrupted, it's supposed to just do the most necessary, non-blocking, cleanup, and exit gracefully.

[lsiepel]

I thikn the order is wrong. First apply the factory settings, and afterwards get the saxParser form the factory. Otherwise it won't have effect.

[digitaldan]

So i'm not sure what to do here, as stated in the authors section, i "borrowed" this class from the Sonos binding actually the upnp binding, which borrowed from Sonos, and left it pretty much untouched since its been in production a very long time.

[Nadahar]

Remember that if you get this wrong, there will be no "immediate bug". It just means that the protection against XXE attacks won't work, which very well could be the case in the locations where the code has been copied from.

It's all about disabling various unsafe options in XML parsing, first and foremost prevent the parser from contacting and arbitrary URL that is specified in the XML document. XXE is used for that, and then other vulnerabilities are used for the actual attack. When you don't need validation against a "live" schema, like with UPnP, all this can and should just be disabled.

[Nadahar]

Fun fact: I once contributed a bugfix to an XXE scanner, it can be used to test for some of the XXE vulnerabilities, but it's primarily focused on Windows XXE exploits. I'm sure there are more practical scan tools available today, this is a bit involved setting up as far as I can remember.

Using such a scanner, one could relatively easily test the mentioned bindings for such vulnerabilities.

[Nadahar]

It turns out that this is quite a mess. I remember that XXE was "the big scare" some years ago, where a lot of people no doubt made a lot of money from "protection applications" against XXE with various tools and scanners. Then people mostly forgot about it, as there were no more money to be extracted.

In reality, I doubt it was ever anything near as "dangerous" as the hype claimed, at the same time, it's "dangerous enough" that measures should be taken to prevent it. Unfortunately, the Java XML parsers haven't made it easy to protect against, because of stupid implementations. It should be enough to have a flag that could tell every parser to never look outside the document itself, never follow links to other documents. But, that would be too easy I guess.

Instead, there are various confusing sources dealing with this, like this one: https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#java

The example code for the SAXParser links to this gist: https://gist.github.com/asudhakar02/45e2e6fd8bcdfb4bc3b2
...which, as far as I can tell, does it wrong. Which would explain why the current OH code is as it is. It does configure the factory after creating the parser... 😖

But, it gets worse. Because Java's XML parser is "pluggable", you don't actually know which parser you will get (unless you "know" how the application is configured and that this will never change). Not all the parsers support the same "flags", which causes a lot of the chaos.

Xerces 2 supports http://apache.org/xml/features/disallow-doctype-decl, which disables most of the vulnerabilities (although some claim that you need to setXIncludeAware(false) as well - even though it's "the default", the default varies between parsers. Xerces 1 on the other hand, and several others, don't support this, and you must instead use http://xml.org/sax/features/external-general-entities in combination with several other options to disable most of the "holes". Most people don't seem to get the details here... so they just throw in a couple flags, and hopes for the best.

But, some parsers throw exceptions if you try to set "features" that they don't support. So, to properly "secure" a parser factory, for an "unknown implementation", you need to set all the different flags encapsulated by separate catch blocks, so that the code will continue even if you set an unsupported "feature". You have to cover all the bases for all the different implementations, only you can't, because some implementations aren't properly secure no matter what, from what I understand.

So, all in all, it's a mess. My guess is that it's possible to figure out which parser OH actually uses and narrow down the mess quite a lot.

But, when looking in the UPnPControl binding, I see that the factory is acquired and configured for every message that it so be parsed. This is just crazy, you're supposed to create and configure the factory once, and then use it to produce, properly configured, parsers. So, there's that as well....

Maybe making Core configure "secure" XML parsers for bindings to use would be a better option? So, that everybody won't have to know all the details of this?

It should be mentioned that there are also situations where you can't enable these options, because it will make parsing fail for "valid" documents. Stupidly, these options don't just make the parser ignore the URIs in the document, they make them refuse to parse documents with URIs in them. Which can be a problem in some situations, so such a "secure" parser might not always be suitable. But, for many cases, I'm sure it would...

[Nadahar]

Yeah, I know that the licenses themselves allow it - I was thinking OH specific, SAT plugin, policies etc. But, discussing this in a comment on a PR review is the wrong place anyway 😉

[lsiepel]

The zwavejs binding and some others make use of the 3rdparty folder structure. Maybe that is what you are looking for?

[Nadahar]

As I said, I found another workaround in this specific case now, but I'm sure similar situations will arise in the future. I think I'll have to revisit it when it does, I have too many balls in the air at the moment 😉

[Nadahar]

I've been distracted with other things (as usual); so I've not yet completed my alternative suggestion for how to do the GENA subscriptions - but, there's no reason that should hold back this binding anyway, since the approach is shared with two other bindings. I suggest that we handle them as one in respect to the subscription handling and XML parsing.

[Nadahar]

My alternative suggestion just met a dead-end. UpnpIOService basically breaks how jUPnP is supposed to work, as it "destroys information" when receiving events before passing them on to listeners in eventReceived():

        protected void eventReceived(GENASubscription sub) {
            Map<String, StateVariableValue> values = sub.getCurrentValues();
            Device device = sub.getService().getDevice();
            String serviceId = sub.getService().getServiceId().getId();

            logger.trace("Receiving a GENA subscription '{}' response for device '{}'", serviceId,
                    device.getIdentity().getUdn());

            for (UpnpIOParticipant participant : participants) {
                Device participantDevice = getDevice(participant);
                if (Objects.equals(participantDevice, device) || Objects.equals(participantDevice, device.getRoot())) {
                    for (Entry<String, StateVariableValue> entry : values.entrySet()) {
                        Object value = entry.getValue().getValue();
                        if (value != null) {
                            try {
                                participant.onValueReceived(entry.getKey(), value.toString(), serviceId);
                            } catch (Exception e) {
                                logger.error("Participant threw an exception onValueReceived", e);
                            }
                        }
                    }
                    break;
                }
            }
        }

It takes the StateVariableValue and "flattens it" using toString() so that most of the information it carries is lost. This deems further processing to just work with strings manually, like the code already does. So, until this is changed, manual parsing is probably the way to go.

I've been trying to do something about some of the many problems around UPnP in OH, but it seems that there's simply no interest: openhab/openhab-core#5137

Nobody bothers to engage or do basic testing, so I guess I will just have to forget about doing anything about it.

It should still be possible to do UPnP properly, but that would mean to skip UpnpIOService entirely, and do much more of the "heavy lifting" in the binding. But, there's probably no interest for that either.

[lsiepel]

See other comment about order

[lsiepel]

Not sure about the size, but we generally only list contents in trace. Maybe also guard with isDebugEnabled()

[lsiepel]

See other comment about logging xml content.

[lsiepel]

Suggested change

	<version>5.1.0-SNAPSHOT</version>
	<version>5.2.0-SNAPSHOT</version>

[Nadahar]

Won't making that change just become a conflict with the main branch? Isn't it better to rebase it on the updated main where the correct version is set?

Forgot it - I'm just being slow 😒

[lsiepel]

Reviewed the remaining files of this binding. After all comments have been fixed, i'll probably need one last pass.

[lsiepel]

Could you reduce the image size ?

[lsiepel]

Suggested change

	| track-uri | String | R | Current track uri |
	| track-uri | String | R | Current track uri |

[lsiepel]

Please algin the table.

[lsiepel]

Suggested change

	3. **Independent Channels**: Each device maintains its own settings for:
	1. **Independent Channels**: Each device maintains its own settings for:

[lsiepel]

Suggested change

	2. **Synchronized Channels**: The following channels are automatically synchronized from leader to all members:
	1. **Synchronized Channels**: The following channels are automatically synchronized from leader to all members:

[lsiepel]

Could you add javadoc to all interface methods.

[lsiepel]

Suggested change

	logger.error("Error joining group: {}", e.getMessage(), e);
	logger.warn("Error joining group: {}", e.getMessage(), e);

[lsiepel]

Suggested change

	logger.error("Error ungrouping: {}", e.getMessage(), e);
	logger.warn("Error ungrouping: {}", e.getMessage(), e);

[lsiepel]

Suggested change

	logger.error("Error getting slave list: {}", e.getMessage(), e);
	logger.warn("Error getting slave list: {}", e.getMessage(), e);

[lsiepel]

Suggested change

	logger.error("Error adding member: {}", e.getMessage(), e);
	logger.warn("Error adding member: {}", e.getMessage(), e);

[digitaldan]

@lsiepel just fyi , I'm currently refactoring a few things out of the thing handler , so taking a little more time , but I'll have that done soon for review again

[digitaldan]

Moving to a draft while i make some larger changes. I'll let you know when its ready.