adafruit / Adafruit_CircuitPython_asyncio

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

update asyncio from MicroPython v1.19.1 #47

Closed dhalbert closed 1 year ago

dhalbert commented 1 year ago

These are updates, including bug fixes, merged from https://github.com/micropython/micropython/tree/v1.19.1/extmod/uasyncio.

They are needed for https://github.com/adafruit/circuitpython/pull/8281, in particular, to make the updated asyncio tests in v1.19.1 pass.

There will be further changes to consider for the v1.20 merge, at least https://github.com/micropython/micropython/pull/10190.

I made this PR in adafruit rather than in dhalbert so I did not change .gitmodules to point to my own repo while testing this in the CircuitPython PR.

jepler commented 1 year ago

This breaks running against 8.x.

I have a plan suggestion for moving forward with compatibility:

The 'price' of this is probably under 100 bytes of flash in the core (6 dict entries + 3 qstrs with 30 chars total)

dhalbert commented 1 year ago

How about keeping the new names in the Python version of TaskQueue, and adding aliases that map the old names to the new names? So the uses would be old names, but the primary definitions in Python would be new names. That will make future merges easier.

jepler commented 1 year ago

Another alternative is to adapt to either naming convention inside asyncio but I am not thrilled about it: https://gist.github.com/jepler/71f02ca767d75d5fa4dd40abf70655f9 -- it does present a merge burden because all the call sites change.

 asyncio/core.py  | 22 ++++++++++++++--------
 asyncio/event.py |  4 ++--
 asyncio/funcs.py |  2 +-
 asyncio/lock.py  |  6 +++---
 asyncio/task.py  |  4 ++--
 5 files changed, 22 insertions(+), 16 deletions(-)

I don't think the "provide both names in core" is much of a merge burden, it just looks like this:

diff --git a/extmod/moduasyncio.c b/extmod/moduasyncio.c
index ea81fbc310..2ba8efb1fe 100644
--- a/extmod/moduasyncio.c
+++ b/extmod/moduasyncio.c
@@ -160,6 +160,11 @@ STATIC const mp_rom_map_elem_t task_queue_locals_dict_table[] = {
     { MP_ROM_QSTR(MP_QSTR_push), MP_ROM_PTR(&task_queue_push_obj) },
     { MP_ROM_QSTR(MP_QSTR_pop), MP_ROM_PTR(&task_queue_pop_obj) },
     { MP_ROM_QSTR(MP_QSTR_remove), MP_ROM_PTR(&task_queue_remove_obj) },
+
+    // CIRCUITPYTHON: remove these after the bundle need not support 8.x
+    { MP_ROM_QSTR(MP_QSTR_push_head), MP_ROM_PTR(&task_queue_push_obj) },
+    { MP_ROM_QSTR(MP_QSTR_push_sorted), MP_ROM_PTR(&task_queue_push_obj) },
+    { MP_ROM_QSTR(MP_QSTR_pop_head), MP_ROM_PTR(&task_queue_pop_obj) },
 };
 STATIC MP_DEFINE_CONST_DICT(task_queue_locals_dict, task_queue_locals_dict_table);
dhalbert commented 1 year ago

Another alternative is to adapt to either naming convention inside asyncio but I am not thrilled about it: https://gist.github.com/jepler/71f02ca767d75d5fa4dd40abf70655f9 -- it does present a merge burden because all the call sites change.

I agree with your conclusion. I was suggesting leaving def push(...) in TaskQueue and then just adding push_sorted = push_head = push in the class. Adding the aliasing to each call site is not needed. Changing the callsites later is really easy.

jepler commented 1 year ago

oh, yes, I see what you mean. I agree, the new names could be adopted now in the bundled pure Python TaskQueue implementation.

jepler commented 1 year ago

This is now compatible with 8.x again, by using the old names