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

Jump to bottom

Add ADC API #173

Merged
merged 6 commits into from Dec 18, 2020
Merged

Add ADC API #173

merged 6 commits into from Dec 18, 2020

Conversation

hannobraun
Copy link
Collaborator

No description provided.

@hannobraun
Add basic ADC API
712c831
@hannobraun
Add ADC example
8e4b9fb
@hannobraun
Add support for setting ADC resolution
38dc9f4
@hannobraun
Add adc::Channel
c03b4a5 
This makes it possible to add channel-specific code to this trait, which
is required for some features, like setting the sampling time, or
internal channels.
@hannobraun
Add support for setting ADC sample time
1ad84e7
MathiasKoch
MathiasKoch requested changes Dec 10, 2020
View reviewed changes
Copy link
Collaborator

@MathiasKoch MathiasKoch left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

Overall this looks good to me!

I will try to test it in the coming week when i have some hardware with ADC available at hand.

Added a minor suggestion on a dead_code allowance to please the CI

src/rcc.rs Show resolved Hide resolved
@hannobraun @MathiasKoch
Silence "unused" warnings on platforms without ADC
785fadf 
Co-authored-by: Mathias Koch <smilykoch@gmail.com>
@hannobraun
Copy link
Collaborator Author

Thanks for the review! I've applied your suggested change.

@hannobraun hannobraun mentioned this pull request Dec 10, 2020
Merged
@hannobraun
Copy link
Collaborator Author

Anything I can do to help this along?

@MathiasKoch
Copy link
Collaborator

Sorry! I have been a bit busy at work lately, so i have not had a chance to test it on actual hardware.
That said, seeing as it is purely additions and the CI is happy with it, i would have no problem merging this as-is..

How about you @korken89 ?

@korken89
Copy link
Collaborator

korken89 commented Dec 17, 2020 •
edited

Hi, could you add using ADC_Common so one can read out the Vref, Vbat and Temperature channels?
I have a PR merged that adds the ADC common register for all L4s, but for now the l4x5 and l4x6 has the register.

This is the only thing missing from a quick look.

@hannobraun
Copy link
Collaborator Author

Sorry! I have been a bit busy at work lately, so i have not had a chance to test it on actual hardware.
That said, seeing as it is purely additions and the CI is happy with it, i would have no problem merging this as-is..

No need to apologize!

For what it's worth, it works for me, and I agree with your analysis here. This isn't going to break anyone's use case.

Hi, could you add using ADC_Common so one can read out the Vref, Vbat and Temperature channels?
I have a PR merged that adds the ADC common register for all L4s, but for now the l4x5 and l4x6 has the register.

Well, I could write the code, but I would have no way to test it (all I have available is an L433). Would it maybe be better to merge this PR now, then extend the API in a separate one once your PR makes its way downstream? Seems like less effort overall.

@korken89
Copy link
Collaborator

korken89 commented Dec 18, 2020 •
edited

@hannobraun Sure, sounds fair to me.
I noticed one more thing that I fixed upstream.
For l4x1, l4x2, l4x3 the ADC peripheral is named ADC and the rest it's ADC1 (which is correct).
Could you add a cfg and use statements at the top to get it working for all MCUs?
When the fix is merged upstream we can remove it and have ADC1 everywhere.

After this I'd say this is ready for merge!

korken89
korken89 reviewed Dec 18, 2020
View reviewed changes
src/lib.rs
@@ -60,6 +60,8 @@ pub use crate::pac as stm32;

pub mod traits;

#[cfg(feature = "stm32l4x3")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

It also needs updating here to support the rest of the MCUs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added that to reflect what I had tested. I'm sure it compiles and works for other targets too (at least some of them).

@korken89
Copy link
Collaborator

Or, you know what, I can fix all that.
I have it fresh in my mind, I'll fix a separate PR for it with the extended MCU support. :)

@korken89 korken89 merged commit db6cb3f into stm32-rs:master Dec 18, 2020
@hannobraun
Copy link
Collaborator Author

Thanks, @korken89!

@hannobraun hannobraun deleted the adc branch December 18, 2020 10:44
@MathiasKoch MathiasKoch mentioned this pull request Jan 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MathiasKoch MathiasKoch MathiasKoch requested changes

@korken89 korken89 korken89 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hannobraun @MathiasKoch @korken89