Add support for the ADC peripheral

[TheZoq2]

This adds the ability to run single AD conversions similar to what was suggested in #92.

Here are a few potential issues with this implementation:

RCC and clocks

Right now, the prescaler for the ADC is set to divide the system clock by 6 as I read somewhere that it is required to get accurate readings when the sysclock is running at 72 MHz.

Hardcoding this value is kind of ugly since you could do less prescaling when the sysclock is slower than 72 MHz. Is that a problem for this initial implementation?

Type safety

This implementation makes sure that the ADC can only be used on a pin which is configured as an analog input and also maps pins to their corresponding channel.

However, this means that you can't run the ADC on channels without an associated pin like chanel 16 which is a temperature sensor. Do we want to make Adc::read pub? What happens if you run a conversion on a pin that is not configured as analog input?

Also, is there a nicer way to implement the mapping between pins and channels?

[burrbull]

Are you planning make this compatible with #https://github.com/rust-embedded/embedded-hal/pull/101 ?

[TheZoq2]

Hmm, I didn't know that PR was a thing. I'll have a closer look later but it looks like it could be supported fairly easily

[therealprof]

@TheZoq2 It would be great if you could also address:

Hardcoding this value is kind of ugly since you could do less prescaling when the sysclock is slower than 72 MHz. Is that a problem for this initial implementation?

Not really a problem but why not address this right away?

[TheZoq2]

Not really a problem but why not address this right away?

Mostly because for my application, I don't need maximum speed. Als because I couldn't find the page where I originally got the idea to set it to 6. I can look into doing it properly tonight.

[TheZoq2]

I found the place in the manual where they explain the required timings and updated the branch to reflect that. It should now ensure that the ADC clock runs at 14 MHz or less

[TheZoq2]

Good catch! I updated it with both suggestions

[burrbull]

Have you noticed you have to use bigger divider?

[TheZoq2]

I did, but for some reason I thought changing it to > instead of >= would fix that.

I changed it back to >= since we want to "round up" the division and updated the divisors, does it look correct now?

[burrbull]

72 MHz / 14 MHz = 5.14 > 4 Div = 6 Real speed 72 / 6 = 12 MHz

[TheZoq2]

72 MHz / 14 MHz = 5.14 > 4 Div = 8 Real speed 72 / 8 = 9 MHz

Unless i'm misstaken, the code, as it looks after my latest commit divides by 6 in this case which gives a real speed of 72/6 = 12 MHz which is correct, right?

[burrbull]

Yes.

[burrbull]

I made screen from CubeMX. You should use APB2 speed as base instead of sysclk

[TheZoq2]

Yes, another good catch. Thanks!

I pushed a new commit with a fix

[burrbull]

implementation of em-hal adc for L0x1:

https://github.com/thenewwazoo/stm32l0x1-hal/blob/master/src/adc.rs

[eldruin]

Great work @TheZoq2!. Just to let you know, the ADC traits have been published in embedded-hal@0.2.2.

[tib888]

Hi, please consider to use this ADC clock configuration instead: https://github.com/tib888/stm32f103xx-hal/blob/adc_clock/src/rcc.rs

[tib888]

Also it would be nice to have a nonblocking read implementation, like it is proposed here: https://github.com/rust-embedded/embedded-hal/blob/master/src/adc.rs

[TheZoq2]

I finally got around to rebasing this against the current version. I'll try and find some time to rewrite it to use the embedded-hal implementation soon.

@tib888 I assume you're talking about allowing the user to chose the frequency, if so I'll try and implement that as well

[TheZoq2]

I have now implemented the embedded_hal adc traits to replace the pin handling I did before

[therealprof]

Looking good.

[tib888]

I finally got around to rebasing this against the current version. I'll try and find some time to rewrite it to use the embedded-hal implementation soon.

@tib888 I assume you're talking about allowing the user to chose the frequency, if so I'll try and implement that as well

Not only that, it actually implements the divider calculation (more precise version) and the config register setting too, so you could have been used the whole file as is.

    let adcpre_bits = match pclk2 / (self.adcclk.unwrap_or(14_000_000) + 1) {
            0...1 => 0b00,
            2...3 => 0b01,
            4...5 => 0b10,
            _ => 0b11,
        };

    let adcclk = (pclk2 >> 1) / u32(adcpre_bits + 1);
    assert!(adcclk <= 14_000_000);

   rcc.cfgr.modify(|_, w| unsafe {
        w.ppre2()
            .bits(ppre2_bits)
            .ppre1()
            .bits(ppre1_bits)
            .hpre()
            .bits(hpre_bits)
            .usbpre()
            .variant(usbpre)
            .sw()
            .variant(if pllmul_bits.is_some() {
                SWW::PLL
            } else if self.hse.is_some() {
                SWW::HSE
            } else {
                SWW::HSI
            })
            .adcpre()
            .bits(adcpre_bits)                     //  <= sets the adc pre here
    });

