Skip to content
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

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

stabler
Copy link
Contributor

@stabler stabler commented Jan 4, 2024

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
Copy link
Contributor Author

stabler commented Jan 4, 2024

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

src/opamp.rs Outdated Show resolved Hide resolved
src/opamp.rs Outdated
.vm_sel()
.output()
.opaintoen()
.variant(OPAINTOEN_A::OutputPin)
Copy link
Contributor

@usbalbin usbalbin Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it would be worth it, but what if we had a trait with an associated const OPAINTOEN_A that was implemented for external and InternalOutput types? Then would it be possible to unify this and fn follower below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at the other change I made on 13d4631 and let me know what you think. I'll play with the trait idea.

Copy link
Contributor

@usbalbin usbalbin Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking that one step further, what if all follower, pga, etc defaulted to internal only? Then the user could enable the output builder style. That would probably compose well with optional inverting inputs etc.

let opamp2 = opamp2.pga(gpioa.pa7) // <-- Noninverting
    .inverting_input(..)
    .output(..);

Copy link
Contributor Author

@stabler stabler Jan 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that but I think complexity will increase a lot (e.g. we would need to store various configurations of outputs attached to the struct). I don't think that makes sense given the limited surface area of the API. I think that is probably the 'correct' way to do it though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if we had a trait with an associated const OPAINTOEN_A that was implemented for external and InternalOutput types

I spent some time trying to get this working- I can't find a way to do it that preserves the compile-time checking of InternalOutput for the ADC.

I tried using an associated type in the trait implementation (e.g. type output = InternalOutput;, type output = PA<>), but that doesn't work because the Rust compiler assumes the trait could be implemented in the future, resulting in a duplicate function definition error (long story short).

@usbalbin
Copy link
Contributor

usbalbin commented Jan 4, 2024

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
Copy link
Contributor Author

stabler commented Jan 4, 2024

For those interested, here's a branch where I'm working on adding the remaining Pga modes:
stabler@861a3e7

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 stabler changed the title More ergonomic opamp API, add lock functionality More ergonomic opamp API, add lock and pga functionality Jan 5, 2024
@stabler
Copy link
Contributor Author

stabler commented Jan 5, 2024

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
Copy link
Contributor Author

stabler commented Jan 5, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants