Skip to content

esp32/main: Fix mp_deinit order.#17423

Closed
Carglglz wants to merge 1 commit into
micropython:masterfrom
Carglglz:fix-esp32-mp-deinit
Closed

esp32/main: Fix mp_deinit order.#17423
Carglglz wants to merge 1 commit into
micropython:masterfrom
Carglglz:fix-esp32-mp-deinit

Conversation

@Carglglz

@Carglglz Carglglz commented Jun 3, 2025

Copy link
Copy Markdown
Contributor

Move mp_deinit to be called before gc_sweep_all to prevent mp_deinit from double freeing memory if MICROPY_PORT_DEINIT_FUNC is defined and the function tries to free memory already freed by gc_sweep_all

Summary

Prevent a core panic on soft-reset caused by "double freeing" memory.

Testing

Tested with esp32 and lvgl/lv_binding_micropython#383

Trade-offs and Alternatives

Probably this PR #16063

Move `mp_deinit` to be called before `gc_sweep_all` to prevent
`mp_deinit` from double freeing memory if `MICROPY_PORT_DEINIT_FUNC` is
defined and the function tries to free memory already freed by
`gc_sweep_all`

Signed-off-by: Carlosgg <carlosgilglez@gmail.com>
@dpgeorge

dpgeorge commented Jun 4, 2025

Copy link
Copy Markdown
Member

I'm not sure this is the right thing to do. Once mp_deinit() is called, no more MicroPython API functions should be called (eg because the GIL is released).

Can you provide a minimal example of the problem you are trying to solve?

@Carglglz

Carglglz commented Jun 4, 2025

Copy link
Copy Markdown
Contributor Author

Can you provide a minimal example of the problem you are trying to solve?

Deinit a USER_C_MODULE, in this case lvgl on soft-reset, @andrewleech explains this with much more detail in #16063 (comment)

So I've found MICROPY_PORT_DEINIT_FUNC could be used as a simple alternative for this, however
I probably need to investigate it further since stm32 port seems to work fine without this change, but the esp32 port raises a core panic and backtrace points to mp_deinit.

What happens is that lvgl uses micropython garbage collector to allocate/free memory, so at soft-reset gc_sweep_all() frees everything and then lvgl.deinit() at mp_deinit will try to free already freed memory which in esp32 case raises a core panic.

Note that moving gc_sweep_all and esp_native_code_free_all after mp_deinit works too.

Once mp_deinit() is called, no more MicroPython API functions should be called

This includes the garbage collector I guess?

@dpgeorge

dpgeorge commented Jun 4, 2025

Copy link
Copy Markdown
Member

This includes the garbage collector I guess?

Correct. It can't run because the GIL is released.

What happens is that lvgl uses micropython garbage collector to allocate/free memory, so at soft-reset gc_sweep_all() frees everything and then lvgl.deinit() at mp_deinit will try to free already freed memory which in esp32 case raises a core panic.

It sounds like lvgl should be changed so that it doesn't free memory in its deinit function. It should rely on gc_sweep_all() to do that for it.

lvgl can also use finalisers, which will always be run as part of gc_sweep_all() if there are any objects remaining at soft reset time.

@Carglglz

Carglglz commented Jun 4, 2025

Copy link
Copy Markdown
Contributor Author

It sounds like lvgl should be changed so that it doesn't free memory in its deinit function. It should rely on gc_sweep_all() to do that for it.

Ah that was it, I've only needed to reset lvgl root pointers, gc_sweep_all() handles the rest, @dpgeorge thanks for the support 🙏🏼

@Carglglz Carglglz closed this Jun 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants