adafruit / Adafruit_CircuitPython_asyncio

CIrcuitPython subset of CPython asyncio library
MIT License
28 stars 16 forks source link

Switch `await core.sleep` to `await core.io_queue.queue_[read|write]` #33

Closed furbrain closed 2 years ago

furbrain commented 2 years ago

This is a fix for issue #32

furbrain commented 2 years ago

I may add some further examples for UART and bluetooth buffers as well

dhalbert commented 2 years ago

See https://github.com/adafruit/Adafruit_CircuitPython_asyncio/pull/30 for use of the _Never class.

jepler commented 2 years ago

I agree, this is probably the same problem again, just in code we weren't testing. If changing to 'yield' works, then changing to use the never class will probably work also, while allowing our linters and more importantly our doc-builder to work with the code.

jepler commented 2 years ago

also, super stoked to see the examples, thank you!

dhalbert commented 2 years ago

As mentioned in #30:

Micropython can use a non-standard construct of yield in an async def to cause the async function to be suspended indefinitely, until some other task causes it to be scheduled again.

It seems to me it should await something that will cause it to be schedule again, instead of this "anonymous" suspension. The _Never class is an implementation of this anonymous suspension. But could it await an Event or something else instead?

An interesting point raised in an asyncio tutorial:

One of the most important points to get across is that the currently executing Task cannot be paused by any means other than awaiting a future (or a custom awaitable object that behaves like one).

jepler commented 2 years ago

I did refactor a bit so that await _never can be written in asyncio itself. I tried but failed to find a way to reuse sleep's singleton code, but probably shouldn't worry about that, it's just a small memory optimization.

diff --git a/asyncio/core.py b/asyncio/core.py
index dcd4b24..f1994eb 100644
--- a/asyncio/core.py
+++ b/asyncio/core.py
@@ -94,11 +94,11 @@ def sleep(t):

 ################################################################################
 # "Never schedule" object"
-# Don't re-schedule the object that awaits the _never singleton.
+# Don't re-schedule the object that awaits _never().
 # For internal use only. Some constructs, like `await event.wait()`,
 # work by NOT re-scheduling the task which calls wait(), but by
 # having some other task schedule it later.
-class _Never:
+class _NeverSingletonGenerator:
     def __init__(self):
         self.state = None
         self.exc = StopIteration()
@@ -117,7 +117,10 @@ class _Never:
            self.exc.__traceback__ = None
            raise self.exc

-_never = _Never()
+def _never(sgen=_NeverSingletonGenerator()):
+    # assert sgen.state is None, "Check for a missing `await` in your code"
+    sgen.state = False
+    return sgen

 ################################################################################
diff --git a/asyncio/event.py b/asyncio/event.py
index 442c5bb..164a269 100644
--- a/asyncio/event.py
+++ b/asyncio/event.py
@@ -62,8 +62,7 @@ class Event:
             self.waiting.push_head(core.cur_task)
             # Set calling task's data to the event's queue so it can be removed if needed
             core.cur_task.data = self.waiting
-            core._never.state = False
-            await core._never
+            await core._never()
         return True

diff --git a/asyncio/lock.py b/asyncio/lock.py
index b5080a3..71c972f 100644
--- a/asyncio/lock.py
+++ b/asyncio/lock.py
@@ -69,8 +69,7 @@ class Lock:
             # Set calling task's data to the lock's queue so it can be removed if needed
             core.cur_task.data = self.waiting
             try:
-                core._never.state = False
-                await core._never
+                await core._never()
             except core.CancelledError as er:
                 if self.state == core.cur_task:
                     # Cancelled while pending on resume, schedule next waiting Task
furbrain commented 2 years ago

It seems a bit odd to have a bunch of places where core._io_queue.queue_[read|write] is called immediately followed by yield or await _never. What about making IOQueue.queue_[read|write] a coroutine and call await _never before exit? This would make the code in stream.py neater and avoid duplication?

dhalbert commented 2 years ago

What about making IOQueue.queue_[read|write] a coroutine and call await _never before exit?

Seems logical to me, but I'd also look to see if something similar is done in CPython asyncio.

furbrain commented 2 years ago

CPython asyncio doesn't seem to have much similarity with the circuitpython code for stream management. I've therefore converted to make IOQueue.queue_read/write methods coroutines.

furbrain commented 2 years ago

Looks like as of today we are running pre-commit hooks on python 3.11. This conflicts with pylint 2.11.1 - so I've upgraded the pylint requirement for pre-commit to 2.13.9

furbrain commented 2 years ago

One remaining issue is error correction. select.poll.register is called by core.IOQueue with the stream that we want to watch out for. There does seem to be c code in there which checks that what is being sent is a sensible object - but it is not working for me. As mentioned in https://github.com/adafruit/Adafruit_CircuitPython_asyncio/issues/4#issuecomment-1299312980 , this makes the device crash, with no error message or explanation.

I note that with the BLE UART I need to pass in uart._rx, which is a bit naughty, but the only way to access the actual underlying stream.

Should we have some checks on initialisation that we are passing an appropriate object in (and then access the ble uart._rx from there?) Problem seems to be that there is no way in CircuitPython of detecting whether an object implements protocol_stream from the Python side, so we'd just have to check if it's an instance of busio.UART, usb_cdc.Serial,usb_midi.PortIn|Out, and_bleio.CharacteristicBuffer`