8000 py: clean up definition and use of `mp_handle_pending()` by dpgeorge · Pull Request #18672 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

py: clean up definition and use of mp_handle_pending() #18672

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.

Sign up for GitHub

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

Jump to bottom
Open
dpgeorge wants to merge 4 commits into
base: master
Choose a base branch
from
Open

py: clean up definition and use of mp_handle_pending() #18672

dpgeorge wants to merge 4 commits into from
+45 −61

Conversation

@dpgeorge
Copy link
Member
@dpgeorge dpgeorge commented Jan 12, 2026

Summary

Commit c57aebf added more functionality to mp_handle_pending(), passing in an enum instead of a bool to select whether exceptions are ignored, cleared or raised.

That commit attempted to keep the original behaviour of mp_handle_pending(bool) with a bool argument by making it static-inline, but that doesn't work in all cases (see #18663).

This PR cleans up the definition and use of this function:

  • remove incorrect and unnecessary declarations of extern void mp_handle_pending(bool); in mpconfigport.h
  • reorder enum so their numerical values match the original bool argument, namely that true maps to MP_HANDLE_PENDING_CALLBACKS_AND_EXCEPTIONS and false maps to MP_HANDLE_PENDING_CALLBACKS_AND_CLEAR_EXCEPTIONS
  • remove the static-inline definition
  • rename mp_handle_pending_internal() to mp_handle_pending()

Functionality should be unchanged, and backwards compatibility with mp_handle_pending() should be retained.

Fixes issue #18663.

Testing

Tested locally on PYBV10.

CI will also test the change (the unix port already has special coverage testing for mp_handle_pending()).

Trade-offs and Alternatives

This touches all code that calls mp_handle_pending() but that's necessary to fully clean it up.

The enum for mp_handle_pending_behaviour_t now defines the first and second values as false and true, which is a bit awkward but will explicitly retain backwards compatibility.

@dpgeorge
port 8000 s: Remove incorrect and unnecessary mp_handle_pending declaration.
5723e4e 
In c57aebf `mp_handle_pending()` became a
static-inline function.  So these declarations in `mpconfigport.h` are
incorrect and removed by this commit.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge
py/runtime: Reorder mp_handle_pending_behaviour_t enum.
38bfcdb 
This is functionally a no-op, although will change compiled code due to the
numerical values of the enums changing.

This is needed for a follow-up commit, to remove the static-inline
`mp_handle_pending(bool)` helper function.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge
py/runtime: Rename mp_handle_pending_internal to mp_handle_pending.
4989ff7 
This commit removes the static-inline `mp_handle_pending()` wrapper
function, which was originally added for backwards compatibility.  It
replaces it with `mp_handle_pending_internal()` which is renamed to
`mp_handle_pending()`, which has the same arguments.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge
all: Use enum instead of bool argument to mp_handle_pending.
4a35eff 
This should be a no-op because the enum and bool have the same numeric
value.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge added ports Relates to multiple ports, or a new/proposed port py-core Relates to py/ directory in source labels Jan 12, 2026
@github-actions
Copy link
github-actions bot commented Jan 12, 2026

Code size report:

Reference:  tools/mpy_ld.py: Optimise MPY trampoline sizes if possible. [0fd0843]
Comparison: all: Use enum instead of bool argument to mp_handle_pending. [merge of 4a35eff]
  mpy-cross:    +0 +0.000% 
   bare-arm:    +0 +0.000% 
minimal x86:    -2 -0.001% 
   unix x64:    +0 +0.000% standard
      stm32:    +4 +0.001% PYBV10
      esp32:    +0 +0.000% ESP32_GENERIC
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +8 +0.001% RPI_PICO_W
       samd:    +4 +0.001% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +4 +0.001% VIRT_RV32

@codecov
Copy link
codecov bot commented Jan 12, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 98.38%. Comparing base (0fd0843) to head (4a35eff).

