Adc #70

usbalbin · 2025-08-27T17:09:07Z

Fixes #35

usbalbin · 2025-08-28T11:49:22Z

.github/workflows/ci.yml

+        adc:
+          - adc
+          -


Not sure how we want to test this... This duplicates the number of tests to run

Does the ADC need to be feature gated?

I think that depends on how we want to do with the extra dependencies.

usbalbin · 2025-08-28T11:49:55Z

Cargo.toml

@@ -67,17 +67,22 @@ defmt = [
    "stm32h5/defmt",
 ]

+adc = ["dep:embedded-hal-02", "dep:nb"]


We need embedded_hal_02 for the adc::Channel trait. However we could probably avoid nb if we provide our own blocking Adc::convert method for those that do not need to be generic using the embedded_hal_02::adc::Oneshot trait.

What's the advantage of using that trait if embedded-hal has dropped it in 1.0?

usbalbin · 2025-08-28T12:14:37Z

examples/adc.rs

+    let mut adc1 = adc::Adc::new(
+        dp.ADC1,
+        4.MHz(),
+        &mut delay,
+        ccdr.peripheral.ADC,
+        &ccdr.clocks,
+        &pwrcfg,
+    );


I am not too happy with the api for setting up the adc. I think I would prefer something more like the G4 hal

let adc12_common = dp.ADC12_COMMON.claim(Default::default(), &mut rcc); let mut adc = adc12_common.claim_and_configure( dp.ADC2, stm32g4xx_hal::adc::config::AdcConfig::default(), &mut delay, );

Here settings are divided into the individual config and the common config. Config structs are used with sensible defaults for those that do not care instead of individual arguments for frequency etc/or setter methods that have to be used after the fact

We can't use extension traits like we use for other peripherals?

Probably, I just have not really changed the api from H7 yet. But yes that would probably be more consistent with the rest of the H5 hal.

The H503 only has one adc so it does not have the ADCC common register block as a separate block so that would probably not work with the above from G4

usbalbin · 2025-09-07T16:27:13Z

This still needs some testing, me or @Wratox will hopefully get to that in the coming days.

In the meantime what do you think of this @astapleton?

There are still a lot that can be done to get this to work with DMA. I have taken configure_channel from the g4 hal to prepare for that but I think the rest of the DMA stuff can wait for another PR.

usbalbin · 2025-09-07T17:13:44Z

src/adc/mod.rs

+    pub fn claim_and_configure<ADC: Instance>(
+        &self,
+        adc: ADC,
+        delay: &mut impl DelayNs,
+        resolution: Resolution,
+    ) -> Adc<ADC, Disabled> {
+        Self::setup_adc(adc, delay, resolution)
+    }
+
+    /// Initialise ADC
+    ///
+    /// Sets all configurable parameters to one-shot defaults,
+    /// performs a boot-time calibration.
+    #[cfg(feature = "rm0492")]
+    pub fn claim_and_configure(
+        self,
+        delay: &mut impl DelayNs,
+        resolution: Resolution,
+    ) -> Adc<ADC1, Disabled> {


To make it as consistent as possible the user has to create an AdcCommon for the H503 just like the other devices, even though it does not have a separate adc common ADCC block(it is part of ADC1 which is the one and only ADC for h503).

Because of this, claim_and_configure looks a bit different here. For H503 the ADC1(acting as the ADCC) was consumed to create the AdcCommon so the user can not pass that as the adc again as for the other devices. Instead self is consumed as opposed to the other devices which just borrows it, to prevent the user from creating multiple instances.

Do you have any other suggestions?

I'm thinking we can do something similar to how the GPIO driver does it:

define an associated type on the ADCC extension trait defines types that must be passed in to the claim method. For H503 it's a (), but it would be (ADC1, ADC2) for the other processors

define an associated type that indicates a struct that owns all ADC peripherals and is returned by the claim method

define an extension trait for the ADC peripherals themselves that enables them.

Something like:

pub trait AdccExt { // AdcCommonExt renamed to reflect similar naming conventions for other peripherals type Parts; type Peripherals; fn claim( self, peripherals: Self::Peripherals, f_adc: Hertz, prec: rec::Adc, clocks: &CoreClocks, pwrcfg: &pwr::PowerConfiguration, ) -> Self::Parts; } trait AdcExt<ADC: Instance>: Sized { fn enable(self, config: AdcConfig) -> Adc<ADC>; } impl<ADC: Instance> AdcExt<ADC> for ADC { fn enable(self, config: AdcConfig) -> Adc<ADC> { // This is essentially the AdcCommon::setup_adc method without being bound to the AdcCommon struct setup_adc(self, config) } } #[cfg(feature = "rm0492")] mod rm0492 { use crate::stm32::ADC1; use super::*; pub struct AdcPeriphs { adc1: ADC1, } impl AdccExt for ADC1 { type Parts = AdcPeriphs; type Peripherals = (); fn claim( self, peripherals: (), f_adc: Hertz, prec: rec::Adc, clocks: &CoreClocks, pwrcfg: &pwr::PowerConfiguration, ) -> Self::Parts { // Check adc_ker_ck_input kernel_clk_unwrap(&prec, clocks); // Enable AHB clock let prec = prec.enable().reset(); // This is AdcCommon::configure_clock without being bound to the AdcCommon struct let _ = configure_clock(&self, f_adc, prec, clocks, pwrcfg); // ADC12_COMMON AdcPeriphs { adc1: self } } } } #[cfg(feature = "rm0492")] pub use rm0492::* #[cfg(feature = "rm0481")] mod rm0481 { use crate::stm32::{ADC1, ADC2, ADCC}; use super::*; pub struct AdcPeriphs { adc1: ADC1, adc2: ADC2, } impl AdccExt for ADCC { type Parts = AdcPeriphs; type Peripherals = (ADC1, ADC2); fn claim( self, (adc1, adc2): (ADC1, ADC2), f_adc: Hertz, prec: rec::Adc, clocks: &CoreClocks, pwrcfg: &pwr::PowerConfiguration, ) -> Self::Parts { // Check adc_ker_ck_input kernel_clk_unwrap(&prec, clocks); let prec = prec.enable().reset(); let _ = configure_clock(&self, f_adc, prec, clocks, pwrcfg); // ADC12_COMMON AdcPeriphs { adc1, adc2 } } } } #[cfg(feature = "rm0481")] pub use rm0481::*

It would then be used as follows:

On H503:

let adc_periphs = dp.ADC1.claim((), 4.MHz(), ccdr.peripheral.ADC, &ccdr.clocks, &pwrcfg); let adc1 = adc_periphs.adc1.enable(config);

On the other parts:

let adc_periphs = dp.ADCC.claim((dp.ADC1, dp.ADC2), 4.MHz(), ccdr.peripheral.ADC, &ccdr.clocks, &pwrcfg); let adc1 = adc_periphs.adc1.enable(config); let adc2 = adc_periphs.adc2.enable(config);

The nice thing about this is that only the claim method changes between processor families for end users, all the differences between the families are encapsulated in these trait implementations (which in turn can be encapsulated in the h5 module), and the extension method for the ADC peripherals is a bit more ergonomic.

As I see it, we need a couple of things

A way to ensure that there are no races configuring Temperature, Vrefint in the common block

Ensure the clocks are configured before allowing the user to try to setup or use the ADCs

astapleton

Thanks for the PR! Thanks for taking the time to demacrofy the H7 version of this driver.

I have some concerns about this implementation so far and I think there are some improvements that can be made to the structure of the driver:

There are a lot of calls to ADCC::steal(), ADC1::steal() etc. This gives me concerns about the soundness of the implementation, and I think it would be best to minimize these if possible. I made some suggestions in the comments; hopefully they are helpful.
I'm also concerned about the soundness of starting a conversion and then allowing the GPIOs to be reconfigured after the conversion has been started (see configure_channel). I think taking ownership of the channel would be an improvement.
I find it hard to reason about code when there are a lot of instructions conditionally compiled, and much prefer to move those processor/feature-dependent differences into submodules, and/or abstract over them with traits. I'd recommend doing so with the conditional compilation directives in the top-level driver file.
I think the API can be made more ergonomic, similar to the GPIO/GPDMA APIs. See the comments about that.
Can we avoid the typestate type parameter and only return an enabled Adc value when initializing? Does it make sense in terms of how the ADC is generally used to take control of a Adc<ADC1, Disabled> or Adc<ADC1, PoweredDown>?

astapleton · 2025-09-08T17:56:15Z

src/adc/mod.rs

Let's keep the module naming scheme consistent: ie: put this in the src/ root and name it adc.rs

astapleton · 2025-09-09T20:16:58Z

src/adc/mod.rs

+
+/// The place in the sequence a given channel should be captured
+#[derive(Debug, PartialEq, PartialOrd, Copy, Clone)]
+pub enum Sequence {


This enum doesn't seem all that useful? Why not just use a usize and check the bounds of the parameter when it's used to configure the channel?

astapleton · 2025-09-09T20:19:21Z

src/adc/mod.rs

+    sample_time: AdcSampleTime,
+    resolution: Resolution,
+    current_channel: Option<u8>,


It doesn't look like these values (sample_time, resolution, and current_channel) need to be stored in the Adc type. In fact, they prevent the struct from being zero-sized for little or no benefit. Let's rather remove them and instead pass in a configuration struct when one is needed.

astapleton · 2025-09-09T20:26:56Z

src/adc/mod.rs

+impl ED for Enabled {}
+impl ED for Disabled {}
+
+pub struct Adc<ADC, ED> {


Can we eliminate the typestate ED here? It's quite an annoying (IMO) pattern to use for something that needs to be held for the lifetime of a program. By the looks of things it's mostly used before the Adc is enabled. Can we instead not either:
a) only return an Adc that is enabled, and disable it when dropping the Adc struct, either through a method that consumes self or a drop implementation, or
b) bound interactions such that the Adc is only enabled in a blocking call, or such that we return some sort of state object that keeps it enabled while it is held or active (ie. until it is passed back to the Adc instance)

astapleton · 2025-09-09T20:31:42Z

src/adc/mod.rs

+    }
+
+    /// No clock checks
+    fn configure_clock_unchecked(


this doesn't need to be a struct method

astapleton · 2025-09-09T23:58:43Z

src/adc/h5.rs

+}
+
+macro_rules! adc_internal {
+    ([$INT_ADC:ident, $INT_ADC_COMMON:ident]; $($input:ty => ($chan:expr, $en:ident)),+ $(,)*) => {


$INT_ADC_COMMON is not used?

astapleton · 2025-09-09T23:59:46Z

src/adc/h5.rs

+    ($ADC:ident, $($input:ty => $chan:expr),+ $(,)*) => {
+        $(
+            #[cfg(feature = "eh-02")]
+            impl embedded_hal_02::adc::Channel<$ADC> for $input {


instead of implementing this for every input, how about providing a blanket implementation for every T that implements AdcChannel.

I tried that, but since (as I understand it) we dont own the trait and can therefore not implement it for all T's where T: AdcChannel<some adc>

Ah right, yeah :/

astapleton · 2025-09-10T00:05:29Z

src/adc/h5.rs

+    pub fn enable(_adc: &mut super::Adc<ADC2, super::Disabled>) {
+        let adc2 = unsafe { ADC2::steal() };
+
+        adc2.or().modify(|_, w| w.op0().bit(true));
+    }
+
+    pub fn disable(_adc: &mut super::Adc<ADC2, super::Disabled>) {
+        let adc2 = unsafe { ADC2::steal() };
+
+        adc2.or().modify(|_, w| w.op0().bit(false));
+    }


Some of these enable/disable methods take in an Adc struct, and some take in the AdcCommon struct, but the parameters are not used. I think we should try to eliminate this inconsistency, and make use of the parameter to avoid "stealing" the peripheral. Perhaps a trait that provides a method to enable a particular channel, or provides a reference to the register block struct would both use the parameter, and avoid "stealing" the peripheral.

astapleton · 2025-09-10T00:27:45Z

src/adc/mod.rs

+        self.rb
+            .cr()
+            .modify(|_, w| w.deeppwd().clear_bit().advregen().set_bit());
+        delay.delay_us(10);


It's kind of annoying to have to create and pass in a Delay struct here for a one off delay like this (it can't be used/owned elsewhere). Can we not calculate the amount of cycles to wait from the HCLK and use cortex_m::asm::delay in here to perform the delay?

[...] it can't be used/owned elsewhere [...]

Why not? It is only passed in by mut ref

I think it might enforce ordering constraints on system initialization that might be inconvenient (need to initialize the ADC before another system component that wants to take ownership of the Delay implementation), and if we could do the delay transparently to the consumer (even if we just chose a static amount of cycles that is longer with a slower clock speed) that would make for a better API.

Fair enough :)

astapleton · 2025-09-10T01:36:45Z

src/adc/mod.rs

+    pub fn claim_and_configure<ADC: Instance>(
+        &self,
+        adc: ADC,
+        delay: &mut impl DelayNs,
+        resolution: Resolution,
+    ) -> Adc<ADC, Disabled> {
+        Self::setup_adc(adc, delay, resolution)
+    }
+
+    /// Initialise ADC
+    ///
+    /// Sets all configurable parameters to one-shot defaults,
+    /// performs a boot-time calibration.
+    #[cfg(feature = "rm0492")]
+    pub fn claim_and_configure(
+        self,
+        delay: &mut impl DelayNs,
+        resolution: Resolution,
+    ) -> Adc<ADC1, Disabled> {


I'm thinking we can do something similar to how the GPIO driver does it:

define an associated type on the ADCC extension trait defines types that must be passed in to the claim method. For H503 it's a (), but it would be (ADC1, ADC2) for the other processors

define an associated type that indicates a struct that owns all ADC peripherals and is returned by the claim method

define an extension trait for the ADC peripherals themselves that enables them.

Something like:

pub trait AdccExt { // AdcCommonExt renamed to reflect similar naming conventions for other peripherals type Parts; type Peripherals; fn claim( self, peripherals: Self::Peripherals, f_adc: Hertz, prec: rec::Adc, clocks: &CoreClocks, pwrcfg: &pwr::PowerConfiguration, ) -> Self::Parts; } trait AdcExt<ADC: Instance>: Sized { fn enable(self, config: AdcConfig) -> Adc<ADC>; } impl<ADC: Instance> AdcExt<ADC> for ADC { fn enable(self, config: AdcConfig) -> Adc<ADC> { // This is essentially the AdcCommon::setup_adc method without being bound to the AdcCommon struct setup_adc(self, config) } } #[cfg(feature = "rm0492")] mod rm0492 { use crate::stm32::ADC1; use super::*; pub struct AdcPeriphs { adc1: ADC1, } impl AdccExt for ADC1 { type Parts = AdcPeriphs; type Peripherals = (); fn claim( self, peripherals: (), f_adc: Hertz, prec: rec::Adc, clocks: &CoreClocks, pwrcfg: &pwr::PowerConfiguration, ) -> Self::Parts { // Check adc_ker_ck_input kernel_clk_unwrap(&prec, clocks); // Enable AHB clock let prec = prec.enable().reset(); // This is AdcCommon::configure_clock without being bound to the AdcCommon struct let _ = configure_clock(&self, f_adc, prec, clocks, pwrcfg); // ADC12_COMMON AdcPeriphs { adc1: self } } } } #[cfg(feature = "rm0492")] pub use rm0492::* #[cfg(feature = "rm0481")] mod rm0481 { use crate::stm32::{ADC1, ADC2, ADCC}; use super::*; pub struct AdcPeriphs { adc1: ADC1, adc2: ADC2, } impl AdccExt for ADCC { type Parts = AdcPeriphs; type Peripherals = (ADC1, ADC2); fn claim( self, (adc1, adc2): (ADC1, ADC2), f_adc: Hertz, prec: rec::Adc, clocks: &CoreClocks, pwrcfg: &pwr::PowerConfiguration, ) -> Self::Parts { // Check adc_ker_ck_input kernel_clk_unwrap(&prec, clocks); let prec = prec.enable().reset(); let _ = configure_clock(&self, f_adc, prec, clocks, pwrcfg); // ADC12_COMMON AdcPeriphs { adc1, adc2 } } } } #[cfg(feature = "rm0481")] pub use rm0481::*

It would then be used as follows:

On H503:

let adc_periphs = dp.ADC1.claim((), 4.MHz(), ccdr.peripheral.ADC, &ccdr.clocks, &pwrcfg); let adc1 = adc_periphs.adc1.enable(config);

On the other parts:

let adc_periphs = dp.ADCC.claim((dp.ADC1, dp.ADC2), 4.MHz(), ccdr.peripheral.ADC, &ccdr.clocks, &pwrcfg); let adc1 = adc_periphs.adc1.enable(config); let adc2 = adc_periphs.adc2.enable(config);

The nice thing about this is that only the claim method changes between processor families for end users, all the differences between the families are encapsulated in these trait implementations (which in turn can be encapsulated in the h5 module), and the extension method for the ADC peripherals is a bit more ergonomic.

usbalbin · 2025-09-10T18:45:36Z

Thank you for the comments :)

usbalbin added 2 commits August 27, 2025 14:55

Add src/adc.rs from H7 hal

cf84bb2

Compiles...

6a45311

usbalbin force-pushed the adc branch from 3d22d9d to 03c5ec8 Compare August 28, 2025 11:07

usbalbin mentioned this pull request Aug 5, 2025

Peripheral overview #49

Open

usbalbin commented Aug 28, 2025

View reviewed changes

usbalbin added 4 commits August 28, 2025 13:55

Clippy and some cleanup

066e18f

Ccdr does not need to consume PowerConfiguration

c2f8714

Add adc examples

604174b

Make adc optional

be9bf31

usbalbin force-pushed the adc branch from 03c5ec8 to be9bf31 Compare August 28, 2025 11:56

usbalbin commented Aug 28, 2025

View reviewed changes

usbalbin added 6 commits August 29, 2025 09:02

H503

670a94d

Require mut ref to adc

958b761

Update ADC API

d4f1397

Cleanup and put eh-02 behind feature

92e78e4

Update CI

806715f

ADC - Add pins for 56x and 573

e202591

usbalbin marked this pull request as ready for review September 7, 2025 16:21

usbalbin commented Sep 7, 2025

View reviewed changes

astapleton requested changes Sep 10, 2025

View reviewed changes

Adc #70

Are you sure you want to change the base?

Adc #70

Uh oh!

Conversation

usbalbin commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usbalbin Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usbalbin commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astapleton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astapleton Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usbalbin commented Sep 10, 2025

Uh oh!

Uh oh!

usbalbin commented Aug 27, 2025 •

edited

Loading

usbalbin Aug 29, 2025 •

edited

Loading

usbalbin commented Sep 7, 2025 •

edited

Loading

astapleton Sep 9, 2025 •

edited

Loading