ianr0bkny / go-sonos

A Go-language library for accessing UPnP AV devices
Other
87 stars 23 forks source link

Locking up during discovery #4

Closed wolfeidau closed 9 years ago

wolfeidau commented 9 years ago

Heya just trying out this library and it seems to freezing during discovery for some reason.

Anyone else had this issue?

I have this line in my code but it never returns.

        unit := sonos.Connect(player, reactor, sonos.SVC_CONTENT_DIRECTORY|sonos.SVC_AV_TRANSPORT|sonos.SVC_RENDERING_CONTROL)
ianr0bkny commented 9 years ago

Hi Mark. I haven't seen this happen. Could you please post more of the code from your example? if you've reached this state then discovery should be complete. Thanks.

--Ian

wolfeidau commented 9 years ago

Heya Ian

I made the following change to resolve the issue, we are testing it now.

https://github.com/ninjasphere/go-sonos/commit/2b5cd7e51277c4ff8b1f11b2c23ae2092f89f0a0

The issue with zero length channels is that is they block writes, so if you assume things will execute in a particular order then you can get your self tied up in knots.

I have a feeling that the web requests your executing are resolving in different orders for some reason.

Will do a pull request for you to test as well.

Thanks!

ianr0bkny commented 9 years ago

Thanks, Mark. I'll take a look.

--Ian

wolfeidau commented 9 years ago

Yeah I haven't dug deep enough to understand the root cause at this stage.

So this is by no means a great fix, hoping you could give me some hints or insight so I can help along the way.

There are quite a few small things i noted so far with the code base, just around readers and parsing which I would like to sort out as well.

Hopefully we can help out to some degree!

ianr0bkny commented 9 years ago

Hi Mark,

I've done some testing and I want to be sure that your code is listening for events prior to calling Connect. Please see the code below, which I've added as a test case in sonos_test.go. If you're not listening for events first, I agree that Connect may never return.

The other alternative would be to dispense with the reactor entirely and pass a nil pointer to the Connect method if you're not interested in UPnP eventing.

Hope this helps.

--Ian

func read_events(c chan upnp.Event) {
        for {
                select {
                case <-c:
                }
        }
}

func TestIssue_4(t *testing.T) {
        log.SetFlags(log.Ltime | log.Lshortfile)
        log.Printf("Discovery: Starting")
        mgr, err := sonos.Discover("eth0", "13104")
        if nil != err {
                panic(err)
        }
        log.Printf("Discovery: Done; Reactor: Starting")
        reactor := sonos.MakeReactor("eth0", "13106")
        go read_events(reactor.Channel()) ///// <------------
        log.Printf("Reactor: Running; Query: Starting")
        qry := ssdp.ServiceQueryTerms{
                ssdp.ServiceKey(sonos.MUSIC_SERVICES): -1,
        }
        res := mgr.QueryServices(qry)
        log.Printf("Query: Done; Connect: Starting")
        if dev_list, has := res[sonos.MUSIC_SERVICES]; has {
                for _, dev := range dev_list {
                        if sonos.SONOS == dev.Product() {
                                if _, err := upnp.Describe(dev.Location()); nil != err {
                                        panic(err)
                                } else {
                                        sonos.Connect(dev, reactor, sonos.SVC_ALL)
                                        log.Printf("Connect: Done")
                                        break
                                }
                        }
                }
        }
}
wolfeidau commented 9 years ago

Code is roughly as follows.

    nlog.Infof("loading reactor")
    reactor := sonos.MakeReactor(NetworkInterface, EventingPort)

    zonePlayers, err := detectZP()
    if err != nil {
        nlog.HandleError(err, "Error detecting Sonos ZonePlayers")
    }

    nlog.Infof("Detected %d Sonos ZonePlayers", len(zonePlayers))

    for zone, player := range zonePlayers {
        nlog.Infof(spew.Sprintf("Found %s %v", zone, player))

        unit := sonos.Connect(player, reactor, sonos.SVC_RENDERING_CONTROL|sonos.SVC_AV_TRANSPORT|sonos.SVC_ZONE_GROUP_TOPOLOGY|sonos.SVC_MUSIC_SERVICES)

        dev, err := NewPlayer(bus, unit)
        if err != nil {
            nlog.HandleError(err, "failed to register media player")
        }

        nlog.Infof(spew.Sprintf("created media player device %v", dev))
    }

With the detectZP function as follows:

func detectZP() (zonePlayers ssdp.DeviceMap, err error) {

    nlog.Infof("loading discovery mgr")
    mgr, err := sonos.Discover(NetworkInterface, DiscoveryPort)
    if nil != err {
        return
    }

    zonePlayers = make(ssdp.DeviceMap)
    for uuid, device := range mgr.Devices() {
        if device.Product() == "Sonos" && device.Name() == "ZonePlayer" {
            zonePlayers[uuid] = device
        }
    }
    return
}
ianr0bkny commented 9 years ago

Yes. You need to be reading from reactor.Channel before calling Connect. This is the cause of the blocking behavior. I'll add documentation to this effect. Thanks.