Improve management handling of multiple instances of analogin ojects #4623

LMESTM · 2017-06-23T13:44:09Z

Description

Few reports has raised issue related to multiple instances of analogin objects:
https://developer.mbed.org/questions/67841/Multiple-analogIn-not-working/

This PR solves the issue by replacing usage of a single global variable to store the adc handle by a new handle members in the analogin_s object.

Status

READY

Tests

CI shield tests PASSED OK. (no regression found)

theotherjimmy · 2017-06-23T19:57:07Z

@LMESTM Does this also reduce the amount of static RAM allocated when an AnalogIn object is not constructed? Does it increase the size of an AnalogIn object?

theotherjimmy · 2017-06-23T20:00:15Z

@LMESTM It looks like Travis CI failed.
The error is reproduced below:

In file included from /home/travis/build/ARMmbed/mbed-os/BUILD/mbed/TARGET_DISCO_F413ZH/TARGET_STM/TARGET_STM32F4/TARGET_STM32F413xH/objects.h:63:0,

                 from /home/travis/build/ARMmbed/mbed-os/BUILD/mbed/TARGET_DISCO_F413ZH/TARGET_STM/TARGET_STM32F4/device.h:38,

                 from /home/travis/build/ARMmbed/mbed-os/BUILD/mbed/hal/analogin_api.h:22,

                 from /home/travis/build/ARMmbed/mbed-os/targets/TARGET_STM/TARGET_STM32F4/analogin_api.c:29:

