Support for custom IRQ Handlers

[nesos]

So this is the first draft of the custom IRQ implementation (sorry for the messy code).
The collection of the default IRQs for the CAN driver works and the forwarder will be generated in the vectors.c. Also the vector tables have the correct forwarder referenced (I only tried this on a F0 so far).
However the build system won't build and link the vectors.c.
I'm not really sure how to hack that.

[salkinium]

However the build system won't build and link the vectors.c.
I'm not really sure how to hack that.

Try to add the file manually here

[nesos]

The forwarder is working (the naming is a bit ugly though). I haven't optimized the call to the ISR yet but as a proof of concept this should do.

[salkinium]

See my branch for some additional commits that you may want to cherry pick. You can drop the work-around commit now with lbuild v1.13.0.

[salkinium]

I also added a validator for the Collector.

ERROR: In Module 'modm:platform:can:1': Failed to collect these values!
In file '../../../src/modm/platform/can/stm32/module.lb:129' in function 'build':
                    "CAN{0}_RX1:Can{0}_Rx1".format(self.instance))

StringCollector(vector_defaults): input 'CAN1_TX0:Can1_Tx' is invalid! Vector 'CAN1_TX0' is unknown!
Available vectors are:

    - WWDG
    - PVD
    - TAMP_STAMP
    - RTC_WKUP
    - [...]
    - DMA1_Stream6
    - ADC
    - CAN1_TX
    - CAN1_RX0
    - CAN1_RX1
    - CAN1_SCE
    - [...]

I don't like the vector_defaults name, perhaps vector_hooks is better? Alternative ideas?

[nesos]

Okay I cleaned up the generated code and the generator. However as already noticed the call to the ISR is not free anymore and inlining the driver function is not really working. I'm open to suggestions.

Also where should we go from here? Should we the add the option to add own calls to the forwarder?

[salkinium]

Hm, The Internet™ says you can with some effort alias to an external function: https://stackoverflow.com/a/58119272

Should we the add the option to add own calls to the forwarder?

Own calls? Or do you mean multiple calls per hook?

Status right now:

• |hooks| = 0: vector aliases to Undefined_Handler.
• |hooks| = 1: vector aliases to Forwarder, with additional overhead due to local aliasing.
• |hooks| >= 2: vector aliases to Forwarder but only one hook gets called (the one added "last").

Show: You can optimize for |hooks| = 1 and implement |hooks| >= 2. (14 points)
To achieve the permission for the exam you must earn 50% of the sum of all points. 😛

[nesos]

Hm, The Internet™ says you can with some effort alias to an external function

Phew that looks kinda dirty to be honest. Does lbuild support to add a step between compiling and building? Plus we had to determine where to find the ISR symbols so we can do the objcopy.

[salkinium]

Phew that looks kinda dirty to be honest. Does lbuild support to add a step between compiling and building? Plus we had to determine where to find the ISR symbols so we can do the objcopy.

I don't think this is a solution. It kinda sucks that the most common use-case comes with overhead, but on the other hand the overhead is like 4(?) instructions or so, if you really need that kinda performance, you're not gonna use the built-in interrupt handlers from modm and probably need to write assembler anyways. I would leave it like this for now.

It would be interesting if we could use extend this to use C++ static member functions, that would at least maintain the namespacing and reduce naming conflicts with user code (ie. using modm::platform::Can1::tx_isr() instead of top-level Can1_Tx()), but C++ doesn't have a separate forward declaration of static member functions.

You can of course declare a partial class again inside the vector.cpp:

namespace modm::platform {
class Can1
{
public:
    static void tx_isr();
}

However, you don't know whether modm::platform::Can1::tx_isr() is in a class or a namespace, so this doesn't really scale. And we cannot name-mangle the C++ name manually in python, since no such library exists (afaik).

So I guess we need to stick to manually declaring C functions, however, we could still wrap it into MODM_ISR_HOOK(modm_platform_Can1_Tx), which at least takes automatic care of extern "C" and prevents people from writing static member functions as hooks.

[nesos]

I got a bad feeling that we took a wrong turn with this solution. So I took a step back and looked at the use case again: I actually just want to have code executed when an interrupt occurs additionally to the driver code. So why not just move the calling of the additional code to the driver itself.

The idea would be to keep the ISR mechanism as it was. Instead of messing around with the vectors we could define an empty weak default hook function for each ISR inside of the driver and call that from the corresponding ISR. So something like this

weak void CAN1_TX_CustomHook(void) {}

This will be called after the body of the CAN1_TX handler of the driver. If it isn't overloaded the compiler will optimize out the function and if we want to do something we can just implement the CustomHook function in the user code.

The pros to this are:

• No overhead for calling ISRs
• Vectors can be generated in the build step again (no post_build order needed anymore)
• The CAN hooks for the F0 would be the same as for the others (although the F0 only has one IRQ for CAN events)

What do you think?

[salkinium]

weak void CAN1_TX_CustomHook(void) {}

That's basically how ST does it. We could still generalize it into a macro:

MODM_ISR_HOOKS(CAN1_TX)
{ /* main body */ }

// expands into
weak modm_extern_c void CAN1_TX_ISR_Enter() {}
weak modm_extern_c void CAN1_TX_ISR_Leave() {}
void CAN1_TX_ISR_Main(); // extern "C" not necessary
modm_extern_c void CAN1_TX_IRQHandler()
{
    CAN1_TX_ISR_Enter();
    CAN1_TX_ISR_Main();
    CAN1_TX_ISR_Leave();
}
static inline void CAN1_TX_ISR_Main()
{ /* main body */}

(This obviously depends on whether locally-defined weak functions really are optimized away! This would need to happen by the linker.)

The CAN hooks for the F0 would be the same as for the others

The above is obviously again a "raw" NVIC vector hook, so shared IRQs which change names (Timer IRQs are also pretty bad) from device to device are still and issue.

So this would make it driver hook, that happens after checking the ST-specific hardware interrupt flags (if necessary) and it's more difficult to clearly define the semantics here, since a driver provides more events to hook in different ways (possibly with arguments and return values).

I got a bad feeling that we took a wrong turn with this solution.

Well… we took a lot of wrong turns with modm 😇.

It would've solved the issue with shared interrupts in a more generic way.
And would be an interesting stepping stone for providing generic hooks for hardware events with a lbuild-generated NVIC demultiplexer:
#248 (comment)

[nesos]

Okay so I'll start a new branch tomorrow implementing this method