How to implement locking between ISR and main() in a HAL-based architecture. What is the added value of __HAL_(UN)LOCK() in the HAL UART source code as it is currently implemented?

[Johi]

Hello,

I need an ISR that fills a circular buffer. Data will be consumed in the main() loop and critical section/mutex locking functionality is required to manage properly. So I looked at the HAL UART code for inspiration and came across __HAL_LOCK(huart)

1) What is the added value of the __HAL_LOCK(huart); and __HAL_UNLOCK(huart); statements in the HAL_UART_*** functions?

__HAL__LOCK sets the Lock member of the uart structure to HAL_LOCKED:

#define __HAL_LOCK(__HANDLE__) \
 do{ \
 if((__HANDLE__)->Lock == HAL_LOCKED) \
 { \
 return HAL_BUSY; \
 } \
 else \
 { \
 (__HANDLE__)->Lock = HAL_LOCKED; \
 } \
 }while (0U)

__HAL_UNLOCK does the opposite, but there seems no active “locking�? logic implemented in the code.

Also the “while (0U)�? loop does not make that much sense to me; If the idea was to implement a kind of critical section behavior, then I would assume some interaction with enabling or disabling interrupts or some other kind of logic but, as far as I can see, there is no such thing implemented?

If I look in the files that make up the UART HAL, what I assume is that the “try lock functionality�? the author tried (?) to implement is nowhere used (return value of _HAL_LOCK() that could be used in an "if (__HAL_LOCK(__HANDLE__)== HAL_BUSY)" kind of statement.)

But even, then, using the code in such a construct seems not ok to me as the while (0U) would result an undefined return value hence undefined behavior.

On top while() loops in ISR are also not the best way to go ether.

2) How to reliably implement locking between main() and ISR?

To me blocking interrupts while addressing the circular buffer in the main() seems the way to go. Could somebody provide a reference to such an approach or present a reliable alternative approach?  (ISR data can arrive any time)

Thanks for any advice,

Johi.

[TDK]

> 1) What is the added value of the __HAL_LOCK(huart); and __HAL_UNLOCK(huart); statements in the HAL_UART_*** functions?

It is primarily to prevent usage errors where the user tries to, for example, call HAL_UART_Transmit_DMA while the previous transaction from HAL_UART_Transmit_DMA is not complete.

The while(0) can be used to restrict local variable scope within the macro, but here there are no variables. It is what it is--a stylistic decision. If it were written the other way, people would ask why it's not written this way instead.

> But even, then, using the code in such a construct seems not ok to me as the while (0U) would result an undefined return value hence undefined behavior.

Huh, how does it make the return value undefined?

> On top while() loops in ISR are also not the best way to go ether.

It's not really a loop because it only executes once. It does not cause delays in the ISR.

> 2) How to reliably implement locking between main() and ISR?

Probably 99+% reliable, which is to say not at all. This has been a longstanding issue. The best approach is to control the peripheral in only one place--either the main loop or the ISR, which is the typical usage.

Alternatively, STREX/LDREX can be used to implement a robust locking mechanism.

[TDK]

But even if you did implement a robust locking mechanism, that doesn't solve the real issue. You still need to implement your code in such a way that the ISR can be delayed if the peripheral is locked in the main loop. This is not something that can be solved in a library without simply disabling interrupts, it requires design decisions. Disabling interrupts has significant consequences, which is what the whole thing is trying to avoid.

[Johi]

Hello TDK, thank you very much for your answer: Coding style is an aspect I did not think of.

Huh, how does it make the return value undefined?

If the MCU follows the branch "(__HANDLE__)->Lock = HAL_LOCKED;", then there is no return statement indicating a defined return value when the code returns whereas the other branch returns "HAL_BUSY". So therefore I assume undefined return value.

Your approach related to my question is still only partially clear to me:

1) ISR only addresses the UART registers => indeed, clear best way to go.

2) ISR manages pointer in circular buffer => OK, ISR has priority over main().

3) main also manages same pointer in circular buffer => I would lock interrupts only during this access but very shortly to make sure pointers remain coherent during access?

Is this approach not the right way to go?

Or should I assume main() only reads the ISR write pointer and as access to 32 bit pointer is "atomic", there is no need to lock?

[TDK]

> If the MCU follows the branch "(__HANDLE__)->Lock = HAL_LOCKED;", then there is no return statement indicating a defined return value when the code returns whereas the other branch returns "HAL_BUSY". So therefore I assume undefined return value.

__HAL_LOCK is a macro, not a function. If it obtains the lock, there is no need to return from the function immediately. Execution will continue, do what it needs, then (possibly) do __HAL_UNLOCK near the end of the function and return HAL_OK.

In other words, the "return HAL_BUSY" statement within __HAL_LOCK acts as the return statement for the HAL_UART_* function.

> Is this approach not the right way to go?

It's not black and white, but yes, disabling interrupts is one way of solving the problem. Your code still needs to be consistent though.

Reading a 32-bit value is atomic, but read-modify-write is not. If main() only reads, there is no need to lock anything.

[Bob S]

If you have only one "producer" (write to buffer) and one "consumer" (read from buffer), it is possible to design a queue/fifo that does not need any locks. For example, @Tilen MAJERLE​ posted his lightweight ring buffer code on GitHub: https://github.com/MaJerle/lwrb . I do not use that code, but I have used its predecessor.

[Piranha]

https://community.st.com/s/question/0D50X0000C5Tns8SQC/bug-stm32-hal-driver-lock-mechanism-is-not-interrupt-safe