MylesMor / nanoleafapi

A Python3 wrapper for the Nanoleaf OpenAPI, for use with the Light Panels, Canvas and Shapes (Hexagons, Triangles and Elements).
https://nanoleafapi.readthedocs.io
MIT License
59 stars 15 forks source link

discover_devices edge case bug #4

Open Phuket2 opened 4 years ago

Phuket2 commented 4 years ago

I have been using nanoleafapi with 3 canvas sets. Today I got a Nanoleaf Rhythm edition set also. So I turn it on and could not pair it in the nano app or Homekit. Out of interest I wanted to see if nanoleafpapi would see it. So the discovery.discover_devices() started to fail.

_Traceback (most recent call last): , line 4, in nanoleaf_dict = discovery.discover_devices() , line 36, in discover_devices nanoleafdict[name] = ip UnboundLocalError: local variable 'ip' referenced before assignment

So I did a hard reset on the light panels and before adding them to the apps I tested discovery.discover_devices() and it was working fie. Did not see the light panels which is fine but did not error out. I know it's a out there edge case, but thought you would like to know.

Python 3.8.2 MacOS 10.15.4

Thanks for doing the work on nanoleafapi, its great to have and a lot of fun and works very well.

MylesMor commented 4 years ago

Hmm, thanks for the report, that is strange. I assumed if the code got that far there would always be an IP field in the response... I'll just add a quick check to ensure the IP variable has been assigned before adding it to the response dictionary.

I'm glad you're enjoying using this package!

Phuket2 commented 4 years ago

No problems. I wish I had been thinking more clearly. At the time I sort of realised/assumed that the lights had already been connected to another network. I was a little mad. But would have been great to seen the malformed string that generated the error. If somehow it ever happens again I will be sure to get more info.

MylesMor commented 4 years ago

If you'd like, with the update I'm planning to do tomorrow to fix this, I'll also add a debug argument to discover_devices which prints out the SSDP string in case you find a similar issue again?

Phuket2 commented 4 years ago

I found the problem. the Nanoleaf Light Panels are returning records that are included in your search (nanoleaves list) but the record format is very different. below is an example of the data.The problem is intermittent because if a canvas record makes it into the list before a light Panel record its ok, otherwise it fails. Hope that helps

Nanoleaf Light Panels (Record) 'HTTP/1.1 200 OK\r\n' 'HOST: 239.255.255.250:1900\r\n' 'EXT:\r\n' 'CACHE-CONTROL: max-age=100\r\n' 'LOCATION: http://192.168.86.43:80/description.xml\r\n' 'SERVER: Linux/3.14.0 UPnP/1.0 IpBridge/1.37.0\r\n' 'hue-bridgeid: 001788FFF**6606\r\n' 'ST: upnp:rootdevice\r\n' 'USN: uuid:2f402f80--11e1-9b23-001***d6606::upnp:rootdevice\r\n' '\r\n',

Nanoleaf Canvas (Record) 'HTTP/1.1 200 OK\r\n' 'S: uuid:c588db09-a654-4dba-b60e-68de3925\r\n' 'Ext: \r\n' 'Cache-Control: no-cache="Ext", max-age = 60\r\n' 'ST: nanoleaf:nl29\r\n' 'USN: uuid:c588db09-a654-4dba--68de39749a25\r\n' 'Location: http://192.168.86.64:16021\r\n' 'nl-deviceid: 4D:EC:F1:**:BC:0A\r\n' 'nl-devicename: Canvas 4A38\r\n' '\r\n',

MylesMor commented 4 years ago

Thanks for the further details! I've just updated the package with code that should recognise the Light Panels from the record you provided (although it will be unable to give a name, just the IP).

Unfortunately, I don't have access to my lights at the current time, so please try updating the package to 1.1.2 and let me know if the issue persists. If you'd like to print out the records, you can now add the debug argument to the function, like this: discover_devices(debug=True).

Again, thank you for the detailed information regarding this!

Phuket2 commented 4 years ago

