Does this code need interrupts disabled?

[peter-h]

I am 100% sure there are no multiple paths there. As you say that is MCU101. Functions which are callable from either interrupt enable context need to save/restore the state but I am not doing that anywhere.

I think there are two areas which might be a problem

1) __disable_irq() doesn't quite do what it is supposed to do i.e. a straight DI, and __enable_irq a straight EI, without side effects

2) The UART ISRs are interruptible by other UART ISRs and there are some critical areas

[paulca]

My understanding is that any NVIC interrupt with a lower (higher) priority number WILL interrupt a higher priority number.

So if your UART2 has priority 1 and UART3 has priority 2, UART2 could interrupt UART3's interrupt.

I believe if they all have equal priority they cannot interrupt each other.  In that case the interrupt flags remain until it gets to it.

Disabling just the UART interrupts will still leave your routine open to being interrupted by other higher priority interrupts.  Even SysTick etc.

Yes, I too have read the discussions and pain people get into, it seems that things are fine until they are not and the first poster usually says something like, "It's all about the priority".  You go and look and all of them are set to 0.  Getting from there to a sensible, well setup priority hierarchy seems to take a foundation degree in ARM NVICs.

[peter-h]

Current interrupt priorities are:

TIM6 1 (used for a general 1kHz timer decrement interrupt - ISR exec time ~10us)
TIM1 1 (not enabled in current project)
UARTs 1
USB  2

A priority of 0 is not used for anything; it appears to be non maskable! This was the default setting for UARTs in Cube MX!

SysTick is 15 - this is standard for FreeRTOS. There is a huge long discussion all over the internet about this; basically FreeRTOS uses Systick to switch tasks but that interrupt must be a lower priority than any ISR, including any timer ISR. The above 1kHz timer ISR doesn't do anything related to FreeRTOS. A key Q is whether you are calling any part of FreeRTOS from an ISR, and I am not.

All my ISRs, even the four UART ISRs (actually 12: 4 x RX, 4 x TX-E, 4 x TX-TC) should be interruptible by anything else. None of them are doing something like a RMW of some config register which applies to more than one UART. This is one aspect I would not put money on.

I would think that within a given level e.g. 1 there must be a priority hierarchy but what does that actually do? In the past when I had multiple devices on the same priority they could not interrupt each other's ISR but the "priority within the same priority level" (hard-wired) would determine the order in which the ISRs are presented for execution, so e.g. if UART1 has a higher priority than UART2, then UART2's ISR would be commenced after UART1's ISR has exited.

The interrupt handler is this

Code: [Select]


// Common interrupt handler for all USART interrupts
static void usart_common_IRQHandler(UART_HandleTypeDef *huart, uint8_t port)
{
// check if we are here because of RXNE interrupt
if (huart->Instance->SR & USART_SR_RXNE)
{ //if RX is not empty
serial_receive_irq_handler(huart, port);
}

// check if we are here because of TXEIE interrupt
if (huart->Instance->SR & USART_SR_TXE)
{ //if TX is empty
serial_transmit_irq_handler(huart, port);
}

// check if we are here because of the TC interrupt
if (huart->Instance->SR & USART_SR_TC)
{ //if TX complete
serial_transmit_complete_irq_handler(huart, port);
}
}

and that calls each of the 12 ISRs with the "huart" (basically a base address of the memory mapped register set for that UART), and "port" which is 1-4.

The above function is in turn called from the following which are pointed to by the vector table

Code: [Select]


/**
  * @brief This function handles USART1 global interrupt - USART1_IRQn 0
  */
void USART1_IRQHandler(void)
{
usart_common_IRQHandler(&huart1, 1);
}

/**
  * @brief This function handles USART2 global interrupt - USART2_IRQn 0
  */
void USART2_IRQHandler(void)
{
usart_common_IRQHandler(&huart2, 2);
}

/**
  * @brief This function handles USART3 global interrupt - USART3_IRQn 0
  */
void USART3_IRQHandler(void)
{
usart_common_IRQHandler(&huart3, 3);
}

/**
  * @brief This function handles USART6 global interrupt - USART6_IRQn 0
  */
void USART6_IRQHandler(void)
{
usart_common_IRQHandler(&huart6, 4);
}

and the vector table is e.g.



What could possibly be wrong with this?

Note I am using uarts 1,2,3,6 for the four ports. This was dictated by some shared pin constraints.

[bugnate]

In STM32 speak, there are really two priorities, a "pre-emption priority" and the "sub priority". The fun part is that which bits are which in the NVIC is configurable, including all or none for one or the other. See screenshot from a fresh CubeMX F417 as an example of how they want you to think about it.

