google / python-lakeside

Apache License 2.0
45 stars 18 forks source link

Devices dislike sequence numbers above 0x80000000 #11

Open jhon opened 5 years ago

jhon commented 5 years ago

Looking through the code it is unclear to me where it swaps from int32 to uint32. The T1201Packet packet the response is being received from specifies int32.

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/homeassistant/helpers/entity_platform.py", line 128, in _async_setup_platform
    SLOW_SETUP_MAX_WAIT, loop=hass.loop)
  File "/usr/local/lib/python3.6/asyncio/tasks.py", line 358, in wait_for
    return fut.result()
  File "/usr/local/lib/python3.6/concurrent/futures/thread.py", line 56, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/usr/local/lib/python3.6/site-packages/homeassistant/components/switch/eufy.py", line 20, in setup_platform
    add_entities([EufySwitch(discovery_info)], True)
  File "/usr/local/lib/python3.6/site-packages/homeassistant/components/switch/eufy.py", line 36, in __init__
    self._switch.connect()
  File "/config/deps/lib/python3.6/site-packages/lakeside/__init__.py", line 229, in connect
    return device.connect(self)
  File "/config/deps/lib/python3.6/site-packages/lakeside/__init__.py", line 66, in connect
    self.update()
  File "/config/deps/lib/python3.6/site-packages/lakeside/__init__.py", line 246, in update
    response = self.get_status()
  File "/config/deps/lib/python3.6/site-packages/lakeside/__init__.py", line 239, in get_status
    packet.sequence = self.get_sequence()
  File "/usr/local/lib/python3.6/site-packages/google/protobuf/internal/python_message.py", line 674, in field_setter
    new_value = type_checker.CheckValue(new_value)
  File "/usr/local/lib/python3.6/site-packages/google/protobuf/internal/type_checkers.py", line 134, in CheckValue
    raise ValueError('Value out of range: %d' % proposed_value)
ValueError: Value out of range: 2388884521

2388884521 = 0x8E637C29

mjg59 commented 5 years ago

Could you test with https://github.com/google/python-lakeside/tree/test_uint32 ?

sebmos commented 5 years ago

I now have that same problem, so I tried this change, but it doesn't work:

Traceback (most recent call last):
  File "test.py", line 9, in <module>
    bulb.connect()
  File "/Users/sebastian/Documents/EufyHome/python-lakeside/lakeside/__init__.py", line 128, in connect
    return device.connect(self)
  File "/Users/sebastian/Documents/EufyHome/python-lakeside/lakeside/__init__.py", line 66, in connect
    self.update()
  File "/Users/sebastian/Documents/EufyHome/python-lakeside/lakeside/__init__.py", line 192, in update
    response = self.get_status()
  File "/Users/sebastian/Documents/EufyHome/python-lakeside/lakeside/__init__.py", line 141, in get_status
    response = self.send_packet(packet, True)
  File "/Users/sebastian/Documents/EufyHome/python-lakeside/lakeside/__init__.py", line 131, in send_packet
    return device.send_packet(self, packet, response)
  File "/Users/sebastian/Documents/EufyHome/python-lakeside/lakeside/__init__.py", line 90, in send_packet
    self.connect()
  File "/Users/sebastian/Documents/EufyHome/python-lakeside/lakeside/__init__.py", line 128, in connect
    return device.connect(self)
  File "/Users/sebastian/Documents/EufyHome/python-lakeside/lakeside/__init__.py", line 66, in connect
    self.update()
  File "/Users/sebastian/Documents/EufyHome/python-lakeside/lakeside/__init__.py", line 192, in update
    response = self.get_status()
  File "/Users/sebastian/Documents/EufyHome/python-lakeside/lakeside/__init__.py", line 141, in get_status
    response = self.send_packet(packet, True)
  File "/Users/sebastian/Documents/EufyHome/python-lakeside/lakeside/__init__.py", line 131, in send_packet
    return device.send_packet(self, packet, response)
  File "/Users/sebastian/Documents/EufyHome/python-lakeside/lakeside/__init__.py", line 90, in send_packet
    self.connect()
  File "/Users/sebastian/Documents/EufyHome/python-lakeside/lakeside/__init__.py", line 128, in connect
    return device.connect(self)

It continues until there's a recursion error (as in https://github.com/google/python-lakeside/issues/15)

mjg59 commented 5 years ago

That looks like a different issue

sebmos commented 5 years ago

I don't the recursive error is a different issue, btw. I don't think the recursive error is the same as in https://github.com/google/python-lakeside/issues/15 - just that it is recursive, as in that issue.

I think the recursive error comes from the fact that while loading the current device state during the connect state, the socket connection closes, because the device doesn't accept the uint32 sequence number. Since that error happens during the initial connection step, it ends up connecting repeatedly. I ran into the same recursive issue in node-eufy-api while testing changing sequence from int32 to uint32.

mjg59 commented 5 years ago

The only way the sequence number is greater than MAX_INT is if the device sent us back something that's bigger than that. The protobuf wire format doesn't distinguish between uint32 and int32, so it's not obvious how it could generate values that it can reject.

sebmos commented 5 years ago

That all makes sense, and yet that is what the behaviour suggests - it's also the same behaviour I saw on the JavaScript port of your library.

In general, closing the socket was a symptom of an "invalid" message being sent, and in this case specifically, the sequence incrementing worked just fine, until the numbers got much larger than initially. I tried the same fix you did, changing the variable type both to int32 & int64, and all it did was continuously close the socket in response to a message with a sequence higher than uint32 max.

It appears to me that this is a bug on the EufyHome device side.

mjg59 commented 5 years ago

Ok. Does simply resetting the sequence number to 0 if we get back something >=0x80000000 work?

sebmos commented 5 years ago

I changed the code like this:

return 0 if response.sequence > 0x80000000 else response.sequence + 1

My (unfortunately hasty) test didn't turn on the plug (forgot that in my test script), but it did connect. The problem is now gone away (sequence is < 0x80000000).

Most likely this means resetting the sequence nr to 0 works, but I can't double-check properly, either.)

sebmos commented 5 years ago

I take that back - one of my plugs still has the problem, and resetting to 0 doesn't fix it/results in the same problem as before. I tried the same with the JS port, which has a bit of extra logging and confirms that the logic is doing what it should be (i.e. what exactly the sequence nr is, what it does, etc.)

sebmos commented 5 years ago

I think this issue should still be open - the problem as specifically described in the issue title might be resolved (no more ValueError), but the actual problem (socket closes immediately, if the value is higher than uint32) is still there..