/home/travis/build/ARMmbed/mbed-os/BUILD/mbed/TARGET_DISCO_F413ZH/TARGET_STM/TARGET_STM32F4/common_objects.h:113:8: error: redefinition of 'struct analogin_s'

 struct analogin_s {

        ^

In file included from /home/travis/build/ARMmbed/mbed-os/BUILD/mbed/TARGET_DISCO_F413ZH/TARGET_STM/TARGET_STM32F4/device.h:38:0,

                 from /home/travis/build/ARMmbed/mbed-os/BUILD/mbed/hal/analogin_api.h:22,

                 from /home/travis/build/ARMmbed/mbed-os/targets/TARGET_STM/TARGET_STM32F4/analogin_api.c:29:

/home/travis/build/ARMmbed/mbed-os/BUILD/mbed/TARGET_DISCO_F413ZH/TARGET_STM/TARGET_STM32F4/TARGET_STM32F413xH/objects.h:43:8: note: originally defined here

 struct analogin_s {

Please fix this compile error.

LMESTM · 2017-06-26T07:09:55Z

@LMESTM Does this also reduce the amount of static RAM allocated when an AnalogIn object is not constructed? Does it increase the size of an AnalogIn object?

Yes and yes as a direct consequence of moving from global to object

LMESTM · 2017-06-26T07:10:08Z

Will check the build failure and update RP

LMESTM · 2017-06-26T09:08:29Z

@theotherjimmy - error reproduced after rebase, then fixed. should be fine now

theotherjimmy · 2017-06-26T14:06:41Z

@LMESTM Thanks for getting on that!

0xc0170 · 2017-06-29T09:19:16Z

retest uvisor

theotherjimmy · 2017-06-29T15:46:51Z

/morph test

mbed-bot · 2017-06-29T16:49:39Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

0xc0170 · 2017-07-03T10:18:14Z

@LMESTM Can you please look at the conflicts?

LMESTM · 2017-07-03T11:52:36Z

@0xc0170 rebased and conflicts solved

LMESTM · 2017-07-04T13:32:35Z

I made a conflict resolution error during last rebase - solved now

studavekar · 2017-07-05T19:31:14Z

/morph test

mbed-bot · 2017-07-05T23:38:41Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 736

Test failed!

ohagendorf · 2017-07-06T08:38:54Z

Maybe a minor thing was overseen in targets/TARGET_STM/TARGET_STM32F2/objects.h

struct analogin_s {
  ADCName adc;
  ADC_HandleTypeDef handle;
  PinName pin;
  uint8_t channel;
 };

In comparison to all other familiies is contains the line:

ADCName adc;

LMESTM · 2017-07-06T09:21:43Z

@ohagendorf good catch - will post update

theotherjimmy · 2017-07-10T16:15:51Z

/morph test

mbed-bot · 2017-07-11T04:16:22Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

LMESTM · 2017-07-11T06:50:17Z

Rebased - so conflicts resolved

0xc0170 · 2017-07-11T13:51:12Z

/morph test

In this commit, the analogin_s structure is moved to commonn_objects.h file to limit the duplicaion. The ADC handle is moved from a global variable to a struct member of the analogin object. This allows multiple ADC instances to work correctly. Note that State needs to be explicitely set to HAL_ADC_STATE_RESET because the object is not zero initialized.

The adc in analogin_t has the same value as the Instance member of ADC_HandleTypeDef. So we can only rely on the later one.

Same as other families applied here

LMESTM · 2017-07-17T11:28:49Z

@0xc0170 There were conflicts due to another merged PR - rebased and conflicts solved

0xc0170 · 2017-07-17T12:19:02Z

@LMESTM thanks, please look at travis failure

/home/travis/build/ARMmbed/mbed-os/targets/TARGET_STM/TARGET_STM32L0/analogin_api.c:44:8: error: 'analogin_t' has no member named 'adc'
     obj->adc = (ADCName)NC;

LMESTM · 2017-07-17T12:38:15Z

@0xc0170 oops - this was also introduced by the conflicting PR and I missed it during rebase. I was too confident ... compiles fine now.

0xc0170 · 2017-07-17T13:22:35Z

Still fails for Travis?

LMESTM · 2017-07-17T14:29:56Z

good job travis !

jeromecoutant · 2017-07-20T12:01:17Z

targets/TARGET_STM/TARGET_STM32F4/analogin_api.c

        // Get the functions (adc channel) from the pin and assign it to the object
        function = pinmap_function(pin, PinMap_ADC);
        // Configure GPIO
        pinmap_pinout(pin, PinMap_ADC);
    } else {
        // Internal channels
-        obj->adc = (ADCName)pinmap_peripheral(pin, PinMap_ADC_Internal);
+        obj->handle.Instance = (ADC_TypeDef *) pinmap_peripheral(pin, PinMap_ADC);


Sorry Laurent,
This should be PinMap_ADC_Internal

jeromecoutant · 2017-07-20T12:03:42Z

targets/TARGET_STM/TARGET_STM32L4/analogin_api.c

        // Get the functions (adc channel) from the pin and assign it to the object
        function = pinmap_function(pin, PinMap_ADC);
        // Configure GPIO
        pinmap_pinout(pin, PinMap_ADC);
    } else {
        // Internal channels
-        obj->adc = (ADCName)pinmap_peripheral(pin, PinMap_ADC_Internal);
+        obj->handle.Instance = (ADC_TypeDef *) pinmap_peripheral(pin, PinMap_ADC);


Same, this should be PinMap_ADC_Internal

jeromecoutant · 2017-07-20T12:05:44Z

targets/TARGET_STM/TARGET_STM32F2/analogin_api.c

        // Get the functions (adc channel) from the pin and assign it to the object
        function = pinmap_function(pin, PinMap_ADC);
        // Configure GPIO
        pinmap_pinout(pin, PinMap_ADC);
    } else {
        // Internal channels
-        obj->adc = (ADCName)pinmap_peripheral(pin, PinMap_ADC_Internal);
+        obj->handle.Instance = (ADC_TypeDef *) pinmap_peripheral(pin, PinMap_ADC);


Same, PinMap_ADC_Internal

jeromecoutant · 2017-07-20T12:06:34Z

targets/TARGET_STM/TARGET_STM32F7/analogin_api.c

        function = pinmap_function(pin, PinMap_ADC);
        // Configure GPIO
        pinmap_pinout(pin, PinMap_ADC);
    } else {
        // Internal channels
-        obj->adc = (ADCName)pinmap_peripheral(pin, PinMap_ADC_Internal);
+        obj->handle.Instance = (ADC_TypeDef *) pinmap_peripheral(pin, PinMap_ADC);


same, PinMap_ADC_Internal

The instance needs to be searched in PinMap_ADC_Internal, not PinMap_ADC. This was a copy paste error...

LMESTM · 2017-07-20T12:16:34Z

@jeromecoutant Thanks !
Too many errors in this PR, about time to take a summer break ;-) !

0xc0170 · 2017-07-21T11:50:14Z

/morph test

mbed-bot · 2017-07-21T15:55:35Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 861

All builds and test passed!

0xc0170 · 2017-07-24T11:08:21Z

@jeromecoutant happy with the update?

0xc0170 · 2017-07-31T13:57:28Z

@jeromecoutant happy with the update?

@LMESTM @jeromecoutant - we assume yes, as no voice against this changeset

theotherjimmy added the needs: review label Jun 23, 2017

theotherjimmy added needs: work and removed needs: review labels Jun 23, 2017

LMESTM force-pushed the analogin_handle branch from aa8d35e to 2c7eef1 Compare June 26, 2017 09:07

0xc0170 added needs: CI and removed needs: work labels Jun 29, 2017

LMESTM force-pushed the analogin_handle branch from 2c7eef1 to c529ecb Compare July 3, 2017 11:51

LMESTM force-pushed the analogin_handle branch from c529ecb to 32f5184 Compare July 4, 2017 13:31

0xc0170 approved these changes Jul 4, 2017

View reviewed changes

LMESTM force-pushed the analogin_handle branch from c5a5868 to 87c1b79 Compare July 11, 2017 06:50

LMESTM added 3 commits July 17, 2017 13:23

STM32: analogin: adc struct member not needed anymore

efbeb0d

The adc in analogin_t has the same value as the Instance member of ADC_HandleTypeDef. So we can only rely on the later one.

STM32: F2: adc; struct member not needed

ea33f8b

Same as other families applied here

LMESTM force-pushed the analogin_handle branch from 87c1b79 to ea33f8b Compare July 17, 2017 11:28

0xc0170 added needs: work and removed needs: CI labels Jul 17, 2017

STM32: L0: adc struct member does not exist anymore

b9d801f

LMESTM force-pushed the analogin_handle branch from a7e435d to b9d801f Compare July 17, 2017 14:29

jeromecoutant reviewed Jul 20, 2017

View reviewed changes

STM32: ADC: Fix PinMap_ADC_Internal

7414b12

The instance needs to be searched in PinMap_ADC_Internal, not PinMap_ADC. This was a copy paste error...

0xc0170 added needs: review and removed needs: work labels Jul 24, 2017

theotherjimmy added ready for merge and removed needs: review labels Jul 31, 2017

theotherjimmy merged commit f08d5a4 into ARMmbed:master Jul 31, 2017

theotherjimmy added release-version: 5.5.5 and removed ready for merge labels Jul 31, 2017

Improve management handling of multiple instances of analogin ojects #4623

Improve management handling of multiple instances of analogin ojects #4623

Uh oh!

Conversation

LMESTM commented Jun 23, 2017

Description

Status

Tests

Uh oh!

theotherjimmy commented Jun 23, 2017

Uh oh!

theotherjimmy commented Jun 23, 2017

Uh oh!

LMESTM commented Jun 26, 2017

Uh oh!

LMESTM commented Jun 26, 2017

Uh oh!

LMESTM commented Jun 26, 2017

Uh oh!

theotherjimmy commented Jun 26, 2017

Uh oh!

0xc0170 commented Jun 29, 2017

Uh oh!

theotherjimmy commented Jun 29, 2017

Uh oh!

mbed-bot commented Jun 29, 2017

Uh oh!

0xc0170 commented Jul 3, 2017

Uh oh!

LMESTM commented Jul 3, 2017

Uh oh!

LMESTM commented Jul 4, 2017

Uh oh!

studavekar commented Jul 5, 2017

Uh oh!

mbed-bot commented Jul 5, 2017

Output

Uh oh!

ohagendorf commented Jul 6, 2017

Uh oh!

LMESTM commented Jul 6, 2017

Uh oh!

theotherjimmy commented Jul 10, 2017

Uh oh!

mbed-bot commented Jul 11, 2017

Uh oh!

LMESTM commented Jul 11, 2017

Uh oh!

0xc0170 commented Jul 11, 2017

Uh oh!

LMESTM commented Jul 17, 2017

Uh oh!

0xc0170 commented Jul 17, 2017

Uh oh!

LMESTM commented Jul 17, 2017

Uh oh!

0xc0170 commented Jul 17, 2017

Uh oh!

LMESTM commented Jul 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LMESTM commented Jul 20, 2017

Uh oh!

0xc0170 commented Jul 21, 2017

Uh oh!