Maybe you could help me with something. I really have no understanding of the socket work going on in the discover_devices func. I found the above problem again before I had seen your post because I was looking to see if I could get a quicker discover time. I hadn’t realised there was a timeout param :(. Anyway, after some testing I found that if I pass 1sec to the func I was getting timing of about 1.4-1.75 sec to complete with a correct dict returned. if i passed say .4 it started to get a little unpredictable empty or malformed dict. Given some networks would be faster/slower than others, once you got a baseline for a network would it remain fairly consistent for the network or is there complicated caching etc. going on that invalidates that idea. I was thinking to call discover_devices without a timeout. recover the dict to use as a baseline to what was discovered then in a loop try lower and lower timeouts to get a threshold of timings that would work reliably for that given network. Is this what people do already? Maybe its something you would like to add as a feature. Anyway, I know it’s a little off topic, but I would appreciate you feedback if you have time.

MylesMor commented 4 years ago

To be totally honest, I don't know a lot about it either, but I managed to figure it out to (mostly) work for that function.

From what I understand, it can take some time after a SSDP M-SEARCH packet is sent for the Nanoleaf to reply, which is why there is a timeout. In some cases, yes, it may be a lot quicker than 30 seconds, but in my limited testing it's not always guaranteed and I think it may be hard to set a baseline because of this.

As I said previously, unfortunately I don't have access to my lights to test this further currently, but I would recommend installing Wireshark and setting a filter for ssdp so you can easily see all the packets whilst you're running discover_devices(). Sorry that I'm not completely sure, but let me know your findings!

Phuket2 commented 4 years ago

Myles, Version 1.1.2 has a consistent error as below. The LOCATION string split fails because the format is different.
Would it not be better to ignore Light Panels all together as they are not valid records for the purpose of the api.

Traceback (most recent call last): File "/Users/user/SynologyDrive/startup/nl_startup.py", line 8, in x = discover_devices(debug=True) File "/Users/user/genv/nleaf/lib/python3.8/site-packages/nanoleafapi/discovery.py", line 38, in discover_devices ip_string = header.split("http://")[1] IndexError: list index out of range

MylesMor commented 4 years ago

Hm, I think I misunderstood what you meant in the previous one. Do the light panels have a record that is similar to that of the Canvas? Would ignoring that record stop the discovery of the Light Panels totally? Unfortunately, I never owned the Light Panels so I could not test with them :(

Phuket2 commented 4 years ago

Myles, Something like below would work for the time being. I don't understand the ssdp search string, but it seems like its looking to match 'ST: nanoleaf:nl29' in the record, but you can see from what I sent previously that string does not appear in the Panel Lights record, so not sure why that record comes back from your query in the first place. I have to disappear for a while. But I have downloaded + installed wire shark. I will see if I can understand it more later on.

    ip, name = '', ''
    for device in nanoleaves:
        if debug:
            print(device)
        headers = device.split('\r\n')
        ip = None
        name = None
        for header in headers:
            # if "Location" or "LOCATION" in header:
            if "Location"  in header:
                ip_string = header.split("http://")[1]
                ip = ip_string.split(":")[0]
            if "nl-devicename" in header:
                name = header.split("nl-devicename: ")[1]
        if ip is not None:
            nanoleaf_dict[name] = ip
    return nanoleaf_dict
MylesMor commented 4 years ago

Alright, I'll update it to what you suggested, for the time being, thanks for your help with this!

Phuket2 commented 4 years ago

Myles, when I come back look at this further, I didn't know much, I even know less now, well sort of :( I also have a hue hub on my network, I think that also is answering to the broadcast. In the spec about the ssdp:ST field they go out of their way to say how simple it is and designed to be. So that to me would seem to suggest that filtering device records is normal/required.

In my mind, the only change required is to filter the list of devices something like I have done below or some version of that.

btw, my light panels are model nl28 the canas being nl29, but I could not get search to return anything for nanoleaf:nl28. I tried various combos. So it stills seems their is something inconsistent with nanoleafs implementation.

Anyway, not sure if you came to the same conclusion or not. I went down a lot of rabbit holes.

import socket

def discover_devices(timeout=30):
    """
    Discovers Nanoleaf devices on the network using SSDP

    :param timeout: The timeout on the search in seconds (default 30)
    :returns: Dictionary of found devices in format {name: ip}
    """
    ssdp = "M-SEARCH * HTTP/1.1\r\nHOST: 239.255.255.250:1900\r\nMAN: \"ssdp:discover\"\r\nMX: 1\r\nST: nanoleaf:nl29\r\n\r\n"

    sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
    sock.settimeout(timeout)
    sock.sendto(ssdp.encode(), ("239.255.255.250", 1900))

    nanoleaves = []

    while True:
        try:
            data = sock.recv(1024).decode()
        except:
            break
        nanoleaves.append(data)

    nanoleaf_dict = {}

    # for device in nanoleaves:
    for device in [dev for dev in nanoleaves if 'nanoleaf:nl29' in dev]:
        headers = device.split('\r\n')
        for header in headers:
            if "Location" in header:
                ip = header.split("http://")[1][:-6]
            if "nl-devicename" in header:
                name = header.split("nl-devicename: ")[1]
        nanoleaf_dict[name] = ip
    return nanoleaf_dict
MylesMor commented 4 years ago

Does anyone else with the Light Panels still have this issue? Since I don't have them myself it's hard for me to test.