An exception with a lower pre-emption priority will preempt regardless of sub priority. If pre-emption priority is the same, then the ISRs will tail-chain (not preempt) according to sub priority and should run in the sequence you expect. If the sub priority is equal, IIRC they will sequence according to their IRQ number (e.g., the value of USART6_IRQn).

You can effectively disable preemption by setting the number of bits allocated to preemption priority to 0 and I often do so for my smaller and tighter projects, or I otherwise try to only preempt in a more whitelist manner. I don't know how much flexibility your app or FreeRTOS allows you.

[peter-h]

Thank you.

To me that sounds like the "pre emption priority" is the 0-15 value I reference above i.e. 1 for the UARTs etc, and "sub priority" is a priority within that. I wasn't aware that the latter is configurable, but it doesn't really matter what it is in my project because all the ISRs are very short.


This is the vector table portion spanning the four UARTs. UART6 is not in use right now. I am using just UART1,2.



I have no idea if this is relevant



or this



and finally these are the UART settings

Code: [Select]

    /* USART1 interrupt Init */
    HAL_NVIC_SetPriority(USART1_IRQn, UART_INT_PRIORITY, 0);  // 37, 1, 0
    HAL_NVIC_EnableIRQ(USART1_IRQn);  // 37


If the sub priority is equal, IIRC they will sequence according to their IRQ number (e.g., the value of USART6_IRQn).

I can't find where the sub priority is configured, but whatever it is, it should not matter. Even if the TIM1 ISR was interrupting the UART2 ISR, it should not matter.

[Sauli]

The pre-emption priority / sub priority configuration is just some evil scheme to confuse the poor programmer. And the STM32Cube uses it's own default configuration which most programmers are not aware of. The best thing to do after the cube hardware initialization is done is to call NVIC_SetPriorityGrouping( 0);  And then use NVIC_SetPriority() and NVIC_EnableIRQ() functions to configure the interrupts.  And forget about the sub priorities. A lower priority number interrupt always pre-empts a higher priority number interrupt.

About __disable_irq(): Do not use it within interrupt service. If you need to modify a register in interrupt service that could also be modified in a higher priority interrupt, then use LDREX/STREX.

[peter-h]

Hmmm.

Why does Cube use NVIC_SetPriorityGrouping(3), and you suggest NVIC_SetPriorityGrouping(0)?

3 seems arbitrary, except that a value of 3 means you can have a few things above and below while a value of 0 means you can have only bigger values. What do bigger values do, relative to 0? If say you have 2 devices, both on priority 1 (like my four UARTs) and NVIC_SetPriorityGrouping(0) was set, does that ensure that none of the UARTs ISRs can enter each other? But surely if NVIC_SetPriorityGrouping(3) is used as the only value, won't that do the same?

This is the function called with (3)

Code: [Select]


/**
  \brief   Set Priority Grouping
  \details Sets the priority grouping field using the required unlock sequence.
           The parameter PriorityGroup is assigned to the field SCB->AIRCR [10:8] PRIGROUP field.
           Only values from 0..7 are used.
           In case of a conflict between priority grouping and available
           priority bits (__NVIC_PRIO_BITS), the smallest possible priority group is set.
  \param [in]      PriorityGroup  Priority grouping field.
 */
__STATIC_INLINE void __NVIC_SetPriorityGrouping(uint32_t PriorityGroup)
{
  uint32_t reg_value;
  uint32_t PriorityGroupTmp = (PriorityGroup & (uint32_t)0x07UL);             /* only values 0..7 are used          */

  reg_value  =  SCB->AIRCR;                                                   /* read old register configuration    */
  reg_value &= ~((uint32_t)(SCB_AIRCR_VECTKEY_Msk | SCB_AIRCR_PRIGROUP_Msk)); /* clear bits to change               */
  reg_value  =  (reg_value                                   |
                ((uint32_t)0x5FAUL << SCB_AIRCR_VECTKEY_Pos) |
                (PriorityGroupTmp << SCB_AIRCR_PRIGROUP_Pos)  );              /* Insert write key and priority group */
  SCB->AIRCR =  reg_value;
}

I am not using __disable_irq within an ISR. It would seem dumb.

If you need to modify a register in interrupt service that could also be modified in a higher priority interrupt, then use LDREX/STREX.

AFAICT I am not doing any such thing, but maybe there is some inter-dependence between the UARTs?

Separately, I am seeing a strong inter-dependence between UART activity and USB CDC (which is wholly interrupt driven), causing a whole 512 byte buffer to be lost, but currently I am suspecting something else. But... ?

