[SonyAudio]Sony Audio Control API #4838
Conversation
|
David (@freke), I hope you don't mind if I comment on this - I've written a whole sony product line binding (TVs, AVRs, soundbars, blurays) for openHAB that pretty much already covers what this does plus alot more (I have a whole dynamic scalarwebapi implementation, ircc, dial and simpleip implementations). All my work has been based on figuring out the APIs from wireshark and observances (can't believe we finally have some API doc)! I'd love to do some collaboration with you on the whole binding since, it appears, you are an actual sony developer! I'm definitely going to look through the documentation/your code and see if/where I'm missing things. Feel free to contact me for additional information Tim |
|
Hi Tim (@tmrobert8), I have had some look at your work at the OpenHAB forum/github for a while (since we started to work on releasing this), I'm still on holiday but was planning to comment at the OpenHAB forum to let you know about the API if you missed it, when I was back. I will happily answers all the questions I can, if I can't I have contacts with the Audio engineers in Tokyo. The reason for this pull request is, that if I understand the setup correctly "eclipse smarthome" is for only officially released API:s therefore this this binding is only for the products/API's that have got clearance from SONY. OpenHAB is for then you are going in to the gray area ;-) If I have misunderstood feel free to correct me. David Åberg |
|
David (@freke), I'll let @kaikreuzer or someone else from the project respond to your question about separation between the projects (and what goes where) - that's above my pay grade ;) However, depending on the answer, I'd be happy to 'upgrade' the plugin code to ESH and do some team development on it with you or whomever. I'm currently in the process of making changes so that it can be merged properly - so I'm close to the review time. Enjoy your holiday and when you get back (no hurry on this) - could you comment on the following... I (quickly) looked through the API and think I got most of it covered (atleast to what I wanted to support) - although the websocket part was all new and I'll be integrating that in shortly. I do have a few questions:
Just wanted to comment that this is very exciting - I believe the openHAB plugin is the most advanced and handles the widest range of sony products of any other plugin out there and would love to have some QA time with developers 'in the know'! BTW - you mentioned the camera API as well. I'd love to hear any ideas about integrating that API - I don't really know the use cases well enough to venture into it (I do have some sony cameras and video records - not sure if they are supported however). THanks, |
|
David (@freke) Verrrryyyy cool - the websocket connection doesn't need authentication! I just modified your code to support my HT-NT5 and it came online and is controllable just fine. I'll definitely be adding that back into my code! |
|
Hi Tim,
You might find this SDK a good reading https://developer.sony.com/develop/cameras/ even if your device is not supported :-) Best Regardes |
|
David, Thanks for the answers - if when you were looking at my code you saw any issues and are open to discussing them, I'm all ears.. Tim |
|
Welcome @freke at ESH, it is awesome to see official support and documentation for Sony Audio products!
Let me clarify/precise this a bit: Any code that is used (or of interest to be used) in a commercial product that is based on Eclipse SmartHome should be part of this project. As anyone that builds a commercial product wants to be sure that device integrations run stable and correct, officially documented APIs and common protocols are clearly the much preferred way here. Having said this doesn't mean that code on the openHAB side is automatically in a "gray area". Many bindings are simply put there because they are only interesting for openHAB and not for any other ESH-based solution, so we do not want to spend the time for the more rigid IP checks at Eclipse. Also, code might be based on reverse-engineering of protocols (such as @tmrobert8's work), which afaik isn't a legal issue as long as it does not break any encryption, but which obviously runs the risk to easily break by any new firmware update of a device. So how shall we best proceed here? Cheers, |
|
Thanks Kai, That sounds like a good idee @kaikreuzer I like the idea of having a ESH biding that is expandable in to the OpenHAB domain, to avoid duplicated functionality/bugs and would like the knowledge of @tmrobert8 do this. And thanks for the clarification, I think I have understood this correctly, "gray area" might have been the wrong term, was not implying that OpenHAB was doing anything illegal, just that it is using things that don't have a clear usages agreement, that can be unsertant for commercial use. Best, |
|
The approach sounds good to me but just for my understanding @kaikreuzer - are you saying that openHAB bindings can reference/extend ESH bindings (as opposed to openHAB bindings cannot reference other openHAB bindings)? If that's the case, then I can certainly recommend some changes that would allow me to reuse some of the code (definitely the websocket connection and we could move some of the json processing/classes from mine up to his). |
@tmrobert8, Almost, but not quite. What I am saying is that a binding can consist out of multiple OSGi bundles, which could reside anywhere (in ESH, OH2 or whereever else). Although this concept has been conceived from the beginning, it was actually never used so far. Note that this only applies at all the code parts that contain thing types/handlers etc and thus belong to the same binding. As we already discussed in the past, another option is the introduction of an io.transport bundle, which could contain code that you might want to share across multiple bindings (e.g. a "Sony Audio" and a "Sony TV" binding). It would simply be a kind of shared library as a common dependency, but it would not bring itself any user facing functionality. |
|
Have rebased and updated the pull request with the two new soundbars SONY is now supported by the Audio Control API. |
There was a problem hiding this comment.
Thanks for the update, @freke, that looks already pretty good!
I left a few first comments regarding the thing xmls. I won't dive deeper into the code yet, but rather wait for your signal that you do not consider it WIP anymore, but think it is getting towards completion. Thanks!
| xsi:schemaLocation="http://eclipse.org/smarthome/schemas/binding/v1.0.0 http://eclipse.org/smarthome/schemas/binding-1.0.0.xsd"> | ||
|
|
||
| <name>SonyAudio Binding</name> | ||
| <description>This is the binding for SonyAudio products (Recievers and wirelesspeakers).</description> |
There was a problem hiding this comment.
Typo: Recievers -> Receivers
I see this more often throughout the code, please fix it at all places.
| <description>The IP or host name of the SONY audio device</description> | ||
| <context>network-address</context> | ||
| </parameter> | ||
| <parameter name="port" type="integer" min="1" max="65335" required="true"> |
There was a problem hiding this comment.
It should not be required as you define a default value.
| <description>Port of the SONY audio device to control</description> | ||
| <default>10000</default> | ||
| </parameter> | ||
| <parameter name="path" type="text" required="true"> |
There was a problem hiding this comment.
It should not be required as you define a default value.
| </parameter> | ||
| <parameter name="path" type="text" required="true"> | ||
| <label>Base path</label> | ||
| <description>Base path to the SONY audio device to control</description> |
There was a problem hiding this comment.
To me it is really not clear, what this path is supposed to be and what is expected from me as a user when manually configuring it.
| @@ -0,0 +1,13 @@ | |||
| # FIXME: please substitute the xx_XX with a proper locale, ie. de_DE | |||
There was a problem hiding this comment.
please remove this file
|
|
||
| <!-- HT-CT800 Thing Type --> | ||
| <thing-type id="HT-CT800"> | ||
| <label>SONY soundbar HT-CT800</label> |
|
|
||
| <!-- HT-MT500 Thing Type --> | ||
| <thing-type id="HT-MT500"> | ||
| <label>SONY soundbar HT-MT500</label> |
| <channel-type id="radioBroadcastFreq"> | ||
| <item-type>Number</item-type> | ||
| <label>Broadcast frequency</label> | ||
| <description>Select the broadcast frequency</description> |
There was a problem hiding this comment.
If this is for "selecting", why did you mark it as readOnly (which means that you cannot send commands to it)?
| @@ -0,0 +1,21 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
Could you switch to using DS annotations? (check other bindings or the latest archetype on how that looks like).
| @@ -0,0 +1,99 @@ | |||
| /** | |||
| * Copyright (c) 2014,2017 Contributors to the Eclipse Foundation | |||
There was a problem hiding this comment.
Could you re-run mvn license:format to update the year in all headers?
|
@kaikreuzer Last build is it my fault it failed or is it travis that had a hick up? |
|
Doesn't look like your fault, I've just retriggered Travis. |
|
I don't think this build fail was with the sonyaudio plugin looks like it was the paperui to me. |
|
Thanks @freke! I haven't yet found the time to have another detailed look, but if you feel the code is ready, please remove the WIP at any time - this is then anyhow the best signal that you wish it to be reviewed and merged! |
|
Hi, thanks first the initial work and i happy to have my Sony Player supportet next :-)
I think special the last point (notification) makes the player a step closer to a smarthome use case at all. Thanks for support Frank |
|
Hi @FrankZimmer
At the moment getting something out that works (even if limited) is better than nothing, in my oppinion. Adding the player functionality and audiosink later if it is seen as a good idee is always possible. David |
freke
changed the title
There was a problem hiding this comment.
Hey @freke, sorry for taking so long with coming back to you!
I agree that this binding makes sense even with the limited set of functionality (and rather hope for some proper DLNA support by another binding), so let's go ahead with this.
The code looks pretty good already and I mainly think it requires some hardening for exception and error handling - please see my comments below and let me know, if you have any questions. Thanks!
| xsi:schemaLocation="http://eclipse.org/smarthome/schemas/binding/v1.0.0 http://eclipse.org/smarthome/schemas/binding-1.0.0.xsd"> | ||
|
|
||
| <name>SonyAudio Binding</name> | ||
| <description>This is the binding for SonyAudio products (Receivers and wirelesspeakers).</description> |
There was a problem hiding this comment.
typo: wirelesspeakers -> wireless speakers
| </parameter> | ||
| <parameter name="port" type="integer" min="1" max="65335"> | ||
| <label>Port</label> | ||
| <description>Port part from X_ScalarWebAPI_BaseURL for the SONY audio device to control. |
There was a problem hiding this comment.
I'd remove the "Port part from X_ScalarWebAPI_BaseURL for the SONY audio device to control." from the description as this sounds rather technical for the average user.
| </parameter> | ||
| <parameter name="path" type="text"> | ||
| <label>Base path</label> | ||
| <description>The path part from the X_ScalarWebAPI_BaseURL for the SONY audio device to control, "/sony" in most cases</description> |
There was a problem hiding this comment.
also better remove "part from the X_ScalarWebAPI_BaseURL" here
| <channel-type id="input-dn1080-zone2"> | ||
| <item-type>String</item-type> | ||
| <label>Input Source</label> | ||
| <description>Select the input source of the reciever</description> |
There was a problem hiding this comment.
typo: reciever -> receiver
| <channel-type id="input-dn1080-zone4"> | ||
| <item-type>String</item-type> | ||
| <label>Input Source</label> | ||
| <description>Select the input source of the reciever</description> |
There was a problem hiding this comment.
typo: reciever -> receiver
| try { | ||
| socket.close(); | ||
| } catch (Exception e) { | ||
| } |
There was a problem hiding this comment.
better at least add a debug logging here
| try { | ||
| socket.open(); | ||
| return socket.isConnected(); | ||
| } catch (Throwable t) { |
There was a problem hiding this comment.
Never catch Throwable, only more specific exceptions.
| socket.open(); | ||
| return socket.isConnected(); | ||
| } catch (Throwable t) { | ||
| logger.error("exception during connect to {}", socket.getURI().toString(), t); |
There was a problem hiding this comment.
do not log errors unless it's a software bug and remove toString() from parameters.
| return item.get("currentValue").getAsString().equalsIgnoreCase("on") ? true : false; | ||
| } | ||
| } | ||
| throw new IOException("Unexpected responces"); |
There was a problem hiding this comment.
typo: responces -> responses
| try { | ||
| List<Channel> channels = getThing().getChannels(); | ||
| for (Channel channel : channels) { | ||
| handleCommand(channel.getUID(), RefreshType.REFRESH); |
There was a problem hiding this comment.
This looks pretty inefficient for those reasons:
- You are doing calls for channels that aren't linked at all - that's a waste of time. Better use the
callback.isChannelLinkedmethod. - This can cause a huge number of external requests; is the connection able to easily handle this load? Afaics, some requests contain data for multiple channels, so that the same requests are done multiple times. If my assumption is true, you might want to consider using the
ExpiringCachesee here.
|
@freke Something just went pretty wrong when you pushed your branch. |
|
@kaikreuzer sorry don't know what happed I was only planing to do a rebase, and something went very wrong :-/ will try to fixit Re added the [WIP] for now... |
freke
changed the title
|
@freke Thanks for the cleanup, looks much better again 👍 . |
There was a problem hiding this comment.
Hi @freke,
I just tested the binding with an HT-ST5000 and it works pretty smooth!
I found a few small further issues, see comments below.
One bigger issue that I noticed is that once the connection is created, I see around 30 (yes, thirty!) threads of WebSocketClient. I know that Jetty uses defaults that are rather meant for servers than for embedded hardware, so we might need to tweak the configuration of that client. What's more is that these threads keep running even if I stop the bundle completely - so there seems to be something wrong with the cleanup in the handler disposal. Could you please check?
Please let me know, if there's anything I can help you with to get things in a mergeable state. Thanks!
| Bundle-Version: 0.10.0.qualifier | ||
| Export-Package: org.eclipse.smarthome.binding.sonyaudio, | ||
| org.eclipse.smarthome.binding.sonyaudio.handler | ||
| Import-Package: com.google.common.collect, |
There was a problem hiding this comment.
We have meanwhile decided to deprecate the use of Guava, so it would be nice if you could remove this dependency (should be simply as it is only used once for an ImmutableSet).
|
|
||
| public void handleVolumeCommand(Command command, ChannelUID channelUID) throws IOException { | ||
| if (command instanceof RefreshType) { | ||
| updateState(channelUID, new DecimalType(connection.getVolume() / 100.0)); |
There was a problem hiding this comment.
Use PercentType instead of DecimalType and you do not need to divide by 100.
| if (command instanceof RefreshType) { | ||
| updateState(channelUID, new DecimalType(connection.getVolume() / 100.0)); | ||
| } | ||
| if (command instanceof DecimalType) { |
There was a problem hiding this comment.
Check for PercentType instead of DecimalType
| updateState(channelUID, new DecimalType(connection.getVolume(zone) / 100.0)); | ||
| } | ||
| if (command instanceof DecimalType) { | ||
| connection.setVolume(((DecimalType) command).intValue(), zone); |
There was a problem hiding this comment.
Check for PercentType instead of DecimalType
|
|
||
| public void handleVolumeCommand(Command command, ChannelUID channelUID, int zone) throws IOException { | ||
| if (command instanceof RefreshType) { | ||
| updateState(channelUID, new DecimalType(connection.getVolume(zone) / 100.0)); |
There was a problem hiding this comment.
Use PercentType instead of DecimalType and you do not need to divide by 100.
| public void updateVolume(int zone, int volume) { | ||
| switch (zone) { | ||
| case 0: | ||
| updateState(SonyAudioBindingConstants.CHANNEL_VOLUME, new DecimalType(volume / 100.0)); |
There was a problem hiding this comment.
Use PercentType instead of DecimalType and you do not need to divide by 100 - same for other zones
| <description>Select the input source of the receiver</description> | ||
| <state> | ||
| <options> | ||
| <option value="btAudio">Bluetooth Audio</option> |
There was a problem hiding this comment.
This should be btaudio instead of btAudio - same for the other thing types.
|
Ouch, you've just messed up your branch again :-( |
|
Thanks for the updates! The HT-ZF9 is now nicely discovered. I have some issues with updates from the soundbar to the binding though; while power and input selection changes are correctly reported, I do not get updates of the volume or mute information (if changed on the soundbar itself; I can control volume and mute from the binding, though). Can you confirm this effect? While the exceptions during startup are gone, the Thing status updates aren't really "user compatible" now: As the devices are available on the network (in this case), I would not except an OFFLINE status at all, but the binding should actually establish the connection immediately. |
|
@kaikreuzer One question is the device powered off then you get this error message? |
|
Yes, powered off, but being configured to be activated through the network. |
|
I found why the HT-ZF9 only worked by pulling for volume and mute and not via notifications, just have to check so the change don't affected any other device. For the Offline message: EDIT: Think I have a theory my pulling interval is much shorter than default, so it might be the WebSocket timing out due to inactivity... I have for one device (STR-DN1080) that is setup via files (not via Paper UI, STR-DN1080 is the only one I have tested to setup in this way) that then it starts it will switch between UNINITIALIZED, INITIALIZING, ONLINE and OFFLINE a few times before settling down on ONLINE, don't know if this is a problem or not? |
A proposal: PR #6200 |
|
Thanks for the updates @freke, I can confirm that the events now work smoothly for the HT-ZF9! While testing, I made a few small changes to the code, which I have put into freke#3, which you can merge, if you are ok with them.
I have only seen this with the STR-DN1080 so far, all other go to ONLINE as expected, even if the device is turned off (but available on the network). Will have to do some further testing myself, if you cannot reproduce it. |
freke
force-pushed
the
sonyaudio
branch
2 times, most recently
from
3de0bc4
to
fbb6da3
Compare
@kaikreuzer I'm not 100% sure but I think I found the reason and I no longer any Errors or unexpected Offline/Online messages in my log, had it running all day today no issues. |
|
Hey @freke, some updates here:
#6200 has been merged, so you should be able to use this feature now.
I have implemented this yesterday and it has been merged today as #6258. Would be great if you'd adapt the code to make use of this new shared instance - let me know, if you have any questions on how to do that, I will certainly help! |
| private ScheduledFuture<?> refreshJob; | ||
|
|
||
| private int currentRadioStation = 0; | ||
| private Map<Integer, String> input_zone = new HashMap<>(); |
There was a problem hiding this comment.
You seem to usually program in a different language - input_zone is not really following Java naming conventions, it should be inputZone instead. Same for all other new fields you have introduced for the caching below.
| try { | ||
| SonyAudioConnection.SonyAudioInput result = input_cache[zone].getValue(); | ||
| if (result != null) { | ||
|
|
There was a problem hiding this comment.
please remove empty line
| try { | ||
| connection = new SonyAudioConnection(ipAddress, port, path, this); | ||
|
|
||
| connection.connect(scheduler); |
There was a problem hiding this comment.
Did you see this comment? Seems it isn't yet addressed?
| connection.connect(scheduler); | ||
|
|
||
| // Start the connection checker | ||
| Runnable connectionChecker = new Runnable() { |
There was a problem hiding this comment.
can you do that with lambdas like
Runnable connectionChecker = () -> {
...
| logger.debug("SrsZr5Handler handleSoundSettings RefreshType"); | ||
| Map<String, String> result = sound_settings_cache.getValue(); | ||
| if (result != null) { | ||
| logger.debug("SrsZr5Handler Updateing sound field to {} {}", result.get("clearAudio"), |
There was a problem hiding this comment.
typo: Updateing -> Updating
| @Override | ||
| public void handleEvent(JsonObject json) { | ||
| int zone = 0; | ||
| JsonObject param = json.get("params").getAsJsonArray().get(0).getAsJsonObject(); |
There was a problem hiding this comment.
Did you see this comment? I really think the code should be hardened here to make it robust wrt missing json data.
| SwitchNotifications switchNotifications = new SwitchNotifications(notifications.enabled, | ||
| notifications.disabled); | ||
| JsonElement switches = socket.callMethod(switchNotifications); | ||
| Gson mapper = new Gson(); |
There was a problem hiding this comment.
Did you see this comment?
|
Hi @kaikreuzer I have been trying to get the WebSocketFactory to work but I'm doing something wrong, see https://github.com/freke/smarthome/tree/common_websocket Then I try to get the WebSocketFactory via @referenc setWebSocketFactory in SonyAudioHandlerFactory. WebSocketFactory get added to the OSGI-INF/*.xml Doing some thing something wrong... just can't figure out what. Best Regards |
|
Hey @freke, sorry for my late reply this time, I have been on holiday for a week... I just checked your code and it looks perfectly fine. Analysing it, I noticed that it was a stupid mistake on my end - the WebSocketFactory service is actually not at all registered with that interface, sorry for that 🙄. #6361 should fix this, so once this is merged, please rebase your branch on it and all should be fine. |
|
Thanks @kaikreuzer, worked for me after update. |
|
Many thanks again, @freke! My impression is that the code is ready as an initial contribution, so the next step is to create a CQ (contribution questionnaire, I will take care of that, no worries) which means that the Eclipse IP team needs to approve the contribution. For this, we need it as a single commit, so may I ask you to squash all commits of this PR into a single one? That's the last thing for you to do right now! 😎 |
Signed-off-by: David AAberg <david.aberg@sony.com>
|
@kaikreuzer no problem, squashed :-) |
|
Perfect thanks! I've now created CQ 18054, so please refrain from doing any further changes on this branch. |
|
Yeah, the CQ has been approved and we are allowed to check-in! |
|
Thanks to you too @kaikreuzer, glad to help and assists as much as I can to @tmrobert8 openhab extension. |
Sony has just released their Audio Control API, this is a first implementation to make use this. And make a extension to control the officially supported products from Sony.
All help/comments to get this accepted would be appreciated.
More info about the Audio Control API can be found at https://developer.sony.com/develop/audio-control-api/
David Åberg
Sony Developer World
Signed-off-by: David AAberg david.aberg@sony.com