More ergonomic opamp API, add lock and pga functionality

stabler commented 10 months ago

Here's an attempt to improve the ergonomics of the opamp API. This PR also adds the remaining PGA modes.

Key changes:

Moved all the opamp traits up a level, so it's the same trait for all opamps (rather than a separate trait definition for each channel).
Removed the Option<> from the output interface, and instead implemented traits on the output pin directly, or a new type InternalOutput that represents the case where the opamp is directly connected to the ADC.
Implemented a Locked<_> struct. Opamps can be moved from any of the implemented modes (Pga, OpenLoop, Follower) into the Locked state, at which point the lock bit is set and you cannot modify or disable the opamp (but you can pass it into the ADC, if appropriately configured)
Added remaining Pga functionality.
Consolidated the ADC implementations. As far as I can tell, the macros adc_op_pga and adc_op_follower were intended to work around limitations of the opamp (certain opamps can only operate in certain modes), but I can't see anything about that in the datasheet or reference manual. Maybe this is an artifact from copy-paste from another chip?

Things I think are good:

I think this makes the interface more ergonomic- particularly when switching between modes (no more option passed back that needs to be unwrapped)
Easy to prevent the user from passing the opamp into the ADC when it isn't configured to output to the internal channel
Easy to support the locked state

Things I don't love

~There's an unwrap in some disable() and all disable_output() functions. This is safe because those functions are only implemented on opamps that contain an output pin, but it relies on the module being implemented correctly.~
I think there's probably a way to reduce the code duplication in the macro and better use traits, but I couldn't find a way to do it that made the compiler happy

Let me know what you think! @usbalbin I know you worked on this code recently, so you may have opinions!

stabler commented 10 months ago

Turns out I didn't need the unwrap after all (ff07d75)

usbalbin commented 10 months ago

I'm just a user, but from what I have seen so far this looks great. I really like not having 6 different modules with their own types. I also like not having to use the Option's for the output :)

Awesome :)

stabler commented 10 months ago

For those interested, here's a branch where I'm working on adding the remaining Pga modes: https://github.com/stabler/stm32g4xx-hal/commit/861a3e77aae5c4d2330a31c1ed80200d956e42cd

It's a bit heavy handed right now (separate structs for each Pga variant), so I'm going to see if I can consolidate a bit (and test it!) before I put a PR up

stabler commented 10 months ago

Ok I cleaned up the Pga implementation (a lot!), so I pulled it into this PR. I haven't tested all of this code on hardware yet (hoping to do that this weekend), but I'm feeling pretty good about the Pga implementation.

stabler commented 10 months ago

Nice compile-time error if you use the wrong input pin in the more complex Pga modes:

stm32-rs / stm32g4xx-hal

More ergonomic opamp API, add lock and pga functionality #113