I want no ISR to enter any other. What is the best way to do that?

[bugnate]

I can't find where the sub priority is configured, but whatever it is, it should not matter. Even if the TIM1 ISR was interrupting the UART2 ISR, it should not matter.

The "NVIC_PriorityGroup_4" line allocates all bits to pre emption and none to sub priority, so the latter is moot in any case. So yes it looks like the 0-15 is all preemption.

Because all of the exceptions (except USB) are the same priority (1), when multiple are pending the one with the lower IRQn is what gets run after the current ISR completes. They will not preempt (interrupt) each other, only run back-to-back. As long as there are sufficient CPU cycles for all, you should be fine in this regard.

3 seems arbitrary, except that a value of 3 means you can have a few things above and below while a value of 0 means you can have only bigger values. What do bigger values do, relative to 0? If say you have 2 devices, both on priority 1 (like my four UARTs) and NVIC_SetPriorityGrouping(0) was set, does that ensure that none of the UARTs ISRs can enter each other? But surely if NVIC_SetPriorityGrouping(3) is used as the only value, won't that do the same?

You are correct, but see edit note at the end.

Separately, I am seeing a strong inter-dependence between UART activity and USB CDC (which is wholly interrupt driven), causing a whole 512 byte buffer to be lost, but currently I am suspecting something else. But... ?

If your UART ISR is sufficiently blocking the USB CDC ISR (and it could, because it is higher priority), that certainly could cause you to miss a PC host timeout or such.

I want no ISR to enter any other. What is the best way to do that?

NVIC_SetPriorityGrouping(NVIC_PriorityGroup_0) would do this at a stroke because the 0-15 values become sub priority values instead of pre emption values. I cannot advise if it would be appropriate for your case.

EDIT: NVIC_PriorityGroup_0 not "0". Suggest using the macros. I don't think "0" is valid.

[peter-h]

Because all of the exceptions (except USB) are the same priority (1), when multiple are pending the one with the lower IRQn is what gets run after the current ISR completes. They will not preempt (interrupt) each other, only run back-to-back. As long as there are sufficient CPU cycles for all, you should be fine in this regard.

That is what I want.

NVIC_PriorityGroup_0 not "0". Suggest using the macros. I don't think "0" is valid.

Aha you are right:

Code: [Select]


/** @defgroup CORTEX_Preemption_Priority_Group CORTEX Preemption Priority Group
  * @{
  */
#define NVIC_PRIORITYGROUP_0         0x00000007U /*!< 0 bits for pre-emption priority
                                                      4 bits for subpriority */
#define NVIC_PRIORITYGROUP_1         0x00000006U /*!< 1 bits for pre-emption priority
                                                      3 bits for subpriority */
#define NVIC_PRIORITYGROUP_2         0x00000005U /*!< 2 bits for pre-emption priority
                                                      2 bits for subpriority */
#define NVIC_PRIORITYGROUP_3         0x00000004U /*!< 3 bits for pre-emption priority
                                                      1 bits for subpriority */
#define NVIC_PRIORITYGROUP_4         0x00000003U /*!< 4 bits for pre-emption priority
   

If your UART ISR is sufficiently blocking the USB CDC ISR (and it could, because it is higher priority), that certainly could cause you to miss a PC host timeout or such.

There are vast threads here on how to implement USB MSC with a sector flash write of 15ms It turns out that the USB sector (MSC; removable storage device, not CDC which is a USB VCP) ISR is not only fine to just hang in the ISR for the 15ms, but USB flash sticks do exactly that, reportedly. The timeouts are huge. And just as well since "elegant" solutions are extremely complex; in fact despite sending weeks on it, I found none that worked (like, returning a "BUSY" status to the USB host so it keeps retrying). Or a RAM cache equal in size to the whole file system

But USB MSC is not same as USB CDC, and CDC could have a shorter timeout. That said, my USB ISRs are all very short - of the order of tens of us max. The UART ISRs are of the order of 5-10us.

NVIC_SetPriorityGrouping(NVIC_PriorityGroup_0) would do this at a stroke because the 0-15 values become sub priority values instead of pre emption values. I cannot advise if it would be appropriate for your case.

I can easily test that, tonight.

But from what you say, it should not make any difference. However, since NVIC_PRIORITYGROUP_0 allocates no bits for pre-emption priority, can one still set the levels 1-15 as I currently do?

EDIT: no luck with NVIC_PRIORITYGROUP_0



It blows up FreeRTOS.

[paulca]

I was fiddling around with those settings in CubeMX this evening after reading here and yea... setting it to 0 bit pre-emption adds mucho red lines everywhere. 

