DRNadler / FreeRTOS_helpers

Helpers for some common problems when using FreeRTOS, GCC, and newlib.
135 stars 20 forks source link

Still needed for STM32CubeIDE v1.7.0 and later? #10

Open g-mocken opened 1 year ago

g-mocken commented 1 year ago

At the time of writing, the current version of STM32CubeIDE is v.1.11.2. Its user manual has a section 2.7 stating:

Thread-safe wizard for empty projects and CDTTM projects STM32CubeIDE includes a thread-safe wizard to generate files to support the use of resources that can be updated by application code and interrupts or when using a real-time operating system.

In this wizard one can select among several strategies, which are described as follows in the manual:

  1. _User defined thread-safe implementation. User defined solution for handling thread-safety. NOTE: The stubs in stm32_lockuser.h needs to be implemented to gain thread-safety.

    1. Allow lock usage from interrupts. This implementation will ensure thread-safety by disabling all interrupts during e.g. calls to malloc. NOTE: Disabling all interrupts creates interrupt latency which might not be desired for this application!

    2. Deny lock usage from interrupts. This implementation assumes single thread of execution. Thread-safety dependent functions will enter an infinity loop if used in interrupt context.

    3. Allow lock usage from interrupts. Implemented using FreeRTOS locks. This implementation will ensure thread-safety by entering RTOS ISR capable critical sections during e.g. calls to malloc. By default this implementation supports 2 levels of recursive locking. Adding additional levels requires 4 bytes per lock per level of RAM. NOTE: Interrupts with high priority are not disabled. This implies that the lock is not thread-safe from high priority interrupts!

    4. Deny lock usage from interrupts. Implemented using FreeRTOS locks. This implementation will ensure thread-safety by suspending all tasks during e.g. calls to malloc. NOTE: Thread-safety dependent functions will enter an infinity loop if used in interrupt context.

I do not remember such options being present in releases earlier than 1.7.0, whose release notes and erratum list "Thread-safe malloc solution" for the first time.

While the above sounds very promising, does it really mean that Dave's excellent FreeRTOS helpers are no longer needed? While they work very well, setup and usage together with STM32CubeIDE's code generation is somewhat tiring, so it would be nice to have an equivalent one-click replacement.

DRNadler commented 1 year ago

Hi Guido - I haven't reviewed this carefully, but some of it looks daft or worse. Malloc in an ISR is never, ever, ever a good idea, WTF? Are they still flogging crap drivers that do that? You CANNOT "reenter a critical section" in an ISR; why are they talking about recursive mutex? I fear morons stuck in an "/infinity loop/". Aaarrrgggg..... Really, I would take a VERY careful look before using any of this, sorry... Best Regards, Dave

PS: We ditched an STM32-based project because their driver quality was so poor. It was too expensive to fix it; cheaper to move to an alternative product.

On 2/20/2023 12:03 PM, Guido Mocken wrote:

At the time of writing, the current version of STM32CubeIDE is v.1.11.2. Its user manual has a section 2.7 stating:

/Thread-safe wizard for empty projects and CDTTM projects STM32CubeIDE includes a thread-safe wizard to generate files to support the use of resources that can be updated by application code and interrupts or when using a real-time operating system./

In this wizard one can select among several strategies, which are described as follows in the manual:

1.

/User defined thread-safe implementation.
User defined solution for handling thread-safety.
NOTE: The stubs in stm32_lock_user.h needs to be implemented to gain
thread-safety./

2.

/Allow lock usage from interrupts.
This implementation will ensure thread-safety by disabling all
interrupts
during e.g. calls to malloc.
NOTE: Disabling all interrupts creates interrupt latency which
might not be desired for this application!/

3.

/Deny lock usage from interrupts.
This implementation assumes single thread of execution.
Thread-safety dependent functions will enter an infinity loop
if used in interrupt context./

4.

/Allow lock usage from interrupts. Implemented using FreeRTOS locks.
This implementation will ensure thread-safety by entering RTOS ISR
capable
critical sections during e.g. calls to malloc.
By default this implementation supports 2 levels of recursive locking.
Adding additional levels requires 4 bytes per lock per level of RAM.
NOTE: Interrupts with high priority are not disabled. This implies
that the lock is not thread-safe from high priority interrupts!/

5.

/Deny lock usage from interrupts. Implemented using FreeRTOS locks.
This implementation will ensure thread-safety by suspending all tasks
during e.g. calls to malloc.
NOTE: Thread-safety dependent functions will enter an infinity loop
if used in interrupt context./

I do not remember such options being present in releases earlier than 1.7.0, whose release notes and erratum list "Thread-safe malloc solution" for the first time.

While the above sounds very promising, does it really mean that Dave's excellent FreeRTOS helpers are no longer needed? While they work very well, setup and usage together with STM32CubeIDE's code generation is somewhat tiring, so it would be nice to have an equivalent one-click replacement.

— Reply to this email directly, view it on GitHub https://github.com/DRNadler/FreeRTOS_helpers/issues/10, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADBYXZC3ACBIFKXSE5QN5XLWYOPX7ANCNFSM6AAAAAAVCDOHKY. You are receiving this because you are subscribed to this thread.Message ID: @.***>

-- Dave Nadler, USA East Coast voice (978) @.***, Skype Dave.Nadler1

g-mocken commented 1 year ago

Thanks for the quick reply, Dave. Meanwhile I finally discovered a more detailed description of what they are actually doing. The document does not specify to which version of STM32CubeIDE it applies, but it is dated late 30/11/2021. Not being a newlib/malloc expert, it will take me a while to study this application note, while of course keeping in mind what you wrote above.

Xenoamor commented 1 year ago

I've had a look at the code generated by STM32CubeMX 6.7.0

I've specifically only looked at what is generated under Project Manager -> Thread-safe Locking Strategy -> FreeRTOS Strategy 4 & 5. I've also used the CMSIS_V2 FreeRTOS option

Strategy 4: as they describe it disables lower priority interrupts during an ISR when you call something like malloc. The issue with this is as they point out if you call malloc in an ISR and a higher priority interrupt comes in and also calls it the whole thing fails. I honestly can't see a use for this. It could easily introduce hard to track bugs. They do openly say "Using C library functions, which may need synchronization, at the same time from both the main application (or thread) and from the ISR, is not recommended." so I don't think they are endorsing this approach but it does seem odd to include it.

Strategy 5: this disables "tasks" when something like malloc is called. I think this is very comparable to this code? There's no _sbrk_r implementation, I'm unsure if a _sbrk_r function is required or if newlib brings a suitable default with it. Personally I always override malloc and friends to point to the FreeRTOS versions. You can implement an "Error_Handler()" which gets called if you try to lock during an interrupt else it will just sit in an infinite loop.

On a side note they implement the __retarget_lock functions. These are needed for things like stdio and FILE operations, not really relevant to bare-metal but maybe someone will use it occasionally. They use newlib's malloc for the locks though which is odd, especially under FreeRTOS. There's a better version of that here which uses the correct primitives. Apparently this can use a fair amount of RAM though as FreeRTOS mutexs are quite beefy

It's worth noting I hit a few bugs with the code compilation for a STM32G061C6T device though.

  1. It was supposed to warn me to enable configUSE_NEWLIB_REENTRANT in the cube but didn't. Looks like it does at compile time though
  2. It set both -DSTM32_THREAD_SAFE_STRATEGY=4 and -DSTM32_THREAD_SAFE_STRATEGY=5 at the same time in my makefile when I wanted strategy 5. This is fairly nasty as it actually makes it use strategy 4 which is the awful one
g-mocken commented 1 year ago

Upon closer inspection of their code and Application Note, their variants #4 and #5 seem to roughly correspond to the behavior one gets from Dave's code with MALLOCS_INSIDE_ISRs either defined or undefined, respectively. The recursive mutex in variant #4 however is still a bit of a mystery to me. If, as they say, high-priority ISRs are strictly not allowed to call malloc(), it should not hurt, while allowing recursive locking inside low-priority ISRs. I do not know if this feature is needed. Also, their new _sbrk() no longer refers to the current stack pointer, but only to constant linker symbol definitions.

(These are just my observations, no actual test was performed.)

DRNadler commented 1 year ago

Thanks for the update. Sounds like STM still have a huge mess...

On 2/21/2023 6:37 AM, Xenoamor wrote:

I've had a look at the code generated by STM32CubeMX 6.7.0

I've specifically only looked at what is generated under Project Manager -> Thread-safe Locking Strategy -> Allow/deny lock usage from interrupts. I've also used the CMSIS_V2 FreeRTOS option

Strategy 4: as they describe it disables lower priority interrupts during an ISR when you call something like malloc. The issue with this is as they point out if you call malloc in an ISR and a higher priority interrupt comes in and also calls it the whole thing fails. I honestly can't see a use for this. It could easily introduce hard to track bugs. They do openly say "Using C library functions, which may need synchronization, at the same time from both the main application (or thread) and from the ISR, is not recommended." so I don't think they are endorsing this approach but it does seem odd to include it.

