Fix AnalogIn implementation on STM32G4 series #13601

AGlass0fMilk · 2020-09-11T17:20:31Z

Summary of changes

This PR reintroduces AnalogIn on the STM32G4 series based targets (eg: NUCLEO_G474RE). It also introduces fixes to the AnalogIn HAL API implementation.

Fixes in this PR include:

Enabling the peripheral clocks for ADCs during initialization
Removing internal mappings from some channels that should be routed to external pin inputs.

@jeromecoutant Please review and possibly update this PR at your discretion... I am not as familiar with how to handle the internal ADC channels (ie: Temperature, VREF, OPAMP, etc). This PR removes all support for internal ADC channels as it stands right now. It seems ADC2 has more inputs than on the F3 series upon which this analog in implementation was based. I tried to use the pin A2 as an analog input on my NUCLEO G474 and the current analog in implementation routed it to an internal OP amp channel instead of the PA4/A2.

Impact of changes

Migration actions required

Documentation

None

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@jeromecoutant

AGlass0fMilk · 2020-09-11T17:23:25Z

@jeromecoutant Should the Nucleo G474 be using USE_PLL_HSE_EXTC or USE_PLL_HSE_XTAL? The current default is USE_PLL_HSE_EXTC but I think the hardware is actually a crystal not an oscillator.

mbed-os/targets/targets.json

Lines 2441 to 2445 in 356effa

    
           "clock_source": { 
        
               "help": "Mask value : USE_PLL_HSE_EXTC | USE_PLL_HSE_XTAL (need HW patch) | USE_PLL_HSI", 
        
               "value": "USE_PLL_HSE_EXTC", 
        
               "macro_name": "CLOCK_SOURCE" 
        
           },

ciarmcom · 2020-09-11T17:30:26Z

@AGlass0fMilk, thank you for your changes.
@jeromecoutant @ARMmbed/mbed-os-maintainers please review.

AGlass0fMilk · 2020-09-11T21:44:57Z

@jeromecoutant Also, since the clock speed on this target is so high by default I am seeing small offsets in my ADC readings due to the short accumulation time.

Setting the ADC prescaler to divide by 4 and setting the sampling time to 640.5 cycles greatly improves the offset but maybe there's a way to get make it so I'm not maxing out the settings?

0xc0170 · 2020-09-16T15:35:27Z

@ARMmbed/team-st-mcd Please review

0xc0170 · 2020-09-17T07:31:06Z

CI started

mbed-ci · 2020-09-17T08:34:28Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-ARM	✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage	✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest	✔️

0xc0170 · 2020-09-17T08:36:19Z

Set back to review, waiting for @jeromecoutant to review

0xc0170 · 2020-09-21T11:08:22Z

@jeromecoutant Can you review? this can go in if approved.

jeromecoutant

Hi

I have tested ci-test-shield, results are OK :-)

But...

it seems there are too many TODO in comments?
it works but I couldn't see any __HAL_RCC_ADC12_CLK_ENABLE()) call for ex... Need to check...
internal channels to check

jeromecoutant · 2020-09-21T14:01:38Z

targets/TARGET_STM/TARGET_STM32G4/analogin_device.c

@@ -153,11 +163,15 @@ uint16_t adc_read(analogin_t *obj)

    // Configure ADC channel
    sConfig.Rank         = ADC_REGULAR_RANK_1;
-    sConfig.SamplingTime = ADC_SAMPLETIME_24CYCLES_5;
+    sConfig.SamplingTime = ADC_SAMPLETIME_247CYCLES_5;


Really need so much time ?

If my math is correct, with the PCLK @ 180MHz and the ADC prescaler set to DIV2, a sample time of 24.5 clock cycles is only ~272 nanoseconds... I think this is quite short for most of the slow signals Mbed users will be trying to read with the simple AnalogIn API.

My comment from before:

Also, since the clock speed on this target is so high by default I am seeing small offsets in my ADC readings due to the short accumulation time.
Setting the ADC prescaler to divide by 4 and setting the sampling time to 640.5 cycles greatly improves the offset but maybe there's a way to get make it so I'm not maxing out the settings?

Perhaps we could increase the ADC prescaler?

AGlass0fMilk · 2020-09-21T15:04:20Z

Hi

I have tested ci-test-shield, results are OK :-)

But...

* it seems there are too many TODO in comments?

* it works but I couldn't see any __HAL_RCC_ADC12_CLK_ENABLE()) call for ex... Need to check...

* internal channels to check

The TODOs are mostly for future reference. I'm not sure where (or if) the ADC is deinitialized. We should deinitialize the clock at the same time to save power. Regardless I think this is an improvement over the current target support.

0xc0170 · 2020-09-21T15:30:56Z

Are TODO's required there? I would rather keep TODO as an issue rather than in the code.

jeromecoutant · 2020-09-21T15:54:46Z

Here is my branch: 885aca7

jeromecoutant · 2020-09-21T16:00:53Z

Note that analogin_free exists since #11032
And this has not been implemented for every targets...

Pull request has been modified.

AGlass0fMilk · 2020-09-21T16:22:47Z

