Implement InterruptConfigurable
#1819
Conversation
There was a problem hiding this comment.
Didn't know this was being worked on, I was gonna suggest a solution for drivers that split up like PARL_IO/LCD_CAM.
The setting of the interrupt handler could be done on a separate object.
struct LcdCam<'d> {
cam: Cam<'d>,
lcd: Lcd<'d>,
interrupt_setter_thingy: IntrSetter<'d>,
}Though, this is yet another thing that can be done in a future PR.
| /// Note that this will replace any previously registered interrupt handler. | ||
| /// Some peripherals offer a shared interrupt handler for multiple purposes. | ||
| /// It's the users duty to honor this. | ||
| fn set_interrupt_handler(&mut self, handler: interrupt::InterruptHandler); |
There was a problem hiding this comment.
There should also be a way to unset the existing handler and/or perhaps disable it.
I haven't looked the implementation yet but would it be possible to return the previous handler was replaced?
There was a problem hiding this comment.
Unsetting would mean to install the default-handler so I guess we could easily provide something like DEFAULT_INTERRUPT_HANDLER: InterruptHandler for that
I'm not sure about a real-world use-case where getting the previous handler is super useful?
There was a problem hiding this comment.
I'm not sure about a real-world use-case where getting the previous handler is super useful?
I'm imagining a scenario where some component built on top of this trait may want to temporary receive interrupts and then restore the previous handler when it's done.
Could even use this to restore the default handler I suppose.
| @@ -607,6 +607,17 @@ impl<T, const SIZE: usize> FlashSafeDma<T, SIZE> { | |||
| } | |||
| } | |||
|
|
|||
| /// Trait implemented by drivers which allow the user to set an | |||
| /// [InterruptHandler] | |||
| pub trait InterruptConfigurable: private::Sealed { | |||
There was a problem hiding this comment.
As is, I'm not sure how I would consume this trait as a user, but with two certain additions, one could make useful abstractions with it.
One is to add an associated type that represent the interrupts bit. Either a boolean struct like in Pcnt driver or an EnumSet like in the Spi driver.
The second is to expose methods to access the INT_CLR, INT_ENA, INT_ST, INT_RAW registers.
Idk if it makes sense for this PR or as a future enhancement but I think one can write a generic Future on top of this. (Barring the awkward case when multiple interrupt handlers share the same INT_* registers like SYS_TIMER)
There was a problem hiding this comment.
I also had some doubts initially about the trait ( #1489 (comment) ) but I agree that with some additions it could become more useful
Looking at the amount of changed files I probably would say that's something for another PR since getting rid of the handler from the ctors is already a good thing on its own (since in most cases there is an annoying None passed)
There was a problem hiding this comment.
Sorry I didn't fully explain my position in the original issue, but I don't see the trait in its current form as final, I just wanted a place to start to have a consistent API and something that we can start to abstract over (I didn't like that we had two ways to set interrupts).
I think your suggestions would be great in a future PR :)
|
I think I would consider this as done for the original scope of the task We should create an issue listing future improvements to not forget about them! |
| @@ -570,6 +513,15 @@ where | |||
| } | |||
| } | |||
|
|
|||
| impl<T> InterruptConfigurable for Timer<T, Blocking> | |||
There was a problem hiding this comment.
I think we also need an impl for the WDT?
There was a problem hiding this comment.
Oh true! I added it now but we currently have no way to configure WDT to do anything else but a system reset. I will create a separate issue for that
Thank you for your contribution!
We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:
Submission Checklist 📝
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly.CHANGELOG.mdin the proper section.Extra:
Pull Request Details 📖
Description
Applies the ideas outlined in #1489
Closes #1489
Testing
All (adapted) examples and test work