Does this code need interrupts disabled?

[peter-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);

That seems to work.
It was (NVIC_PRIORITYGROUP_4); which is 3:

Code: [Select]

#define NVIC_PRIORITYGROUP_4         0x00000003U /*!< 4 bits for pre-emption priority
                                                      0 bits for subpriority */
and since ST have only 4 bits for the pre-emption priority anyway, what is supposed to be the difference?

Also see here
https://freertos.org/RTOS-Cortex-M3-M4.html

Most systems default to the wanted configuration, with the noticeable exception of the STM32 driver library. If you are using an STM32 with the STM32 driver library then ensure all the priority bits are assigned to be preempt priority bits by calling NVIC_PriorityGroupConfig( NVIC_PriorityGroup_4 ); before the RTOS is started.

I don't understand how one could design anything simpler than this.

That's because you are much more clever than I am, and like to show it I don't mind. I really enjoy working with people much smarter than me, because I learn from them.

There seems to be a lot of ST expertise in Finland. Do they have a design team there?

[ejeffrey]

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?).

The `index` variable doesn't need to be volatile, as written that is a local variable that can't be accessed in any other context.

What needs to be volatile are serial_transmit_buffer_put_index and serial_transmit_buffer -- the data structures that are accessed / modified from both interrupts and your application main thread.

[peter-h]

I posted it here and will see if anyone bites
https://community.st.com/s/question/0D53W000024XLcbSAG/is-nvicprioritygroup4-3-same-in-effect-as-0-32f417

A value of 3 or 0 selects the same row, surely



The above is 32F4 specific - see table 51 here
https://www.st.com/resource/en/programming_manual/pm0214-stm32-cortexm4-mcus-and-mpus-programming-manual-stmicroelectronics.pdf

The confusion seems to be from the silly #defines.

[Siwastaja]

>show unified ARM interface which has nothing to do with ST
>initialization is one simple function call with argument zero
>interrupt is enabled using a function which says EnableIRQ in name, and takes single argument: IRQ number
>priority is set using SetPriority function which takes two arguments: IRQ number and priority level number
>interrupt priorities are simple numbers where lower number preempts higher, same numbers do not preempt each other
>"I don't see how this could be any simpler"
<"that's because you are a genius and ST expert!"

Are you serious? Why do you waste time in something like this? You definitely are mentally capable of understanding this concept, I'm 110% sure, because you are doing things many orders of magnitude more complex all the time.

[paulca]

ST Community sucks.

It's just the fantasy of HAL people to set it again to the same value, like with lots of other registers. It seems that HAL authors don't trust any reset values specified in tech docs.

This is why we can't have nice tings.  It's because people just don't understand software engineering.  Function pre-conditions.  Setting up the state.  Verifiying the state.  Performing the transaction, verifying the outcome, post conditions, error conditions, recovery conditions etc. etc.

Not all are used in every function/unit of code, but it pays to at least check the basics.  All of these things get exponentially harder as the code gets bigger, but a lot lot harder when one engineer is not enough anymore and code flux rises rapidly.

If your function to enable widget X, assumes the reset state of the device from the docs and later that changes  or the code calling it changes, the code it calls changes, the code might "appear" to be work fine in 99% percent of cases and in the other 1% present as a bizarre bug in a completely unrelated part of the code.... because nobody cared to stop erroneous input because they were so damn sure of themselves they never checked or reset state to the expected values before proceeding on down the call hierarchy.   Nobody ever does negative testing anymore.  Technically as soon as one function assumes something incorrectly the entire application is at that point corrupt and compromised and if it does not have checks in place to detect this and fail safe or recover, it will propagate through teh system and emerge in a completely unexpected place and be next to impossible to trace.

I'm not directing this at anyone in particular, it's just a rant about out of date practices, particularly in the embedded space.  People seem to think the small size and foot print frees them from the entirety of software engineering pracitces.

Practice what I preach?  From time to time yes.  But MCU world is hobby space for me.  I like breaking all the rules, it makes me feel free and a breath of fresh air from enterprise software were it's 90% other thing than writing code.  Box ticking, etc. etc.

[peter-h]

I don't have a problem with loading a reset value.

The main problem is people not reading a post properly.

I am trying to keep the questions focused, and the focus naturally moves along from one thing to another.

Right now the confusion is that ST define a value NVIC_PRIORITYGROUP_4 as 3 but calling NVIC_PriorityGroupConfig(NVIC_PRIORITYGROUP_4) with this is same as NVIC_PriorityGroupConfig(0) which was suggested by others further back.

