-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix cardputer boot loop, and do some cleanup #10607
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
Conversation
49024ef to
c121d73
Compare
- For `cardputer_keyboard.c`, allocate `DemuxKeyMatrix` it creates and the objects it holds (all the way down) on the port heap, because it lives past VM's. - To do the above, add support for non-VM heap for `DemuxKeyMatrix` and the `keypad` allocations it needs by passing an arg `use_gc_allocator`, as is done in `lvfontio`. - Add `mp_obj_port_malloc()`, `mp_obj_port_malloc_var()`, `mp_obj_new_port_tuple()`, all of which allocate on the port heap, not the VM heap. - Add `port_malloc_zero()`, because `port_malloc()` did not zero out its allocation, unlike VM heap allocations. I got caught by this. - Move `port_gc_collect()` from `main.c` to `supervisor/port.c` for consistency. This function is not actually used by anyone, but I was considering using it. - Add an `MP_WEAK` `mp_board_init()`, for board-specific initializations needed when a VM starts. Also considered, but not used. Added anyway for possible future use. - Add an `MP_WEAK` `board_gc_collect()`, for board-specific gc. Not used. - Add an `MP_WEAK` `port_gc_collect()`, for port-specific gc. Not used. - Don't duplicate keyboard code between `boards/m5stack_cardputer` and `boards/m5stack_cardputer_ros`. Instead, add a `ports/espressif/module` directory for this code, in the style of a port-specific `module` directory, like the port-specific `bindings` directories.
c121d73 to
de2efe0
Compare
Avoids the new Wunterminated-string-literal when compiled with gcc 15.1. It would be preferable to just disable this warning, but Clang -Wunknown-warning-option kicks in even when disabling warnings so this becomes fiddly to apply. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
Avoids the new Wunterminated-string-literal when compiled with gcc 15.1. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
|
I cherry-picked two commits from micropython#17269 to fix a gcc 15 build problem on Windows. |
tannewt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor thing. Thanks!
supervisor/shared/board.c
Outdated
| // Do-nothing so not all boards need to provide this function. | ||
| MP_WEAK void mp_board_init(void) { | ||
| } | ||
|
|
||
| // Do-nothing so not all boards need to provide this function. | ||
| MP_WEAK void board_gc_collect(void) { | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you actually need these? I'd love to move away from MP_WEAK since these can be used even when not expected. In other words, the compiler doesn't check for correctness here. A #define to enable the function would be compile checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't need them, so I'll just remove them. I just spent the time figuring out where to put them, but I can come back to the commit to find that.
I left port_gc_collect() because it was already there.
I'll open an issue about replacing MP_WEAK functions with some guards instead. Did you mean that one might accidentally replace a weak function by mistake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean that one might accidentally replace a weak function by mistake?
Nope, you may think you are overriding one but don't actually. So, it'll compile and link but not run the implementation you expected. (Lots of IDF issues for us were this.)
tannewt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Detailed changes:
cardputer_keyboard.c, allocateDemuxKeyMatrixit creates and the objects it holds (all the way down) on the port heap, because it lives past VM's.DemuxKeyMatrixand thekeypadallocations it needs by passing an arguse_gc_allocator, as is done inlvfontio.mp_obj_port_malloc(),mp_obj_port_malloc_var(),mp_obj_new_port_tuple(), all of which allocate on the port heap, not the VM heap.port_malloc_zero(), becauseport_malloc()did not zero out its allocation, unlike VM heap allocations. I got caught by this.port_gc_collect()frommain.ctosupervisor/port.cfor consistency. This function is not actually used by anyone, but I was considering using it.-
Add anMP_WEAKmp_board_init(), for board-specific initializations needed when a VM starts. Also considered, but not used. Added anyway for possible future use.Add anMP_WEAKboard_gc_collect(), for board-specific gc. Not used.boards/m5stack_cardputerandboards/m5stack_cardputer_ros. Instead, add aports/espressif/moduledirectory for this code, in the style of a port-specificmoduledirectory, like the port-specificbindingsdirectories.My original approach to this was to make the
cardputer_keyboardobject be allocated when the VM started up. That required less pervasive changes, and it worked fine, except for thePress any key...when the VM finished. So I had to make it live across VM's.I tested and @dglaude also tested (thanks!).