Initial version #2
Conversation
emontnemery
force-pushed
the
initial_version
branch
from
6642c8e to
f6f5304
Compare
| return_when=asyncio.FIRST_COMPLETED, | ||
| ) | ||
| for task in pending: | ||
| task.cancel() |
There was a problem hiding this comment.
This might complain about an unretrieved exception if it's not awaited
There was a problem hiding this comment.
The variable name is misleading, these are futures, not tasks.
I think the potential error here is if the cancelled futures have their result set after we cancel them?
There was a problem hiding this comment.
I think the potential error here is if the cancelled futures have their result set after we cancel them?
That is a risk if you await them as they will propagate cancelation from the point of await
There was a problem hiding this comment.
As discussed on Discord the code is fine, but the variable names are misleading and unclear. I'll clean it up.
|
|
||
| _LOGGER.debug( | ||
| "%s: Subscribe to notifications; RSSI: %s", self.name, self.rssi | ||
| ) |
There was a problem hiding this comment.
Maybe verify all the char uuids are present and if not try clearing the cache and reconnecting to the device so the services get resolved again.
There was a problem hiding this comment.
Can you link to an example doing that?
There was a problem hiding this comment.
There was a problem hiding this comment.
I see, that's more or less a workaround for a missing feature in bleak IMHO, and the same implementation is duplicated in multiple BLE-packages: eulife_ble_client, ld2410_ble, led_ble, switchbot.
Maybe it could be moved to its own package, bleak_utils or bleak_helpers?
I'll copy the implementation to py-improv-ble for now though.
There was a problem hiding this comment.
I'm happy to accept a PR to bleak_retry_connector
| error_fut: asyncio.Future[prot.Error] = asyncio.Future() | ||
| provisioned_fut = self.receive_response(prot.WiFiSettingsRes) | ||
|
|
||
| try: |
There was a problem hiding this comment.
Does this need a timeout? I may be easier to handle cancelation inside the inner functions on timeout if you limit the scope of the timeout to this function
There was a problem hiding this comment.
To not simplify things I chose to only have a single global timeout, handled by the ImprovBLEClient._disconnect_timer.
Could you give an example of how cancellation could be simplified if the timeout scope is narrowed?
There was a problem hiding this comment.
You have to make sure all the code running under the timeout is cancellation safe (ie cleans up and disconnects ok if it gets canceled)
By limiting the scope of where we expect cancellation to happen to just the area where its going to timeout thats a bit easier, but its also better to have all the code handle cancellation
There was a problem hiding this comment.
Right, I prefer to handle the the cancellation at one place higher up instead of in several places further down.
There was a problem hiding this comment.
Its probably safe since there is a disconnect timer that won't be canceled when the task is cancelled.
Maybe it would be good to add a test to make sure if any operation is cancelled by the task its running in being canceled, it still disconnects.
emontnemery
force-pushed
the
initial_version
branch
from
ca5d582 to
d94cf68
Compare
improv_ble_client/client.py
Outdated
| disconnect_task = asyncio.create_task(_disconnect()) | ||
| self._background_tasks.add(disconnect_task) | ||
| disconnect_task.add_done_callback(self._background_tasks.discard) |
There was a problem hiding this comment.
This could be self._async_create_background_task() since you have the same pattern in _disconnect
|
One small comment above, and some lint issues, but otherwise LGTM 👍 |
Initial version of the library