Nested CRITICAL_REGION_ENTER/CRITICAL_REGION_EXIT

[RK]

Are you supposed to be able to nest critical regions using CRITICAL_REGION_ENTER and CRITICAL_REGION_EXIT? Here I mean you have a function which uses a critical region to surround some work, and you call that function from within another critical region set up by another function. So you end up calling CRITICAL_REGION_ENTER twice, then CRITICAL_REGION_EXIT() twice.

Looking at the code for CRITICAL_REGION_ENTER, it makes sense. If the softdevice isn't enabled, it just turns interrupts off, else it relies on the softdevice to have turned off the application ones. However the code for CRITICAL_REGION_EXIT doesn't make so much sense. It's here..

#define CRITICAL_REGION_EXIT()                                                              \
        if (CURRENT_INT_PRI != APP_IRQ_PRIORITY_HIGH)                                       \
        {                                                                                   \
            uint32_t ERR_CODE;                                                              \
            __enable_irq();                                                                 \
            ERR_CODE = sd_nvic_critical_region_exit(IS_NESTED_CRITICAL_REGION);             \
            if (ERR_CODE != NRF_ERROR_SOFTDEVICE_NOT_ENABLED)                               \
            {                                                                               \
                APP_ERROR_CHECK(ERR_CODE);                                                  \
            }                                                                               \
        }                                                                                   \
    }

This calls __enable_irq() directly, thus immediately turning on all interrupts so the critical region is effectively ended at this point even if you were nested and sd_nvic_critical_region_exit() was not going to disable the interrupts.

Should the code not do the inverse of the CRITICAL_REGION_ENTER() code and only call __enable_irq() in the case the softdevice isn't active? Something like

   #define CRITICAL_REGION_EXIT()   
            if (CURRENT_INT_PRI != APP_IRQ_PRIORITY_HIGH)  
            {     
                uint32_t ERR_CODE= sd_nvic_critical_region_exit(IS_NESTED_CRITICAL_REGION);
                if (ERR_CODE == NRF_ERROR_SOFTDEVICE_NOT_ENABLED) 
                { 
                    __enable_irq();  
                }
                else 
                {   
                    APP_ERROR_CHECK(ERR_CODE);  
                }  
            }   
        }

Or do you have to ensure that you never call functions which end up nesting critical regions like this?

[John]

From examining the SDK documentation on the macros and the soft device critical region functions, you cannot nest the critical regions using the macros. You can if you call the soft device functions yourself and manage the is nested flag yourself. As for the exit macro implementation, I'm not knowledgeable enough to comment.

[RK]

I finally managed to set up a test case to test this by using functions which call each other, each of which makes its own critical scope. Sort of like this

void foo( void )
{
    CRITICAL_REGION_ENTER()
    .. do some work ..
    CRITICAL_REGION_EXIT()
}

void bar( void )
{
    CRITICAL_REGION_ENTER()
    .. do some other work ..
    foo();    // call foo
    .. and do a little cleanup work .. 
}

That's not a particularly unusual pattern, when you have a protected resource it does happen you end up protecting your work and also then calling a helper function which protects the same thing.

If you have the softdevice enabled then it works. The reason for that is that the sd_nvic_critical_region_enter()/exit() functions disable/enable the individual user interrupts, they don't use __irq_disable() to disable everything, so the call to __irq_enable() makes no difference, only when the finally non-nested sd_nvic_critical_region_exit() is called are the individual interrupts re-enabled and then they fire.

However if the soft device is disabled then it does fail, but for a different reason than I expected. The first entry to the critical region disables interrupts. When you come to the second entry (this is before you exit the first one, because they are nested) sd_nvic_critical_region_enter() is called again, however interrupts are at that point disabled and emitting an SVC call with interrupts disabled is a hard fault, so it hard faults.

Since I do have this issue I'll write my own version of the wrapper which checks only the first time if the SD is enabled/disabled and then just calls it or the _irq* functions and nests properly, my library code has to work with and without a softdevice enabled and needs the nested critical regions. I need more than just calling sd_nvic_critical_region() however as if the SD is disabled, I do need to at some point call __irq_disable() to actually get the criticality.

[Joe Merten]

When wrote your own CRITICAL_REGION_ENTER/EXIT(), should you also take care of irq enabled at the first call of CRITICAL_REGION_ENTER()? Hmm, what's the call to determine if irq is enabled or not?

[Joe Merten]

Looks that I could check the lsb of PRIMASK (determined by __get_PRIMASK()) to check if irq currently is enabled or not.

[RK]

You could do all those things. I wondered about this too and why this wasn't part of the CMSIS routines and eventually decided that it's because reading the current enabled state of an interrupt is fairly useless and subject to loads of race conditions.

If you are a level 0 interrupt, it doesn't matter, you aren't getting interrupted anyway. If you are anything less, than reading the current state of the interrupts opens a window for a higher priority interrupt to change them and then you go set it back to what it was before and break it.

If you disable all interrupts it doesn't matter what they were before, you disabled all interrupts globally and re-enable them globally.

A function to read the current enabled state of an interrupt isn't needed as it's just a function which will eventually hit a race condition and break something.