search

delfick / photons

Python3.6+ asyncio framework for interacting with LIFX devices
https://photons.delfick.com
MIT License
74 stars 6 forks source link

interactor: introduce zeroconf #79

Closed delfick closed 2 years ago

Djelibeybi commented 3 years ago

While playing with something else, Interactor decided to stop playing after reporting this warning:

14:59:35 WARNING zeroconf        Received invalid packet from ('192.168.254.1', 5351) at offset 52819 while unpacking b'\x02\x81\x00\x00\x00\x00\x1c \x00\x07F\xba\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xb4\xdd\xd9ZD\xe7\xd0^\xce1\x10\xab\x11\x00\x00\x00\x14\xe9\x14\xf0\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xff\xffg3r\xa3'
Traceback (most recent call last):
  File "/Users/amiller/Git/Djelibeybi/photons/venv/lib/python3.9/site-packages/zeroconf/_protocol/incoming.py", line 111, in _parse_data
    parser_call()
  File "/Users/amiller/Git/Djelibeybi/photons/venv/lib/python3.9/site-packages/zeroconf/_protocol/incoming.py", line 105, in _initial_parse
    self.read_others()
  File "/Users/amiller/Git/Djelibeybi/photons/venv/lib/python3.9/site-packages/zeroconf/_protocol/incoming.py", line 185, in read_others
    domain = self.read_name()
  File "/Users/amiller/Git/Djelibeybi/photons/venv/lib/python3.9/site-packages/zeroconf/_protocol/incoming.py", line 275, in read_name
    self.offset = self._decode_labels_at_offset(self.offset, labels, seen_pointers)
  File "/Users/amiller/Git/Djelibeybi/photons/venv/lib/python3.9/site-packages/zeroconf/_protocol/incoming.py", line 315, in _decode_labels_at_offset
    raise IncomingDecodeError("Corrupt packet received while decoding name")
zeroconf._exceptions.IncomingDecodeError: Corrupt packet received while decoding name

~I didn't look carefully, but Interactor shouldn't be listening for Zeroconf, just advertising.~

Once started, Zeroconf is responsible for answering requests for services until Interactor shuts down, so this requires further investigation. Either Zeroconf is breaking on a valid packet, or something is sending a weird packet that we need to protect against.

delfick commented 3 years ago

weird. zeroconf bug?

Djelibeybi commented 3 years ago

Dunno. I just saw it, so I figured I'd log it for one of us to look at later. I'm busy working on the thing that needs this thing, so I'll likely have a poke at it at some point.

Djelibeybi commented 3 years ago

It looks like it's either a ZeroConf or UniFi mDNS reflector issue. Will look deeper later.

Djelibeybi commented 3 years ago

Updated to match RFC and other implementation standards after spending way too long staring at the output of dns-sd.

Djelibeybi commented 3 years ago

weird. zeroconf bug?

Known issue: https://github.com/jstasiak/python-zeroconf/issues/993

delfick commented 2 years ago

where did we get with whether this is viable?

Djelibeybi commented 2 years ago

Last I recall, it works. We were disagreeing over network interface selection semantics.

Djelibeybi commented 2 years ago

On an only-ever-so-slightly-related but very ironic note, I think bad network interface selection is what's causing most of the issues with the current LIFX integration for Home Assistant.

delfick commented 2 years ago

ah yeah. lol.

I thought a problem here was that if interactor and hass were on the same box they can't both be a zeroconf server or something?

On Sat, 8 Jan 2022, 8:02 am Avi Miller, @.***> wrote:

On an only-ever-so-slightly-related but very ironic note, I think bad network interface selection is what's causing most of the issues with the current LIFX integration for Home Assistant.

— Reply to this email directly, view it on GitHub https://github.com/delfick/photons/pull/79#issuecomment-1007738091, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2V5M7S6LPNVDGFL3LWVTUU5IFHANCNFSM5HPOGPIQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

Djelibeybi commented 2 years ago

ah yeah. lol. I thought a problem here was that if interactor and hass were on the same box they can't both be a zeroconf server or something?

No idea where this idea came from. We need Interactor to send mDNS/zeroconf broadcasts so that Home Assistant can see it. There may have been another disagreement over the amount of data we send.

delfick commented 2 years ago

I swear I saw something that made this seem unviable. If you say it's fine, then I'll believe you, can always revert if not the case hahah.

This PR seems fine, do you see problems with merging?

Djelibeybi commented 2 years ago

I haven't looked at it for a while, but as long as the tests still pass, I think we're good. We could switch it to disabled by default so it's essentially a no-op until enabled?

delfick commented 2 years ago

makes sense