Illogical EXTI code generation in CubeMX

[Clonimus74]

Posted on November 01, 2017 at 10:50

Hi all,

I find the code generated by the CubeMX for EXTI line illogical.

Every EXTI ISR (e.g. EXTI0_IRQHandler, EXTI1_IRQHandler calls for the same function: HAL_GPIO_EXTI_IRQHandler, which in turn also calls for the same function: HAL_GPIO_EXTI_Callback.

That means that both must be re-entrant functions, which makes the implementation riskier.

With HAL_GPIO_EXTI_IRQHandler I understand, since it should not change and it relies on HAL_GPIO_EXTI_Callback.

The thing is that if I want to not rely on HAL_GPIO_EXTI_Callback and prevent using re-entrant code, I need to check the flag (__HAL_GPIO_EXTI_GET_IT) in the specific ISR (e.g. EXTI0_IRQHandler), the flag is checked in a not so logical place, in HAL_GPIO_EXTI_IRQHandler after some of my code was already executed in the specific ISR.

I would think a specific callback for each EXTI should have been generated to prevent the risk of re-entrancy.

Thoughts?

[Alan Chambers]

Posted on November 01, 2017 at 12:26

The higher numbered pins (5-9, 10-15) share EXTI ISRs anyway, so cannot interrupt each other. If you set the priorities of all your EXTI interrupts the same, none of them can interrupt each other.

I think it's odd that HAL does not take the opportunity to split out the pins from the shared ISRs. You could have a series of HAL_GPIO_EXTI_n_Callback() functions to implement, rather than having to switch on the GPIO_Pin argument passed to HAL_GPIO_EXTI_Callback(). Not sure how this would solve issues of re-entrancy, though. Aren't such issues generally related to *non-local* data structures which need to be modified atomically? However you slice it, you still need to guarantee atomicity. Briefly disabling interrupts works pretty well, or you can use RTOS synchronisation features.

[Tesla DeLorean]

Posted on November 01, 2017 at 13:28

>>

If you set the priorities of all your EXTI interrupts the same, none of them can interrupt each other.

Exactly, unless they preempt interrupts will be handled in order.

With the LRWAN they do actually break down the EXTI into groups using multiple IRQ Handlers. But still call all. It is a generic solution. It allows pin changes at a higher level without having to chase down all the low level nodes.

Based on my review of the code over the years I'd observe that there is not strong experience with multi-threaded or async stack programming.

Frequently code in callback (interrupt context) that blocks, or relies on software timers which must preempt. Lot of opportunity for deadlocks or random failure.

If you think as CubeMX as a rough framing tool you'll be in a much safer place, edit the code to reflect your understanding of what you're doing.

[Clonimus74]

Posted on November 01, 2017 at 13:57

Most of the application I designed required different priorities to different EXTI lines. When designing the HW I take into consideration the fact the 5-9 and 10-15 share priority.

If you have multiple callbacks as oposed to one you need not fret using local static variables of the same name (just one example), that will be a pain in re-entrant code. Also having 'one function to rule them all' creates a difficult to read code.

I agree with Clive as that CubeMX creates a frame with which you work and change to your liking, but I know work on a project where the HW is not set and changing the initial HW means changing the Cube code, and so if I need to make 'serious' structural changes now, with every change I need to redo the changes with every code generation from Cube, that sucks (for that I use SVN and check modification from last commit comparing what the new generated code did).

[Tesla DeLorean]

Posted on November 01, 2017 at 14:08

Well I guess we need to be careful how we express 'priority', because you'll only get reentry on Cortex-Mx parts if there is preemption.

LRWAN does this

void EXTI0_1_IRQHandler(void)

{ HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_0);

HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_1);

}

void EXTI2_3_IRQHandler(void)

{ HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_2);

HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_3);

}void EXTI4_15_IRQHandler(void){ HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_4);

HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_5);

HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_6);

HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_7);

HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_8);

HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_9);

HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_10);

HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_11);

HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_12);

HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_13);

HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_14);

HAL_GPIO_EXTI_IRQHandler(GPIO_PIN_15);

}

I would tend to avoid using CubeMX as an iterative tool, and in that case tend to prefer to sandbox development code and merge in new stuff so I understood what changed and how it changed.

[Alan Chambers]

Posted on November 01, 2017 at 15:14

As Clive says, I would not recommend CubeMX for iteration. In fact, I would not use the code it generates at all. I generally use the GUI to help select pins and peripherals for a new board, but that's about it.

Fair point about local statics. My own EXTI handling uses a table of callback functions. The IRQ handler looks up the function for the relevant pin and, if not NULL, forwards the call. Would it help to implement HAL_GPIO_EXTI_Callback() similarly?

As for re-entrancy, I can't say it's ever been a huge problem in my projects. Code in both ISRs and thread mode should *always* serialise access to data structures which are accessed from different contexts (whether RTOS threads or ISRs). The simplest serialisation method is to disable interrupts for the duration *any* code (ISR or otherwise) which accesses those structures. It works for me, at any rate.

[Clonimus74]

Posted on November 02, 2017 at 08:10

'As Clive says, I would not recommend CubeMX for iteration. In fact, I would not use the code it generates at all. I generally use the GUI to help select pins and peripherals for a new board, but that's about it'

I generally agree, though it saves a lot of time, even though I can't help checking every line of code it generates, and find some mistakes, optimization needs (lots) etc.

Fair point about local statics. My own EXTI handling uses a table of callback functions. The IRQ handler looks up the function for the relevant pin and, if not NULL, forwards the call. Would it help to implement HAL_GPIO_EXTI_Callback() similarly?

It can, but depends on implementation, assuming the branching out from the table, obviously, is done from one place. I don't see the point of unifying the EXTI ISRs and then branching out. I prefer to totally separate them, it is safer, especially when one have many preemption levels.

Code in both ISRs and thread mode should *always* serialise access to data structures which are accessed from different contexts (whether RTOS threads or ISRs). The simplest serialisation method is to disable interrupts for the duration *any* code (ISR or otherwise) which accesses those structures. It works for me, at any rate.

I can't say I agree with that, serialization does not allow preemption and all the systems I worked on required that. In my current version I use almost all IO EXTI lines (about 12), and preemption is a big part of making all fit together and timed correctly. For example I might have very high timing accuracy for one EXI, while others can wait for a while and others are 'mid range' tolerated to timing... 

[waclawek.jan]

Posted on November 02, 2017 at 09:30

I find the code generated by the CubeMX for EXTI line illogical.

Whatever the design choices in a 'library', inevitably there will be a group of users who find them illogical.

By using any 'library' or library you voluntarily agree to be bound to a subset of possible functionality.

JW

[Alan Chambers]

Posted on November 02, 2017 at 09:36

I think we got crossed wires regarding pre-emption. I have found it generally unsafe to access *shared* data structures without some form of serialisation: Bad Things happen. Of course, if different ISRs (or threads) do not modify the same data structures, all is well.

BASEPRI is useful for blocking only some interrupts, which gives the opportunity to allow pre-emption by higher priority interrupts while protecting shared data accessed by lower priority interrupts. FreeRTOS uses this register for exactly this purpose. Anyway, I think we have drifted from your original point.

[Clonimus74]

Posted on November 02, 2017 at 10:48

BASEPRI is useful for blocking only some interrupts, which gives the opportunity to allow pre-emption by higher priority interrupts while protecting shared data accessed by lower priority interrupts. FreeRTOS uses this register for exactly this purpose.

Anyway, I think we have drifted from your original point.

Thanks, I wasn't aware of that, it is a good digression.

How do you set this register in regard to preemption and sub-preemption (e.g. I have 1 bit for sub and 3 bits for preemption)?