ByteDev / ByteDev.Sonos

Set of classes and tools to help control Sonos devices.
MIT License
19 stars 5 forks source link

AddTrackToQueue throwing invalidOperationException #1

Open georgeinva2004 opened 5 years ago

georgeinva2004 commented 5 years ago

Very nice package! Thanks for putting it together. I'm trying to queue up an item from Sonos favorites, but the call to AddTrackToQueueAsync returns System.InvalidOperationException: 'Sequence contains no elements'. After stepping through the code with a debugger, it looks like the call to InvokeFuncWithResultsAsync is returning an error 402. I'm using the defaults for desiredFirsttrackNumberEnqueued, and enqueueAsNext, this URL: x-sonosapi-hls:r%3athepulse?sid=37&flags=8480&sn=1

Interestingly, when I execute the same action with the same parameters through Intel's Device Spy, the invocation completes successfully, and the media item is queued.

I'd appreciate any help you can provide.

ByteDev commented 5 years ago

402 is Payment Required no? Are you adding a track from Spotify or somewhere online? Or are you trying to add a track that is on your network/NAS?

The AddTrackToQueueAsync method is definitely a bit wobbly :-). If I remember correctly I have only ever got it working with files that were on my NAS.

Could you provide a snipit of the line of code when you make the call as well. Thanks.

I'll take a look into this more next week. Glad you are getting some benefit from it :-)

georgeinva2004 commented 5 years ago

The UPnP docs say 402 is invalid argument. The URI comes from my favorites and is a Sirius XM channel. FYI, giving it a URI from a network share works flawlessly, thank-you!

Here's my call to the function: await _avt.AddTrackToQueueAsync(gSonosMedia.Res, 0, true);

gSonosMedia.Res is a string containing: x-sonosapi-hls:r%3athepulse?sid=37&flags=8480&sn=1