I was suspicious re using 0 because of the various places where NVIC_PRIORITYGROUP_4 is recommended, including that FreeRTOS site link. But it seems to be genuine nonsense, a repetition of something from the past, and possibly relevant to other ARM CPUs, whereas with M4 NVIC_PRIORITYGROUP_4 is same as using 0. Both 011 and 000 have bit 2 = 0 in PRIGROUP - the yellow line above

Anyway I always try the simplest possible setting and 0 works.

On the ST forum this has come up before, 2 years ago. But not exactly this.

[paulca]

Sorry my rant didn't help.

I read your post several times, still didn't get it. 

What I got hung up on is ..

Code: [Select]

0x5FAUL
All I see is

Code: [Select]

magicUndefinedNumber

[peter-h]

To write to AIRCR, you need to load 0x5FA into a part of it. That's all that is.

The function __NVIC_SetPriorityGrouping() takes the input value, ANDs it with 0x7, so just 3 bits of it are used, and load it into the 3 AIRCR bits in Table 51 above, PRIGROUP 2..0.

The point I was trying to get to the bottom of was that an input value of 3 will have the same effect as an input value of 0 - given that if bit 2 of PRIGROUP is 0, the other two bits are xx

I can think of two reasons why ST provide the NVIC_PRIORITYGROUP_4 #define and the FreeRTOS website recommends using it:

1) Loading NVIC_PRIORITYGROUP_4 (=3 i.e. 0b011) is not the same as loading 0b000 if some other code is reading back AIRCR and reading the 3 PRIGROUP bits and doing something based on bits 1,0 which are now 11. The cryptic comment on the FreeRTOS website, telling you to load NVIC_PRIORITYGROUP_4, suggests that this may be a possibility. And, stepping through the RTOS, they do read PRIMASK very early on but that's not the same register and it doesn't seem to get used for anything (I didn't step on for hours) except to detect if it is being called from an ISR. I see no evidence of PRIGROUP or AIRCR being accessed by FreeRTOS.

2) The NVIC_PRIORITYGROUP_4 thing is fossil code from another CPU and the FreeRTOS website writer wasn't aware.

3) Some other reason

In the meantime the person who really understands this has decided I am stupid and has left the thread

I now have three targets running with a (0) parameter. I've documented all this in my project document so one can go back to it if something is broken.

Also the comment in Cube

Code: [Select]

#define NVIC_PRIORITYGROUP_4         0x00000003U /*!< 4 bits for pre-emption priority
                                                      0 bits for subpriority */
is actually correct. You get 4 bits (16 priority levels) for both 3 and 0 parameters. The other mystery is why there isn't a define like

Code: [Select]

#define NVIC_PRIORITYGROUP_STARTUP_DEFAULT         0

[Siwastaja]

Code: [Select]

0x5FAUL
All I see is

Code: [Select]

magicUndefinedNumber

Well, it literally is a magic number; in this case, the idea is to make access harder by purpose to avoid accidental changes e.g. because of corrupted pointers. Of course, either #defining or // commenting about it being MAGIC would be helpful.

("magic number" is an actual programming-related term, although people sometimes misuse it when discussing poor quality code.)

[peter-h]

These magic numbers don't actually work well, because if a program crashes, it will very quickly enter the function where you are loading the magic number One discovers this pretty fast when using the old parallel EEPROMs like the 28C256 which needs a magic number to enable writing. Any program crash results in a rapid corruption.

On the earlier topic, it's quite amusing to google for NVIC_PRIORITYGROUP_4 and discover how many people have been blindly copying everybody else's code which in turn was copied from Cube and which in turn has an origin which you will probably never discover because ST do not participate anywhere (except rarely and usually uselessly on the ST forum).

I came across something interesting here: https://www.freertos.org/FreeRTOS_Support_Forum_Archive/April_2014/freertos_Use_FreeRTOS_on_STM32f4_725438dej.html

  vTaskEnterCritical();
You cannot call that function from an interrupt.  In fact, the function is not part of the documented API and should not even be called directly from a task, and definitely never called directly on a Cortex-M part.  If you want a critical section inside an interrupt you need to use portSETINTERRUPTMASKFROMISR()/portCLEARINTERRUPTMASKFROMISR(). 

I am not actually doing that (disabling higher priority interrupts inside ISRs) anyway, but what is wrong with using
__disable_irq()
__enable_irq()
inside an ISR?

In the old days, Z80, H8, etc, an ISR would never be interrupted by another ISR. If you wanted that (and sometimes you did want to e.g. make a timer ISR, full of crap, to be interruptible by comms interrupts) you had to do

