Add macros for critical sections

[antoinealb]

I know some people don't like it, but personally I find it useful to be able to lock interrupts for a very short amount of time, for example to copy all parameters of a PID filter before computing the filter. We could do this with a mutex but they have some overhead because releasing a mutex cause a reschedule. Typically this is solved using macros like OS_ENTER_CRITICAL() and OS_EXIT_CRITICAL().

So what do you think ? Do we allow that kind of behaviour from application code ?

[antoinealb]

My suggestions is to implement an ATOMIC_BLOCK macro that would be used like this :

ATOMIC_BLOCK() {
    ...
}

This would guarantee we always re-enable the interrupts at the end of the block.

[pierluca]

Agreed, but I would still call it CRITICAL_SECTION , so that we know what we're talking about :-) The only issue I see is that macros containing code can get really ugly. We have to make triple sure it's working and not breakable by what you put inside those braces.

You might want to take note of the hack they present here, to avoid a possible pitfall http://www.krten.com/~rk/coding/critical.html

[froj]

http://www.krten.com/~rk/coding/critical.html

My eyes are bleeding...

[antoinealb]

I agree that this is ugly. But I don't see any alternative we really want a block system.

[pierluca]

I agree. It's horrible.

Then again, it's the same for most macros...

On Fri, Jun 13, 2014 at 1:33 PM, froj notifications@github.com wrote:

http://www.krten.com/~rk/coding/critical.html

My eyes are bleeding...

— Reply to this email directly or view it on GitHub https://github.com/cvra/platform-abstraction/issues/16#issuecomment-46001216 .

[froj]

IMO a critical block is always (as the name says) critical and needs special attention. I'd opt for

CRITICAL_SECTION() {
    ...
}

which does basically

OS_ENTER_CRITICAL()
...
OS_EXIT_CRITICAL()

and people need to take care when they use it.

[antoinealb]

Ok, so we need to use fugly macros.

[froj]

It would probably come down to

#define CRITICAL_SECTION() \
    OS_ENTER_CRITICAL(); \
    for(int __control = 1; __control; OS_EXIT_CRITICAL(), __control=0)

[antoinealb]

With a second for nested to avoid issues with break.

Also we should take a look at the macro expansion of OS_EXIT_CRITICAL. Maybe it needs to be put in a wrapper function.

[froj]

That or we call it CRITICAL_SECTION_DONT_TRY_TO_EXIT_WITH_BREAK().

[antoinealb]

CRITICAL_SECTION_IF_YOU_LEAVE_BLOCK_I_WILL_FIND_YOU_AND_I_WILL_KILL_YOU

[Stapelzeiger]

Our enter critical & exit critical should contain a compiler memory barrier: asm volatile("" ::: "memory"); (http://en.wikipedia.org/wiki/Memory_ordering#Compiler_memory_barrier)

[pierluca]

I'm taking care of this macro, I'll put forward a pull request later.

[antoinealb]

Ok, I set you as assignee.

[pierluca]

Right now, I'm driven to think this is not a good idea because of the API restrictions / design choices in uC/OS-III.

The critical section code is designed to be used as such

void myFunction()
{
int myVariable;
char myOtherVariable;
CPU_SR_ALLOC(); // called after ALL variable declarations, but before ANY function

// .. some code here ... 

CPU_CRITICAL_ENTER();

// access resources here

CPU_CRITICAL_EXIT();

}

The restrictions on CPU_SR_ALLOC imply that any function using our custom critical section would need to be aware of how the critical section is implemented (which is Bad (TM))

What's your thought on this?

References: https://doc.micrium.com/display/osiiidoc/Disable-Enable+Interrupts https://doc.micrium.com/display/cpudoc/CPU_SR_ALLOC

[froj]

I think CPU_CRITICAL_ENTER() really isn't for application code. There's also OS_CRITICAL_ENTER(), but this uses CPU_CRITICAL_ENTER(), too and I'm not sure if you need CPU_SR_ALLOC for that. It's further described in the pdf version of the doc.

[pierluca]

As someone pointed out already, OS_CRITICAL_ENTER is specifically indicated as NOT to be used by application code. Instead, page 218 of the manual indicates that CPU_CRITICAL_ENTER is the preferred way. This is also noted in the elements when porting uC/OS-II code to uC/OS-III.

Still, the point is that CPU_SR_ALLOC will have to be called. We can keep this behavior and have the macro, but I'm not sure we're gaining much here. Anyways, I'll post a candidate macro later and then we decide whether it's worth it to use it or not.

[antoinealb]

Fixed in #19 .