Strategy 5: this disables "tasks" when something like malloc is called. I think this is very comparable to this code? There's no _sbrk_r implementation, I'm unsure if a _sbrk_r function is required or if newlib brings a suitable default with it. Personally I always override malloc and friends to point to the FreeRTOS versions. You can implement an "Error_Handler()" which gets called if you try to lock during an interrupt else it will just sit in an infinite loop.

On a side note they implement the __retarget_lock functions. These are needed for things like stdio and FILE operations, not really relevant to bare-metal but maybe someone will use it occasionally. They use newlib's malloc for the locks though which is odd, especially under FreeRTOS. There's a better version of that here https://gist.github.com/thomask77/65591d78070ace68885d3fd05cdebe3a which uses the correct primitives.

It's worth noting I hit a few bugs with the code compilation for a STM32G061C6T device though.

  1. It was supposed to warn me to enable configUSE_NEWLIB_REENTRANT in the cube but didn't. Looks like it does at compile time though
  2. It set both -DSTM32_THREAD_SAFE_STRATEGY=4 and -DSTM32_THREAD_SAFE_STRATEGY=5 at the same time in my makefile when I wanted strategy 5. This is fairly nasty as it actually makes it use strategy 4 which is the awful one

— Reply to this email directly, view it on GitHub https://github.com/DRNadler/FreeRTOS_helpers/issues/10#issuecomment-1438329274, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADBYXZEDVO4S7D24V5PZENDWYSSHTANCNFSM6AAAAAAVCDOHKY. You are receiving this because you commented.Message ID: @.***>

-- Dave Nadler, USA East Coast voice (978) @.***, Skype Dave.Nadler1

DRNadler commented 1 year ago

Last I looked (couple years back) they STM drivers called malloc (USB etc). Anyway, thanks for the update, sounds like STM still have a huge mess...

On 2/21/2023 7:00 AM, Guido Mocken wrote:

Upon closer inspection of their code and Application Note, their variants #4 https://github.com/DRNadler/FreeRTOS_helpers/issues/4 and #5 https://github.com/DRNadler/FreeRTOS_helpers/issues/5 seem to roughly correspond to the behavior one gets from Dave's code with MALLOCS_INSIDE_ISRs either defined or undefined, respectively. The recursive mutex in variant #4 https://github.com/DRNadler/FreeRTOS_helpers/issues/4 however is still a bit of a mystery to me. If, as they say, high-priority ISRs are strictly not allowed to call malloc(), it should not hurt, while allowing recursive locking inside low-priority ISRs. I do not know if this feature is needed. Also, their new _sbrk() no longer refers to the current stack pointer, but only to constant linker symbol definitions.

(These are just my observations, no actual test was performed.)

— Reply to this email directly, view it on GitHub https://github.com/DRNadler/FreeRTOS_helpers/issues/10#issuecomment-1438357236, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADBYXZHN5D4TY7LRLE6DPADWYSU5BANCNFSM6AAAAAAVCDOHKY. You are receiving this because you commented.Message ID: @.***>

-- Dave Nadler, USA East Coast voice (978) @.***, Skype Dave.Nadler1

g-mocken commented 1 year ago

I cannot comment on previous or current USB drivers, as I only ever used I2C, SPI, UART, GPIO HAL-drivers together with your MALLOCS_INSIDE_ISR-undefined variant, but I might now go ahead and try out their variant 5. If it fails, I know that I have a fallback option here.

DRNadler commented 1 year ago

You have to do the analysis, not just "wait til it fails" when discussing race and concurrency conditions... Be careful out there!

On 2/21/2023 9:26 AM, Guido Mocken wrote:

I cannot comment on previous or current USB drivers, as I only ever used I2C, SPI, UART, GPIO HAL-drivers together with your MALLOCS_INSIDE_ISR-undefined variant, but I might now go ahead and try out their variant 5. If it fails, I know that I have a fallback option here.

-- Dave Nadler, USA East Coast voice (978) @.***, Skype Dave.Nadler1

tdjastrzebski commented 1 year ago

The latest CubeMX keeps _sbrk in sysmem.c file so it seems that to use heap_useNewlib_ST it is enough just to exclude heap_4.c and sysmem.c and include heap_useNewlib_ST.c.
It may be a good idea to update the docs.
Next weeks I go to Embedded World expo in Nuremberg, Germany mainly to look for the alternatives to ST. I think ST is yet to realize that these days software is at least equally important as hardware.