Port to asyncio

[oyvindwe]

Initial port to use asyncio instead of threads. Also support callbacks to avoid polling for data.

I'm also preparing a PR for https://github.com/echoromeo/hanobo that works with this version. This makes changes immediately show up in Home Assistant.

Note that this version is not backwards compatible, but I'd like to get your thoughts on it.

Also, reconnect is not implemented, so I'm marking the PR as a draft.

[echoromeo]

Wow, great work! I do not exactly know where to start understanding asyncio, but removing polling is always a good thing :)
So which changes are actually breaking here?

[oyvindwe]

The API has changed somewhat:

Need to call await hub.start() after hub = nobo(…)

Several methods are now async and must be called with await:

await nobo.discover_hubs(…)
await hub.connect_hub(…)
await hub.send_command(…)
await hub.get_response(…)
await hub.socket_receive(…)
await hub.create_override(…)
await hub.update_zone(…)

Also note that discover_hubs() is a class method, so it can be used to discover any hubs on the local network without instantiating. I use this in the setup process in Home Assistant to list hubs to the user and ask for the complete serial number. connect_hub() can still be used in the existing way.

It may be possible to rename all these to async_* and make sync wrappers - it looks like this is how it's done in Home Assistant, but I'd still want to keep the start() method as a new method. If using callbacks, the caller need to register the callback before starting the connection to avoid a race condition in case of any updates immediately after the initial response to G00.

In addition, I've introduced register_callback() (there's also deregister_callback() and stop(). It is still possible to poll for status is the old way though.

My original attempt was to only introduce callbacks whenever the status is updated, but it was quite straight forward to remove the threading and low level socket usage with asyncio.

I've only used async/await in Javascript/TypeScript prior to this. I watched this as an introduction to asyncio - highly recommended: https://developers.home-assistant.io/docs/asyncio_101

What worries me most though, is that reconnect_hub() is currently not called. I need to play a little bit with my connection to force a disconnect.

[capelevy]

I have not played with asyncio before, but it looks just right for this application. I like the updates, and cleanup :)

We need the working reconnect, and I think wrappers for those not using (or used to) asyncio is a good idea.

Users must be allowed to register callbacks after calling start(). The main risk is that all the initial updates (G00) are done, and you do not get called until after the next update. I think this would be solved by always calling a callback when it is registered (I would think of this as an update to the current state).

[oyvindwe]

@capelevy thank you very much for the review and comments! I have addressed all but the reconnect - still working on reproducing disconnects.

I did not make sync wrappers for get_response and socket_receive, as I consider these to be for internal use only, but I provided it for all the other async methods.

Executing the callback in register_callback fails in Home Assistant. The way I ended up using this in Home Assistant, is that the platform starts the hub, and then each zone register the callback and requests initial state (which is already in memory). The callback has to propagate self.async_schedule_update_ha_state(), but that cannot be called before the zone is registered.

To avoid the race condition, it is sufficient to read state between await hub.start() and hub.register_callback(my_callback), which I think is perfectly acceptable.

Example use in climate.py:

async def async_setup_entry(hass: HomeAssistant, config_entry: config_entries.ConfigEntry, async_add_devices):
    hub = hass.data[DOMAIN][config_entry.entry_id]
    await hub.start() # is instantiated in component configuration (__init.py___) to allow for multiple platform instances
    [set up off and on commands]
    async_add_devices(NoboZone(zones, hub, command_off_id, command_on_by_id.get(zones)) for zones in hub.zones)

class NoboZone(ClimateEntity):
    def __init__(self, id, hub: nobo, command_off_id, command_on_id):
        [set initial variables]
        # Register for callbacks before initial update to avoid race condition.
        self._nobo.register_callback(self._after_update)
        self.update()

    @callback
    def update(self):
        [read state]

    @callback
    def _after_update(self, hub):
        self.update()
        self.async_schedule_update_ha_state()

[oyvindwe]

The improved error handling should fix both #9 and #14

[capelevy]

I am also able to provoke TimeoutError at this point:

