Skip to content

cpu/stm32: add ADCAL operation#20044

Merged
maribu merged 2 commits intoRIOT-OS:masterfrom
bissell-homecare-inc:stm32_adc_cal
Nov 28, 2023
Merged

cpu/stm32: add ADCAL operation#20044
maribu merged 2 commits intoRIOT-OS:masterfrom
bissell-homecare-inc:stm32_adc_cal

Conversation

@david-vankampen
Copy link
Copy Markdown
Contributor

@david-vankampen david-vankampen commented Nov 2, 2023

The STM32G0 has an ADC Calibration procedure ad described in RM0454 Section 14.3.3
image

Contribution description

Add the software calibration procedure on startup as described in STM's documentation.

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Nov 2, 2023
benpicco
benpicco reviewed Nov 3, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two style nits:

  • also prefix the commit message with cpu/stm32 like you did for the PR title
  • use /* this comment style */ instead of // this comment style for consistency.

@david-vankampen david-vankampen force-pushed the stm32_adc_cal branch 2 times, most recently from e478574 to 60fc505 Compare November 6, 2023 12:51
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 6, 2023
@riot-ci
Copy link
Copy Markdown

riot-ci commented Nov 6, 2023 •
edited
Loading

Murdock results

✔️ PASSED

b09713e Merge branch 'RIOT-OS:master' into stm32_adc_cal

Success Failures Total Runtime
7957 0 7957 09m:37s

Artifacts

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Nov 6, 2023

This doesn't build on stm32f0 based boards and the ADC implementation is shared between f0 and g0:

make BOARD=nucleo-f072rb -C tests/periph/adc/
make: Entering directory '/work/riot/RIOT/tests/periph/adc'
Building application "tests_adc" for "nucleo-f072rb" with MCU "stm32".

"make" -C /work/riot/RIOT/pkg/cmsis/ 
"make" -C /work/riot/RIOT/boards/common/init
"make" -C /work/riot/RIOT/boards/nucleo-f072rb
"make" -C /work/riot/RIOT/boards/common/nucleo
"make" -C /work/riot/RIOT/core
"make" -C /work/riot/RIOT/core/lib
"make" -C /work/riot/RIOT/cpu/stm32
"make" -C /work/riot/RIOT/cpu/cortexm_common
"make" -C /work/riot/RIOT/cpu/cortexm_common/periph
"make" -C /work/riot/RIOT/cpu/stm32/periph
/work/riot/RIOT/cpu/stm32/periph/adc_f0_g0.c: In function 'adc_init':
/work/riot/RIOT/cpu/stm32/periph/adc_f0_g0.c:78:17: error: 'ADC_CR_ADVREGEN' undeclared (first use in this function); did you mean 'ADC_CCR_VREFEN'?
   78 |     ADC1->CR |= ADC_CR_ADVREGEN;
      |                 ^~~~~~~~~~~~~~~
      |                 ADC_CCR_VREFEN
/work/riot/RIOT/cpu/stm32/periph/adc_f0_g0.c:78:17: note: each undeclared identifier is reported only once for each function it appears in
/work/riot/RIOT/cpu/stm32/periph/adc_f0_g0.c:88:29: error: 'ADC_ISR_EOCAL' undeclared (first use in this function); did you mean 'ADC_ISR_EOC'?
   88 |         while ((ADC1->ISR & ADC_ISR_EOCAL)) {}
      |                             ^~~~~~~~~~~~~
      |                             ADC_ISR_EOC
make[3]: *** [/work/riot/RIOT/Makefile.base:146: /work/riot/RIOT/tests/periph/adc/bin/nucleo-f072rb/periph/adc_f0_g0.o] Error 1
make[2]: *** [/work/riot/RIOT/Makefile.base:31: ALL--/work/riot/RIOT/cpu/stm32/periph] Error 2
make[1]: *** [/work/riot/RIOT/Makefile.base:31: ALL--/work/riot/RIOT/cpu/stm32] Error 2
make: *** [/work/riot/RIOT/tests/periph/adc/../../../Makefile.include:761: application_tests_adc.module] Error 2
make: Leaving directory '/work/riot/RIOT/tests/periph/adc'

I haven't checked the f0 datasheet to see if the changes in the PR could apply to f0 (which I doubt) so maybe just add some
#if defined(ADC_CR_ADVREGEN)/#endif around the code added would be enough.

@david-vankampen david-vankampen force-pushed the stm32_adc_cal branch 2 times, most recently from c8fa344 to a0b9ff3 Compare November 6, 2023 18:29
benpicco
benpicco approved these changes Nov 8, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test this yet, but code looks good.
Can you provide some results?

@david-vankampen
Copy link
Copy Markdown
Contributor Author

david-vankampen commented Nov 8, 2023

@benpicco I don't have hard numbers with me, but this change, coupled with the adjustment using Vrefint (to compensate for a moving or unknown Vref) got us an improvement of about 50-100mV in accuracy on the ADC reading. I don't have details to attribute which change provided more improvements, however.

@maribu
Copy link
Copy Markdown
Member

maribu commented Nov 17, 2023

I think this needs a rebase due to the migration from bors to Github merge queues. @david-vankampen would you mind to rebase this on current master?

@david-vankampen
Copy link
Copy Markdown
Contributor Author

david-vankampen commented Nov 27, 2023

@maribu looks like this should now be all set on that front?

@benpicco benpicco enabled auto-merge November 27, 2023 20:22
@benpicco benpicco added this pull request to the merge queue Nov 27, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 28, 2023
@maribu
Copy link
Copy Markdown
Member

maribu commented Nov 28, 2023

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 28, 2023

Fantastic, just minutes before the build was completed 😢

@maribu maribu added this pull request to the merge queue Nov 28, 2023
Merged via the queue into RIOT-OS:master with commit 7790f85 Nov 28, 2023
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@DipSwitch DipSwitch Awaiting requested review from DipSwitch

@vincent-d vincent-d Awaiting requested review from vincent-d vincent-d is a code owner

Assignees

No one assigned

Labels

Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms

Projects

None yet

Milestone

Release 2024.01

Development

Successfully merging this pull request may close these issues.

6 participants

@david-vankampen @riot-ci @aabadie @maribu @benpicco @MrKevinWeiss