Here's where the exception occurs: `public async Task AddTrackToQueueAsync(string enqueuedUri, int desiredFirstTrackNumberEnqueued = 0, bool enqueueAsNext = false) { if(desiredFirstTrackNumberEnqueued < 0) throw new ArgumentException("DesiredFirstTrackNumberEnqueued must either be zero (place at end of queue) or one or more (position in queue).", nameof(desiredFirstTrackNumberEnqueued));

        // enqueueAsNext = Whether this URI should be played as the next track in shuffle mode. This only works if PlayMode = SHUFFLE

        var enqueuedUriMetaData = "";

        var xml = await _upnpClient.InvokeFuncWithResultAsync("AddURIToQueue", new List<UpnpArgument>
        {
            new UpnpArgument("EnqueuedURI", enqueuedUri),
            new UpnpArgument("EnqueuedURIMetaData", enqueuedUriMetaData),
            new UpnpArgument("DesiredFirstTrackNumberEnqueued", desiredFirstTrackNumberEnqueued),
            new UpnpArgument("EnqueueAsNext", enqueueAsNext.ToInt())
        });

        **return new AddUriToQueueResponseFactory().CreateFor(ActionXNamespace, xml);**
    }`

Again, providing the same URI to Intel Device Spy, and invoking the action works. Unfortunately, I can't really see the SOAP call being made by Device Spy to compare it against the SOAP call your library is making. Do you think it could be related to escaping the URI? I did try passing an XML "escaped" URI as a test, but got the same issue. BUT, I'm not really following how the library is handling escaping down in it's guts.

Thanks again, for this great library.

ByteDev commented 5 years ago

Are you saying the following line throws the exception?:

return new AddUriToQueueResponseFactory().CreateFor(ActionXNamespace, xml);

Could you provide the full exception? i.e. the stack trace/ToString() of the exception thrown. Thanks :-).

This is a little hard for me to debug as obviously I will never have a file at location:

x-sonosapi-hls:r%3athepulse?sid=37&flags=8480&sn=1

If AddUriToQueueResponseFactory.CreateFor is the problem maybe worth running the code and stepping through the method to see what exactly the problem is. I'm probably making an assumption about what will be returned in the response, assuming something will be there and it isn't. What is the raw XML response you get back? If you need an easy way to run that method and debug just modify and run of the integration tests in AvTransportServiceTest.AddTrackToQueueAsync.

I would be very keen to see what is going on. I've struggled a little in some places with adding items to the queue, as you can problem see by looking at the IntTests for AvTransportServiceTest.AddTrackToQueueAsync :-).

georgeinva2004 commented 5 years ago

That is the line that throws the exception. It exception itself says "Sequence contains no elements". When I examine the Data property I see: Enumeration yielded no results.

Here's the stack trace:

at System.Linq.Enumerable.First[TSource](IEnumerable1 source) at ByteDev.Sonos.Upnp.Services.Factories.AddUriToQueueResponseFactory.CreateFor(XNamespace actionXNamespace, String xml) in D:\Users\georgejm\source\repos\ByteDev.Sonos\src\ByteDev.Sonos.Upnp\Services\Factories\AddUriToQueueResponseFactory.cs:line 27 at ByteDev.Sonos.Upnp.Services.AvTransportService.d__16.MoveNext() in D:\Users\georgejm\source\repos\ByteDev.Sonos\src\ByteDev.Sonos.Upnp\Services\AvTransportService.cs:line 128 at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at System.Runtime.CompilerServices.TaskAwaiter1.GetResult() at GeorgeHATester.Program.Main(String[] args) in D:\Users\georgejm\source\repos\ByteDev.Sonos\src\GeorgeHATester\Program.cs:line 29

The parameters passed to the CreateFor method are:

ActionXNamespace: urn:schemas-upnp-org:service:AVTransport:1 Xml: <s:Envelope xmlns:s="http://schemas.xmlsoap.org/soap/envelope/" s:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/"><s:Body><s:Fault><faultcode>s:Client</faultcode><faultstring>UPnPError</faultstring><detail><UPnPError xmlns="urn:schemas-upnp-org:control-1-0"><errorCode>402</errorCode></UPnPError></detail></s:Fault></s:Body></s:Envelope>

Tracing through, it looks like it's actually this statement in the CreateFor method that's generating the exception var responseNode = xElement.Descendants(actionXNamespace + "AddURIToQueueResponse").First();

I'm guessing that since the return from the Sonos was an error, it's not finding the XML element it wants in the response, so it's throwing the error. The real problem is likely is further upstream.

Stepping through UpnpRequestContentFactory.CreateFor, I see the following content string being returned from the CreateSoapMessage method: <?xml version="1.0" encoding="utf-8"?><s:Envelope xmlns:s="http://schemas.xmlsoap.org/soap/envelope/"><s:Body><u:AddURIToQueue xmlns:u="urn:schemas-upnp-org:service:AVTransport:1"><InstanceID>0</InstanceID><EnqueuedURI>x-sonosapi-hls:r%3athepulse?sid=37&flags=8480&sn=1</EnqueuedURI><EnqueuedURIMetaData></EnqueuedURIMetaData><DesiredFirstTrackNumberEnqueued>0</DesiredFirstTrackNumberEnqueued><EnqueueAsNext>0</EnqueueAsNext></u:AddURIToQueue></s:Body></s:Envelope>

I'm betting that the "EnquedURI" element needs to have the '&' characters escaped.

georgeinva2004 commented 5 years ago

That was it!!!!! In CreateSoapMessageBody, I changed two lines in the foreach loop to replace the ampersand character with the escaped version. I tried to put the actual code here, but the post didn't show the actual change (it thought I was escaping the string). I found a post which referred me to the XML Specification where this is all described.

https://stackoverflow.com/questions/6898259/when-is-it-required-to-escape-characters-in-xml

Typically, when I escape strings I use one of the built-in .net functions. In this case, I didn't want to introduce more variability and just dealt with the "&".

georgeinva2004 commented 5 years ago

Turns out that I needed to deal with more than just the "&". I took a few liberties and changed the CreateSoapMessageBody method to look like this:

`