ISR start:
.. ; all ints are now disabled
..
(disable this interrupt source)
ld hl, label
push hl
ei  ; this enables ints but only after the "reti"
reti  ; ints are enabled after this
label:
(rest of ISR, all ints are enabled, except the one which got us here)
..
..
di
(re-enable this interrupt source)
ei  ; ints are enabled after the "ret" below
ret

So the ST ARM seems to do the above anyway, managed by the int "pre-emption priority" order. But if you want a critical section within an ISR, how would you do that? A google on portSETINTERRUPTMASKFROMISR finds ST didn't implement it in their FreeRTOS port.

[Siwastaja]

These magic numbers don't actually work well, because if a program crashes, it will very quickly enter the function where you are loading the magic number

If by "crash" you mean runaway Program Counter, then yes, it won't protect against it (with clever programming, partial protection against runaway PC is possible, but this is rarely done, instead we use wrappers just like SetPriorityGrouping()).

They still work somewhat, for example when writing to variables pointed to by corrupted pointers. Or massive over/underindexing; good old "off by ones" do not hit the peripheral/CPU register address space anyway.

[paulca]

I vaguely understand the other meaning of magic numbers.  I vaguely recall diagnosing X11's magic cookies which were basical auth seeds.  Fixed salt hashes, the salt might be considered a magic number?  Well, nobody just drops a 128bit UUID into code in hex form as if it didn't happen.  Likes literally like taking a shit in the bosses drawer.

If you aren't going to eat it, name it.

I get even more anal and ask that people assign or comment function parameters like "true" and "false".  EG:

thing->doSomething( understandable, localVar, true, false, false, true, 42 );

Won't pass strict code review.  Not even when you take out 42 and name it.

[paulca]

In the most part I don't actually dislike the code style in HAL.  I much prefer it to Arduino.  It feels like it's adult if that makes sense.

But, it's obvious to me it's not all written by the same person and most of them aren't first language English either.

Some of the code look like it jumped the fence into release without testing.  Some of it looks like downright experimentally permenant (like half the 2.x linux kernel) and some even looks like it's been converted from C++ to C by an automated tool.

But the coding style and at least the attempt at comments I have no problem with.

Not that this helps you, sorry for ranting on your thread again.

[Siwastaja]

I call that just "undocumented". You can document it two ways, either
confRocket(/*enable_rocket =*/ true);
or
bool enable_rocket = true;
confRocket(enable_rocket);

Although one could argue this isn't needed if IDEs that are capable of popping up the definition are used. One could even argue there is less room for human error that way.

Magic numbers just mean the generic pattern of deliberately inventing a number which is used for identification purposes (protocol identifier, prevention of unwanted transactions, etc.), but some people call undocumented values in code "magic numbers", this is like calling people you don't like "gay".

[Sauli]

ARM has not made the interrupt system easy to understand.

The interrupt priority registers have 8 priority bits for each interrupt source. A silicon vendor can decide how many of these are used. ST uses 4 bits. The rest are always 0. ARM has decided that the unused bits are the lower end of the 8-bit priority value. See the attached table from ARMv7-M Architecture Reference Manual.

AIRCR.PRIGROUP value defines how many lowest bits of the 8-bit priority value are used for subpriority. The ST NVIC_PRIORITYGROUP_4 value (=3) sets the number of subpriority bits to 4. Which means that there are no subpriorities at all since the four lowest interrupt priority bits for ST are always 0. The same PRIGROUP value of 3 for a processor that implements more than 4 interrupt priority bits however means that there are subpriorities. Using PRIGROUP value 0 guarantees that there are no subpriorities no matter how many priority bits are implemented. The reset value of PRIGROUP by the way is 0.

[peter-h]

So the NVIC_PRIORITYGROUP_4 (=3) is old BS left over from code for another CPU and which just happens to work right (same as 0) on M4. And anyway is the same as the power-up reset value.

And the FreeRTOS site says you have to do it but doesn't explain why, either.

Happy to note that I am not stupid after all

Would anyone know anything about my question above
https://www.eevblog.com/forum/microcontrollers/does-this-code-need-interrupts-disabled/msg4648273/#msg4648273

[uer166]

but what is wrong with using
__disable_irq()
__enable_irq()
inside an ISR?

Architecturally? A few things. Practically? Nothing inherently wrong with doing that and it'll work as expected. Upon execution of __enable_irq() you'll of course get interrupted by any pending ISRs that are higher priority than what is currently executing.

[peter-h]

What I was wondering is the effect relative to ARM interrupt handling which uses "magic numbers" on the stack to indicate specific operations are requested. These values have high bits set to distinguish them from any physically possible address / PC value.

