Make HAL defined interrupt handlers overridable

[bjoernQ]

Fixes #856

When using the async feature the HAL implements interrupt handlers for various peripherals.
It was not possible to override them in user code (and not using the async feature of that driver)

This makes all the handlers implemented by the HAL weak so user code can override them.

If the user uses the async feature of a peripheral AND provides an alternative implementation of the interrupt handler it won't work. We cannot detect that at compile time but we could check it at runtime. I haven't implemented that - we can see if this ever becomes a real problem and do it then when needed.

Drawback might be that we need to maintain the weak_isrs.x file when adding a new HAL defined interrupt handler implementation but on the other hand forgetting that will become obvious very quickly

Alternatives to this

• not support it at all (but there are valid use-cases for this)
• feature gate each async driver (would be a lot of features)

[jessebraham]

I think this looks fine in general, thanks for taking a swing at it.

One nitpick, not a huge fan of the name interrupt_internal, mostly just because it doesn't really tell you how it's different from the previous interrupt macro. Maybe something like #[weak_interrupt] or something would be a bit more informative?

I will take a closer look at the changes and do some testing in a bit, let me know your feelings on the renaming (we don't need to change it, after all).

[bjoernQ]

Sure - weak_interrupt is probably a better fit. I used internal_interrupt to discourage using it in user code but the description should make that clear already - we could also not re-export it (but then we need to list everything we want to export)

[jessebraham]

we could also not re-export it (but then we need to list everything we want to export)

Yeah probably outside the scope of this PR but I was thinking we can probably do something like this. At least for now, as you mentioned the comment should hopefully discourage use.

[MabezDev]

I'm a bit worried about silently breaking async code by defining an interrupt, at least with the current mechanism it stops you from shooting yourself in the foot. I think at the very least we need to make the interrupt code for handling async public so that users can call it too in their own handlers.

Long term I think we might move away from using linkage to resolve interrupt handlers and instead move to something at runtime. At the same time, we can start treating interrupts as a resource and take them in peripheral constructors etc, this solves this issue quite neatly, as well as the forgetting to enable the interrupt problem we also have.

[bjoernQ]

Yes, probably good to allow users to call the interrupt handler from their own handlers if needed

Long term I think we might move away from using linkage to resolve interrupt handlers and instead move to something at runtime. At the same time, we can start treating interrupts as a resource and take them in peripheral constructors etc, this solves this issue quite neatly, as well as the forgetting to enable the interrupt problem we also have.

Agreed - might give some performance penalties but it's probably a non-issue in most cases and when low-latency interrupts are needed, already vectored and saving/restoring the full context isn't too helpful

[bjoernQ]

Ok after thinking about this I'm about to close this PR:

I don't think that having your own interrupt handler but wanting to use the async feature of that driver is a real use case?
I assume that people would like to use async for a few peripherals and non-async for others

If that assumption is true, then having a feature for each of the async drivers (e.g. async-spi) and the async feature would just activate all of those features would be a better solution.

That way we would keep the compile time (link time) safety - if the user doesn't activate e.g. async-uart they just can't use it async and implementing the interrupt handler is totally fine

Probably it wouldn't be needed to check every combination of these features in CI - we could continue to check async and if that is fine it's somewhat safe to assume that also every combination of the sub-features will be fine (although it's not guaranteed)

There is a limit of 300 features per crate on crates.io but I don't think we'll get close to that (even if we would add more per-driver features)