RTOS won't like it as it has to at least have 2 priorities surely?  IIRC, especially if you want idle watchdogs.  Either that or the RTOS time source wasn't getting a change to fire.

[Sauli]

Have a look at the attached picture from ARMv7-M Architecture Reference Manual.

What this means?
- Set PRIGROUP value to 0 and you get the maximum number of pre emption levels. How many is implementation defined. You can still control the pre emption by the priorities you select for the interrupt sources.
- Set PRIGROUP value to 7 and all interrupts are at the same pre emption level, i.e. an interrupt service is not interrupted by another interrupt service. This setting breaks RTOS.

So, to confuse things even more, ST has decided to call PRIGROUP value 7  NVIC_PRIORITYGROUP_0 and has not even defined PRIGROUP value 0.

By using ST HAL functions it is easy to lose track of which interrupts can preempt which. Therefore I once again recommend using the CMSIS functions instead.

[peter-h]

I would expect ST spent some time porting the various open source bits to their CPU in Cube MX and HAL. Not always the right way, and rarely efficiently, but I am sure they fixed a zillion issues on the way, and mostly didn't document what they did and why. There are docs describing e.g. FreeRTOS port but not in detail.

And these OS modules themselves are "old code" e.g. LWIP is > 15 years old and the coders lost interest > 10 years ago (google hits are mostly from 2011 or earlier). FreeRTOS and FatFS similarly. No support anywhere. It's all old stuff, but it is mostly good stuff which does work.

So... the consensus seems to be that none of my ISRs are interrupting each other - correct?

If so, I am going to draw a line under this part of the job now.

[tggzzz]

So... the consensus seems to be that none of my ISRs are interrupting each other - correct?

Don't guess and hope. Test and verify.

On entry to an ISR set a GPIO bit, and clear it before exiting. Use a separate bit for each ISR. Trigger a scope or L.A. if more than one bit is set at any time.

If necessary use resistors to form a crude analogue adder, and trigger a scope of the voltage goes above the relevant voltage.

[Siwastaja]

Forget the sub priority bullcrap. It's only interesting in some weird corner cases where you want to optimize the order of nearly simultaneously occurring events. Specifically the case where you have event1 and even2 in such way that you DON'T want them to pre-empt each other, but STILL want to control the order in which they are processed in case they are both pending after even more important event3. I have never found any use for this feature. Use priority grouping 0, and then the priority will be just a simple number.

[peter-h]

But that breaks FreeRTOS - see above.

Siwastaja - do you think the ISRs discussed here interrupt each other?

In addition to

Code: [Select]

#define NVIC_PRIORITYGROUP_4         0x00000003U /*!< 4 bits for pre-emption priority
                                                      0 bits for subpriority */
and with no definition for grouping 0, should I add

Code: [Select]

#define NVIC_PRIORITYGROUP_5         0x00000000U /*!< 8 bits for pre-emption priority
                                                      0 bits for subpriority */
But surely 16 priority levels is loads, and what else will break if I change the config? Using 0 doesn't obviously blow anything up but why tempt fate? This thread
https://community.st.com/s/question/0D50X00009XkbuPSAR/nvic-priority-grouping
covers that; I already have zero sub-priorities. The above NVIC_PRIORITYGROUP_4  comment says the same. So is there any need to change anything?

I also read elsewhere that ST didn't implement what is in the ARM manuals. They still have only 4 bits for the pre-emption priority. So what is actually wrong with what I have as Cube default?

A little side note. I timed the copy loop, the one which updates the index only at the end, and it takes 140ns per byte

Code: [Select]

// Use a local write index
volatile uint32_t index = serial_transmit_buffer_put_index[port];

// Copy the data to the circular buffer
for (uint16_t i = 0; i < length; i++)
{
serial_transmit_buffer[port][index++] = data[i];
if( index >= SERIAL_TX_BUFFER_SIZE)
{
index = 0;
}
}

// Only update the write index after all data copied.
serial_transmit_buffer_put_index[port] = index;
One could split this into two memcpy() calls but (previous threads) the newlib memcpy is no faster. Is there some cunning way to use DMA, in circular buffer mode, to copy a linear buffer into a circular buffer? One would read out the final circular buffer pointer from the DMA register at the end of the transfer. This code is not time-critical, however.

Removing "volatile" speeds it up from 140ns to 120ns which is roughly 2-3 clocks. I did the volatile after dire warnings from compiler experts, but is that actually meaningful? Fairly obviously the index is otherwise held in a register, but nothing else will be accessing it anyway.

And the old version of the loop (start of this thread) which uses the mod % operator takes another 20% longer; unsurprisingly since that involves a division (11 clocks?).

