-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Bindable EXTI interrupts, and removal of AnyChannel support for ExtiInput #4922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bindable EXTI interrupts, and removal of AnyChannel support for ExtiInput #4922
Conversation
…ged Exti driver to accept custom bindings without sacrificing its ability to accept type-erased Channels
…ave touch sensing, updated examples, added generation of convenience method to bind_interrupts for easier type erasure
|
Alright, I think this is ready now. There were a few other issues to iron out, including handling the shared EXTI2_TSC interrupt on chips with TSC peripherals; I actually needed to add a cfg option for when the metapac contains a TSC peripheral but no registers for it, signaling that TSC is present in the silicon even though embassy can't yet implement the driver. (Example: STM32F398VE.) This involved adding a check to the build script, as well as a conditional branch to the impl_exti macro. This previously worked due to the hardcoded EXTI handlers being defined through the for_each_interrupt macro, but now the EXTI peripherals themselves need to reference their IRQ, so for_each_interrupt won't work anymore. In addition to affecting dozens of examples, this change affected the example used in the layer-by-layer section of the embassy book, so I had to update the language there too. While I was at it, I expanded the answer in the FAQ about manual interrupts to recommend using (Also, I added a commented-out line to .vscode/settings.json to run rustfmt with +nightly, as a non-breaking hint to anyone else who gets bitten by there being nightly-only options set in the rustfmt 🙂) Let me know if you have any thoughts on the approach here - I know I'm really diving into the deep end (all because I wanted my EXTI dependent code to run a little faster), and I hope my solution seems in line with the project philosophy. |
|
@WillaWillNot I deleted my previous comment, the decision is that we are going to remove |
|
@xoviat sounds good - I had considered proposing removing AnyChannel myself, I was just unsure about which side of the tradeoff "more exposed complexity, less edits in preexisting complex codebases" (AnyBinding) vs "less exposed complexity, more edits in preexisting complex codebases" (remove AnyChannel) would be better. Eliminating AnyChannel type erasure might be a bit of a pain in the butt in many codebases, but it's not that bad, you just might need some unrolled structs and matches instead of the convenience of iterating. I'll have a new version within the next few days that does that. I think this might also allow me to hide the complexity of the user needing to know whether to use e.g. EXTI13 or EXTI15_10 on their chip when writing their bind_interrupts. Maybe. Because of the fact that bind_interrupts needs to define both the binding and the precisely named C linkage ISR, it may still require either a proc_macro or a dependency on the |
…xtiInput to accept AnyPin and AnyChannel arguments, added ExtiPin trait for all pins which is lost on converstion to AnyPin and contains type-level ExtiChannel information
…ing if it's not present in the PAC, removed macro magic in exti that was working around this omission
|
Done! Wasn't able to find a way to hide the weird multipurpose interrupt mapping from the user, but I noticed a similar situation is already shown in the layer-by-layer example so I just added some text there for anyone who gets surprised by it. Also, the TSC thing was even stickier than I initially thought - there's some chips that don't shadow EXTI2 with the TSC, it's on its own independent line. So, now I just have the build script search for EXTI2_TSC in the interrupt list, and if present, inject it into the EXTI interrupt mapping. It's ugly but it works. At least I'm not adding a cfg option anymore, and impl_exti! doesn't need the branching logic. Really, that should be fixed in the PAC, and I might look into submitting a PR over there. It should list EXTI2_TSC as the line for EXTI2. Until then though, this ugly bandaid logic is needed to make sure the mapping happens. (Making ExtiInput check the bindings at compile time rather than runtime surfaced that issue via a failing build - one good reason to try to stick with compile time checks when possible!) |
|
Updating the PAC is not a big deal, since the git version is used. So feel free to fix the tsc issue, if you want. |
Following up on #4028, getting this working sure was a puzzle!
Unlike other peripheral drivers, Exti needs to cope with the need to assign a single handler at the code level to a vector which represents interrupts for several drivers. Also unlike other drivers that look for bindings, the parameter that tells Exti which binding to look for... can be type-erased!
The first issue isn't that hard, the Exti's in-library handler just has to be generic over the interrupt rather than the instance, and the user will need to deal with the implementation detail of sometimes needing to bind to EXTI0_1 or similar in their bind_interrupts! invocation, rather than intuitively binding to EXTI0, EXTI1, etc.
For the second issue, after trying many different approaches, the best I could come up with was a type-erased AnyBinding, which is unfortunately kind of awkward to cast into. Binding has to be implementable by the user, which means you can't seal it with a sealed supertrait. Which means you can't blanket implement
From<Binding<I, H<I>>> for AnyBinding, because Binding is user-implementable, and you clash with the blanketFrom<T> for Timplementation. This also means the binding has to be checked at runtime, not compile time.This also required representing the type of the handler in the binding somehow, so I had to add a HandlerType enum. (Interrupts already have an exhaustive enum for this.) This is implemented in a defaulted constant in the Handler trait, with a privacy guard to keep user code using the User type. Rather than trying to make an exhaustive change across the entire codebase to assign types, I just left everything but Exti using the default, since nothing else is expecting AnyBindings.
Not thrilled that I have to propose a brand new type erased interface for something used so widely across the codebase just to get bindable EXTI working, but I couldn't find another way. I know it's a bit inelegant, but I'm trying to build the minimum viable horse to put in front of my cart.
This is a draft because I'm aware there's a giant pile of examples I need to edit before it's ready to merge, but I wanted to get eyes on a breaking API change sooner rather than later. It's only breaking for
embassy_stm32::exti::Exti::new(),everything else I was able to make backwards compatible.