drivers: dac: API + STM32L0 driver implementation #21805

martinjaeger · 2020-01-09T21:19:02Z

Here is a suggestion to implement the DAC driver.

As it is the first time I'm implementing a driver for Zephyr, please be forgiving if something was not done the usual or correct way. Just let me know what can be improved.

I tested the driver with an STM32L073RZ Nucleo board. As soon as we agree on the API, i can also add additional STM32 MCUs. I've got a board with STM32G431 for testing aswell.

Unfortunately, I don't know very much about DAC features of other manufacturer's MCUs. I decided not to include special STM features like sawtooth wave generation into the generic API (not sure if that's a typical feature). Also DMA is currently not supported. The only configuration is whether a channel should be buffered or not.

Similar to the ADC driver, the resolution is defined for each write request rather than once per channel at the beginning.

zephyrbot · 2020-01-09T21:27:41Z

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

erwango

Thanks for this welcome addition. Seems like a pretty good start.
Can you add one more commit with the api only ?

martinjaeger · 2020-01-10T09:36:30Z

Can you add one more commit with the api only ?

Ok, makes sense. Just split the first commit.

Above checkpatch warning "TRAILING_SEMICOLON: macros should not use a trailing semicolon" is actually a false positive, as it will not compile with the trailing semicolon removed. Maybe the parser can't handle such complex defines? To whom should this be reported?

jfischer-no · 2020-01-10T10:17:23Z

Can you add one more commit with the api only ?

Ok, makes sense. Just split the first commit.

Above checkpatch warning "TRAILING_SEMICOLON: macros should not use a trailing semicolon" is actually a false positive, as it will not compile with the trailing semicolon removed. Maybe the parser can't handle such complex defines? To whom should this be reported?

Why is it false positive?

diff --git a/drivers/dac/dac_stm32.c b/drivers/dac/dac_stm32.c
index 42a3727c8a..c01b090818 100644
--- a/drivers/dac/dac_stm32.c
+++ b/drivers/dac/dac_stm32.c
@@ -134,21 +134,21 @@ static struct dac_stm32_data dac_stm32_data_##index = {           
        \
 DEVICE_AND_API_INIT(dac_##index, DT_DAC_##index##_NAME, &dac_stm32_init,\
                    &dac_stm32_data_##index, &dac_stm32_cfg_##index,    \
                    POST_KERNEL, CONFIG_KERNEL_INIT_PRIORITY_DEFAULT,   \
-                   &api_stm32_driver_api);                             \
+                   &api_stm32_driver_api)
 
 
 #ifdef CONFIG_DAC_1
-STM32_DAC_INIT(1)
+STM32_DAC_INIT(1);
 #endif /* CONFIG_DAC_1 */
 
 #ifdef CONFIG_DAC_2
-STM32_DAC_INIT(2)
+STM32_DAC_INIT(2);
 #endif /* CONFIG_DAC_2 */
 
 #ifdef CONFIG_DAC_3
-STM32_DAC_INIT(3)
+STM32_DAC_INIT(3);
 #endif /* CONFIG_DAC_3 */
 
 #ifdef CONFIG_DAC_4
-STM32_DAC_INIT(4)
+STM32_DAC_INIT(4);
 #endif /* CONFIG_DAC_4 */

btw, none of DAC are enabled in the sample.

martinjaeger · 2020-01-10T11:02:35Z

Why is it false positive?

Cool, didn't realize that possible fix. I took parts of this from ADC driver, but it has a } at the end of the define, so that's why checkpatch doesn't complain there.

Will wait for further comments and fix this in the next force-push.

btw, none of DAC are enabled in the sample.

Oops, you are right. I had DAC_1 configured as default y in drivers/dac/Kconfig.stm32 before, but removed it and didn't realize it's not enabled in the sample anymore. My .config had it enabled already.

Where should it be enabled ideally? In the sample's proj.conf or in the board Kconfig.defconfig if DAC is enabled?

jfischer-no · 2020-01-10T11:36:17Z

Where should it be enabled ideally? In the sample's proj.conf or in the board Kconfig.defconfig if DAC is enabled?

I think in the board Kconfig.defconfig if DAC is enabled. 👍

erwango

Few points otherwise LGTM.
Would be nice to have inputs from potential users.

soc/arm/st_stm32/stm32l0/dts_fixup.h

