Adds DMA support for ADC1 #73

thalesfragoso · 2019-06-13T00:12:21Z

I also had to make some minor changes in the dma and serial files to better accommodate the adc, but they were non breaking, the examples remain the same and compile fine.

I would like @burrbull to take a look to make sure that I didn't make any changes that would have a bad effect on present or future implementations.

I tested the rx transfer on the stm32f103 and it seems that everything is working as expected. I will probably hard test the circular implementation too, but it should be working fine. I also fixed a typo in a comment on the RCC.

burrbull · 2019-06-13T00:48:17Z

It looks good in first view. I'll test it tomorrow .

But can you modify it to support a sequence (array) of ADC channels, not a one?

burrbull · 2019-06-13T03:00:06Z

It looks like you measure voltage each adcclk cycle.
What if I need to use external timer for start measuring?

See ADCx.CR2.EXTSEL + TIMx.CR2.MMS

burrbull · 2019-06-13T05:11:35Z

@TheZoq2 , @therealprof
I have idea. What about to add such interface for peripheral wrappers?:

    pub unsafe fn configure(&mut self, f: fn(&mut PER)) {
        f(&mut self.rb)
    }

Unsafe user fns for advanced manual configure if required functionality is not present in HAL yet?
It can be a solution for difficult situations.

thalesfragoso · 2019-06-13T17:49:45Z

It looks good in first view. I'll test it tomorrow .

But can you modify it to support a sequence (array) of ADC channels, not a one?

Right now, the only way I can think of is exposing a trait and trust the users to implement it, like we do with the pwm, which I think is totally fine, we just need to provide some documentation. However, there are probably others ways to achieve this, but either way I would like it to be like another with_dma method to keep the api simple for simple cases.

It looks like you measure voltage each adcclk cycle.
What if I need to use external timer for start measuring?

See ADCx.CR2.EXTSEL + TIMx.CR2.MMS

Not exactly at each adcclk cycle, it takes more for conversion, but I get what you are saying. Anyway, I would say it would be the same thing as I said above.

@TheZoq2 , @therealprof
I have idea. What about to add such interface for peripheral wrappers?:
    pub unsafe fn configure(&mut self, f: fn(&mut PER)) {
        f(&mut self.rb)
    }
Unsafe user fns for advanced manual configure if required functionality is not present in HAL yet?
It can be a solution for difficult situations.

I think you can already do that with the ::ptr() method, and I also think they will make steal really "steal" things, see rust-embedded/svd2rust#238 and rust-embedded/cortex-m#147.

burrbull · 2019-06-13T18:13:04Z

I think you can already do that with the ::ptr() method, and I also think they will make steal really "steal" things, see rust-embedded/svd2rust#238 and rust-embedded/cortex-m#147.

I know, that I can use ptr (and I often do so). But it creates new instance, that leads to many bugs.

This PR I approve to merge as it is. Maybe later I will try to extend this API.

geomatsi · 2019-06-13T19:38:57Z

examples/adc-dma-rx.rs

+    let (_buf, adc_dma) = adc_dma.read(buf).wait();
+    asm::bkpt();
+
+    let (_adc1, _adc_ch0, _dma_ch1) = adc_dma.split();


Could you please clarify what are these lines for ? Even more, maybe it worth adding short comments for all logical blocks in this example ?

Done, now I also changed some of your adc code, please take a look to make sure I did not do something wrong, thanks.

thalesfragoso · 2019-06-13T20:01:30Z

To clarify my last commit:
Since there was recently a breaking change in the adc api, I took the opportunity to "rebreak it" to enforce some delay time required for startup as mentioned in the reference manual.

geomatsi · 2019-06-13T20:10:20Z

src/adc.rs

+                        if two_adc_cycles > already_delayed {
+                            delay(two_adc_cycles - already_delayed);
+                        }
+                    }


According to datasheet, calibration is recommended after each power-up. If so, then why don't we move all the delays into power_up() function rather than keeping several pieces in different places ?

Well, that is a good point, but since it is just recommended I kinda did not want to enforce it.

I probably didn't make myself clear. I mean, why don't we move that additional delay code into power_up function ?

Because it would enforce that we delayed for calibration every time we power_up even when we don't want to calibrate it. Right now, it does not matter, since the methods are not public and we are powering up only once and we are calibrating. But maybe in some future implementation that could be different.

Anyway, I also think that we should calibrate after each power up, but I did not want to enforce it, otherwise I would just move the calibration code to power up and remove the calibration method.

geomatsi · 2019-06-13T20:10:53Z

src/adc.rs

+                    self.power_down();
+                    self.disable_clock(apb2);
+                    self.rb
+                }


Is there any reason why you don't want to return $ADC from this function ?

Well, no, but it is returning ADC. isn't it ?

Oh, typo. Sorry for confusion. The question is "why do you want to return ADC from this function". I am asking not because I have any objections, I just want to understand the use-case.

Because it is a singleton, I am returning it so people can use it to start the adc again if they want to.

geomatsi · 2019-06-13T20:21:05Z

