stm32f4 uart and adc enhancement #8624

StefJar · 2018-06-28T16:24:49Z

Added some features for the stm32f4xx MCUs

added UART1 dts config for PA15(tx) & PA10(rx)
added ADC driver:
simple stm32hal based ADC driver.
Channels configurable through menuconfig.
Pin config for ADC channels are done inside the driver. no connect to the pinmux so far.
ADC channel conversion time is fixed so far.
driver is an alpha

codecov-io · 2018-06-28T16:48:34Z

Codecov Report

Merging #8624 into master will decrease coverage by 11.76%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #8624       +/-   ##
===========================================
- Coverage   64.17%   52.41%   -11.77%     
===========================================
  Files         429      198      -231     
  Lines       41161    24912    -16249     
  Branches     6926     5185     -1741     
===========================================
- Hits        26416    13058    -13358     
+ Misses      11587     9754     -1833     
+ Partials     3158     2100     -1058

Impacted Files	Coverage Δ
subsys/net/l2/ethernet/arp.c	`54.33% <0%> (-9.4%)`	⬇️
arch/posix/soc/inf_clock/soc.c	`97.82% <0%> (-2.18%)`	⬇️
subsys/net/l2/ethernet/ethernet_mgmt.c	`47.82% <0%> (-1.2%)`	⬇️
subsys/net/ip/net_core.c	`70.67% <0%> (-0.86%)`	⬇️
drivers/ethernet/eth_native_posix.c	`21.42% <0%> (-0.68%)`	⬇️
subsys/net/l2/ethernet/ethernet.c	`51.22% <0%> (-0.51%)`	⬇️
subsys/net/ip/utils.c	`79.82% <0%> (-0.35%)`	⬇️
subsys/net/ip/ipv6.c	`46.01% <0%> (-0.3%)`	⬇️
lib/posix/pthread.c	`68.86% <0%> (-0.17%)`	⬇️
subsys/net/ip/net_if.c	`63.32% <0%> (-0.14%)`	⬇️
... and 288 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e860775...ea2f412. Read the comment docs.

galak · 2018-06-28T19:55:31Z

Is this driver using the new ADC API that is in #7691

erwango · 2018-06-29T13:31:10Z

Hi @StefJar ,

Thanks for proposing this driver!

Some comments to begin:

There are some checks in CI that could block a PR, one them is that commit should contain at least a short comment
It would be better to split your changes into 2 PRs, one with ADC the other with pinmux UART change.
If you used HAL, your driver should be compatible with other STM32 series(when someone will have a try, made some changes and validate it), then this will be the only adc driver for STM32. Please remove reference to keep it compatible with others.
Please use C standard comments (/* */) instead of C++ (//)

Thanks

StefJar · 2018-06-29T13:59:52Z

I am happy to get some quick feedback on my first pull. I am new to zephyr and is infrastructure. Pls help me to understand.

So how to carry on?

@hal:
to make this driver an STM32 one, there must me some SOC arch adds in the Kconfig.stm32f4. My knowledge of the whole SMT32 arch is limited. Now you can activate the ADC2 for arch were the unit is not available. This will break the compiling process.

@C/C++:
These are minor changes which I can do quickly.

@split: shure
@ci: For what stands CI? Were should I do some comments?

therealprof · 2018-07-01T13:57:54Z

dts/arm/st/stm32f4-pinctrl.dtsi

@@ -27,6 +27,12 @@
 					tx = <STM32_PIN_PB6 (STM32_PINMUX_ALT_FUNC_7 | STM32_PUSHPULL_NOPULL)>;
 				};
 			};
+			usart1_pins_d: usart1@3 {
+				rx_tx {
+					rx = <STM32_PIN_PA10 (STM32_PINMUX_ALT_FUNC_7 | STM32_PUSHPULL_PULLUP)>;


Not sure it is actually required (I've never mucked with the drive settings for UARTs on STM32) but the other definitions are using STM32_PUPDR_NO_PULL.

True ´- it's a copy and paste error. Still seems to work. ha ha

As mentioned, for rx no resistor is needed.

StefJar

changes regarding rx pin are done

StefJar · 2018-07-01T18:36:07Z

dts/arm/st/stm32f4-pinctrl.dtsi

@@ -27,6 +27,12 @@
 					tx = <STM32_PIN_PB6 (STM32_PINMUX_ALT_FUNC_7 | STM32_PUSHPULL_NOPULL)>;
 				};
 			};
+			usart1_pins_d: usart1@3 {
+				rx_tx {
+					rx = <STM32_PIN_PA10 (STM32_PINMUX_ALT_FUNC_7 | STM32_PUSHPULL_PULLUP)>;


True ´- it's a copy and paste error. Still seems to work. ha ha

erwango · 2018-07-02T07:44:07Z

@StefJar :

@hal:
to make this driver an STM32 one, there must me some SOC arch adds in the Kconfig.stm32f4. My knowledge of the whole SMT32 arch is limited. Now you can activate the ADC2 for arch were the unit is not available. This will break the compiling process.

You could define whatever ADC instance exist in whole STM32 family here.
Then, control of activated instance will be done from device tree.

@C/C++:
These are minor changes which I can do quickly.

Good, thanks

@split: shure
@ci: For what stands CI? Were should I do some comments?

CI is continuous integration. Whenever you push or update a PR automatics tests are performed. Some are done on style and some on code. Reports are provided by 'Shippable' down here: 'All checks have failed'.
For more information, you could have a look to :
http://docs.zephyrproject.org/contribute/contribute_guidelines.html

So you should provide some comment in the commit message (also, if you look to a git tree, you'll see all commits have a message)

erwango

Some additional comments on style, otherwise you'r doing good.
Would be nice to introduce device tree support. For exemple of adc node, you could have a look to : https://elixir.bootlin.com/linux/v4.18-rc3/source/arch/arm/boot/dts/stm32f429.dtsi#L465

erwango · 2018-07-02T07:31:16Z

drivers/adc/Kconfig.stm32f4

+
+if ADC_STM32F4
+
+	menu ADC_0


Don't indent here. Kconfig files should remain flat

think the best solution would be a flat ADC driver + the instance setup (like channels) in the dts. Never done this before. Is there a document/example that describes how to connect between dts node and driver?

@StefJar , there is a doc there: http://docs.zephyrproject.org/devices/dts/device_tree.html

To sum up:
1- Populate the node in device tree (you can base initially on linux dts)
2- Provide a yaml file (here: dts/bindings/iio/adc/) to instruct dts script on how to interpret the adc node. File name should be same as node compatible. With this, at build, you should get a bunch of #define's generated in output folder, in zephyr/include/generated/generated_dts_board.h.
3-Provide CONFIG_XXX to generated #define's matching in .fixup files (arch/arm/soc/st_stm32/stm32f4/dts.fixup for series)
4- Define HAS_DTS_ADC for the series where adc device node is populated

Then you're good to use whatever values populated in device tree directly in your driver

Then you're good to use whatever values populated in device tree directly in your driver

Note that the device tree should describe the hardware, not the configuration.

It would be better to check the Linux driver (https://elixir.bootlin.com/linux/v4.17/source/drivers/iio/adc/stm32-adc-core.c and https://elixir.bootlin.com/linux/v4.17/source/drivers/iio/adc/stm32-adc.c) and corresponding DTS binding (https://elixir.bootlin.com/linux/v4.17/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt) to make sure that we stay compatible.

merci - I will take a deeper look when I have time for that

erwango · 2018-07-02T07:32:17Z

drivers/adc/Kconfig.stm32f4

+	select USE_STM32_HAL_ADC_EX
+	select USE_STM32_HAL_CORTEX
+	help
+		Enable the driver implementation for the stm32f4xx ADC


Text of 'help' section should be indented by 2 spaces (you can look at other files as exemple)

erwango · 2018-07-02T07:45:12Z

drivers/adc/adc_stm32f4.c

+/* adc_dw.c - Designware ADC driver */
+
+/*
+ * Copyright (c) 2015 Intel Corporation


Put your own Copyright here.

erwango · 2018-07-02T07:47:01Z

drivers/adc/adc_stm32f4.c

+			// none
+			break;
+	}
+	if(GPIOport) HAL_GPIO_Init(GPIOport, &gpioCfg);


In Zephyr base code, we enforce the use of braces, even for one liners.
This should looks like:

if(GPIOport) { HAL_GPIO_Init(GPIOport, &gpioCfg); }

erwango · 2018-07-02T07:48:02Z

drivers/adc/adc_stm32f4.c

+	activeChannels = config->activeChannels;
+	i = adcChannel_0;
+	rc = stm32adcError_None;
+	while(activeChannels) {


Put a space after while, do the same after if's.
Apply everywhere

erwango · 2018-07-02T07:50:50Z

drivers/adc/adc_stm32f4.c

+	SYS_LOG_INF("init adc%u", (unsigned int)config->adcDevNum);
+
+	switch(config->adcDevNum) {
+		#ifdef CONFIG_ADC_0


Put #ifdef at the start of a line

erwango · 2018-07-02T07:54:11Z

drivers/adc/adc_stm32f4.h

@@ -0,0 +1,82 @@
+//


Please use following template for copyright:

/* * Copyright (c) 2015 Intel Corporation * * SPDX-License-Identifier: Apache-2.0 */

vaussard

Hi, I did a quick review to point out some problems, mostly style issues that can be easily addressed.

I think that the ADC driver could be greatly simplified by configuring the channels at runtime.

As mentioned by another reviewer, it would be also good to make it more generic to run on other families. Thanks.

vaussard · 2018-07-03T06:51:33Z

drivers/adc/Kconfig.stm32f4

+			default n
+			depends on ADC_0
+			help
+			  Enables ADC channel 0 at pin PA0


I would not mention the pin, as it will probably change between families (ADC1_IN0 is connected to Vrefint on STM32L4 for instance). But I would propose to drop these Kconfig completely (see below).

maybe we can do the configuration of the driver instance in the dts?
Think configuration in run time is a waste of resources. We do not need to check the config before every adc conversion. It can be checked at the init of the driver once(and then never again)

DTS is only to describe the hardware, not the configuration.

I agree that configuring at runtime will have a cost, but it is way more flexible. Static configuration may be OK for a simple project, but it is not enough for a more complex usecase. Think for example that you may have to reconfigure your ADC at several points in the lifetime of your product's lifecycle (i.e. production/testing, shipping, everyday use, low-power saving mode, etc.), which is not possible with the current implementation.

I do not remember if the new ADC API is better with respect to this point.

vaussard · 2018-07-03T06:57:12Z

drivers/adc/adc_stm32f4.h

+
+
+//! type and enum for channel ids
+typedef enum adcChannels {


Zephyr follow Linux coding style, so you should use snake_case instead of camelCase for identifiers, function names, etc.

vaussard · 2018-07-03T06:59:01Z

drivers/adc/adc_stm32f4.h

+//
+// @return Integer: 0 for success, error otherwise.
+//
+int adc_stm32f4_init(struct device *dev);


Why is this function public? It should not be necessary to call it by hand if you use DEVICE_INIT or a similar macro (see include/device.h).

there was no "How to write an (ADC)driver?". I used the adc_dw as blueprint. It was in there so it means for me that it had its relevance. Don't ask me why ;)

vaussard · 2018-07-03T07:01:13Z

drivers/adc/adc_stm32f4.c

+
+#ifdef CONFIG_ADC_0
+
+struct adc_drvData adc_drvData_dev0 = {


static and the closing brace should not be indented (it is even not needed at all as you perform no initializations).

vaussard · 2018-07-03T07:03:20Z

drivers/adc/adc_stm32f4.c

+	};
+
+static struct adc_config adc_config_dev0 = {
+		.adcDevNum = 0,


It looks like there is too much indentation, only one tab is needed.

vaussard · 2018-07-03T07:08:35Z

drivers/adc/adc_stm32f4.c

+	}
+	SYS_LOG_INF("use %u multiplexed channels", (unsigned int) drvData->hadc.Init.NbrOfConversion);
+
+	// start the adc


Nit: this does not really start the ADC, but configure it.

vaussard · 2018-07-03T07:12:25Z

drivers/adc/adc_stm32f4.c

+		}
+		activeChannels >>= 1;
+		i++;
+	}


I think that configuring the channels at compile time is a bit limiting:

It adds a looooot of Kconfigs

You cannot have several sets of sequences, so it limits to basic usecases

You cannot specify the order inside the sequence, which might be important depending on what hardware you sample

Why not configuring the channels in the .read callback using seq_table? In addition this is way easier to perform, so it would remove a good chunk of code.

I agree that this API is limiting, but hopefully the next iteration will be better.

To implement this we need to check/setup the adc sequencer at each adc run. This will cost some computing time. Think usually the user has a static setup which does not change at runtime.

A limiting factor to the sequencing structure is, that you can not define a real delay between the conversation of the channels. This Zephyr feature is not supported by the stm32f4 hardware. Of course we can try to add this via software. But is this really needed?

Unfortunately this is not my setup, I need to reconfigure the ADC sequence at runtime to address some complex needs.
I do not use the conversion delay from the sequencing structure so this is not really a problem for me.

vaussard · 2018-07-03T07:14:49Z

drivers/adc/adc_stm32f4.c

+
+	SYS_LOG_INF("init adc%u", (unsigned int)config->adcDevNum);
+
+	switch(config->adcDevNum) {


You should have a space between the keyword and the opening parenthesis switch (. Same applies to if, for, etc. This is following the Linux coding style.

vaussard · 2018-07-03T07:16:49Z

drivers/adc/adc_stm32f4.c

+			// none
+			break;
+	}
+	if(GPIOport) HAL_GPIO_Init(GPIOport, &gpioCfg);


Why initializing the GPIO? This should not be needed for the ADC to work as we should multiplex the pin as analog. This removes most of the complexity of this function, not mentioning that it is not portable across families.

vaussard · 2018-07-03T07:19:15Z

drivers/adc/adc_stm32f4.c

+{
+	const struct adc_config *config = dev->config->config_info;
+
+	SYS_LOG_DBG("adc%u enable", (unsigned int)config->adcDevNum);


You should calibrate the ADC before using it with a call to HAL_ADCEx_Calibration_Start().

StefJar · 2018-07-05T17:41:18Z

Thanks for the feedback. I am on it.
In the meantime I changed the driver using the ADC interrupt. The communication via ISR and read function is done by a message queue. The OS polling on this is much better than the HAL timeout.
I will do some of the changes. The DTS combination seems to generate some extra work(because of lack of documentation how to do a driver and it's connection to other ones via dts)

…e reading for an interrupt in that a semaphore is set. No STM32HAL waiting - instead use of sem_take function.

StefJar · 2018-07-20T17:02:38Z

done some of the changes. Major change is the switch to isr based adc conversion.

Olivier-ProGlove · 2018-09-06T12:36:51Z

As @galak mentionned, ADC API (now merged into master) has significantly changed. This pull-request needs to be refactored on the new ADC API.

How specific ADC and UART drivers are to stm32f4? Should not they be specific to STM32?

erwango · 2018-10-19T14:11:26Z

@StefJar , any update?

StefJar · 2018-10-22T13:40:22Z

I "unmerged" my stm32 adc driver out of my zephyr kernel after the last major adc driver api changes.
You can close the merge request.

Stefan Jaritz added 2 commits June 28, 2018 17:16

added: smt32f4 dts support for uart1 using PA15 and PA10

41590f2

added: experimental stm32f4 adc driver

223d848

StefJar requested review from anangl and erwango as code owners June 28, 2018 16:24

galak added area: ADC Analog-to-Digital Converter (ADC) platform: STM32 ST Micro STM32 labels Jun 28, 2018

therealprof reviewed Jul 1, 2018

View reviewed changes

changed rx pin to "STM32_PUPDR_NO_PULL"

67acdaf

As mentioned, for rx no resistor is needed.

StefJar commented Jul 1, 2018

View reviewed changes

erwango requested changes Jul 2, 2018

View reviewed changes

vaussard suggested changes Jul 3, 2018

View reviewed changes

changed: irq based sample conversion. Reading adc now is waiting whil…

ea2f412

…e reading for an interrupt in that a semaphore is set. No STM32HAL waiting - instead use of sem_take function.

galak added the area: UART Universal Asynchronous Receiver-Transmitter label Sep 12, 2018

StefJar closed this Oct 22, 2018

erwango mentioned this pull request Oct 29, 2018

Add STM32 ADC driver #10896

Closed


		SYS_LOG_INF("init adc%u", (unsigned int)config->adcDevNum);

		switch(config->adcDevNum) {

stm32f4 uart and adc enhancement #8624

stm32f4 uart and adc enhancement #8624

Conversation

StefJar commented Jun 28, 2018

codecov-io commented Jun 28, 2018 • edited Loading

Codecov Report

galak commented Jun 28, 2018

erwango commented Jun 29, 2018

StefJar commented Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefJar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwango commented Jul 2, 2018

erwango left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaussard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefJar commented Jul 5, 2018

StefJar commented Jul 20, 2018

Olivier-ProGlove commented Sep 6, 2018

erwango commented Oct 19, 2018

StefJar commented Oct 22, 2018

codecov-io commented Jun 28, 2018 •

edited

Loading

StefJar commented Jun 29, 2018 •

edited

Loading

erwango left a comment •

edited

Loading