ports/renesas: Replace MICROPY_EVENT_POLL_HOOK with mp_event_wait. #15929

andrewleech · 2024-09-29T11:26:51Z

Summary

Basic update to the renesas to replace the traditional MICROPY_EVENT_POLL_HOOK with the newer mp_event_wait_indefinite() / mp_event_wait_ms() as appropriate.

Testing

Renesas imxrt1010 EVK

python3 test_serial.py from #15909 has been run on both usb and uart and works without any failures.

~/micropython/tests$ ./run-tests.py --target renesas-ra --device /dev/ttyACM0 has been run before and after MR with no changes (fails on builtin_pow3_intbig machine_spi_rate in both cases).

dpgeorge · 2024-09-30T00:16:01Z

Thanks. We need to eventually do this for all ports.

andrewleech · 2024-10-01T10:43:28Z

@iabdalkader I had to also make a similar change in drivers/esp_hosted_wifi: Replace EVENT_POLL_HOOK with mp_event_wait_ms., included in this PR. I don't have any hardware to test this on though, does the change look ok to you?

I will endevour to get something set up to test esphosted as I'm keen to use it in the near future, but not sure when I'll have it running.

andrewleech · 2024-10-01T10:44:52Z

@dpgeorge I've now done some basic testing and switched to mp_event_wait_ms() in a few places similar to a quick review of other ports.

andrewleech · 2024-10-01T11:04:53Z

drivers/esp-hosted/esp_hosted_bthci.c