samples/drivers/dac/src/main.c

drivers/dac/dac_stm32.c

martinjaeger · 2020-01-16T08:45:18Z

So if everyone is fine with the current status, I could add support for some other STM32 MCUs before we merge.

erwango · 2020-01-16T10:44:12Z

So if everyone is fine with the current status, I could add support for some other STM32 MCUs before we merge.

@martinjaeger It will be easier to merge in current state with fewer commits. Let's get this one merged first (we're not there yet)

drivers/dac/Kconfig.stm32

erwango · 2020-04-15T16:34:21Z

soc/arm/st_stm32/stm32l0/Kconfig.defconfig.series

+if DAC
+
+config DAC_STM32
+	default y
+
+endif # DAC


Not needed if Kconfig configured thanks to dt_compat_enabled

DAC (digital to analog converter) peripheral driver with a generic API suitable for most MCUs (only basic DAC features considered). Signed-off-by: Martin Jäger <martin@libre.solar>

martinjaeger · 2020-04-17T08:53:24Z

Thanks @erwango and @henrikbrixandersen for your additional review. It's sometimes a bit difficult to keep track of all changes happening in the background, e.g. new DT macros and those changes in CMakeLists.txt. But I hope it's now all up-to-date and can finally be merged.

With the DT and Kconfig updates we could remove >100 lines of redundant code. Nice.

Changes decided in API meeting are also included @carlescufi.

henrikbrixandersen

Looks good!

erwango

Few more minor comments + missing alias to get loopback test compiling.

Also, I think you can squash "drivers: dac: Convert to new DT_INST macros" into "drivers: dac: Add support for STM32L0 series".

I don't have nucleo_l073rz, so I've tried to test on nucleo_l053r8 which share the same pinmux. Api test was working but loopback test failed (bridging Arduino A0/A2):

    Assertion failed at ../../src/test_dac.c:121: test_task_loopback: (m_sample)
Value 615 read from ADC does not match expected range.

Any clue why this is not working ?

drivers/dac/Kconfig.stm32

boards/arm/nucleo_l073rz/nucleo_l073rz.dts

drivers/dac/dac_stm32.c

martinjaeger · 2020-04-17T09:59:29Z

Also, I think you can squash "drivers: dac: Convert to new DT_INST macros" into "drivers: dac: Add support for STM32L0 series".

Done

I don't have nucleo_l073rz, so I've tried to test on nucleo_l053r8 which share the same pinmux. Api test was working but loopback test failed (bridging Arduino A0/A2):
    Assertion failed at ../../src/test_dac.c:121: test_task_loopback: (m_sample)
Value 615 read from ADC does not match expected range.     
Any clue why this is not working ?

For the Nucleo L073 you need to bridge A1 and A2.

But you were right regarding the compile error in the loopback test, of course. Fixed that and renamed the aliases to adc0 and dac0 as suggested. Both tests are compiling and passing now.

First implementation for STM32L0 series MCUs to be used for testing. Signed-off-by: Martin Jäger <martin@libre.solar>

Adds support for the new DAC driver to ST Nucleo board with STM32L073RZ MCU. Signed-off-by: Martin Jäger <martin@libre.solar>

erwango

Approved

For the Nucleo L073 you need to bridge A1 and A2.

Bridging A1/A2 work on nucleo_l053r8 works on my side as well.
But my eyes still tell me this should not work as ADC1 IN0 is set to PA0 which is A0 according to https://www.st.com/resource/en/user_manual/dm00105823-stm32-nucleo-64-boards-mb1136-stmicroelectronics.pdf.
Unless this doc is wrong and then arduino_r3_connector.dtsi should be fixed for these 2 boards.

martinjaeger · 2020-04-17T10:17:48Z

Bridging A1/A2 work on nucleo_l053r8 works on my side as well.
But my eyes still tell me this should not work as ADC1 IN0 is set to PA0 which is A0 according to https://www.st.com/resource/en/user_manual/dm00105823-stm32-nucleo-64-boards-mb1136-stmicroelectronics.pdf.
Unless this doc is wrong and then arduino_r3_connector.dtsi should be fixed for these 2 boards.

We are reading ADC1_IN1 at PA1 (see tests/drivers/dac/dac_loopback/src/test_dac.c#L41):

#define ADC_DEVICE_NAME		DT_LABEL(DT_ALIAS(adc1))
#define ADC_CHANNEL_ID		1

ADC channels also start counting at 0 in the driver.

martinjaeger · 2020-04-17T11:19:57Z

Can someone please explain what's the issue with Shippable? From the build log I don't understand what's wrong exactly or if it is at all caused by the changes introduced in this PR.

erwango · 2020-04-17T11:32:41Z

Can someone please explain what's the issue with Shippable? From the build log I don't understand what's wrong exactly or if it is at all caused by the changes introduced in this PR.

I don't think it is linked. QEMU testing happens to be unstable. You can try to close/reopen the PR to launch a new shippable run and see if it gets fixed.

erwango · 2020-04-17T11:57:05Z

tests/drivers/dac/dac_loopback/boards/nucleo_l073rz.conf

@@ -0,0 +1 @@
+CONFIG_ADC_1=y


I missed this one. Driver is now fully device tree configured, this line is not needed

Ok, removed it. Gives us another chance to pass Shippable :)

and it passed \o/

This example generates a sawtooth signal to show how to use the DAC. Signed-off-by: Martin Jäger <martin@libre.solar>

Test runs with nucleo_l073rz board. Signed-off-by: Martin Jäger <martin@libre.solar>

The loopback test reads back the DAC output with the ADC and checks that it is in a narrow range. Signed-off-by: Martin Jäger <martin@libre.solar>

carlescufi · 2020-04-20T15:42:25Z

@martinjaeger thank you very much for this contribution and for your patience driving it through.

martinjaeger requested review from carlescufi, dbkinder, erwango, galak, MaureenHelm, nashif and rsalveti as code owners January 9, 2020 21:19

zephyrbot added area: Devicetree area: Boards area: API Changes to public APIs area: Samples Samples labels Jan 9, 2020

martinjaeger force-pushed the dac_support branch 2 times, most recently from c6b0d5e to a88d153 Compare January 10, 2020 06:46

erwango added the RFC Request For Comments: want input from the community label Jan 10, 2020

erwango requested changes Jan 10, 2020

View reviewed changes

martinjaeger force-pushed the dac_support branch from a88d153 to dd606dd Compare January 10, 2020 09:31

martinjaeger force-pushed the dac_support branch from dd606dd to 3667979 Compare January 10, 2020 11:48

martinjaeger requested a review from erwango January 11, 2020 08:22

erwango requested changes Jan 13, 2020

View reviewed changes

soc/arm/st_stm32/stm32l0/dts_fixup.h Outdated Show resolved Hide resolved

samples/drivers/dac/src/main.c Outdated Show resolved Hide resolved

drivers/dac/dac_stm32.c Outdated Show resolved Hide resolved

dbkinder removed their request for review January 14, 2020 22:07

martinjaeger force-pushed the dac_support branch from 3667979 to b7772af Compare January 16, 2020 08:23

martinjaeger requested a review from erwango January 16, 2020 08:26

martinjaeger force-pushed the dac_support branch from b7772af to 0cbf15d Compare January 16, 2020 08:39

erwango requested changes Apr 15, 2020

View reviewed changes

drivers: dac: Add API for DAC peripherals

b4e0c3d

DAC (digital to analog converter) peripheral driver with a generic API suitable for most MCUs (only basic DAC features considered). Signed-off-by: Martin Jäger <martin@libre.solar>

martinjaeger force-pushed the dac_support branch from 00e7be3 to 314576a Compare April 17, 2020 08:30

martinjaeger requested review from erwango and henrikbrixandersen April 17, 2020 08:30

martinjaeger force-pushed the dac_support branch 2 times, most recently from 902e2e5 to 9c5e545 Compare April 17, 2020 08:45

henrikbrixandersen approved these changes Apr 17, 2020

View reviewed changes

erwango requested changes Apr 17, 2020

View reviewed changes

martinjaeger force-pushed the dac_support branch from 9c5e545 to 52fe265 Compare April 17, 2020 09:51

martinjaeger added 2 commits April 17, 2020 12:00

drivers: dac: Add support for STM32L0 series

7637ba9

First implementation for STM32L0 series MCUs to be used for testing. Signed-off-by: Martin Jäger <martin@libre.solar>

boards: nucleo_l073rz: Add DAC peripheral driver

a74ba88

