ADC configuration register constraints

[stabler]

Hi,

I've been running into some issues configuring the ADC in a non-standard configuration (I'm using injected channels, I realize that isn't currently supported by the library, but I'm trying to use the library as much as possible and I hope to contribute a PR in the near future).

I find that in certain cases the ADC will hang in the calibration step. I believe I've tracked it down to section 21.4.10 of the reference manual (screenshot below), which states that writes to CFGR, SMPRx, TRy, SQRy, JDRy, OFRy, OFCHRy and IER registers are only allowed when the ADC is enabled but not started (and there's no hardware protection against this, and it can result in undefined ADC behavior).

In other words, most of the configuration accessors implemented on impl Adc<stm32::$adc_type, Disabled> should instead be implemented on impl Adc<stm32::$adc_type, Configured> (set_resolution, set_align, set_external_trigger, set_continuous, etc.).

In my case I see different behavior on 0.0.1 compared to head of main, with main being more sensitive to the register access sequencing.

Does this sound plausible? I was surprised by this section of the reference manual, but it seems to fix the issue in my case.

[usbalbin]

This is probably what I stumbled (unknowingly) upon in in #78 . Great to hopefully have found the actual cause, my atempted fix perhaps only fixed the issue in my use case and made others worse.

[stabler]

@usbalbin I think you may be right. I had hoped this would fix the weird die temperature sensor readings as well, but it doesn't seem to (although it does look like the measured values change a bit).

Would it be helpful if I go through the fields and put up a PR?

[stabler]

Hmm. I'm reading through the code in the ST standard peripheral library, and the implementation there doesn't seem to match the reference manual either: https://github.com/STMicroelectronics/stm32g4xx_hal_driver/blob/master/Src/stm32g4xx_hal_adc.c#L2794
https://github.com/STMicroelectronics/stm32g4xx_hal_driver/blob/master/Src/stm32g4xx_hal_adc_ex.c#L1873-L1878

Maybe this is a red herring, I'll keep looking

[no111u3]

@stabler ST cube hal driver partial wrong about some flags in ADC, also generated code work sometimes incorrectly.
A little example:
that code demonstrate how to work with ADC with timer rising event, this work incorrectly (interrupts generated more than one time per sampling): https://github.com/no111u3/stm32cubeideworkspace/tree/main/tim_adc
also examples in this hal more incorrect: wrong interrupt flags and wrong setup.
but I try to write from scratch using only register library: https://github.com/no111u3/g474re_nucleo_ral/blob/main/src/main.rs — this works perfect as expected in manual

Be careful than you use some reference of code, it might be wrong anyway

[usbalbin]

Judging by example snippets in src/adc.rs that file might have been taken from another mcu and reworked for g4xx, could there been some incorrect assumptions made there?

[stabler]

@no111u3 yes, wouldn't be surprised if the std periph library has bugs (and I agree that it looks like most of it has been copied from other chips). But it's the most commonly used library, and I searched community.st.com for any similar symptoms (ADC stuck in calibrate) and didn't find much, which leads me to think that the std library is OK and the reference manual may be misleading.

Thanks for linking your code. What are you getting for the temperature reading? I just ran it on my hardware and got ~920 counts, which I think is incorrect (negative temperature). But I'm not sure if that's an ADC misconfiguration, or something else. Also I think the sample time for temperature and vref measurements are too low (datasheet specifies min 4us for voltage reference and 5us for the temperature sensor, if I'm reading your code correctly I think you're at 1/8e646.5=3.25us for both).

Note that the sequencing of configure/enable/start in your code also matches stm32g4xx-hal and the std periph library, but seems inconsistent with the reference manual (RM says write configuration of conversions only if ADEN = 1 && ADSTART = 0). So all three software libraries are doing the same thing here. I'm wondering if the RM actually means to say (ADEN=1 && ADSTART = 0) OR (ADEN = 0).

I do see that you have injected channels working without the stuck-in-calibrate issue, which is interesting, so I'm going to take a look and see if I can find any differences between your code and stm32g4xx-hal sequence.

@usbalbin could be, I'm going to try to build/run the STCube examples in the next few days and compare register accesses, see if I can narrow anything down.

[stabler]

Alright, ignore everything in this thread related to the temperature sensor. It looks like ~920 counts (mentioned above) is the correct value for my hardware, I wasn't compensating for vdda correctly. I just opened a PR (#111) that improves the temperature sensor calculations, and I'm now getting reasonable values.

[no111u3]

@stabler if you want ready to work example for temperature and other feature please feel free to use another one repos: https://github.com/no111u3/g474re_nucleo_robo_rs
in folder examples you can found any usage cases that I have, also I fixed factory constants (please check your code for scale to real power voltage): https://github.com/no111u3/g474re_nucleo_robo_rs/blob/main/examples/tim_adc_dma_rtic.rs#L144
Don't worry about comment lines - I'm working on fix dma related issues but without dma it might be work correctly.

[usbalbin]

@stabler and @no111u3 , do you consider this resolved?

[stabler]

Yes, thanks all for the help!