@jeromecoutant I implemented analogin_free. Let me know your thoughts. It's pretty rudimentary (enable counting) but I think it will work. I will look at your branch and merge the internal ADC channel changes.

@0xc0170 I removed the TODOs.

Co-authored-by: jeromecoutant <[email protected]>

AGlass0fMilk · 2020-09-21T16:30:41Z

@jeromecoutant I merged your internal ADC channel changes: bbc7355

jeromecoutant

Maybe only 2 counters are needed ? adc12 and adc345 ?

jeromecoutant · 2020-09-22T07:38:35Z

targets/TARGET_STM/TARGET_STM32G4/analogin_device.c

@@ -158,6 +204,10 @@ uint16_t adc_read(analogin_t *obj)
    sConfig.OffsetNumber = ADC_OFFSET_NONE;
    sConfig.Offset       = 0;

+    /**
+     * TODO - what about internal channels? VBAT, VREF, Temperature?


To remove in the next commit ?

jeromecoutant · 2020-09-22T09:16:41Z

targets/TARGET_STM/TARGET_STM32G4/analogin_device.c

@@ -163,7 +199,7 @@ uint16_t adc_read(analogin_t *obj)

    // Configure ADC channel
    sConfig.Rank         = ADC_REGULAR_RANK_1;
-    sConfig.SamplingTime = ADC_SAMPLETIME_247CYCLES_5;
+    sConfig.SamplingTime = ADC_SAMPLETIME_24CYCLES_5;


Maybe you should keep 247 cycles value,
some tests are failing with 24 value...

Perhaps I should increase the ADC clock prescaler?

AGlass0fMilk · 2020-09-22T13:27:43Z

Maybe only 2 counters are needed ? adc12 and adc345 ?

That was my initial implementation but then I figured having a counter for each ADC would allow us to deinitialize each ADC individually as well.

AGlass0fMilk · 2020-09-22T13:32:53Z

@jeromecoutant Please review

0xc0170 · 2020-09-30T10:49:18Z

CI started

mbed-ci · 2020-09-30T12:26:33Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage	✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest	✔️

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Sep 11, 2020

ciarmcom requested review from jeromecoutant and a team September 11, 2020 17:30

ciarmcom added the needs: review label Sep 11, 2020

0xc0170 previously approved these changes Sep 16, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Sep 16, 2020

mergify bot added ready for merge and removed needs: CI labels Sep 17, 2020

0xc0170 added needs: review and removed ready for merge labels Sep 17, 2020

jeromecoutant suggested changes Sep 21, 2020

View reviewed changes

AGlass0fMilk added 2 commits September 21, 2020 12:19

Fix AnalogIn implementation on STM32G4 series

e9d6c9c

Implemented adc deinitialization functionality

bbc15f6

AGlass0fMilk force-pushed the fix-g474-adc branch from 356effa to bbc15f6 Compare September 21, 2020 16:20

Merge internal ADC channel changes from ST

bbc7355

Co-authored-by: jeromecoutant <[email protected]>

jeromecoutant reviewed Sep 22, 2020

View reviewed changes

Revert sampling time decrease and remove todos.

76d488d

jeromecoutant approved these changes Sep 22, 2020

View reviewed changes

0xc0170 approved these changes Sep 30, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Sep 30, 2020

mergify bot added ready for merge and removed needs: CI labels Sep 30, 2020

0xc0170 merged commit fbe0409 into ARMmbed:master Sep 30, 2020

mergify bot removed the ready for merge label Sep 30, 2020

mbedmain added release-version: 6.4.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Oct 20, 2020

Fix AnalogIn implementation on STM32G4 series #13601

Fix AnalogIn implementation on STM32G4 series #13601

Uh oh!

Conversation

AGlass0fMilk commented Sep 11, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

@jeromecoutant

Uh oh!

AGlass0fMilk commented Sep 11, 2020

Uh oh!

ciarmcom commented Sep 11, 2020

Uh oh!

AGlass0fMilk commented Sep 11, 2020

Uh oh!

0xc0170 commented Sep 16, 2020

Uh oh!

0xc0170 commented Sep 17, 2020

Uh oh!

mbed-ci commented Sep 17, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

Uh oh!

0xc0170 commented Sep 17, 2020

Uh oh!

0xc0170 commented Sep 21, 2020

Uh oh!

jeromecoutant left a comment

Choose a reason for hiding this comment

Uh oh!

jeromecoutant Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

AGlass0fMilk Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

AGlass0fMilk commented Sep 21, 2020

Uh oh!

0xc0170 commented Sep 21, 2020

Uh oh!

jeromecoutant commented Sep 21, 2020

Uh oh!

jeromecoutant commented Sep 21, 2020

Uh oh!

AGlass0fMilk commented Sep 21, 2020

Uh oh!

AGlass0fMilk commented Sep 21, 2020

Uh oh!

jeromecoutant left a comment

Choose a reason for hiding this comment

Uh oh!

jeromecoutant Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

jeromecoutant Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

AGlass0fMilk Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

AGlass0fMilk commented Sep 22, 2020

Uh oh!

AGlass0fMilk commented Sep 22, 2020

Uh oh!

0xc0170 commented Sep 30, 2020

Uh oh!

mbed-ci commented Sep 30, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

Uh oh!

Uh oh!