That is how ARM implements (or avoids, if you like) the classic old "RETI" or equivalent. You can write an ISR wholly in C without any "interrupt" or similar keyword. When an interrupt is serviced, a magic value is pushed on the stack, the ISR can do what it likes stack-wise, and when eventually the CPU gets around to popping that magic value off the stack, that is how it knows you are doing an "RETI".

Quite cunning.

What I was wondering is whether __disable_irq() etc has any effect on this scheme. After all, __enable_irq() enables interrupts globally, whereas the "RETI" enables only lower priority ones.

Architecturally? A few things.

What are those?

you'll of course get interrupted by any pending ISRs that are higher priority than what is currently executing.

And upon the final stack pop of the "RETI" magic value, I will get interrupted by any pending ISRs that are lower priority than the ISR just finished?

[uer166]

What are those?

I think you're overthinking too much.. But just to throw fuel on the fire:

• It adds latency jitter to higher priority ISRs that is equal to the critical value section. This should be obvious, and may or may not be a problem.
• It can indicate a hack/architectural issue that is better solved some other way. E.g. bit-banding for atomic flags, double buffering with atomic 32-bit variables, etc.

And upon the final stack pop of the "RETI" magic value, I will get interrupted by any pending ISRs that are lower priority than the ISR just finished?

No, it shouldn't. A lower priority ISR shouldn't magically interrupt your running ISR, the NVIC isn't that broken.

[peter-h]

OK, got all that.

After the "RETI" the lower priority ints should execute as they normally would.

Perhaps an interesting Q might be if you do

isr_function fred
{
blah
blah
__disable_irq
blah
blah
__enable_irq
}

that will generate a tail of

 __ASM volatile ("cpsie i" : : : "memory");
