New ADC V2 with Async support #814
Conversation
Automatically convert pins to correct mode in Channel::with_pins
`wait_flags` and buffered read methods
Fix overrun errors on thumbv6 targets
|
@rnd-ash, if you're happy with this, I am too. I think you've been doing extensive testing recently. Only two things to get CI green and get this merged:
|
|
I'll be able to fix these next week or this weekend when I'm back in the UK and have access to all my test boards (SAMD21 I need to verify it doesn't read old results in the ADC during sequential reads) |
|
@ianrrees @jbeaurivage As far as I am concerned now, this is ready to Merge. If you guys want to do 1 final review feel free. As discussed with Ian, I fixed the pygamer example my deleting the neopixel examples |
| // The double trigger here is in case the VREF value changed between | ||
| // reads, this discards the conversion made just after the VREF changed, | ||
| // which the data sheet tells us to do in order to not get a faulty reading | ||
| // right after changing VREF value |
There was a problem hiding this comment.
I don't understand this comment - why not have start_conversion() just do one conversion and handle triggering+discarding that faulty reading when VREF is changed?
My concern is that this could be used in settings with tight timing requirements, so probably shouldn't spend time on things that aren't strictly necessary.
There was a problem hiding this comment.
Ill do some testing tomorrow, since this was to prevent stagnant readings from the ADC when cycling between channels very quickly (Read 1 sample, cycle to another channel,...), but since then, I found the powering down the ADC and powering it up between samples helped, so Ill test without this double trigger and see if the problem is still gone
|
|
||
| let adc_settings = Config::new() | ||
| .clock_cycles_per_sample(5) | ||
| // Overruns if clock divider < 128 in debug mode |
There was a problem hiding this comment.
I wonder if we shouldn't re-work the example, so that it's a little more robust - eg maybe just get one sample at a time so that it is impossible to get an overrun scenario. That limitation could motivate using DMA, which I think a lot of practical ADC applications will need to use anyway
There was a problem hiding this comment.
Agreed, DMA is my next thing to work on with the ADC generally speaking
hal/src/peripherals/adc/config.rs
Outdated
|
|
||
| /// This adjusts the number of ADC clock cycles taken to sample a single | ||
| /// sample. The higher this number, the longer it will take the ADC to | ||
| /// sample each sample. |
There was a problem hiding this comment.
What is the maximum value, and assuming it's not 8b what happens if a config is specified that's over it?
hal/src/peripherals/adc/config.rs
Outdated
| /// * Single accumulation (No averaging or summing) | ||
| /// | ||
| /// ## Additional reading settings by default | ||
| /// * Use VDDANA as reference voltage for a full 0.0-3.3V reading |
There was a problem hiding this comment.
It looks like the implementation actually uses the 1V reference
There was a problem hiding this comment.
Intvcc1 - VDDANA reference voltage (Refselselect register), but ill modify the comment to show this is the case
|
@rnd-ash, @ianrrees still had a few comments that are worth being addressed. I've fixed a bunch in rnd-ash#10. The only outstanding item that might require more discussion is the handling of the sampling parameters, ie, if we should force users to explicitly specify everything or not. To simplify things a bit, I suggest that if you want add convenience methods to setup the ADC to a specific number of cycles per sample, it could be done in a follow-up PR. |
|
I've been working on an ESP32 based project for a couple weeks now, and TBF it's reinforced my feelings that for things like UARTs (and I'd say ADC/DAC as well - just haven't used them in ESP32), it's really best to make the API explicit about settings where there is no clear default. However, I'm just one person, and I respect that the ESP32 HAL is quite successful, so maybe it's not worth worrying too much about... @rnd-ash - it would be great to get this merged, what's your feeling about the next steps with it? If changing the config builder is too much of a pain, I'm happy either leaving it as-is or having a crack at it myself if you'd like. |
Another pass at refactoring the ADC module
Doc and formatting fixes
|
@ianrrees i think we are ready to merge this, just 1 minor bug with the SAMD21's temperature sensor not reading accurately, but I can (For now at least), just disable the cpu temperature function for SAMD21 if it proves to be a blocker. I have not tested this on a D11 target either, as I don't have one, would it be possible for you to verify functionality? |
|
Awesome, thanks! Yep, my next few days are going to be a bit variable in terms of computer time, but will try to get to it soon. |
Summary
This PR adds a new ADC API, based around a similar system to channels used by EIC and DMA peripherals.
At the moment I'll keep this as a draft PR for feedback whilst I am still finalizing bits and also adding SAMD11/21 support
Features
Progress checklist