Improve handling of interrupt allocation

[jessebraham]

trafficstars

Currently when using the async (and some other) features, a number of interrupts are automatically bound to handlers.

This can be problematic for various reasons. One such example is implementing the embedded_hal_async::delay::DelayNs trait, which when implemented for TIMG$n/SYSTIMER conflict with the Embassy time drivers. In other cases, users may want to define their own interrupt handlers for certain peripherals, while still using the default interrupt handlers defined via the async feature in the HAL. This is presently not possible.

I'm not sure what the solution for this is yet. #1047 tried making the interrupt handlers overrideable, but we did not end up merging this; see the discussion in the PR for details. I would like to hear people's thoughts on potential solutions to this problem.

Progress:

• https://github.com/esp-rs/esp-hal/pull/1231
• https://github.com/esp-rs/esp-hal/pull/1278
• https://github.com/esp-rs/esp-hal/pull/1299

Peripheral conversion

• [x] UART: | Blocking,Async | https://github.com/esp-rs/esp-hal/pull/1294
• [x] RMT: | Blocking,Async | https://github.com/esp-rs/esp-hal/pull/1341
• [x] AES: | Blocking #1300
• [ ] ANALOG: | Blocking
• [ ] DELAY #1348
  • [ ] SYSTIMER: | Blocking #1348
  • [ ] TIMG: | Blocking #1348