[tib888]

@TheZoq2 have you tested the read_raw implementation? Repeatedly calling it while waiting for the result will run the first two lines of code many times. I'm afraid that it will cause coninous restarts of conversion and never return a result. I hope I'm wrong...

[TheZoq2]

@tib888 I tested it briefly by running the examples/adc.rs and it seems to work. Though it might not be a bad idea to check if a conversion is already happening...

[TheZoq2]

I'm not sure why my test didn't catch this earlier, but @tib888 was right, it does get stuck in an infinite loop. Unfortunately I can't reproduce it in the example but I did experience it in my main project.

I have pushed a fix which works but does have issues when running more than one read at a time:

let (mut chan1, mut chan2) = (None, None);

while chan1 == None && chan2 == None {
    if let Ok(val) = pa1.read() {chan1 = Some(val)};
    if let Ok(val) = pa2.read() {chan2 = Some(val)};
}

In this case, the values of chan1 or chan2 would depend on the timings of the read function and the rest of the system. The best solution I can come up with is to add an Error::AdcBusy which would be returned if you try to start a conversion on another channel while the ADC is already running. Does that sound like a good idea or is there a better option?

[tib888]

@TheZoq2 What about this solution below? It is using type states to block starting a new conversion while an other is running. Also in this case you do not need to check if a conversion is started already or not.

pub struct Adc<ADC> {
    adc: ADC
}

pub struct AdcConversion<ADC, PIN> where
PIN: Channel<$ADC, ID=u8>, 
{
    adc: Adc<ADC>,
    pin: PIN
}

impl<ADC> Adc<ADC> {

    /// powers up the ADC...
    pub fn init(adc: ADC, apb2: &mut APB2, clocks: &Clocks) -> Self {
        ...
    }

    /// takes the ownership of the adc and the given pin for the time of the conversion(s)
    pub fn start_conversion<PIN>(self, pin: PIN) -> AdcConversion<ADC, PIN> where PIN: Channel<$ADC, ID=u8>, 
    {
        unsafe { 
            self.adc.sqr3.modify(|_, w| w.sq1().bits(PIN::channel())) 
        };
        self.adc.cr2.modify(|_, w| { w.adon().set_bit()});

        AdcConversion<ADC, PIN> {
            adc: self,
            pin: pin
        }
    }
}

impl<ADC, PIN> AdcConversion<ADC, PIN> where
PIN: Channel<$ADC, ID=u8>, 
{
    /// tries to read the result of adc conversion
    pub fn read(&mut self, restart_on_success: bool) -> nb::Result<u16, Void> {
        // Check if the data is ready for reading
        if self.adc.sr.read().eoc().bit() == false {
            Err(nb::Error::WouldBlock)
        } else {
            self.adc.sr.modify(|_, w| w.strt().clear_bit());
            let value = self.adc.dr.read().data().bits();
            if restart_on_success {
                unsafe { 
                    self.adc.sqr3.modify(|_, w| w.sq1().bits(PIN::channel())) 
                };
                self.adc.cr2.modify(|_, w| { w.adon().set_bit() });
            }
            Ok(value)
        }
    }

    /// returns the ownership of adc and the pin so new conversions can be started
    pub fn split(self) -> (Adc<ADC>, PIN)
    {
        if self.adc.sr.read().strt().bit_is_set() {
            //todo stop the conversion here??
        }
        (self.adc, self.pin)
    }
}

It could be used like this:

let adc = adc::Adc::adc1(p.ADC1, &mut rcc.apb2, &clocks);
let conversion = adc.start_conversion(pb1);

// adc, pb1 is not accessible anymore...

let reading = block!(conversion.read(false)).unwrap();
let (adc, pb1) = conversion.split();

// adc, pb1 can be used here again...

[TheZoq2]

@tib888 That's defenitively a solution that I would prefer over runtime errors. However, as far as I can tell, it is not compatible with the Adc trait in embedded_hal.

[tib888]

@tib888 That's defenitively a solution that I would prefer over runtime errors. However, as far as I can tell, it is not compatible with the Adc trait in embedded_hal.

@TheZoq2 I agree, so we could propose there an improvement in embedded_hal. However before that it would be nice to try it out in a real environment. Also, lets think how this concept could be extended to achieve multi channel conversion sequences...