@@ -126,7 +126,7 @@ int mp_bluetooth_hci_controller_init(void) {
        if (mp_bluetooth_hci_uart_any()) {


Bit off topic, but @iabdalkader does this just block for 2.5 seconds at boot every time? Does the esp chip take that long to boot, or is it just extra timeout in case?

Yes, because the firmware is configured to output debug messages on boot.

I think you meant to comment on this line:

micropython/drivers/esp-hosted/esp_hosted_bthci.c

Line 125 in acecb0f

while ((mp_hal_ticks_ms() - start) < 2500) {

Thanks, I did, but github only seems to let me start a comment on a line x rows from an actual change... this was as close as I could get!

That feels like a long time to block, but if that's needed / intended I won't worry about it too much until I have hw and time to actually work on it myself :-D

iabdalkader · 2024-10-04T10:03:44Z

@iabdalkader I had to also make a similar change in drivers/esp_hosted_wifi: Replace EVENT_POLL_HOOK with mp_event_wait_ms., included in this PR. I don't have any hardware to test this on though, does the change look ok to you?

I can test it next week and let you know.

iabdalkader · 2024-10-10T08:30:18Z

I tested this, sorry for the delay. I ran multitests and they pass just as before. Unrelated, but most of the sslcontext tests are skipped not sure why (this was the case before this change).

andrewleech · 2024-10-10T08:56:57Z

Thanks @iabdalkader for the testing! No apologies necessary, this was just a bit of opportunistic cleanup for me and not in a rush for any reason.

dpgeorge · 2025-04-22T01:43:11Z

Although this PR has correct behaviour I think it will increase power consumption at an idle REPL (and other idle locations) because it's now doing a busy loop rather than executing WFI.

To fix that you'll need to define MICROPY_INTERNAL_WFE. Since renesas-ra is not tickless (unlike rp2 which is tickless) this macro probably needs to be:

#define MICROPY_INTERNAL_WFE(TIMEOUT_MS) __WFI()

And please put that in mphalport.h.

andrewleech · 2025-04-23T04:29:41Z

Although this PR has correct behaviour I think it will increase power consumption at an idle REPL (and other idle locations) because it's now doing a busy loop rather than executing WFI.

To fix that you'll need to define MICROPY_INTERNAL_WFE. Since renesas-ra is not tickless (unlike rp2 which is tickless) this macro probably needs to be:
#define MICROPY_INTERNAL_WFE(TIMEOUT_MS) __WFI()
And please put that in mphalport.h.

Thanks for the review @dpgeorge
I added a more slightly more detailed variant of this define which uses __WFI() if no timeout is given, else __WFI() for a timeout.

dpgeorge · 2025-04-23T13:49:48Z

ports/renesas-ra/mphalport.h

+#define MICROPY_INTERNAL_WFE(TIMEOUT_MS)                        \
+    do {                                                        \
+        if ((TIMEOUT_MS) < 0) {                                 \
+            __WFE();                                            \


I'm not sure WFE is correct to use on this port. It doesn't do SEV anywhere, and SCB_SCR_SEVONPEND is not enabled (as far as I can tell), so it's unlikely that the WFE will ever wake up.

Eg this WFE will be run each time mp_hal_stdin_rx_chr() loops around, and so it's supposed to wake whenever a new character is available, ie an "event" occurs. But I can't see how that would work in the current code (would need to add SEV's in the right locations).

Oh interesting, I don't know much about these internals.
I had done some minimal testing at repl (over uart) which I just repeated:

anl@STEP:~/micropython/ports/renesas-ra$ mpremote u0 Connected to MicroPython at /dev/ttyUSB0 Use Ctrl-] or Ctrl-x to exit this shell >>> MPY: sync filesystems MPY: soft reboot MicroPython v1.26.0-preview.25.g0f1c2c1f25.dirty on 2025-04-24; EK-RA4W1 with RA4W1 Type "help()" for more information. >>> >>> import time >>> time.sleep(1) >>> while True: ... time.sleep(1) ... Traceback (most recent call last): File "<stdin>", line 2, in <module> KeyboardInterrupt: >>> >>> while True: ... pass ... Traceback (most recent call last): File "<stdin>", line 2, in <module> KeyboardInterrupt: >>>

It may be that interrupts automatically set the event flag on these MCUs. In that case, WFI and WFE are equivalent, in which case this macro should just unconditionally use one of them.

We need to get this right because it's tricky and can have lots of impacts on the system if not done the right way.

I went hunting on their website for a reference manual that covers the core in this way, found nothing.

Looking at the source, these do just both go to cmsis that implements them as the two wfi/wfe assembly commands which doesn't really help.

Based on the lack of data I'm certainly inclined to take the route of simple / safe and just use the one-liner WFI command instead.

Yes, I think just a simple WFI would be best here.

Signed-off-by: Andrew Leech <andrew@alelec.net>

dpgeorge added the port-renesas-ra label Sep 30, 2024

andrewleech self-assigned this Oct 1, 2024

andrewleech force-pushed the renesas_mp_event_wait_indefinite branch from 1cf893a to 956e6a1 Compare October 1, 2024 10:19

andrewleech changed the title ~~DRAFT: ports/renesas: Replace MICROPY_EVENT_POLL_HOOK with mp_event_wait.~~ ports/renesas: Replace MICROPY_EVENT_POLL_HOOK with mp_event_wait. Oct 1, 2024

andrewleech force-pushed the renesas_mp_event_wait_indefinite branch from ac1c916 to acecb0f Compare October 1, 2024 11:03

andrewleech commented Oct 1, 2024

View reviewed changes

dpgeorge added this to the release-1.26.0 milestone Apr 22, 2025

andrewleech force-pushed the renesas_mp_event_wait_indefinite branch 2 times, most recently from ed8555c to 0f1c2c1 Compare April 23, 2025 04:28

dpgeorge reviewed Apr 23, 2025

View reviewed changes

pi-anl added 2 commits May 1, 2025 11:09

ports/renesas: Replace MICROPY_EVENT_POLL_HOOK with mp_event_wait.

c67c92b

Signed-off-by: Andrew Leech <andrew@alelec.net>

drivers/esp_hosted: Replace EVENT_POLL_HOOK with mp_event_wait_ms.

57e605b

Signed-off-by: Andrew Leech <andrew@alelec.net>

andrewleech force-pushed the renesas_mp_event_wait_indefinite branch from 0f1c2c1 to 57e605b Compare May 1, 2025 01:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ports/renesas: Replace MICROPY_EVENT_POLL_HOOK with mp_event_wait. #15929

ports/renesas: Replace MICROPY_EVENT_POLL_HOOK with mp_event_wait. #15929

andrewleech commented Sep 29, 2024 •

edited

Loading

dpgeorge commented Sep 30, 2024

andrewleech commented Oct 1, 2024

andrewleech commented Oct 1, 2024

andrewleech Oct 1, 2024

iabdalkader Oct 1, 2024

iabdalkader Oct 1, 2024

andrewleech Oct 3, 2024 •

edited

Loading

iabdalkader commented Oct 4, 2024

iabdalkader commented Oct 10, 2024

andrewleech commented Oct 10, 2024

dpgeorge commented Apr 22, 2025

andrewleech commented Apr 23, 2025

dpgeorge Apr 23, 2025

andrewleech Apr 23, 2025

dpgeorge Apr 24, 2025

andrewleech May 1, 2025

dpgeorge May 1, 2025

		@@ -126,7 +126,7 @@ int mp_bluetooth_hci_controller_init(void) {
		if (mp_bluetooth_hci_uart_any()) {

ports/renesas: Replace MICROPY_EVENT_POLL_HOOK with mp_event_wait. #15929

Are you sure you want to change the base?

ports/renesas: Replace MICROPY_EVENT_POLL_HOOK with mp_event_wait. #15929

Conversation

andrewleech commented Sep 29, 2024 • edited Loading

Summary

Testing

dpgeorge commented Sep 30, 2024

andrewleech commented Oct 1, 2024

andrewleech commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewleech Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

iabdalkader commented Oct 4, 2024

iabdalkader commented Oct 10, 2024

andrewleech commented Oct 10, 2024

dpgeorge commented Apr 22, 2025

andrewleech commented Apr 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewleech commented Sep 29, 2024 •

edited

Loading

andrewleech Oct 3, 2024 •

edited

Loading