Settings.toml-configurable pystack

[bill88t]

This pr is the replacement for #7396.
It aims to provide a way to configure pystack size after the build.
It also will provide a way to use stackless without rebuilding.

Currently working:

• pystack is now a supervisor allocation.
• Resize via settings.toml

Support stackless by setting value to 0.

[bill88t]

Stackless will not be implemented with this pr as it would require modification of every gc function call.
And honestly there isn't much benefit to that since adjustments can be made on-the-fly.

Also, the settings.toml variable CIRCUITPY_PYSTACK_SIZE will be multiplied by size_t, so that the user cannot give invalid sizes (pystack_size % sizeof(size_t) != 0)

Do note, stackless is still usable by setting the build flag the old way.

[dhalbert]

it would require modification of every gc function call

Could you explain why this is? Could you use a global instead?

[bill88t]

Sure, gc_collect_start initially defined in py/gc.h:61 and populated in py/gc.c:365 contains the problematic piece of code:

#if MICROPY_ENABLE_PYSTACK
// Trace root pointers from the Python stack.
ptrs = (void **)(void *)MP_STATE_THREAD(pystack_start);
gc_collect_root(ptrs, (MP_STATE_THREAD(pystack_cur) - MP_STATE_THREAD(pystack_start)) / sizeof(void *));
#endif

This code block is disabled on stackless.
However, leaving it in, when a pystack is not allocated and initialised leaves pystack_start and all of those variables undefined.
These variables cannot be reused as placeholders as they would make the system try to use the stack as if it's present there.
And regardless, we would have to get pystack & pystack_size in here forcing the modification of the function definition and the definition of gc_collect in turn. And the definition of gc_collect is used pretty much everywhere.
Making it global would perhaps help, but still we would have to do some weird hacks to force gc to not run that piece of code.
As a reminder, I am still a noob in microcontroller internals and my C experience is rather small, I may be wrong in all of this.

[tannewt]

Thank you so much for persevering on this! You are getting there. I added a few suggestions around the structure.

[bill88t]

Yea I kinda reworked it a bit, now it realloc's on reload (if needed) and it no longer multiplies the value it gets by sizeof(size_t). It got too confusing to calculate, even for me who wrote it.

Ended up turning them back to globals in hopes of simplifying.

[bill88t]

Pystack has to be allocated before heap, so this means it will have to be allocated before start_mp, hence the function I made.

[bill88t]

Added error messages, and safety for allocation failures. Now it's pretty much perfect.

[gneverov]

It's looking good!

[bill88t]

Once again, I think I am done.

As a final note, I remind you all that the old next_stack_limit was already broken and did not provide any functionality.
Should you guys still want it back in and removed at 9.0, I will gladly put it back, however it will still do nothing.

[bill88t]

Turns out next_stack_limit has a purpose. I put it back.
If CI doesn't pass I will just disable settable pystack for the failing boards.

[bill88t]

Alright and kicksat-sprite doesn't need to have rainbowio disabled. It just won't have resizable pystack.
So this pr removes no functionallity from any board.

[dhalbert]

kickstat-sprite has no use for rainbowio. The board is designed for space satellites, which don't need NeoPixel animations. If you can enable pystack on it with rainbowio disabled, that is better, since it's more likely to be useful for how it's used.

[bill88t]

This sadly doesn't build. 400 more bytes are needed.

[dhalbert]

_pixelmap and/or adafruit_pixelbuf could be dropped from kicksat-sprite. Test a build locally with the largest language that doesn't fit. (make BOARD=kicksat-sprite TRANSLATION=...)

[bill88t]

244240 bytes used, 1520 bytes free in flash firmware space out of 245760 bytes (240.0kB).
31044 bytes used, 165564 bytes free in ram for stack and heap out of 196608 bytes (192.0kB).

For TRANSLATION=ja, which takes up the most space.

[tannewt]

Looks good to me! Thank you @bill88t!

[anecdata]

Usage is via settings.toml, e.g.: CIRCUITPY_PYSTACK_SIZE=2048?

And supervisor.runtime.next_stack_limit is intact and adjusts the C stack?

[bill88t]

Precisely.