Skip to content
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

[fiber] Add more concurrency classes #1026

Merged
merged 11 commits into from May 20, 2024

Conversation

salkinium
Copy link
Member

@salkinium salkinium commented May 20, 2023

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.

  • Add mutex and shared_mutex headers back avr-libstdcpp#35
  • modm::fiber::sleep -> modm::this_fiber::sleep_for
  • modm::this_fiber::sleep_until
  • modm::this_fiber::get_id
  • Align modm::fiber::Task to std::jthread interface, incl. stop_token.
  • stop_token and stop_source (simplified)
  • mutex
  • timed_mutex
  • recursive_mutex
  • recursive_timed_mutex
  • counting_semaphore (=binary_semaphore)
  • shared_mutex
  • shared_timed_mutex
  • once_flag
  • call_once
  • latch
  • barrier
  • condition_variable
  • unit tests
  • More documentation
  • Hardware testing
    • STM32F723
    • STM32G071
    • ATmega2650

@salkinium salkinium added this to the 2023q2 milestone May 20, 2023
@salkinium salkinium removed this from the 2023q2 milestone Jul 10, 2023
@salkinium salkinium force-pushed the feature/fiber_concurrency branch 2 times, most recently from 97c4cb4 to d9e13d0 Compare April 14, 2024 10:03
@salkinium salkinium added this to the 2024q2 milestone Apr 14, 2024
@salkinium salkinium force-pushed the feature/fiber_concurrency branch 3 times, most recently from 3526783 to 3f372b6 Compare April 14, 2024 21:19
@salkinium
Copy link
Member Author

salkinium commented Apr 14, 2024

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
Copy link
Member

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
Copy link
Member

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

@salkinium salkinium force-pushed the feature/fiber_concurrency branch 2 times, most recently from c8575fb to 377553c Compare April 20, 2024 16:44
@salkinium
Copy link
Member Author

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 salkinium force-pushed the feature/fiber_concurrency branch 2 times, most recently from 0d3fa53 to 6803ce0 Compare April 21, 2024 16:01
Copy link
Member

@chris-durand chris-durand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

I haven't managed to look at all the synchronization primitives, but will do tomorrow.

src/modm/processing/fiber/functions.hpp Outdated Show resolved Hide resolved
src/modm/processing/fiber/functions.hpp Outdated Show resolved Hide resolved
src/modm/processing/fiber/functions.hpp Outdated Show resolved Hide resolved
src/modm/processing/fiber/barrier.hpp Outdated Show resolved Hide resolved
src/modm/processing/fiber/mutex.hpp Outdated Show resolved Hide resolved
src/modm/processing/fiber/shared_mutex.hpp Outdated Show resolved Hide resolved
src/modm/processing/fiber/shared_mutex.hpp Outdated Show resolved Hide resolved
src/modm/processing/fiber/shared_mutex.hpp Outdated Show resolved Hide resolved
src/modm/processing/fiber/shared_mutex.hpp Outdated Show resolved Hide resolved
src/modm/processing/fiber/shared_mutex.hpp Outdated Show resolved Hide resolved
@salkinium salkinium force-pushed the feature/fiber_concurrency branch 8 times, most recently from d9b837a to 72ab66d Compare May 6, 2024 14:51
src/modm/processing/fiber/barrier.hpp Outdated Show resolved Hide resolved
src/modm/processing/fiber/barrier.hpp Outdated Show resolved Hide resolved
src/modm/processing/fiber/condition_variable.hpp Outdated Show resolved Hide resolved
@salkinium salkinium force-pushed the feature/fiber_concurrency branch 3 times, most recently from ae6328c to 407e1a6 Compare May 10, 2024 21:36
@salkinium salkinium marked this pull request as ready for review May 12, 2024 07:34
@salkinium
Copy link
Member Author

@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.

@salkinium salkinium force-pushed the feature/fiber_concurrency branch 2 times, most recently from c067a44 to 1dea124 Compare May 12, 2024 11:35
@chris-durand
Copy link
Member

@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
Copy link
Member

chris-durand commented May 13, 2024

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

@chris-durand
Copy link
Member

@salkinium
Copy link
Member Author

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
 %% 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
 %% 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
 %% 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
Copy link
Member

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

@salkinium
Copy link
Member Author

salkinium commented May 15, 2024

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!

@salkinium salkinium force-pushed the feature/fiber_concurrency branch 3 times, most recently from 13e0760 to 460bcee Compare May 20, 2024 17:10
@salkinium salkinium added the ci:hal Triggers the exhaustive HAL compile CI jobs label May 20, 2024
@salkinium salkinium merged commit 39a9f4d into modm-io:develop May 20, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
advanced 🤯 ci:hal Triggers the exhaustive HAL compile CI jobs feature 🚧
Development

Successfully merging this pull request may close these issues.

None yet

2 participants