ADC Interrupts Vector is wrong on STM32F103

[asmfreak]

This block from modm/src/modm/platform/adc/stm32/adc_interrupts.cpp.in is wrong on STM32F103.

extern "C" void
ADC_IRQHandler(void)
{

The real name of this handler is

extern "C" void
ADC1_2_IRQHandler(void)

[salkinium]

Then the interrupt vector needs to be searched for like for UART:
https://github.com/modm-io/modm/blob/develop/src/modm/platform/uart/stm32/module.lb#L97

[asmfreak]

As per modm/ext/modm-devices/devices/stm32/stm32f0-42-4_6.xml:

      <vector position="12" name="ADC1"/>

as per modm/ext/modm-devices/devices/stm32/stm32f1-03-8_b.xml:

      <vector position="18" name="ADC1_2"/>

[asmfreak]

I get it. Will make PR pronto

[salkinium]

I really don’t like ST’s inconsistencies about peripheral instances across their devices, it’s super annoying.

[salkinium]

I recommend doing an ack "<vector .*? name=\"(ADC.*?)\"" | sort | uniq or similar inside the modm-devices repo to get all the different spellings of that vector.

[salkinium]

Uff, this is going to be annoying:

Different vectors for different instances:

stm32l4-76_86.xml
49:      <vector position="18" name="ADC1_2"/>
78:      <vector position="47" name="ADC3"/>

Different vector names for different devices:

stm32f3-18_28.xml
32:      <vector device-name="18" position="18" name="ADC1"/>
33:      <vector device-name="28" position="18" name="ADC1_2"/>

[salkinium]

And the stm32f0 and stm32f3 implementation do not even have the AdcInterrupt class implemented… Why is this so inconsistent?

I feel like this AdcInterrupt should not be part of modm, but should be delegated to the user?

 $ ack "vector .*? name=\"(ADC.*?)\"" -h | sort | uniq
      <vector device-name="18" position="18" name="ADC1"/>
      <vector device-name="28" position="18" name="ADC1_2"/>
      <vector device-name="31" position="12" name="ADC1"/>
      <vector device-name="51" position="12" name="ADC1_COMP"/>
      <vector device-name="51" position="18" name="ADC1"/>
      <vector device-name="71" position="18" name="ADC1_2"/>
      <vector device-name="71" position="47" name="ADC3"/>
      <vector position="12" name="ADC1"/>
      <vector position="12" name="ADC1_COMP"/>
      <vector position="127" name="ADC3"/>
      <vector position="18" name="ADC"/>
      <vector position="18" name="ADC1"/>
      <vector position="18" name="ADC1_2"/>
      <vector position="47" name="ADC3"/>
      <vector position="61" name="ADC4"/>
      <vector position="62" name="ADC5"/>

 $ ack "vector .*? name=\"(ADC.*?)\"" -h --output "\$1" | sort | uniq
ADC
ADC1
ADC1_2
ADC1_COMP
ADC3
ADC4
ADC5

[asmfreak]

What do you think about a generic class in line of:
given name and ISR_NAME
GenericInterrupt.hpp:

class {{name}}Interrupt{
protected:
    typedef void (*Handler) ();
public:
    static inline void
    attachInterruptHandler(Handler handler=modm::dummy){
        {{name}}Interrupt::handler = handler;
    }
public:
    static Handler handler;
}

GenericInterrupt.cpp:

{{name}}Interrupt::Handler     {{name}}Interrupt::handler = modm::dummy;
MODM_ISR(ISR_NAME){
      {{name}}Interrupt::handler();
}

[asmfreak]

Offtopic: what is that ack tool?

[salkinium]

https://beyondgrep.com
Like a simpler, less annoying grep.

[salkinium]

What do you think about a generic class in line of given name and ISR_NAME

I've been thinking about an NVIC wrapper for some time now.
The NVIC hardware is pretty feature complete with enable/disable, priority, pending flags, software trigger, so I'd like to keep the API that way.
The way the interrupts are currently implemented is that the interrupt vector table entries are weakly aliased to Undefined_Handler, which raises an assertion if the IRQ is enabled, but no vector has been declared for it.

However, you still have to use the right IRQ name, which is really annoying. I cannot change the IRQ names (they are defined like this in the Reference Manual), nor can I create aliases (like ADC1 = ADC1_2, ADC2 = ADC1_2) since then the user overwrites the settings in the NVIC:

NVIC_EnableIRQ(ADC1);
NVIC_DisableIRQ(ADC2); // overwrites ADC1

etc…

So even if we have this {{Peripheral}}Interrupt::Handler class, I still somehow have to arbitrate the settings for this shared interrupt, which just kinda moves the problem now to a different person (ie. the user).
So you would have to create a complete wrapper for the NVIC code, so that you can arbitrate "shared_priority = max(individual_priorities)", "shared_enable = logical_or(individual_enables)" etc.

[salkinium]

There are also more interesting problems here: For GPIO EXTI I should be able to attach an handler per pin, but how do I do this without dynamic memory or a massive static array? How is the timing of this solution going to be? Would any such API still work on AVRs or at least be backwards compatible?

[asmfreak]

Offtopic:

The way the interrupts are currently implemented is that the interrupt vector table entries are weakly aliased to Undefined_Handler, which raises an assertion if the IRQ is enabled, but no vector has been declared for it.

I quite like this feature. We were able to find the described bug using UndefinedHandlers.

[salkinium]

I thought that you could generate one linker script section per shared interrupt that would contain all the function pointers to the individual interrupts declared by the user (maybe with some metadata, like a mask containing the pending flag(s?)). That would probably be the fastest and least space consuming solution, but it would include a lot of magic linkerscript stuff. (We already have this for the .hardware_init section, used for example to enable all GPIO ports before main, but only if the :platform:gpio module is generated).

But adding linkerscript sections to the AVR is a complete nightmare, due to the weird Harvard-Architecture with the completely separate address spaces, requiring you to do these horrible address acrobatics with INSERT AFTER, since avr-gcc already predefines their own linkerscript and then still use pgm_read_word. I would probably just not implement this feature for AVRs.

[asmfreak]

So, using the linkerscript, calls to MODM_ISR(ADC1) and MODM_ISR(ADC1) will generate the functions, that will go into the array, of say, .hardware_ADC_1_2_Interrupts, which will be called by MODM_ISR(ADC1_2)?

[salkinium]

Yes, but the MODM_ISR(ADC1_2) would require some metadata to know which function in .hardware_ADC_1_2_Interrupts needs to be called from looking at the ADC interrupt pending flags.
So it would probably be something generated like MODM_ISR_ADD_ADC1(function) or similiar, that pushes a struct with generated mask and user-provided function (pointer) into the section.
That would be somewhat type-safe in the sense that if the peripheral does not exist, the macro is not found during compilation.

I think this mechanism would be pretty flexible, but it does add some overhead when you just use one function (although you might be able to use inline assembly to speed things up) and you need to declare the original IRQ handler somewhere and still do the NVIC settings arbitration (especially not accidentally turning off a shared interrupt, that can quickly get ugly). Somewhat complicated still.

[salkinium]

I mean the STM32 UART already does this, but in a manual way, minus the linkerscript. Due to the lbuild instance submodules (ie. :platform:uart:1) the generator knows what functions are declared, so it can call them directly. This works well, HOWEVER, the shared Uart IRQ is actually shared beyond UART, so it's currently cannibalizing the LPUART1 IRQ in favor of itself:

 $ ack "vector .*? name=\"(USART\d_.*?)\"" -h | sort | uniq
      <vector device-name="30" position="29" name="USART3_6"/>
      <vector device-name="70" position="29" name="USART3_4"/>
      <vector device-name="71" position="29" name="USART3_4"/>
      <vector device-name="78" position="29" name="USART3_4"/>
      <vector device-name="91" position="29" name="USART3_8"/>
      <vector device-name="98" position="29" name="USART3_8"/>
      <vector position="14" name="USART4_5"/>
      <vector position="29" name="USART3_4"/>
      <vector position="29" name="USART3_4_LPUART1"/>

but only on this one STM32G0 device:

stm32g0-71_81.xml
68:      <vector position="29" name="USART3_4_LPUART1"/>

Still not a very generic solution.

[salkinium]

So… I can't stop thinking about this, here are a couple of interesting but incomplete thoughts:

1. The NVIC is just a middle man, you're actually looking to hook hardware events, a lot of which are ORed together in hardware into one IRQ (EXTI being an extreme example) but you will most likely not use all of them at the same time.
2. The IRQ mechanism forces hardware events together into groups with the same properties, which are arbitrated against other groups of hardware events, which creates limits on the real-time properties of the events (ie. primarily through IRQ priority, and then order of handlers within the group).
3. But you can sort linker sections by object name, so you can actually define the order of handler execution within an IRQ group by encoding the priority in the object name.
4. modm-devices does not contain STM32 hardware events nor their mapping to IRQs (yet).
5. On STM32 at least, the hardware events always have a separate enable and pending bit. You can enable all NVIC IRQs, but if you have not enabled any hardware events, no IRQ is called.
6. You can set any NVIC IRQs pending via software even if they are not enabled, but afaik, you cannot set hardware events pending on STM32 via software, especially not if the peripheral is not enabled.
7. Unused IRQs are aliased to Undefined_Handler by default, but they can all be set to different functions without much effort. This could be a way to implement a default demultiplexing function for every IRQ, that can still be overwritten by the user.
8. You will probably want to define a global set of priorities for each hardware event separately and have some code generate the best compromise depending on the targets specific IRQs and their usage.
9. Not all hardware events priorities can be resolved beforehand, for example DMA may need to arbitrate priorities at runtime depending on the current resource usage. In that case providing a pre-computed, reduced set of conflicts can make the runtime arbitration much more efficient than a generic approach, checking all conflicts (probably even beating fancy data structure approaches).
10. This could be combined with a RTFM scheduler (C++ implementation: CRECT), where instead of IRQs you operate on hardware events.

[asmfreak]

As an offtopic question:
I've watched talk about crect by its author at emBO++18, I really liked the idea, but I didn't really look into its souce. Since I've watched the talk I was wondering, if it is possible to use modm with crect? And if yes - how?

[salkinium]

Since I've watched the talk I was wondering, if it is possible to use modm with crect? And if yes - how?

Similarly to how you can add the :freertos module and use pre-emptive threading even though nothing in modm is thread-safe, you can probably get CRECT and modm to compile together, but everything in modm is not written with the resource encapsulation that CRECT uses, and thus you would have to manually wrap modm into CRECT tasks. modm will also interfere with the NVIC by predefining some ISRs (I think only ADC, UART, I2C).

I know that CRECT (or any NVIC-based RTFM scheduler) has an issue with tasks using a shared interrupts, since they then have to use the same priority. That was very similiar to the discussion above which is why the solution to this would be some generated demultiplexor for both modm and CRECT.

[salkinium]

Also note that the Rust version of CRECT has significantly more features, these blog posts are a good overview over what is possible: https://blog.japaric.io/tags/rtfm/

[asmfreak]

Thanks! It is in the lines of what I thought. I'll see more into it later.