Discover fails

[markus-orblom]

Following the README to setup a connection to the Heos control system I do:

const heos = require('heos-api')

heos.discoverAndCreateConnection() .then(() => console.log('Connection established! 🌈'))

This results in nothing happening. No output.

Digging a little deeper into the code it seems like the discover promise is not resolving. Neither of the 'response' or 'error' triggers are triggered following the client.search(searchTargetName).

Do you have any ideas on what the problem could be?

[juliuscc]

It has worked before (but not always). I will look into it this weekend. Thanks for bringing this to my attention!

The discover function is the root of 90% of the bugs with this tool so I would really like to fix that. I recently split discover and connect so that if you know the ip of the device you want to connect to you can just open a tcp-connection.

Is it possible for you to try to connect without using the discover function?

[markus-orblom]

I'll test that now.

[markus-orblom]

That completed successfully. Thanks for your help. As information for your troubleshooting, I have 5 Heos groups (containing either 1 or 2 speakers each). I dont know how (or if) it affects the UPnP SSDP library you are using is working.

[juliuscc]

I think I might have to write that part myself. It clearly has problems when trying to search for multiple devices, and the discover function should probably resolve with an array or something. Maybe it should not be a promise but a stream or possibly an event dispatcher.

[morblom]

In the majority of cases you are only interested in getting hold of one device and you dont really care which one it is since they communicate with each other. For this case you would just need to get the first positive response from the UPnP SSDP discovery.

For more complex systems there might be a need for selection of what specific device to connect to. In this case the discovery function should return either a promise array or a stream. An event dispatcher seems to me to be a bit overkill. Do you see a need for multiple listeners that will react to the completion of the device discovery?

[juliuscc]

Sorry for the long post. Thanks for all suggestions and I would love too keep the conservation going, so that this module actually corresponds to what developers need and want. Please tell me if you agree or disagree with anything, or if you want anything clarified or explained.

Multiple devices vs one

I agree that you often only need access to one device if you only want to turn on or off music, but when you want to configure groups or whatever you need access to multiple devices. If you want to create a desktop app in electron that has the same functionality as the mobile app this would be necessary for example.

Solving the bug

I only have one heos device and it works perfectly for me. I have trouble reproducing the issue. As it is now, if you can find your device by yourself everything works without any issues. Maybe there should be a warning in the Readme warning about the discovery method not working perfectly and that the work around right now is to bypass the discovery method and connect to an IP that you gather in another way. This is a temporary fix.

How discovery should work

Regarding the question about how the discovery method should work I do not see another solution than an event dispatcher working (if it should be possible to discover more than one device). I will discuss the three options and why I only think one of them works.

As a background, the way discovery works is that the client sends a UDP-broadcast asking all the heos devices to identify themself. When a heos device receives this message they return one or multiple UDP messages letting the client (me) know that they exist. As UDP works these messages does not come together in a butch or anything. The client has to wait a while and it is impossible to know if there still exists a message on the way.

The three proposed solutions

1. Promise: If you make it a Promise with an array you could never know when to resolve the promise. Let's say you send out your request to make the devices identify themself and you after 1 second have gotten 3 answers. At that point you can not return an array with 3 devices as you do not know if there is a forth message that will come very soon. Of course you could resolve after finding the first one, but that would not let users discover all the devices on the network. A way to make promises work, which i strongly advice against doing, is to make the promise resolve after a time out and then present all devices found by then.
2. Stream: This would work like a stream that the client subscribes too which would give every device one after another till it stops. The problem with this is the same as with promises as that it is impossible to know when to stop.
3. Event dispatcher: This makes most sense to me. You would subscribe to an event dispatcher that dispatches every time a new device is found. If you only want to find one device you can stop your subscription after the first one. If you want to show all the available devices on a gui you can incrementally add every new device to the gui when they are found, and stop subscribing first when the user has interacted with the gui and possibly "chosen" a device to continue their interaction with.

Why event dispatcher

This solution has no problems with finding an end to the stream or promise of finding devices. This corresponds well with how the discovery system actually works. If someone prefers making a timeout promise or a readable stream it is not difficult to wrap the event mechanism with either of those, but still the event dispatcher offers the greatest flexibility. As a compromise there could exist a help function discoverOne() that returns a promise that resolves when the first device is found.

What I suggest

I suggest three things:

1. The readme gets a warning about using the discovery function before it is known if it works properly with multiple devices.
2. The discovery method gets rebuilt to work as an event dispatcher where you subscribe to devices being found. Of course it could be extended with a class that also keeps track of a list with all found devices as of yet, and so on.
3. A help function called discoverOne(), findOne() or something similar that is a promise that only finds one device. This would make it easier to work with the api if it is not important to choose wich device to work with. This would also be perfect if you only have one device in the system.

[markus-orblom]

I have spent some time understanding the underlying upnp ssdp library and now understand why we have this behavior.

The bug with discovery right now (ignoring that it needs to support multiple devices, etc.) is that it is not blocking the termination of the program. I cant find any devices because my script exits before the promise is resolved. See below for a short example. This ends before the promise is resolved. If I add the setTimeout line then it works since the timeout blocks the exit of the script.

new Promise((resolve, reject) => {
  client.on('response', function (headers, statusCode, rinfo) {
    console.log('resolving');
    resolve(rinfo);
  });
  // setTimeout(resolve, 10000);
}).then(() => console.log('completed'))

Regarding the additional behaviour of the discovery part, I agree with your post and suggestion.

If it is ok with you I would like to start contributing to this repo.

[juliuscc]

Yes I would love your contribution. Please create a pull request with any changes you suggest.

Do you mean that if i run discover().then(console.log) that the program terminates before letting the discovery promise resolve (or reject)?

How should this be solved? Is this something that is the responsibility of this module or the users of the module?

Maybe adding a very large timeOut could solve the issue. Also when the timeOut is reached and no devices are found, maybe the promise could reject.

[markus-orblom]

Yes,

const heos = require('heos-api') heos.discover().then(() => console.log('Connection established!'))

Exits before any response is received.

In the simplest case, a script with just:

new Promise(resolve => { }).then(() => console.log('test'))

will not wait for the promise to resolve before exiting. I do not know why node does this, but I guess a simple promise do not count as a blocking operation.

I think this module should be responsible for handling this. The discovery method could have a customizable timeout. I think that would work well, and as you say reject the the promise if no devices are found. The discovery method should close down the upnp ssdp listener when it either rejects or resolves the promise.

[juliuscc]

I think that sounds as a great idea. One extra thing. The timeout function argument should be able to be omitted, and then the promise should never resolve before at least one device is found. This could easily be done like this:

const keepAlive = timeOut
    ? setTimeout(timeOut, reject)
    : setInterval(timeOut, () => {})

[markus-orblom]

I think it is good to have to option to wait indefinitely for a response, but should that really be the default? I think it would be better if the timeout would have a default value of something sensible, e.g., 5 s and if you specify a negative timeout it will then wait indefinitely.

[juliuscc]

Sure that could work. I think 5 seconds is a good default for this.

[markus-orblom]

I would appreciate it if I could get write access to simplify the pull request procedure, if that is ok with you.

[juliuscc]

I would feel more comfortable if you would create a pull request. I can assist you in the steps.

1. You fork this repo by pressing the Fork button in the top of the page
2. You make the changes in your fork of this repo
3. You create a pull request from your repo to this repo

If you are new to git or github I would recommend using a GUI like GitKraken

Please let me know if anything is unclear or if you require further assistance 😄

[markus-orblom]

Ok, no problem :). It is simple enough.

[markus-orblom]

I have built a solution based on what we discussed and created a pull request. Let me know if you have any comments. The commit message should be quite descriptive.