[Siwastaja]

STM32 MCUs just implement 4 bits of interrupt priority anyway, so you can't have 8 bits for priority. Some other MCU manucturers do have more bits available. But I agree 16 levels is usually more than enough.

All that crap is some super complicated way of saying the same: "use all 4 bits for priority; disable sub-priority stuff". It's all about getting those 4 bits in the correct place in the configuration register.

ISRs with same priority do not preempt each other.

[peter-h]

disable sub-priority stuff

Isn't that what

Code: [Select]

#define NVIC_PRIORITYGROUP_4         0x00000003U /*!< 4 bits for pre-emption priority
                                                      0 bits for subpriority */
does? That is the default setting from Cube.

[Siwastaja]

I don't know, it says so in comments.

I have just used the NVIC_SetPriorityGrouping() which internally handles the write to the correct part of the register. I don't understand the idea of replacing this simple interface with another one taking in weirdly named arguments - if NVIC_PRIORITYGROUP_4 means 4 bits, why does NVIC_PRIORITYGROUP_5 mean 8 bits, I have no idea. I prefer not to use confusing interfaces to a simple register.

[peter-h]

NVIC_PRIORITYGROUP_5 was my suggestion, and the comment on it is wrong (because ST implements only 4 bits). NVIC_PRIORITYGROUP_4 is what Cube comes with - as I posted above.

ST provide the said code to get somebody started without reading the 2000 page RM

[NorthGuy]

Removing "volatile" speeds it up from 140ns to 120ns which is roughly 2-3 clocks. I did the volatile after dire warnings from compiler experts, but is that actually meaningful? Fairly obviously the index is otherwise held in a register, but nothing else will be accessing it anyway.

You don't need "volatile" here. "index" is a local variable which is not accessible from anywhere else.

[paulca]

ST provide the said code to get somebody started without reading the 2000 page RM

If it worked more than 50% of the time... yes.

[paulca]

If I am understanding.

If you have 4xUART interrupts and you would like them to interrupt/pre-emptively other lower priority interrupts, you'd need to move those interrupts out of "0" pre-emption.

You'd want all 4 UARTs to share both their pre-emption level and sub-priority, such that each can prempt others of higher pre-empt value, but not each other.

0 - Things that should never be "pended".  aka RTOS/systick.
1 - High priority user stuff (UARTS)
2 - Can wait and pend for a while.

Does that work?

[peter-h]

You don't need "volatile" here. "index" is a local variable which is not accessible from anywhere else.

Great - so I thought. Thanks.

If you have 4xUART interrupts and you would like them to interrupt/pre-emptively other lower priority interrupts, you'd need to move those interrupts out of "0" pre-emption.

I already have moved the four to priority (pre-emption level) 1. Sub priority, I don't care, but AIUI NVIC_PRIORITYGROUP_4 already has zero bits for sub priority.

You'd want all 4 UARTs to share both their pre-emption level and sub-priority, such that each can prempt others of higher pre-empt value, but not each other.

Correct.

Things that should never be "pended".  aka RTOS/systick.

There is much online about this and loads of people are confused, but it seems intended that Systick priority for FreeRTOS is supposed to be very low. It is set to 15



Yes it is working.

It seems I have the right config although it's not obvious how one got here

[Sauli]

Once more, but I promise this is the last time.

The CMSIS functions in the core_cmX.h header are very useful. X here depends on which ARM core you are using.
Including the MCU family header also brings in the CMSIS core header. In this particular case that would be:
    #include <stm32f4xx.h>

For handling the NVIC there are a buch of functions named NVIC_something. To configure the interrupts, first of all allocate zero bits for the subpriority field:
    NVIC_SetPriorityGrouping( 0);

Then you can assign the priority for each interrupt and enable it, for instance:
    NVIC_SetPriority( USART1_IRQn, 2);
    NVIC_EnableIRQ( USART1_IRQn);
   
You can assing all used USARTs to the same priority level and they won't preempt each other.

The number of priority levels depends on the vendor imprementation of the NVIC, but practically all support at least 8 levels (0..7). Always leave the lowest priority - that is highest level number - unused because RTOS uses it. RTOS assigns the lowest priority for it's own use, whatever the lowest level is (7/15/31/63/127/255).

[Siwastaja]

What Sauli said. Those functions are trivially easy to use, examples of a good interface. What's best, not STM32 specific, works for all vendors. No need to look at the manual at all.

After SetPriorityGrouping(0), all further interrupt priorities are simple number, the lower the more urgent. Same numbers won't preempt each other. I don't understand how one could design anything simpler than this.