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

[Core]: Add support for CMSIS RTOS 2 Thread synchronization #458

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ad3154
Copy link
Member

@ad3154 ad3154 commented Mar 28, 2024

Describe your changes

This is an attempt to provide support for embedded RTOSes that use the CMSIS RTOS 2 abstraction, such as FreeRTOS, Keil RTX, and others.

My thinking is that this is 50 percent of the battle to supporting STM32 and other small ARM devices, with the other half being hardware plugins with specific HAL integrations for each micro.

In order to actually consume this, you'd need to also implement a proper hardware time-base for the stack using a hardware timer on those devices, which maybe we could also provide as part of a CAN plugin eventually. Additionally, you'd also need to start the RTOS before using the stack, which I think is acceptable (osKernelInitialize, and osKernelStart).

Either way, the point is, CMSIS RTOS2 is a common RTOS abstraction which would be nice to support.

The CMSIS RTOS 2 API is documented here

I added some super obvious infinite loops to catch issues between us and the kernel. I used loops because assert may not be supported on the target platforms, and since this file is included in the logger, I can't emit any log output here.

How has this been tested?

This is a draft until I can test it. I have a cheap STM32L4 eval board coming to help test it. Will update once I have more to share.

Tested on STM32L476 Nucleo dev kit with an external CAN transceiver running FreeRTOS and CMSIS 6.0 via the CMSIS_RTOS2 api.

The dev kit loaded the VT example and worked normally, even using > 1 thread as expected when used with the stm32l4 plugin which is not yet merged in,.

@ad3154 ad3154 added the enhancement New feature or request label Mar 28, 2024
@ad3154 ad3154 requested a review from GwnDaan March 28, 2024 21:12
@ad3154 ad3154 self-assigned this Mar 28, 2024
@ad3154 ad3154 mentioned this pull request Mar 31, 2024
@ad3154 ad3154 force-pushed the adrian/cmsis2-thread-sync branch from af62b24 to 35cbc69 Compare April 6, 2024 02:29
@ad3154 ad3154 force-pushed the adrian/cmsis2-thread-sync branch from 35cbc69 to 767458e Compare April 6, 2024 02:30
Copy link

sonarqubecloud bot commented Apr 6, 2024

@ad3154
Copy link
Member Author

ad3154 commented Apr 6, 2024

Marking as ready since it's now tested. It's a bit more invasive than I expected I guess, but latching onto the CMSIS abstraction for RTOSes seems very valuable, as it should give us very easy access to:

  • FreeRTOS
  • Zephyr
  • RTX
  • ThreadX
  • Azure RTOS
  • Others?

@ad3154 ad3154 marked this pull request as ready for review April 6, 2024 03:16
Comment on lines +149 to +153
#ifdef USE_CMSIS_RTOS2_THREADING
static void receive_thread_function(void *parent);
#else
void receive_thread_function();
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, since we'll be limited by C now I'd suggest we just refactor the old function instead of duplicating it. So we use the C void * pattern again which we previously refactored away from I believe 😆

Comment on lines +177 to +181
#if defined USE_CMSIS_RTOS2_THREADING
static void update_thread_function(void *);
#else
static void update_thread_function();
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Same with the duplication here

if (!target->receive_can_frame())
{
// There was no frame to receive, and osThreadYield may not exist
osDelay(1);
Copy link
Member

Choose a reason for hiding this comment

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

nice-to-have (non-blocking): I guess we can make generalize the thread "delay / sleep / yield" functions in the future

// or reduce its usage by statically allocating your thread's stack(s)
}
}
return nullptr != handle;
Copy link
Member

Choose a reason for hiding this comment

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

nit: handle can never be nullptr here, not sure if this is here for future plans otherwise might be better to change the function to like return void and name it prepare_handle?

nullptr,
0
};
handle = osMutexNew(&attributes);
Copy link
Member

Choose a reason for hiding this comment

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

Missing the while loop here?

Comment on lines +345 to +350
namespace std
{
using mutex = isobus::Mutex;
using recursive_mutex = isobus::RecursiveMutex;
using condition_variable = isobus::ConditionVariable;
} // namespace std
Copy link
Member

Choose a reason for hiding this comment

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

nit(non-blocking): I don't really like that we add parts to the std namespace here. If we want to keep expanding to more platforms that don't have the mutex / thread / condition_variable header file implemented, then it might be worth it to do more investigation there and see how to best structure the library such that we do it nice and organized (with proper documentation hehe 😄)

{
/// @brief A wrapper around a CMSIS RTOS 2 mutex.
/// @details See definition at https://www.keil.com/pack/doc/CMSIS/RTOS2/html/group__CMSIS__RTOS.html
class Mutex
Copy link
Member

Choose a reason for hiding this comment

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

thought: Mutex is not a template class, so we should separate the implementation and definition?

/// @brief A template class for a lock free queue.
/// @tparam T The item type for the queue.
template<typename T>
class LockFreeQueue
Copy link
Member

Choose a reason for hiding this comment

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

nit: LockFreeQueue is already in the file below. I think it makes sense if we just use a separate #ifdef directive there that covers both cases

Comment on lines +132 to +139
while (true)
{
// If your code is stuck in here, that means you did something
// very wrong, like recursively locked this mutex, or tried to
// lock the mutex before the OS was initialized, or called this in
// an interrupt service routine.
// osRetVal may contain more information.
}
Copy link
Member

Choose a reason for hiding this comment

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

I like the way you used these for loops. It's a clever trick to help with finding where things go wrong with debugging. Though I wonder if it's always the way to go in production? I'm not sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants