cpu/stm32l0 : implementation of ADC driver by Marc-Aurele · Pull Request #7815 · RIOT-OS/RIOT

Marc-Aurele · 2017-10-24T13:16:05Z

Now, ADC is available on stm32l0 family. A calibration is
done at each new adc_sample to avoid error, especially during voltage variation. Moreover, when conversion is done, VREFINT and temperature sensor are dactivated to save power and reactivated at next adc_sample.

Signed-off-by: Aurelien Fillau [email protected]

aabadie

Thanks for your PR. As usual I found some (minor) style issues.
I'll test on hardware when time permits.

aabadie · 2017-10-24T13:33:13Z

boards/nucleo-l073/include/periph_conf.h


 /**
- * @name    ADC configuration
+ * @name   ADC configuration


This change is not 'valid', we want to keep 4 spaces after @name

aabadie · 2017-10-24T13:33:35Z

boards/nucleo-l073/include/periph_conf.h

 * @{
 */
-#define ADC_NUMOF           (0)
+#define ADC_CONFIG {             \


The backslashes could be vertically aligned

aabadie · 2017-10-24T13:33:55Z

boards/nucleo-l073/include/periph_conf.h

+    { GPIO_PIN(PORT_A, 6), 6 },  \
+}
+
+#define ADC_NUMOF           (1U)


Shouldn't this be 3 ?

aabadie · 2017-10-24T13:34:48Z

cpu/stm32l0/periph/adc.c