Files with missing lines Patch % Lines
py/profile.c 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##           master   #18672   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files         171      171           
  Lines       22298    22298           
=======================================
  Hits        21937    21937           
  Misses        361      361

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dpgeorge
Copy link
Member Author
dpgeorge commented Jan 12, 2026

@iabdalkader @Gadgetoid FYI

@dpgeorge dpgeorge mentioned this pull request Jan 12, 2026
Open
@iabdalkader
Copy link
Contributor
iabdalkader commented Jan 12, 2026

This looks good to me. However, since you're making changes to all those files anyway, with the exception or unix port's main.c, shouldn't we just call mp_event_handle_nowait?

@dpgeorge
Copy link
Member Author
dpgeorge commented Jan 12, 2026

shouldn't we just call mp_event_handle_nowait?

You are right that we need to convert the remaining ports to use the new event functions, like mp_event_handle_nowait().

But changing mp_handle_pending() to mp_event_handle_nowait() is not the correct way to do it. Instead the port needs to remove its definition of MICROPY_EVENT_POLL_HOOK and instead define MICROPY_INTERNAL_WFE. On stm32 (with threading disabled) that would be defined to __WFI();, but on esp32 it's a bit more complicated and requires some thought and testing.

I would therefore like to convert the remaining ports one-by-one, to make sure they are properly tested.

For now, this PR has much simpler scope, just to clean up mp_handle_pending().

@iabdalkader
Copy link
Contributor
iabdalkader commented Jan 12, 2026

Instead the port needs to remove its definition of MICROPY_EVENT_POLL_HOOK and instead define MICROPY_INTERNAL_WFE

Note that no port defines MICROPY_EVENT_POLL_HOOK_FAST, so they'll all fallback to:

    #else
    // Process any port layer (non-blocking) events.
    MICROPY_INTERNAL_EVENT_HOOK;
    mp_handle_pending(true);
    #endif

And with the exception of esp8266, renesas, no port defines MICROPY_INTERNAL_EVENT_HOOK either, so calling mp_event_handle_nowait() should basically the same as calling mp_handle_pending(true); for the rest of the ports.
Now might be a good time to switch to _nowait, vs using those enums and then refactoring them again later, your call of course.

@dpgeorge
Copy link
Member Author
dpgeorge commented Jan 13, 2026

And with the exception of esp8266, renesas, no port defines MICROPY_INTERNAL_EVENT_HOOK either, so calling mp_event_handle_nowait() should basically the same as calling mp_handle_pending(true); for the rest of the ports.

No, that's not how it's supposed to work.

The current design is:

  • Upper layer: mp_event_handle_nowait(), mp_event_wait_indefinite(), mp_event_wait_ms() -- these use the lower layers as part of their implementation
  • New lower layer (defined by the port): MICROPY_INTERNAL_EVENT_HOOK, MICROPY_INTERNAL_WFE
  • Old lower layer (defined by the port): MICROPY_EVENT_POLL_HOOK, MICROPY_EVENT_POLL_HOOK_FAST

If MICROPY_EVENT_POLL_HOOK was defined in terms of mp_event_handle_nowait() then it would potentially lead to circular definitions!

As I mentioned above, we need to convert (one by one) each port to use the new lower layer.

The change in this PR is kind of orthogonal to that.

@dpgeorge
Copy link
Member Author
dpgeorge commented Jan 13, 2026

I converted stm32 to the new scheme in #18679. If we convert all the ports first, then this PR becomes a lot simpler, although that might take some time to do.

@dpgeorge
Copy link
Member Author
dpgeorge commented Jan 14, 2026

I converted the webassembly port to the new event scheme in #18682.

@dpgeorge
Copy link
Member Author
dpgeorge commented Jan 14, 2026

I converted the zephyr port to the new event scheme in #18685.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@dlech dlech dlech approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ports Relates to multiple ports, or a new/proposed port py-core Relates to py/ directory in source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dpgeorge @iabdalkader @dlech
0