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

Differential ADC #233

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

Differential ADC #233

wants to merge 17 commits into from

Conversation

Yatekii
Copy link
Contributor

@Yatekii Yatekii commented Sep 27, 2020

This is just a preview on what the API could be like.
I use this successfully on my hardware I and I tested all of it (including the conversion methods) on a nRF52811 (Which is a mix of the 52810 and 52840 feature wise :)). Surely there will be corner cases as usual :/

IF you folks like it, I will try and clean it up a little.

Also, it would be cool if anyone has an idea how to integrate the OneShot trait.
Currently the trait does only allow for a single input pin and you have to pass it everytime you start a read transaction, even tho that is not required.
This is why I have made a custom channel config API so you can configure the channel once and then just read.
I will extend this further such that the ADC multiplexer & sequencer can be used properly, even with multiple channels at once :)

Excited to hear your feedback :)

Base automatically changed from 52811 to master September 28, 2020 16:44
@jonas-schievink
Copy link
Contributor

Looks like this needs a rebase

@Yatekii
Copy link
Contributor Author

Yatekii commented Feb 25, 2021

Rebasing gave me some trouble. I hope merging is okay (I don't know what policy we have). If not, we can squash maybe.

I was using this code since Sept/Oct for inductive battery charging and nothing exploded so far :P

Sure it could use some extensions but I think the additional functionality warrants a merge :)

@jonas-schievink jonas-schievink added the breaking change Requires a major semver bump label Mar 2, 2021
Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

Thanks! Finally got around to reviewing this.

Saadc { saadc }
}

pub fn channel(&mut self, n: usize) -> Channel {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub fn channel(&mut self, n: usize) -> Channel {
pub fn channel(&mut self, n: usize) -> Channel<'_> {
assert!(n < 8);

}

/// Sample channel `PIN` for the configured ADC acquisition time in differential input mode.
/// Note that this is a blocking operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

It should probably not return nb::Result if it ends up blocking regardless

Comment on lines +178 to +181
Variant::Val(Resolution::_8BIT) => 256,
Variant::Val(Resolution::_10BIT) => 1024,
Variant::Val(Resolution::_12BIT) => 4096,
Variant::Val(Resolution::_14BIT) => 16384,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Variant::Val(Resolution::_8BIT) => 256,
Variant::Val(Resolution::_10BIT) => 1024,
Variant::Val(Resolution::_12BIT) => 4096,
Variant::Val(Resolution::_14BIT) => 16384,
Variant::Val(Resolution::_8BIT) => 1 << 8,
Variant::Val(Resolution::_10BIT) => 1 << 10,
Variant::Val(Resolution::_12BIT) => 1 << 12,
Variant::Val(Resolution::_14BIT) => 1 << 14,

Ok(val)
}

pub fn as_micros(&self, value: i16) -> i32 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we use raw integers everywhere (and not something like uom), let's clarify that this returns microvolts and has nothing to do with Duration::as_micros

Suggested change
pub fn as_micros(&self, value: i16) -> i32 {
/// Converts a raw reading from this channel to µV.
pub fn to_microvolts(&self, value: i16) -> i32 {

(value as i64 * 1000 * 1000 / lsbs * gain.1 * reference.0 / gain.0 / reference.1) as i32
}

pub fn as_millis(&self, value: i16) -> i32 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub fn as_millis(&self, value: i16) -> i32 {
pub fn to_millivolts(&self, value: i16) -> i32 {

}

impl<'a> Channel<'a> {
pub fn configure(
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this have to be called for every channel that is used? What's the default state that would be used by the existing impls if this isn't called?

macro_rules! psel_mappings {
( $($psel:ident => $pin:ident,)*) => {
$(
impl<STATE> Into<Pseln> for crate::gpio::p0::$pin<STATE> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it okay to use pins in any state here, even Disconnected?

@jonas-schievink jonas-schievink removed the breaking change Requires a major semver bump label May 3, 2022
@eflukx
Copy link

eflukx commented Jun 22, 2022

Would be a very welcome addition.. Is this change still alive?

@Yatekii
Copy link
Contributor Author

Yatekii commented Jun 23, 2022

I kind of forgot about this. Mostly because I do not work on this paid anymore and my sparetime is already full with other stuff. If you want to you are more than welcome to take this over :)

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

4 participants