whatever is the asm for "RET" (there isn't one as such; they do a "POP PC").

and the lower priority ints will be allowed in in between the two above lines. Should not matter... In classic ISRs, ending with

EI
RET

the EI enables ints only after the RET.

[peter-h]

A little update on this old saga.

Having been doing other coding on this project I returned to the old problem whereby 1 byte gets lost every 50-100 megabytes.

Someone pointed out to me that it could be that the TX may not have room in the TX holding register. This is even though the TX loading event "cannot happen" unless there was a preceeding TXE interrupt.

Well, it is exactly what does happen!

Code: [Select]

// Use a local write index
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;

__disable_irq();

// If transmit interrupt is disabled, restart transmit

if( (serial_port_handles[port]->Instance->CR1 & UART_IT_TXE) == 0 )
{

// Load the first byte and enable the TXE and TC interrupts.
// First clear TC. This is done with a read of SR and a write to DR.

// Read SR
// serial_port_handles[port]->Instance->SR; // happens below

// Hang until TX has room - should never happen
while( (serial_port_handles[port]->Instance->SR & UART_FLAG_TXE) == 0 )
{
LED(true); // this pulses a GPIO pin
LED(false);
}

// This is the write of DR
serial_port_handles[port]->Instance->DR = serial_transmit_buffer[port][serial_transmit_buffer_tx_index[port]++];
if( serial_transmit_buffer_tx_index[port] >= SERIAL_TX_BUFFER_SIZE)
{
serial_transmit_buffer_tx_index[port] = 0;
}

g_comms_act[port+1]=LED_COM_TC; // indicate data flow on LED 1-4

// Enable TX complete and TX empty interrupts. The order may matter! (doesn't appear to)
serial_port_handles[port]->Instance->CR1 |= UART_IT_TC;
serial_port_handles[port]->Instance->CR1 |= UART_IT_TXE;

}
__enable_irq();
I measured the pulse width of LED() above and it is 19us at 38400 baud UART config and 38us at 19200 baud UART config. So it is something to do with some UART internal clock speed, but nothing as obvious as 1 bit time, etc.

It doesn't really matter except that this wait is being done with interrupts disabled, which is going to produce unwanted jitter elsewhere. So I will try to move that check to outside the ints-disabled region.

[wek]

Is this still STM32F407/417?

> if( (serial_port_handles[port]->Instance->CR1 & UART_IT_TXE) == 0 )

WTF is UART_IT_TXE?

(And yes, I know what it is, and it's wrong, even if it does not cause the observed behaviour; and even is factually harmless, it is still wrong.)

> while( (serial_port_handles[port]->Instance->SR & UART_FLAG_TXE) == 0 )

UART_FLAG_TXE too.


> I measured the pulse width of LED() above

Is there one pulse? Many?

> LED(true);   // this pulses a GPIO pin

I don't believe.

Now to the observed behaviour:

> This is even though the TX loading event "cannot happen" unless there was a preceeding TXE interrupt.

What is "TX loading event"? UARTx_DR = some_data?

> unless there was a preceeding TXE interrupt.

I am going into crystal ball mode now: in the TX ISR, after loading data from circular buffer into UARTx_DR, you advance tail and check if tail meets head thus circular buffer is emptied and there you clear UARTx_CR1.TXEIE. But at that point, transmitter is not empty yet and it takes time until it's emptied (and then TC interrupt kicks in etc.).

You are checking UARTx_CR1.TXEIE as the guard in the code above.

JW

[NorthGuy]

Code: [Select]

// If transmit interrupt is disabled, restart transmit

if( (serial_port_handles[port]->Instance->CR1 & UART_IT_TXE) == 0 )

Shouldn't you be checking for TC instead?

[NorthGuy]

See how it works:

[peter-h]

One is always torn between posting too little (and getting replies from irritated people saying I didn't post enough info) and posting too much (and getting no replies at all)

Transmit mode terminates with TX interrupt finding no more data to send:

Code: [Select]

void serial_transmit_irq_handler(UART_HandleTypeDef *huart, uint8_t port)
{

port--; // offset from 0

// Is there anything left to send?
if (serial_transmit_buffer_tx_index[port] == serial_transmit_buffer_put_index[port])
{
// Disable the TXEmpty interrupt
huart->Instance->CR1 &= ~(UART_IT_TXE);
}
else
{
// Send the next byte
huart->Instance->DR = serial_transmit_buffer[port][serial_transmit_buffer_tx_index[port]++];
if( serial_transmit_buffer_tx_index[port] >= SERIAL_TX_BUFFER_SIZE)
{
serial_transmit_buffer_tx_index[port] = 0;
}
g_comms_act[port+1]=LED_COM_TC; // indicate data flow on LED 1-4
}

}

So, after TX int has been disabled by the above, when new data has been placed into the circular buffer, you have to load 1 byte into the UART TX and enable TX interrupt. This is standard "UART stuff". Then the thing goes off by itself, transmitting under interrupt.

The UART_IT_TXE bit is also being used - as suggested further back - as an indicator as to whether transmit mode is active. If 1, then you just put new data into the buffer. If 0, then you have to do the above. Previously I used a separate flag for this but it turned out to be just a copy of IT_TXE bit.

TC is a separate thing here. The TC ISR is basically used just for disabling RS485 drivers:

Code: [Select]

// This ISR turns off the 485 driver on p3/p4
// https://peter-ftp.co.uk/screenshots/202210015115900110.jpg

void serial_transmit_complete_irq_handler(UART_HandleTypeDef *huart, uint8_t port)
{

if ( (port == 3) || (port == 4) )
{
// If this is an RS485 port and auto driver mode is set, disable the driver now
if (serial_driver_mode[port-1] == 2)
{
serial_485_driver_control_internal(port, 0);
}
}

// Transmit complete
serial_transmit_active[port-1] = false;

// Clear and disable the TC interrupt. This is supposed to be done by a read of SR and a write to DR
// but in this case we have nothing to write to DR so we clear the TC bit by writing a 0 to it but
// avoiding RMW on SR - see http://efton.sk/STM32/gotcha/g9.html
huart->Instance->SR = ~(UART_FLAG_TC);
huart->Instance->CR1 &= ~(UART_IT_TC);

}

32F417 yes

LED(true) and LED(false) set/reset a GPIO pin, producing a pulse.

Maybe I have done something stupid but a) I can't see what it is and b) the issue occurs extremely rarely.

To re-iterate: After TX int has been disabled (in the TXE ISR, upon finding the circular queue empty) when new data has been placed into the circular buffer, and the 1 byte has been loaded directly into the UART TX holding register, the UART appears to be very occassionally discarding this byte, even though before that the TXE interrupt has been generated which could only be if there is room in the holding register. That is what TXE is: there is room for 1+ bytes. In the 32F417 it will be just one byte (although if you got the timing just right you might be able to get two bytes in, but I am not doing that). The hack (waiting for TXE=1, before loading this byte) works but the wait takes a little too long in this particular application.

Putting it another way, it appears that the TXE interrupt is sometimes generated before the byte in the tx holding register has been moved to the tx shift register.

The data loss is of the order of 1 byte in 10^7 bytes so in the vast majority of applications would not be noticed. This is the frequency of the pulse I see on the scope. Just one every few hours or so. Just seen one which was 52us which is even less related to 19200 baud than previous ones.

If anyone can think if a better way to address this, I am all ears, otherwise I will implement the wait for TXE=1 just above the DI/EI region where an extra few tens of us doesn't matter.