src/adc.rs

@@ -375,9 +403,7 @@ macro_rules! adc_hal {
                    type Error = ();

                    fn read(&mut self, _pin: &mut PIN) -> nb::Result<WORD, Self::Error> {
-                        self.power_up();


If I understand correctly, we opt to sacrifice power consumption for accuracy here. Right ? Alternatively we could calibrate before each read, but that might be tooooo slow...

That is correct, and if you don't want to calibrate you still would need to wait 1us. Moreover, that was the reason I created the release method, so if you want to save power you could use it only in the time you needed it, in contrast to have to wait 1us+ every conversion.

And as a side note, I think that the first time you would call the old method it would cause some problem, because the adc was already up, so if you set the ADON bit you would start a conversion.

I confirm the observation regarding the first measurement in the previous approach. However with new approach (w/o power_up/power_down around convert) all the values look strange, not only the first one. At least this is the case for temperature sensor. I need to do more testing using voltage inputs...

I did more tests with various ADC clock settings. It turns out that sampling settings for temperature sensor (suggested in my previous PR #67 ) should be modified as follows to work well with introduced delays:

diff --git a/src/adc.rs b/src/adc.rs index b1a73c2..d4752a9 100644 --- a/src/adc.rs +++ b/src/adc.rs @@ -468,7 +468,7 @@ impl Adc<ADC1> { 2_400_001 ... 3_100_000 => AdcSampleTime::T_28, 3_100_001 ... 4_000_000 => AdcSampleTime::T_41, 4_000_001 ... 5_000_000 => AdcSampleTime::T_55, - 5_000_001 ... 10_000_000 => AdcSampleTime::T_71, + 5_000_001 ... 14_000_000 => AdcSampleTime::T_71, _ => AdcSampleTime::T_239, };

Could you please meld this fixup into you PR ?

Done, thank you for helping out :)

geomatsi · 2019-06-14T07:55:46Z

LGTM. Thanks !

burrbull · 2019-06-15T03:04:59Z

src/adc.rs

@@ -429,14 +461,14 @@ impl Adc<ADC1> {
        // recommended ADC sampling for temperature sensor is 17.1 usec,
        // so use the following approximate settings
        // to support all ADC frequencies
-        let sample_time = match self.clk.0 {
+        let sample_time = match self.clocks.adcclk().0 {
            0 ... 1_200_000 => AdcSampleTime::T_1,


... syntax is deprecated in nightly. Use ..= instead

Thanks for catching ! Maybe I can do it it in a separate 'cleanup' PR ? Otherwise we are going to have too much unrelated things in this DMA PR.

therealprof

LGTM, thanks.

thalesfragoso added 2 commits June 12, 2019 20:01

Adds DMA support for ADC

83742a1

Some fixes after rebasing

75bee83

geomatsi reviewed Jun 13, 2019

View reviewed changes

Adds delays for start up times

9f16750

thalesfragoso force-pushed the adcDma branch from 18e8b1c to 9f16750 Compare June 14, 2019 04:42

burrbull reviewed Jun 15, 2019

View reviewed changes

therealprof approved these changes Jun 20, 2019

View reviewed changes

therealprof merged commit 4fdc432 into stm32-rs:master Jun 20, 2019

thalesfragoso deleted the adcDma branch July 16, 2019 00:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds DMA support for ADC1 #73

Adds DMA support for ADC1 #73

thalesfragoso commented Jun 13, 2019

burrbull commented Jun 13, 2019

burrbull commented Jun 13, 2019 •

edited

Loading

burrbull commented Jun 13, 2019

thalesfragoso commented Jun 13, 2019

burrbull commented Jun 13, 2019

geomatsi Jun 13, 2019

thalesfragoso Jun 13, 2019

thalesfragoso commented Jun 13, 2019

geomatsi Jun 13, 2019

thalesfragoso Jun 13, 2019

geomatsi Jun 13, 2019

thalesfragoso Jun 13, 2019

geomatsi Jun 13, 2019

thalesfragoso Jun 13, 2019

geomatsi Jun 13, 2019

thalesfragoso Jun 13, 2019

geomatsi Jun 13, 2019

thalesfragoso Jun 13, 2019 •

edited

Loading

geomatsi Jun 13, 2019

geomatsi Jun 13, 2019

thalesfragoso Jun 14, 2019

geomatsi commented Jun 14, 2019

burrbull Jun 15, 2019

geomatsi Jun 15, 2019

therealprof left a comment

Adds DMA support for ADC1 #73

Adds DMA support for ADC1 #73

Conversation

thalesfragoso commented Jun 13, 2019

burrbull commented Jun 13, 2019

burrbull commented Jun 13, 2019 • edited Loading

burrbull commented Jun 13, 2019

thalesfragoso commented Jun 13, 2019

burrbull commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thalesfragoso commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thalesfragoso Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geomatsi commented Jun 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

therealprof left a comment

Choose a reason for hiding this comment

burrbull commented Jun 13, 2019 •

edited

Loading

thalesfragoso Jun 13, 2019 •

edited

Loading