Skip to content

update asyncio from MicroPython v1.19.1 #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 12, 2023
Merged

update asyncio from MicroPython v1.19.1 #47

merged 3 commits into from
Oct 12, 2023

Conversation

dhalbert
Copy link
Contributor

@dhalbert dhalbert commented Aug 17, 2023

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

They are needed for adafruit/circuitpython#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 micropython/micropython#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.

@adafruit adafruit deleted a comment from github-actions bot Aug 17, 2023
gather is now async, and the one `yield` that was in it was replaced by `await core._never()`.

Incorporated adafruit/circuitpython@55169e0#diff-01b6761622028d47c6d1c2293fa365de3d8f03feb485d42e42ea9050198e1635 which never got into the asyncio library previously. (I fixed the bug independently, but didn't understand why it was needed, until I found that PR.)
@jepler
Copy link
Contributor

jepler commented Aug 21, 2023

This breaks running against 8.x.

I have a plan suggestion for moving forward with compatibility:

  • Revert the portion of this PR that uses the "new" names (push, pop) and continue to use the old names
  • In CP 9.x, support old & new names (all of push, push_head, push_sorted, pop and pop_head)
  • Once 8.x is dropped from the bundle, switch to using the new names (probably when CP 10.x goes into the bundle)
  • In CP 10.x, drop the old names as they will no longer be needed

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

@dhalbert
Copy link
Contributor Author

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
Copy link
Contributor

jepler commented Aug 21, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

jepler commented Aug 21, 2023

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

choose the way that is compatible with 8.x and 9.x
@jepler
Copy link
Contributor

jepler commented Aug 21, 2023

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

@dhalbert dhalbert marked this pull request as ready for review August 21, 2023 21:43
@dhalbert dhalbert requested a review from jepler August 21, 2023 21:44
@tannewt tannewt mentioned this pull request Oct 10, 2023
@dhalbert dhalbert merged commit 723b428 into main Oct 12, 2023
@dhalbert dhalbert deleted the 9.0.0-updates branch October 12, 2023 21:53
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Oct 13, 2023
Updating https://github.com/adafruit/Adafruit_CircuitPython_asyncio to 1.1.0 from 0.5.24:
  > Merge pull request adafruit/Adafruit_CircuitPython_asyncio#49 from tannewt/1.20-fixes
  > Merge pull request adafruit/Adafruit_CircuitPython_asyncio#47 from adafruit/9.0.0-updates

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Added the following libraries: Adafruit_CircuitPython_AD569x

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants