Async rework WIP #803
Async rework WIP #803
Conversation
|
|
There was a problem hiding this comment.
Obviously not complete yet, but basically looks reasonable to me. I know you have further work planned around reworking the way the async code works, which I assume touches more than just the connections, so some of it will fall into how that integration works.
Which is all a lot of words for "looks good, keep going" I guess, haha
meshtastic/connection.py
Outdated
| async def listen(self) -> AsyncGenerator[FromRadio]: | ||
| while not self.on_disconnect.is_set(): | ||
| yield await self.recv() |
There was a problem hiding this comment.
How do you anticipate this being used in the interface class that would wrap this? Obviously this plus send is basically the core API here. In the existing API, we're of course not really expecting callers to set up threads or such themselves, just set up listeners and make an interface, and otherwise go about their business. Part of why I ask is that we also do various management like updating the in-memory node data, and presumably we'd still want that to happen. Would this then only really be async-iterated-over by the interface class, or would it be used by clients directly?
Either way we should probably document it, anyway.
There was a problem hiding this comment.
I figured the listen function would be used by the MeshInterface to grab every mesh message, and dispatch pubsub events and such appropriately. I haven't quite worked out the specifics but you're right that it would be problematic to have more then one listen call per connection instance, as implemented.
I think what i'm gonna do is just put a lock on the listen method. Clients using the MeshInterface API won't need to even touch Connection instances directly anyways. I can also document everything before everything's ready to be merged.
meshtastic/connection.py
Outdated
| for port in serial.tools.list_ports.comports(): | ||
| if port.hwid != "n/a": | ||
| yield port.device |
There was a problem hiding this comment.
presuming this is planned to eventually flesh out to something similar to what we do in meshtastic.util.findPorts and this is effectively a stub for now?
There was a problem hiding this comment.
Basically, yes. I think it's more sensible to define this behavior alongside the rest of the connection code. I also intend to use this method to implement bluetooth scanning when i get to the BLE connection impl
Refactor of the interface code to work better with async code in library consumers, starting with separating the connection logic to a new class, as discussed with @ianmcorvidae on discord.
At this moment, this is only a first draft for the new
MeshConnectionclass.