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.