search

stm32-rs / stm32l0xx-hal

A hardware abstraction layer (HAL) for the STM32L0 series microcontrollers written in Rust
BSD Zero Clause License
96 stars 60 forks source link

Add encoder input support for TIM2 and TIM21 #145

Closed jamwaffles closed 3 years ago

jamwaffles commented 3 years ago

Adds encoder (QEI) input support to TIM2 and TIM21. I've tested the interrupt handling with an RTIC 0.6 project, and have included a short non-RTIC example to get users off the ground.

This leaves a few pin combinations and a few timers without an implementation currently, as I have no way to test them (or couldn't get them working) - I'll leave that as a job for other people who need it :)

jamwaffles commented 3 years ago

Thanks for the review and the feedback!

I'm not a fan of the EncoderExt trait (and those kind of extension traits in general). Just seems like unnecessary complexity where a simple constructor will do.

That's fair. I mirrored the API in timer.rs so one can do dp.TIMx.encoder which seems to be somewhat common, at least in RTIC contexts.

I don't like that most of the implementation lives in a macro. I much prefer to abstract over peripheral instances with an Instance trait (whose implementations are macro-generated) and build as much of the code as possible on top of that Instance trait, outside of the macro. See pwm.rs, for example.

Also fair! I'd prefer to do it without macros for the record. I took a brief look at doing it without them and gave up pretty quickly. I'll have another go and open a new PR if I get a clean solution together.

hannobraun commented 3 years ago

That's fair. I mirrored the API in timer.rs so one can do dp.TIMx.encoder which seems to be somewhat common, at least in RTIC contexts.

Yes, it's definitely common, but I think that's mostly cargo cult. At least I haven't heard anyone argue that it should be that way. And there's definitely been a broad movement away from that approach in various HALs.

Also fair! I'd prefer to do it without macros for the record. I took a brief look at doing it without them and gave up pretty quickly. I'll have another go and open a new PR if I get a clean solution together.

Yeah, we usually need macros to abstract over the different peripheral instances. Things get messy quick without that. But usually it's possible to minimize the code within the macro using the trait Instance approach. Sometimes it's not easy, unfortunately, if there are many subtle differences between peripheral instances.

But as I said in the review, even though a follow-up PR is definitely welcome, I'm fine with this as-is.