Skip to content

Conversation

Geens
Copy link
Contributor

@Geens Geens commented Feb 7, 2021

Allow for configuration of the internal opamp peripheral. Only the disabled, voltage-follower and open-loop modes are implemented. Other states can be added later on.

It's my first time implementing such a module. Feedback and remarks are more than welcome!

@pawelchcki
Copy link
Contributor

hey @Geens, Thanks for implementing it! I haven't started work on Hal part of OpAmp myself, so great timing :) as I'll have use for it in my project.

In the mean time I've added OpAmp enum definitions to stm32-rs crate stm32-rs/stm32-rs#486
I think once it lands you might be able to remove some of the magic values from your PR. You can try building that crate locally and add it to local build in cargo, to play with it before its merged.

I'll appreciate any feedback on the Enum PR too - if you think anything is missing, or should be differently named.

@Geens
Copy link
Contributor Author

Geens commented Feb 12, 2021

Hi @pawelchcki, cool that you're working on this too. I just had a quick look at your commit and I can definitely see the advantage of using the right enum definitions. I also noticed the table you mentioned and realized I did not enable opamp 6 for the devices that have opamps 1, 2, 3, and 6.

I'll try and build with your stm32-rs branch this weekend and update to use the enum defs. Since your branch is not yet part of any release, how do we fix the dependencies for this crate? Do we just leave this pull-request open until the stm32-rs is updated?

@pawelchcki
Copy link
Contributor

@Geens I'm hoping to get this reviewed quite soon, will ping the maintainer tomorrow (don't want to bother them too often).

Once it lands in stm32-rs master it will be available via https://github.com/stm32-rs/stm32-rs-nightlies - that you can use directly in HALs cargo.toml. I think if you like the Enum's it would make sense to review and wait until the stm32 is released, and only then merge this into this crate.

Actually I'll try to push the updated stm32 in my own nightly repository - could be useful for myself too.

@pawelchcki
Copy link
Contributor

Ok seems it worked, feel free to use this repo/branch to work with these changes.

[dependencies.stm32g4]
git = "https://github.com/pawelchcki/stm32-rs-nightlies"
branch = "opamp"
features = ["stm32g431", "rt"]

@Geens
Copy link
Contributor Author

Geens commented Feb 20, 2021

@pawelchcki, The stm32g471 is missing opamp 6 in the PAC. This was mentioned in STM32G4 #486. For me, it's not really necessary so I don't mind commenting it out. But it seems like it's not hard to add it either.

Also there are no features for the stm32g491 and stm32g4a1 in this crate yet. Also not really needed for me but I would think it's better to add them sooner than later.

How would you continue?

@pawelchcki
Copy link
Contributor

@pawelchcki, The stm32g471 is missing opamp 6 in the PAC. This was mentioned in STM32G4 #486. For me, it's not really necessary so I don't mind commenting it out. But it seems like it's not hard to add it either.

I'm not sure stm32g471 actually has the 6th Opamp - the manual says shoud have it, but STM Cube sources only have first 3 opamps defined there. From what I've read, sadly the STM CubeMX generated files are usually most accurate.

Also there are no features for the stm32g491 and stm32g4a1 in this crate yet. Also not really needed for me but I would think it's better to add them sooner than later.

I don't have strong opinion there. We can always add them in a separate PR

@Geens
Copy link
Contributor Author

Geens commented Feb 21, 2021

Thanks for the input! I removed opamp6 from the stm32g471.

@dotcypress I think this pull request is ready for review.

Copy link
Member

@dotcypress dotcypress left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@dotcypress dotcypress merged commit b799d94 into stm32-rs:master Feb 21, 2021
@dotcypress
Copy link
Member

Thank you @Geens and @pawelchcki

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.

3 participants