private static string CreateSoapMessageBody(SoapAction sa, UpnpArgumentList list) { var body = new StringBuilder();

        body.Append($"<u:{sa.Action} xmlns:u=\"{sa.ActionNamespace}\">");

        foreach (var property in list.Arguments)
        {

            string tVal = System.Security.SecurityElement.Escape(property.Value.ToString());
            body.Append($"<{property.Name}>{tVal}</{property.Name}>");
        }

        body.Append($"</u:{sa.Action}>");

        return body.ToString();
    }`

Every argument is now escaped and things are working fine!

ByteDev commented 5 years ago

I've put up a new version (1.1.0) on nuget with your fix. Thanks for spotting that I wasn't escaping the soap argument values. I also added a simple AddQueueTrackAsync method to SonosController that at the moment just calls through to the AV transport.

Out of interest when you add the particular track you were talking about does it add the track and display the correct meta data in a Sonos controller app (on your phone, PC etc.)? When I add it it just comes up with the name as "x-sonosapi-hls:r%3athepulse?sid=37&flags=8480&sn=1" (and it doesnt play) obviously because I have no Sirius account with access.

georgeinva2004 commented 5 years ago

It does show the metadata (and artwork) properly.

FYI, I also extended the AVT service to include the SetAVTransportURI method. I can send you the code if you like, but it's a very short method modeled after your other methods.

Have you considered adding support for event subscriptions? At the moment, I poll media players for status. I was thinking it might be more efficient to just respond to events.

Finally, have you noticed a memory leak using the Intel UPnP library for device discovery. Memory use grows overtime until it simply consumes all available memory. I stopped using the intel library for SSDP because of that years ago. I currently use the RSSDP library for device discovery.

CobraCalle commented 4 years ago

Finally, have you noticed a memory leak using the Intel UPnP library for device discovery. Memory use grows overtime until it simply consumes all available memory. I stopped using the intel library for SSDP because of that years ago. I currently use the RSSDP library for device discovery.

I'have noticed that too... and back those days I switched to an other sonos nuget, but that hat only a very small feature set. Does the memory leak onyl show up when I use discovery? So when I just controll the speakers by creating a controller over the IP-address, there is no memory leak?

CobraCalle commented 4 years ago

Currently I`m trying to add music from Spotify (Tracks an Playlists) to Sonos-Queue. But when I add a track to queue I can only see the provider uri in the sonos app, it does not show art and it does not show a playing position... is that correct?

Scenario:

  1. Add a Track from Spotify to Sonos
  2. Read the queue with the API (track is for example: "x-sonos-spotify:spotify%3atrack%3a0GiwV6v3AgJfdu59tj719Y?sid=9&flags=8224&sn=8"
  3. Clear the queue with the API
  4. Use AddQueueTrackAsync to add the Spotify-Track (excat same uri as above)

Result: image

Am I doing something wrong?

georgeinva2004 commented 4 years ago

You're a little more ambitious than I've been. I only support MP3 files on my local LAN and Sonos Favorites in my controller. Both of those show the artwork (and play position for the MP3). Have you tried using UPnP Device Spy to see if there's any metadata required when the Sonos app itself tries to play the track?

On your question regarding UPnP memory leak. I was having that issue when I was using the Intel UPnP library and tools to build a generic UPnP controller. I traced it back to discovery. However, using RSSDP to do discovery solved that problem. I no longer have a memory leak.

CobraCalle commented 4 years ago

You're a little more ambitious than I've been. I only support MP3 files on my local LAN and Sonos Favorites in my controller. Both of those show the artwork (and play position for the MP3). Have you tried using UPnP Device Spy to see if there's any metadata required when the Sonos app itself tries to play the track?

On your question regarding UPnP memory leak. I was having that issue when I was using the Intel UPnP library and tools to build a generic UPnP controller. I traced it back to discovery. However, using RSSDP to do discovery solved that problem. I no longer have a memory leak.

Cool... thanks a lot... at the moment I have the problem, that I cannot connect to multiple spotify accounts at the same time... as soon as this is solved, I'll try to port over my Sonos-integration to this nuget and then I'll test it... I dont need discovery.... so fingers crossed :-)

ByteDev commented 4 years ago

Does the memory leak onyl show up when I use discovery? So when I just controll the speakers by creating a controller over the IP-address, there is no memory leak?

Does the memory leak onyl show up when I use discovery? Yes, only ByteDev.Sonos.Upnp.dll uses that upnp library.

So when I just controll the speakers by creating a controller over the IP-address, there is no memory leak? Correct.

ByteDev commented 4 years ago

RSSDP

You're a little more ambitious than I've been. I only support MP3 files on my local LAN and Sonos Favorites in my controller. Both of those show the artwork (and play position for the MP3). Have you tried using UPnP Device Spy to see if there's any metadata required when the Sonos app itself tries to play the track?

On your question regarding UPnP memory leak. I was having that issue when I was using the Intel UPnP library and tools to build a generic UPnP controller. I traced it back to discovery. However, using RSSDP to do discovery solved that problem. I no longer have a memory leak.

RSSDP, are you talking about: https://www.nuget.org/packages/Rssdp/ ? :-)

georgeinva2004 commented 4 years ago

That's the one!!!!!!