adc: add support for channels 16 and 17 in ADC1 #67
Conversation
|
Thanks for the PR! The code and docs look very nice, though the docs mention that "The recommended sampling time for the temperature sensor is 17.1 μs" and as far as I can tell, that isn't done here. With that addressed, i'd say this can be merged |
|
Hi @TheZoq2 Good point. However current ADC implementation in this crate has no knowledge about its clock settings. So one possible option would be to ask user to set proper sampling time from the application, where ADC clock settings are known. On the other hand I agree that it is not convenient and it make a lot of sense to configure and then restore sampling time in read_temp function. I tried to use maximum sampling time (239.5 cycles) as a conservative setting that could fit all the ADC frequencies. But it didn't work for some reason. So I will take a further look and push more commits as soon as I figure out the solution. Regards, |
|
@geomatsi The functions can take the current clocks a parameter. |
|
@therealprof Sure, but I would rather suggest to add current clock parameter into ADC constructor. IIUC we should instantiate ADC after RCC clock configuration is frozen, so it is possible to pass current clock value only once. |
Add methods to read from ADC1 special channels: - channel 16: temperature sensor - channel 17: internal reference voltage Note that these methods should be implemented only for ADC1 block since in ADC2 and ADC3 those channels are connected to Vss. Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
Recommended ADC sampling time for temperature sensor is 17.1 usec. So use approximate sampling time settings to support the whole range of possible ADC frequencies. Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
Update existing ADC example according to ADC API changes. Add new ADC example to read ambient temperature using ADC1 CH16. Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
|
Hi @TheZoq2 , @therealprof , and all, Regards, |
|
Looks good to me, but I think I prefer having adc not take clocks as that would make this a non-breaking change which would be nice. Or do we anticipate adc requiring clock data in more situations? |
|
Taking clocks looks fine for me. |
|
In that case, i'm fine with merging this, unless you have any other concerns @therealprof |
examples/adc_temperature.rs
Outdated
| } | ||
|
|
||
| #[exception] | ||
| fn HardFault(ef: &ExceptionFrame) -> ! { |
There was a problem hiding this comment.
Most examples use panic_halt rather than semihosting, which removes the need for defining these exceptions. Is there a specific need for them here?
There was a problem hiding this comment.
No. I will remove them.
examples/adc_temperature.rs
Outdated
| //let clocks = rcc.cfgr.use_hse(8.mhz()).sysclk(36.mhz()).pclk1(12.mhz()).adcclk(6.mhz()).freeze(&mut flash.acr); | ||
| //let clocks = rcc.cfgr.use_hse(8.mhz()).sysclk(32.mhz()).pclk1(16.mhz()).adcclk(8.mhz()).freeze(&mut flash.acr); | ||
| //let clocks = rcc.cfgr.use_hse(8.mhz()).sysclk(40.mhz()).pclk1(20.mhz()).adcclk(10.mhz()).freeze(&mut flash.acr); | ||
| //let clocks = rcc.cfgr.use_hse(8.mhz()).sysclk(48.mhz()).pclk1(24.mhz()).adcclk(12.mhz()).freeze(&mut flash.acr); |
There was a problem hiding this comment.
Well, these are just several examples of clock settings that can be done with external oscillator. My main purpose was to demonstrate more or less the whole range of ADC frequencies. Besides, I think the example for use_hse is helpful regardless of adc usage.
There was a problem hiding this comment.
I removed those extra examples in comments. One use_hse example should be enough.
There was a problem hiding this comment.
I think it may be useful to document the relationships between the possible frequencies for the different registers in case there are some restrictions to the accepted values. This would clarify what you have to do if you want to set the ADC clock to 12 MHz, for example.
Going a bit further, I could also think it would be nice to document these value relationships in general here somewhere as a shortcut to some paragraph about prescaler config in the datasheet.
There was a problem hiding this comment.
I agree, but i'm not sure if this example is the right way to do this. I'd say it should either be in the documentation or a separate example.
Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
|
Looks good to merge, thanks for yet another great PR :) |
Add reading methods to read from ADC1 special channels:
Note that these methods should be implemented only for ADC1 block
since in ADC2 and ADC3 those channels are connected to Vss.
Signed-off-by: Sergey Matyukevich geomatsi@gmail.com