• [x] GPIO: | Blocking,Async | https://github.com/esp-rs/esp-hal/pull/1231
• [ ] HMAC: | Blocking (Peripheral doesn't generate interrupts)
• [ ] I2C: | Blocking,Async
• [x] I2S: | Blocking,Async #1300
• [ ] LCD_CAM: | Blocking
• [ ] LEDC: | Blocking (Interrupt support not implemented yet)
• [ ] MCPWM: | Blocking
• [ ] USB OTG_FS: | Blocking
• [ ] PCNT: | Blocking
• [ ] RNG: | Blocking (Interrupt support missing)
• [x] RSA: | Blocking,Async #1354
• [x] SHA: | Blocking #1354
• [x] SPI: | Blocking,Async #1300
• [ ] TWAI: | Blocking,Async
• [ ] USB_SERIAL_JTAG: | Blocking |
• [ ] ETM: | Blocking | (Peripheral doesn't generate interrupts)
• [x] ECC: | Blocking | #1354
• [x] PARL_IO | Blocking,Async | #1300

[bjoernQ]

I like the idea of runtime binding of interrupt handlers as suggested here: https://github.com/esp-rs/esp-hal/pull/1047#issuecomment-1864602855

I had a brief look at our code and I think it won't add much more latency - if any - for the vectored interrupt handling (which I guess is the most used option)

[MabezDev]

Fleshing out my ideas from https://github.com/esp-rs/esp-hal/pull/1047#issuecomment-1864602855.

I believe that we might want to remove the async feature from the hal, we can keep it if we like of course, but what I really mean is that a feature should not determine a driver whether a driver is async or blocking, it should be determined by how its initialized.

Let's use Uart as an example. Imagine we introduce a new typestate for the mode Uart<T: Instance, M>, in which the mode can be two concrete types, Blocking and Async. We can now have custom constructor for each "mode". Instead of explaining in text, here is a concise playground link with the basic idea: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1ed57ede8b38f89bed84058ca5dbc3b4.

With the above in mind, we need changes to the current interrupt mechanism. The current mechanism uses linkage to bind the interrupt handlers, but we need to turn this into a runtime mechanism. The code inside the interrupt module, will most likely stay the same. What will change is how we install the interrupt fn ()'s into the __INTERRUPTS array. Most likely this will be done through the constructor of peripheral drivers.

[bjoernQ]

Basically, had a similar thing in mind (minus the type-state).

Most likely this will be done through the constructor of peripheral drivers.

And in case of non-async we probably want to provide a function to register a user provided interrupt handler.

The code inside the interrupt module, will most likely stay the same.

I also think so - I think we need to place __INTERRUPTS in RAM for RISCV - IIRC it's currently on flash

[MabezDev]

Basically, had a similar thing in mind (minus the type-state).

FYI the reason I added the type-state is so we only implement certain traits in specific modes, for example, we don't want to implement the async traits if a driver is initialized in blocking mode.

[MabezDev]

After some discussion, I think we're all on board with trying to explore the proposal above.

We found four areas we'll need to investigate before we can commit to this design:

• [ ] User facing, how do we map interrupts as resources? How do we customize handlers? How do we pass them into peripheral drivers?
• [x] Internally, what work do we need to do to handle the new interrupts, how to enable them? What do we do with the current interrupt modules API?
• [x] Linkage - Hopefully the linker changes should be minimal, but we should look into what we need to remove from the pacs, and where to place the interrupt array now that we are binding at runtime.

Extended goals, not critical to the initial design:

• [x] Edge cases - What do we do with DMA channels? DMA channels that can be split? DMA Channels that are tied to a specific peripheral? Are there any other edge-case peripherals to consider?

[MabezDev]

Thanks to @bjoernQ for investigating much of this! With the exploratory PR in #112, we can check off three of the four items in the list. The last side to explore is the user facing side, how we change the driver constructors and how users interact with interrupts.

For now we are focusing on unifying the HAL.

[jessebraham]

@MabezDev what is this blocked on?

[bjoernQ]

I wanted to start working on the real implementation of this after the next HAL release (I certainly can start behind the curtain earlier). Good thing is we don't need to have it in a big-bang

[jessebraham]

@MabezDev @bjoernQ no big rush, but whenever we get a chance could we please try to summarize what work still remains to be done in order to close this issue?

[MabezDev]

The next steps are as follows:

@bjoernQ will be taking care of DMA, which is another special case like GPIO.

The next stage for the rest of the drivers needs to be designed. I've posted some ideas earlier in this thread, but this isn't a full solution, but it's pretty close I think. We can probably skip the interrupt as a resource bit for now, and keep interrupt enable outside of the driver (though we can move it in the driver for async mode).

I probably won't get to it today, but I'll try and get a small-ish driver converted (maybe uart?) and see how it feels. Once we have a pattern to copy we can split the work up a bit.

[jessebraham]

For some reason there were some peripherals listed in the task list that we do not currently support interrupts for, so I have removed those as they're irrelevant to this task.

[jessebraham]

Working on the usb_serial_jtag driver.

[bjoernQ]

Working on TWAI now

[JurajSadel]

Working on debug-assist

[bjoernQ]

I'll look into SoftwareInterruptControl and friends next

[jessebraham]

Once the remaining items in the task list have been completed, we should do a review to ensure that we have not missed any peripherals. While doing this, it may also be beneficial to determine which existing drivers have not implemented interrupt support, though that probably deserves a separate issue for tracking.

[bjoernQ]

Once the remaining items in the task list have been completed, we should do a review to ensure that we have not missed any peripherals.

At least there shouldn't be any usage of #[interrupt] anymore. IIRC we also want to remove that macro once all drivers are changed

While doing this, it may also be beneficial to determine which existing drivers have not implemented interrupt support, though that probably deserves a separate issue for tracking.

Definitely another issue, agreed. I think there are also some drivers which can bind an interrupt handler now but it's almost impossible to really make use of that in user code. But that's probably another issue on its own then

[playfulFence]

I'll take PCNT

[JurajSadel]

Working on rtc-wdt

[SergioGasquez]

I think the changes are not required for MCPWM, just removed it from the list

[jessebraham]

There are only two items remaining in our current task list which do not already have PRs opened:

• LCD_CAM - Status unsure of what's left to do; still waiting on hardware to arrive to Brno office (has been ordered) so we can test this
• RTC_WDT - Being worked on by @JurajSadel

We will need to review the code base in its current form and ensure that we have not missed anything, but I think we're pretty close to being done here. Thanks a bunch to everybody who has helped out with this already!

[bjoernQ]

Seems like LCD_CAM currently doesn't offer or use any interrupt functionality. I suggest to remove it from the list - whenever interrupt support is added it will follow the new way of handling interrupts

EDIT: I removed it

[jessebraham]

Looks like we're done here! I'm going to go ahead and close this issue, if we discover any other drivers which we've missed we can open issues on a per-case basis.

Thanks again everybody for helping out with this!