ERROR:pynobo:Unhandled exception [Errno 60] Operation timed out
<...>
TimeoutError: [Errno 60] Operation timed out

could we check for that as well?

[oyvindwe]

Yes, we can reconnect on timeout. But I'm curious to how this happened. Do you have steps to reproduce?

[capelevy]

I get this when disabling WIFI. After three to four DEBUG:pynobo:sending: ['HANDSHAKE'] I get the exception, triggered from self.get_response(), line 513. This happens on macOS, not sure about other platforms...

[oyvindwe]

Reproduced (also on macOS) by disabling ethernet after connection is made. Can also be reproduces by disconnecting ethernet cable, so this is a quite likely error to happen.

The timeout probably happens because asyncio sets a 60 seconds timeout on the reader, which is quite acceptable, since we expect a handshake every 14 seconds (recommended) to 30 seconds (max).

It was quite tricky to reconnect though, as the hub was rediscovered before local IP address was reassigned on DHCP, so I needed some error handling in reconnect_hub as well.

[oyvindwe]

The warning below is when the first call to asyncio.open_connection below failed with ENETUNREACH.
The HELLO message afterwards is after a successful call to asyncio.open_connection.

2021-04-28 21:25:04,738 - pynobo - DEBUG - sending: ['HANDSHAKE']
2021-04-28 21:25:18,742 - pynobo - DEBUG - sending: ['HANDSHAKE']
2021-04-28 21:25:32,743 - pynobo - DEBUG - sending: ['HANDSHAKE']
2021-04-28 21:25:46,745 - pynobo - DEBUG - sending: ['HANDSHAKE']
2021-04-28 21:25:58,830 - pynobo - INFO - Reconnecting due to [Errno 60] Operation timed out
2021-04-28 21:25:58,830 - pynobo - INFO - reconnecting to hub
2021-04-28 21:26:06,425 - pynobo - INFO - broadcast received: __NOBOHUB__102000013 from <ip redacted>
2021-04-28 21:26:07,843 - pynobo - WARNING - Failed to connect to ip set(): [Errno 51] Network is unreachable
2021-04-28 21:26:08,845 - pynobo - DEBUG - sending: ['HELLO', '1.1', <serial redacted>, '20210428212608']

[capelevy]

Good work, Øyvind! The reconnect seems to be working well. I also like the async wrappers. This is turning out to be a nice upgrade :)

[capelevy]

Good work @oyvindwe! Quick responses and well thought out changes.

I suspect this fixes Issues #9 and #14, @echoromeo do you have any further comments?

[oyvindwe]

I suspect this fixes Issues #9 and #14

Yes. errno.EHOSTUNREACH will trigger reconnect, which is the error reported in both issues. I actually think #9 and #14 are duplicates, as both report disconnect after OSError: [Errno 113], only with different text messages.

Note that the error code depends on OS, but the constants in errno handles that.

[oyvindwe]

For the record - from errno.h on Linux:

#define EHOSTUNREACH    113     /* No route to host */

sys/errno.h on macOS:

#define EHOSTUNREACH    65              /* No route to host */

In CPython, these constants are generated compile time based on the OS. See https://github.com/python/cpython/blob/63298930fb531ba2bb4f23bc3b915dbf1e17e9e1/Modules/errnomodule.c

On Linux:

$ python
>>> import errno
>>> print(errno.EHOSTUNREACH)
113

On macOS:

$ python
>>> import errno
>>> print(errno.EHOSTUNREACH)
65

[oyvindwe]

@echoromeo Is it OK to merge this PR? I am ready to submit a PR to make an official integration for Home Assistant, but that requires an updated version on Pypi.

See https://github.com/oyvindwe/home-assistant/tree/nobo_hub

[echoromeo]

Hi, sorry for the delay. I trust the approval of @capelevy so I will merge.
Unfortunately I will not have the time to deploy a new version on PyPi before the 18th a the earliest.

[oyvindwe]

Unfortunately I will not have the time to deploy a new version on PyPi before the 18th a the earliest.

No worries - I'll be busy as well. :)

The HA PR may take a little more time than i thought, to get to 100% test coverage of the configuration flow.