[upnpcontrol] UPnP control binding initial contribution

[mherwege]

This binding controls UPnP servers and renderers. It finds UPnP servers and renderers in the network and allows querying the server content directory and sending it to a renderer for playback.
It should be device independent and only implement (a subset of) the UPnP AV Media 1.0 standard. There is no device specific logic, and this should be kept out of this binding. E.g. switching on/off a device is not part of the UPnP standard and should be done using a different mechanism not part of this binding.
It is roughly based on previous work done in the Sonos binding.

All media renderers will also be registered as audio sinks in openHAB.

This binding has been in development for a while and has been discussed on the forum in this thread.

The binding uses DynamicStateDescriptionProvider and DynamicCommandDescriptionProvider extensively. Current UI's have limited support for a dynamic refresh of these option lists. See openhab-core#1505 and openhab/openhab-addons#7396 (comment).
Also, it would benefit from better support of text input fields in UI's, see openhab-core#1523.

Anyway, it has proven useful for a number of users in the current state, therefore submitting this for review.

Signed-off-by: Mark Herwege mark.herwege@telenet.be

[5iver]

Thank you, Mark! I have used this binding from the time it was made available on the forum and it has worked very well for me with 12 renderers and several media servers.

In case anyone is going to test this build (https://openhab.jfrog.io/openhab/libs-pullrequest-local/org/openhab/addons/bundles/org.openhab.binding.upnpcontrol/2.5.7-SNAPSHOT/org.openhab.binding.upnpcontrol-2.5.7-SNAPSHOT.jar), be sure the remove all of the Things created with older versions of the binding. This may require force removal. If not, you will run into some errors for each renderer.

[Hilbrand]

Thanks Mark. I did a review. Mostly minor comments.

[mherwege]

@Hilbrand Many thanks for your extremely rapid review. I have taken much more time going through it, but learned a lot in doing so. Especially the ListIterator got me stuck. The fit was less natural than anticipated.

[TravisBuddy]

Travis tests were successful

Hey @mherwege,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[Hilbrand]

I did a second review, just some additional comments.

[TravisBuddy]

Travis tests were successful

Hey @mherwege,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[mherwege]

@Hilbrand Thank you again for your very quick review. I believe I have addressed your comments. Can you check? I did not change stop to a trigger channel. Please comment.

[TravisBuddy]

Travis tests were successful

Hey @mherwege,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[TravisBuddy]

Travis tests were successful

Hey @mherwege,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[TravisBuddy]

Travis tests were successful

Hey @mherwege,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[fwolter]

There are two checkstyle warnings left:

[WARNING] .binding.upnpcontrol\README.md:[39]
The line after a Markdown list must be empty.
[WARNING] .binding.upnpcontrol\README.md:[65]
The line after a Markdown list must be empty.

[mherwege]

There are two checkstyle warnings left:

[WARNING] .binding.upnpcontrol\README.md:[39]
The line after a Markdown list must be empty.
[WARNING] .binding.upnpcontrol\README.md:[65]
The line after a Markdown list must be empty.

@fwolter Not sure what to do with this. When I insert an empty line, it will create a new list entry. That is not the purpose. There are multiple sentences on each list entry, to be concatenated into one list entry. Guidline is to start each sentence on a new line. That's why I ignored the warning.

[fwolter]

@fwolter Not sure what to do with this. When I insert an empty line, it will create a new list entry. That is not the purpose. There are multiple sentences on each list entry, to be concatenated into one list entry. Guidline is to start each sentence on a new line. That's why I ignored the warning.

Ok, that sounds reasonable.

[TravisBuddy]

Travis tests were successful

Hey @mherwege,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[mherwege]

@fwolter Many thanks for your review and help on this.
I believe I have addressed your review comments, but still would like you to carefully review synchronization in the binding. I did not make a change to the currentQueue synchronization. I am not sure it is required, but am happy for you to guide me on this if I don't understand it right. I did introduce some synchronization in other places. Can you check if I did that right?
Let me know what you think. I will run through a few more tests to make sure I didn't break anything in the mean time.

[TravisBuddy]

Travis tests were successful

Hey @mherwege,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[mherwege]

@fwolter I did test it a few more times and did not discover issues so far. Looking forward to your feedback.

[5iver]

@mherwege, I will do some more tests this weekend, but it appears that the states of the Items are not updating properly with this version of the binding. The logs seem to show the subscriptions are OK and being renewed. I'm specifically noticing it with the state of the player. Rolling back to an older version gets them working again.

[mherwege]

@openhab-5iver

I will do some more tests this weekend, but it appears that the states of the Items are not updating properly with this version of the binding. The logs seem to show the subscriptions are OK and being renewed. I'm specifically noticing it with the state of the player. Rolling back to an older version gets them working again.

Thanks for reporting. Is it all items? Can you get me a debug log? Any idea when the problem was introduced?

[5iver]

I've been testing the newer version since you submitted it, but I'd noticed my audio alerts were timing out, since the player Items were not reporting their state. I suspect it is all of the Items though. I plan to do some more testing and debugging this weekend.

[TravisBuddy]

Travis tests were successful

Hey @mherwege,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[TravisBuddy]

Travis tests were successful

Hey @mherwege,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[fwolter]

Looks good, but there are some formatting issues. You can view them with mvn spotless:check -Dspotless.check.skip=false and fix them with mvn spotless:apply.

[mherwege]

@fwolter Thank you. Formatting should be OK now.

[TravisBuddy]

Travis tests were successful

Hey @mherwege,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[mherwege]

@fwolter Thank you for your review.
@openhab-5iver Thank you for testing. I have continued working on this binding and added a number of things I will submit in a follow-up PR. I added a number of channels to the renderer, and volume and mute should now also properly update in openHAB when changed on the device. It is already in my github, a compiled version is here. I would be keen to hear your feedback before submitting the follow-up PR.

[5iver]

@mherwege, I've tested your updates and the volume works great. Changing the volume though OH works. Changing the volume in another control point or manually on the speaker reflects the new state in OH.

The mute is a little odd though. When it turns on, it immediately turns off, but the message is sent through UPnP and the speaker mutes. You need to turn it on again to be able to turn it off. Possibly an issue with autoupdate?

2020-08-14 18:48:14.742 [INFO ] [smarthome.event.ItemCommandEvent] - Item 'DS_FamilyRoom_Speaker_Mute' received command ON
2020-08-14 18:48:14.744 [INFO ] [smarthome.event.ItemStatePredictedEvent] - DS_FamilyRoom_Speaker_Mute predicted to become ON
2020-08-14 18:48:14.748 [INFO ] [smarthome.event.ItemStateChangedEvent] - DS_FamilyRoom_Speaker_Mute changed from OFF to ON
2020-08-14 18:48:14.886 [INFO ] [smarthome.event.ItemStateChangedEvent] - DS_FamilyRoom_Speaker_Mute changed from ON to OFF

2020-08-14 18:48:14.749 [DEBUG] [org.openhab.binding.upnpcontrol.internal.handler.UpnpRendererHandler] - Handle command ON for channel upnpcontrol:upnprenderer:5f9ec1b3-ed59-1900-4530-0007f522dcaf:mute on renderer Family Room
2020-08-14 18:48:14.757 [DEBUG] [org.openhab.binding.upnpcontrol.internal.handler.UpnpHandler] - Upnp device Family Room invoke upnp action SetMute on service RenderingControl with inputs {InstanceID=0, Channel=Master, DesiredMute=1}
2020-08-14 18:48:14.758 [DEBUG] [org.openhab.binding.upnpcontrol.internal.handler.UpnpHandler] - Upnp device Family Room invoke upnp action SetMute on service RenderingControl reply {}
2020-08-14 18:48:14.884 [DEBUG] [org.openhab.binding.upnpcontrol.internal.handler.UpnpRendererHandler] - Upnp device Family Room received variable LastChange with value <Event xmlns="urn:schemas-upnp-org:metadata-1-0/RCS/">
  <InstanceID val="0">
    <Mute channel="Master" val="1"/>
  </InstanceID>
</Event> from service RenderingControl
2020-08-14 18:48:14.885 [DEBUG] [org.openhab.binding.upnpcontrol.internal.handler.UpnpRendererHandler] - Upnp device Family Room received variable InstanceID with value 0 from service RenderingControl
2020-08-14 18:48:14.885 [DEBUG] [org.openhab.binding.upnpcontrol.internal.handler.UpnpRendererHandler] - Upnp device Family Room received variable MuteMaster with value 1 from service RenderingControl

I see Channels for Volume, LF Volume, and RF Volume. Same for Mute. What are these for? Main, left, and right stereo channels? None of these Channels seemed to change anything on my speakers. A more descriptive name would be helpful.

[mherwege]

@openhab-5iver The error with mute should be fixed in my branch.
LF stands for left front, RF stands for right front. These are used in stereo for the left and right channels, but also other setups. In that case more channels for center, left and right back, bass etc. That's why I sticked to the lfvolume and rfvolume naming, matching the upnp channel naming used internally. I might add other channels in the future. leftfrontvolume just looked too long. The channel descriptons and readme contain the full text.

[5iver]

The channel descriptons and readme contain the full text.

This was my concern... all I saw for the description was LF/RF Volume.

The error with mute should be fixed in my branch.

Excellent. I'll try out the latest. I was also noticing this with Stop. I just spent a few hours learning to Browse and Search. I almost have it working, but I must be doing something wrong, since it always seems to take two tries before a playlist will update, even with long sleeps separating the steps. I'll do more testing and provide more details later.

[mherwege]

This was my concern... all I saw for the description was LF/RF Volume.

The label is abreviated, the description in the channel type definition is not.

I was also noticing this with Stop.

That's a different thing. Stop is not a state, only a command channel. Therefore, you can only give the stop command and the player control channel should then go to pause mode. For that reason, I veto autoupdate for this channel, which means it will pass on the stop command to the renderer, but not update the UI. In the UI, it will act like a pushbutton, see the readme for a sitemap example.

I just spent a few hours learning to Browse and Search. I almost have it working, but I must be doing something wrong, since it always seems to take two tries before a playlist will update, even with long sleeps separating the steps.

I have always tested this from a UI (PaperUI or BasicUI). While you have to refresh the browser each time you change a value (the DynamicState and DynamicCommand Descriptions don't get updated otherwise), this worked for me. However, the thing about a UI is that you wait until all data has been refreshed. Doing it from rules may indeed create timing issues here.

The binding essentially never waits for a response and returns immediately after giving a upnp command. The response is handled as they come in. But that may create issues if a new command is dependent on the previous response. I have solved that in a few instances by introducing some futures, and may have to do the same thing when browsing or searching (i.e. don't browse or search when the previous browse or search didn't return a result yet). I could make everything synchronuous, but I fear this will keep the handlers tight up for too long and make the whole thing less responsive. Using some futures will insert some waits, but only when absolutely required. See how far you get without this before me introducing that.