Adds support for the new DAC driver to ST Nucleo board with STM32L073RZ MCU. Signed-off-by: Martin Jäger <martin@libre.solar>

martinjaeger force-pushed the dac_support branch from 52fe265 to 4313aac Compare April 17, 2020 10:02

martinjaeger requested a review from erwango April 17, 2020 10:03

erwango approved these changes Apr 17, 2020

View reviewed changes

erwango reviewed Apr 17, 2020

View reviewed changes

martinjaeger added 3 commits April 17, 2020 14:22

samples: drivers: dac: Add API example

e6ead6c

This example generates a sawtooth signal to show how to use the DAC. Signed-off-by: Martin Jäger <martin@libre.solar>

tests: drivers: dac: Add write_value API test

f12fe60

Test runs with nucleo_l073rz board. Signed-off-by: Martin Jäger <martin@libre.solar>

tests: drivers: dac: Add loopback test with ADC

6a7d853

The loopback test reads back the DAC output with the ADC and checks that it is in a narrow range. Signed-off-by: Martin Jäger <martin@libre.solar>

martinjaeger force-pushed the dac_support branch from 4313aac to 6a7d853 Compare April 17, 2020 12:24

carlescufi merged commit 5c9e414 into zephyrproject-rtos:master Apr 20, 2020

martinjaeger mentioned this pull request Jul 28, 2020

DAC features and API enhancements #27191

Open

martinjaeger deleted the dac_support branch October 14, 2020 12:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: dac: API + STM32L0 driver implementation #21805

drivers: dac: API + STM32L0 driver implementation #21805

martinjaeger commented Jan 9, 2020 •

edited by carlescufi

Loading

zephyrbot commented Jan 9, 2020 •

edited

Loading

erwango left a comment •

edited

Loading

martinjaeger commented Jan 10, 2020 •

edited

Loading

jfischer-no commented Jan 10, 2020

martinjaeger commented Jan 10, 2020

jfischer-no commented Jan 10, 2020

erwango left a comment

martinjaeger commented Jan 16, 2020

erwango commented Jan 16, 2020 •

edited

Loading

erwango Apr 15, 2020

martinjaeger commented Apr 17, 2020

henrikbrixandersen left a comment

erwango left a comment

martinjaeger commented Apr 17, 2020

erwango left a comment

martinjaeger commented Apr 17, 2020

martinjaeger commented Apr 17, 2020

erwango commented Apr 17, 2020

erwango Apr 17, 2020

martinjaeger Apr 17, 2020

erwango Apr 17, 2020

carlescufi commented Apr 20, 2020

		@@ -0,0 +1 @@
		CONFIG_ADC_1=y

drivers: dac: API + STM32L0 driver implementation #21805

drivers: dac: API + STM32L0 driver implementation #21805

Conversation

martinjaeger commented Jan 9, 2020 • edited by carlescufi Loading

zephyrbot commented Jan 9, 2020 • edited Loading

erwango left a comment • edited Loading

Choose a reason for hiding this comment

martinjaeger commented Jan 10, 2020 • edited Loading

jfischer-no commented Jan 10, 2020

martinjaeger commented Jan 10, 2020

jfischer-no commented Jan 10, 2020

erwango left a comment

Choose a reason for hiding this comment

martinjaeger commented Jan 16, 2020

erwango commented Jan 16, 2020 • edited Loading

erwango Apr 15, 2020

Choose a reason for hiding this comment

martinjaeger commented Apr 17, 2020

henrikbrixandersen left a comment

Choose a reason for hiding this comment

erwango left a comment

Choose a reason for hiding this comment

martinjaeger commented Apr 17, 2020

erwango left a comment

Choose a reason for hiding this comment

martinjaeger commented Apr 17, 2020

martinjaeger commented Apr 17, 2020

erwango commented Apr 17, 2020

erwango Apr 17, 2020

Choose a reason for hiding this comment

martinjaeger Apr 17, 2020

Choose a reason for hiding this comment

erwango Apr 17, 2020

Choose a reason for hiding this comment

carlescufi commented Apr 20, 2020

martinjaeger commented Jan 9, 2020 •

edited by carlescufi

Loading

zephyrbot commented Jan 9, 2020 •

edited

Loading

erwango left a comment •

edited

Loading

martinjaeger commented Jan 10, 2020 •

edited

Loading

erwango commented Jan 16, 2020 •

edited

Loading