earlephilhower / arduino-pico

Raspberry Pi Pico Arduino core, for all RP2040 and RP2350 boards
GNU Lesser General Public License v2.1
2.03k stars 421 forks source link

wrong PICO_STACK_SIZE #1814

Closed traxanos closed 11 months ago

traxanos commented 11 months ago

hi,

the PICO_STACK_SIZE ist specified with 2048. but in map you used 0x1000 (4096). This means that the stack addresses cannot be determined correctly with:

0x20042000 - PICO_STACK_SIZE

there is another problem with the dynamic memory allocation for core1. i have no way of determining the current memory address so that i can determine the consumption. https://github.com/earlephilhower/arduino-pico/blob/master/cores/rp2040/main.cpp#L140-L144

Thats my current helper:

uint32_t stackPointer()
{
    uint32_t *sp;
    asm volatile("mov %0, sp"
                 : "=r"(sp));
    return (uint32_t)sp;
}

int freeStackSize()
{
    const uint16_t stackSize = 0x1000; // Bug: PICO_STACK_SIZE deliver wrong size
    const unsigned int sp = stackPointer();
    uint32_t ref = 0x20042000 - stackSize;
    if (rp2040.cpuid() == 1) ref -= stackSize;
    return sp - ref;
}

Why don't you solve the problem as follows:

multicore_launch_core1_with_stack(main1, (uint32_t *)(0x20042000 - PICO_STACK0_SIZE - PICO_STACK1_SIZE), PICO_STACK1_SIZE);
multicore_launch_core1_with_stack(main1, (uint32_t *)(0x20042000 - PICO_STACK_SIZE - PICO_STACK_SIZE), PICO_STACK_SIZE);

then i could continue to determine the stack usage cleanly. which would also not be bad if the functions were to migrate to the RP2040 class

earlephilhower commented 11 months ago

The allocation is optional for core1 and it was only to allow some folks who asked for a full 8K for both cores.

The PICO_STACK_SIZE macro is correct at 2048 (it's defined as the # of 4-byte words, not a byte count) in single core mode (probably 95%+ of users) or dual-core with allocated stack for core1.

So I'm not really seeing any issue here. If you want to make a PR that assigns the allocated stack for core1 (only in the case it's enabled) to a global or something, I'd be happy to look at it.

Thx!

traxanos commented 11 months ago

well we use core1 and we always have the problem that the stack memory is too small. that is also the reason why we monitor the free size. now i want to enlarge the stack, but i can’t until i know the addresses.

hence my question, why this happens dynamically at runtime and why the size is fixed. that’s why my suggestion was to find a different or better solution.

here is another suggestion:

extern "C" {
    uint16_t __stackSizeCore0 = 0x2000;
    uint16_t __stackSizeCore1 = 0x0;
    uint32_t *__stackAddressCore0 = (uint32_t *)(0x20042000 - __core0StackSize);
    uint32_t *__stackAddressCore1 = nullptr;

    if (setup1 || loop1) {
        if(core1_separate_stack) {
            __stackSizeCore1 = 0x2000;
            __stackAddressCore1 = (uint32_t*)malloc(__stackSizeCore1)
            // multicore_launch_core1_with_stack(main1, __stackAddressCore1, __stackSizeCore1);
        } else {
            __stackSizeCore0 = 0x1000;
            __stackSizeCore1 = 0x1000;
            __stackAddressCore0 = (uint32_t *)(0x20042000) - __core0StackSize;
            __stackAddressCore1 = __stackAddressCore0 - __stackSizeCore1;
        }
    }
};

i would have preferred to control the whole thing by define, but the proposal fits in with the current implementation.

traxanos commented 11 months ago

even at the risk of building it for free, i would prefer to make a suggestion including a merge request.