modm-io / modm

modm: a C++23 library generator for AVR and ARM Cortex-M devices
https://modm.io
Mozilla Public License 2.0
748 stars 132 forks source link

[fiber] Add more concurrency classes #1026

Closed salkinium closed 5 months ago

salkinium commented 1 year ago

Fibers need a standard set of efficient constructs. I modelled these classes after the C++ thread concurrency interfaces. Not sure if useful, but also probably not really much room for improvement anyways.

salkinium commented 6 months ago

It's still missing unit tests for latch, barrier and condition_variable and a bunch of documentation, however, it would be amazing if this could get reviewed with a special focus on C++ "conformity". This implementation should be interrupt-safe (when it makes sense for the functions), so that we can use then in the HAL as signalling primitives for operation completions in the future.

I also noticed that avrlibstd++ does not have <atomic>, I assume it was really annoying to port, @chris-durand? If it's an issue with the builtin gcc atomics not being implemented for avr-gcc, we can reuse the Cortex-M0 ones now.

chris-durand commented 6 months ago

I also noticed that avrlibstd++ does not have <atomic>, I assume it was really annoying to port, @chris-durand? If it's an issue with the builtin gcc atomics not being implemented for avr-gcc, we can reuse the Cortex-M0 ones now.

I haven't tried to be honest but I expect that the same solution as for Cortex-M0 will work. The header is mostly implemented using compiler builtins. I'd expect it to fall back to calling a library function in case the operation is not atomic on AVR. If you are lucky just dropping in the headers and compiling atomics_c11_cortex.cpp.in for AVR could be sufficient.

C++20 atomic wait which requires OS support shouldn't be an issue because it is only enabled when _GLIBCXX_HAS_GTHREADS is set.

chris-durand commented 6 months ago

I'll take a look at the PR over the weekend.

salkinium commented 6 months ago

If you are lucky just dropping in the headers and compiling atomics_c11_cortex.cpp.in for AVR could be sufficient.

Yup that just works, thanks!

salkinium commented 5 months ago

@chris-durand, I added ARMv8-M stack protection, could you check if the example/generic/fiber still works? I don't have a Cortex-M33 in my collection.

chris-durand commented 5 months ago

@chris-durand, I added ARMv8-M stack protection, could you check if the example/generic/fiber still works? I don't have a Cortex-M33 in my collection.

When running this on an STM32L5 I get a hardfault. The stack overflow bit (STKOF, bit 4) in UFSR is set. The fault happens inside modm_context_jump, triggered from the call to yield() inside the lambda of fiber_y1.

chris-durand commented 5 months ago

Let me know if there is something specific I can check.

chris-durand commented 5 months ago

Screenshot_20240514_005023

Listing: https://gist.github.com/chris-durand/4f3deae41aacbfd0cad2c9d067df0b5a

salkinium commented 5 months ago

Hm, so setting MSPLIM is no problem, setting PSPLIM once on context_start is no problem either, but then on the first context switch it fails? ugh.

Could you try to change the assembly in context_jump?

  1. %% elif with_psplim
    -       "ldm r1, {r1-r2}    \n\t"
    -       "mov sp,  r1        \n\t"   // Set PSP to ctx->sp
    -       "msr psplim, r2     \n\t"   // Set PSPLIM to ctx->bottom
    +       "ldm r1, {r2-r3}    \n\t"
    +       "mov sp,  r2        \n\t"   // Set PSP to ctx->sp
    +       "msr psplim, r3     \n\t"   // Set PSPLIM to ctx->bottom
    %% else
  2. %% elif with_psplim
    -       "ldm r1, {r1-r2}    \n\t"
    -       "mov sp,  r1        \n\t"   // Set PSP to ctx->sp
    -       "msr psplim, r2     \n\t"   // Set PSPLIM to ctx->bottom
    +       "ldr sp, [r1]       \n\t"   // Set PSP to ctx->sp
    +       "ldr r2, [r1, #4]   \n\t"
    +       "msr psplim, r2     \n\t"   // Set PSPLIM to ctx->bottom
    %% else
  3. %% elif with_psplim
    -       "ldm r1, {r1-r2}    \n\t"
    -       "mov sp,  r1        \n\t"   // Set PSP to ctx->sp
    -       "msr psplim, r2     \n\t"   // Set PSPLIM to ctx->bottom
    +       "ldm r1, {r2-r3}    \n\t"
    +       "msr psplim, r3     \n\t"   // Set PSPLIM to ctx->bottom
    +       "mov sp,  r2        \n\t"   // Set PSP to ctx->sp
    %% else
  4. %% elif with_psplim
    -       "ldm r1, {r1-r2}    \n\t"
    -       "mov sp,  r1        \n\t"   // Set PSP to ctx->sp
    -       "msr psplim, r2     \n\t"   // Set PSPLIM to ctx->bottom
    +       "ldr r2, [r1, #4]   \n\t"
    +       "msr psplim, r2     \n\t"   // Set PSPLIM to ctx->bottom
    +       "ldr sp, [r1]       \n\t"   // Set PSP to ctx->sp
    %% else
chris-durand commented 5 months ago

No success with any of the options. I've also tried replacing the code in both context_start and context_jump.

salkinium commented 5 months ago

Ok, then I'll descope it for now and will order a bunch of new development boards and debug it some more. Thanks for your help!