+    }
+
+    if ((ADC1->CR & ADC_CR_ADEN) != 0) /* (1) */
+    {


The opening curly brace should be at the end of the if line

aabadie · 2017-10-24T13:34:57Z

cpu/stm32l0/periph/adc.c

+    }
+
+    if ((ADC1->CR & ADC_CR_ADEN) == 0)
+    {


aabadie · 2017-10-24T13:35:22Z

cpu/stm32l0/periph/adc.c

+    prep();
+
+    switch (res) {
+    case ADC_RES_6BIT:


the case block should be indented

aabadie · 2017-10-24T13:35:35Z

cpu/stm32l0/periph/adc.c

+
+    /* Disable ADC before renenabling it */
+    if ((ADC1->CR & ADC_CR_ADEN) != 0) /* (1) */
+    {


curly brace on same line as if

aabadie · 2017-10-24T13:36:02Z

cpu/stm32l0/periph/adc.c

+    }
+
+    if ((ADC1->CR & ADC_CR_ADEN) == 0)
+    {


curly brace

aabadie · 2017-10-24T13:38:44Z

cpu/stm32l0/periph/adc.c

+        gpio_init_analog(adc_config[line].pin);
+    }
+
+    if ((ADC1->CR & ADC_CR_ADEN) != 0) /* (1) */


What does (1) mean ?

Some code could also be factorized in a static function here, I see can the same code some lines below in adc_sample

aabadie · 2017-10-24T13:39:24Z

cpu/stm32l0/periph/adc.c

+
+    /* Disable ADC */
+    if ((ADC1->CR & ADC_CR_ADEN) != 0) /* (1) */
+    {


curly brace

Marc-Aurele

Tested on NUCLEO-L073 and a custom board.

Marc-Aurele · 2017-10-24T15:05:16Z

Here is the code that i used to test :

#define ADC_CAL_TEMPA_VALUE			(* ( ((uint16_t*) ((uint32_t) 0x1FF8007A))))
#define ADC_CAL_TEMPB_VALUE			(* ( ((uint16_t*) ((uint32_t) 0x1FF8007E))))
#define ADC_CAL_TEMP_VOLTAGE		(3000)			//voltage in mV
#define ADC_CAL_TEMPA_TEMP			(300)			//temperature in tenth of degrees
#define ADC_CAL_TEMPB_TEMP			(1300)			//temperature in tenth of degrees
#define ADC_CAL_VREFINT_VALUE		((uint32_t) *((uint16_t*) 0x1FF80078))
#define ADC_VREF_VCC_ON_CAL			(3000)

int32_t get_internal_voltage(void) {
    uint16_t sample = adc_sample(0,ADC_RES_12BIT);
	int32_t voltage = (ADC_VREF_VCC_ON_CAL * ADC_CAL_VREFINT_VALUE) / sample;

	_vdda = voltage;

	return voltage;
}

int32_t get_internal_temp(void) {
	volatile int32_t temp = -1;

	uint16_t rawA = ADC_CAL_TEMPA_VALUE;
	uint16_t rawB = ADC_CAL_TEMPB_VALUE;
	//temperature measure
	temp = adc_sample(1,ADC_RES_12BIT) * _vdda; //max is 4096*4000
	temp/= ADC_CAL_TEMP_VOLTAGE;//max is 4096*4/3
	temp-= rawA; //max is 4096*4/3
	temp *= ADC_CAL_TEMPB_TEMP - ADC_CAL_TEMPA_TEMP; // max is 4096 * 4000  / 3
	temp /= rawB - rawA;
	temp+= ADC_CAL_TEMPA_TEMP;

	return temp;
}

int32_t get_external_voltage(void) {
    uint16_t sample = adc_sample(2,ADC_RES_12BIT);

	int32_t voltage = (_vdda * sample) / 4095;

	return voltage;
}

Don't forget to initialize ADC :

    adc_init(0);
    adc_init(1);
    adc_init(2);

aabadie · 2017-10-24T15:09:04Z

For testing, you can also use the application in tests/periph_adc

aabadie · 2017-10-29T09:47:03Z

boards/nucleo-l073/include/periph_conf.h

 */
-#define ADC_NUMOF           (0)
+#define ADC_CONFIG {             \
+    { GPIO_PIN(PORT_A, 0), 17 }, \


According to the nucleo boards datasheet section 6.11, table 18 page 46, the default configuration of ADC pins should be something like:

/** * @name ADC configuration * @{ */ #define ADC_CONFIG { \ { GPIO_PIN(PORT_A, 0), 0 }, \ { GPIO_PIN(PORT_A, 1), 1 }, \ { GPIO_PIN(PORT_A, 4), 4 }, \ { GPIO_PIN(PORT_B, 0), 8 }, \ { GPIO_PIN(PORT_C, 1), 11 },\ { GPIO_PIN(PORT_C, 0), 10 } \ } #define ADC_NUMOF (6) /** @} */

aabadie · 2017-10-29T09:48:19Z

cpu/stm32l0/periph/adc.c

+static void _enable_adc(void)
+{
+	if ((ADC1->CR & ADC_CR_ADEN) != 0) {
+		ADC1->CR |= ADC_CR_ADDIS;


indentation is off: 8 spaces instead of 4. Same issue in the block below

aabadie · 2017-10-29T09:49:14Z

cpu/stm32l0/periph/adc.c

+    prep();
+
+    if ((adc_config[line].chan != 17) && (adc_config[line].chan != 18)) {
+        /*configure the pin */


nit: space missing after /*

aabadie · 2017-10-29T09:50:20Z

cpu/stm32l0/periph/adc.c

+        gpio_init_analog(adc_config[line].pin);
+    }
+
+    ADC1->CFGR1 = 0; //no watchdog, no discontinuous mode, no auto off, single conv, no trigger, right align, 12bits, no dma, no wait


Please use C-style comments, e.g /* */ here and below. For readability and reviewability in Github, you should also split the long lines.

aabadie · 2017-11-07T16:31:30Z

Thanks for updating @Marc-Aurele.

Code style wise, everything is ok now. I gave it a try on my nucleo-l073 using the tests/periph_adc test application. Unfortunately it doesn't work: all measured values are < 0 and the output is like that:

2017-11-07 17:30:34,475 - INFO # ADC_LINE(0): 10-bit resolution not applicable
2017-11-07 17:30:34,479 - INFO # ADC_LINE(1): 10-bit resolution not applicable
2017-11-07 17:30:34,484 - INFO # ADC_LINE(2): 10-bit resolution not applicable
2017-11-07 17:30:34,488 - INFO # ADC_LINE(3): 10-bit resolution not applicable
2017-11-07 17:30:34,491 - INFO # ADC_LINE(4): 10-bit resolution not applicable
2017-11-07 17:30:34,496 - INFO # ADC_LINE(5): 10-bit resolution not applicable

Can you test on your side ? Maybe you have an idea of what is missing/wrong ?

Marc-Aurele · 2017-11-07T21:21:12Z

Thank you very much for the reviews. I'm sry for the inconveniences, i will try to improve my patches in the future.
Regarding this patch, some resolutions were not handled and i had only tested this driver in 12 bits resolution, but now it should work properly for any resolutions.

aabadie · 2017-11-08T08:29:11Z

@Marc-Aurele your updated version works like a charm now, good job !

Can you squash your commits into 2 with comments:

cpu/stm32l0: add ADC driver implementation
boards/nucleo-l073: add ADC configuration

each commit should be related to changes in the corresponding directories.

Murdock is passing now, so after squashing it can be merged.

Marc-Aurele · 2017-11-08T10:59:41Z

I have run the test tests/periph_adc on a nucleo board but it seems there is a kernel panic due to xtimer_periodic_wakeup. I'm not sure this panic is related to my patch.
Do you have the same issue ?

aabadie · 2017-11-08T11:58:16Z

Do you have the same issue ?

I tested on nucleo-l073 since it's the only one configured by your PR. On which one have you tried (could nucleo-l053 or nucleo32-l031) ?

aabadie

Just had another look and I found 2 remaining minor issues.

aabadie · 2017-11-08T12:04:10Z

cpu/stm32l0/periph/adc.c

+#include "mutex.h"
+#include "periph/adc.h"
+
+#ifdef ADC_CONFIG


Here it shoud be:

#ifdef ADC_NUMOF

Are you sure about this ? Because it is done like this everywhere else.

Are you sure about this ?

Yes

Because it is done like this everywhere else

For ADC but not for the other peripheral drivers (look at the stm32-common ones. And there's #7277 that will use the same pattern once merged (see this line.

aabadie · 2017-11-08T12:04:36Z

boards/nucleo-l073/include/periph_conf.h

+    { GPIO_PIN(PORT_C, 0), 10 } \
+}
+
+#define ADC_NUMOF           (6)


Maybe update to:

#define ADC_NUMOF (6U)

Now, ADC is available on stm32l0 family. A calibration is done at each new adc_sample. Moreover, when conversion is done, VREFINT and temperature sensor are dactivated to save power. Signed-off-by: Aurelien Fillau <[email protected]>

ADC configuration added for nucleo-l073 board Signed-off-by: Aurelien Fillau <[email protected]>

Marc-Aurele · 2017-11-08T13:35:07Z

Now, test is running very well. I have only nucleo-l073 and custom boards i have made to test. I'm pretty sure the kernel panic i have faced was not related to this patch. I'm suspecting a PSP (process stack pointer) not initialized or something like that because the board was panicking at the first context switch but not sure.

aabadie

Regarding the kernel panic, this looks weird indeed. But the L0 clock initialization is not perfect as @haukepetersen said one day. It could be one of the reason. Since it's unrelated (I think), let's merge.

ACK and go, thanks @Marc-Aurele !

aabadie self-assigned this Oct 24, 2017

aabadie added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: drivers Area: Device drivers labels Oct 24, 2017

aabadie requested changes Oct 24, 2017

View reviewed changes

Marc-Aurele commented Oct 24, 2017

View reviewed changes

aabadie changed the title ~~cpu/stm32l0 : implemention of ADC driver~~ cpu/stm32l0 : implementation of ADC driver Oct 29, 2017

aabadie reviewed Oct 29, 2017

View reviewed changes

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 8, 2017

Marc-Aurele force-pushed the adc_l0 branch from 1213cf7 to 26e7e9b Compare November 8, 2017 09:54

aabadie requested changes Nov 8, 2017

View reviewed changes

Marc-Aurele force-pushed the adc_l0 branch from 26e7e9b to dbed6ca Compare November 8, 2017 12:30

Marc-Aurele added 2 commits November 8, 2017 14:29

cpu/stm32l0: add ADC driver implementation

9907908

Now, ADC is available on stm32l0 family. A calibration is done at each new adc_sample. Moreover, when conversion is done, VREFINT and temperature sensor are dactivated to save power. Signed-off-by: Aurelien Fillau <[email protected]>

boards/nucleo-l073: add ADC configuration

9825245

ADC configuration added for nucleo-l073 board Signed-off-by: Aurelien Fillau <[email protected]>

Marc-Aurele force-pushed the adc_l0 branch from dbed6ca to 9825245 Compare November 8, 2017 13:30

aabadie approved these changes Nov 8, 2017

View reviewed changes

aabadie merged commit 509ad6e into RIOT-OS:master Nov 8, 2017

aabadie mentioned this pull request Nov 8, 2017

cpu/stm32: unified ADC driver implementations #7277

Closed

aabadie added this to the Release 2018.01 milestone Jan 18, 2018

Conversation

Marc-Aurele commented Oct 24, 2017

Uh oh!

aabadie left a comment

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!

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!

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!

Marc-Aurele left a comment

Choose a reason for hiding this comment

Uh oh!

Marc-Aurele commented Oct 24, 2017

Uh oh!

aabadie commented Oct 24, 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!

aabadie commented Nov 7, 2017

Uh oh!

Marc-Aurele commented Nov 7, 2017

Uh oh!

aabadie commented Nov 8, 2017

Uh oh!

Marc-Aurele commented Nov 8, 2017

Uh oh!

aabadie commented Nov 8, 2017

Uh oh!

aabadie left a comment