Basic DAC Driver, implementation on stm32f4discovery #2018

brummer-simon · 2014-11-16T21:45:31Z

Creation of DAC Driver, much like the current ADC Driver.
Implementation of the DAC Driver for the stm32f4discovery board.

dac_init(dac_t dev, dac_precision_t precision)
Initializing a DAC Device with a given precision, configures the Device and Channels.

dac_write(dac_t dev, uint8_t channel, uint16_t value)
Writes a value on a specific channel for a device.

dac_poweron(dac_t dev)
Enables a given DAC Device.

dac_poweroff(dac_t dev)
Disables a given DAC Device.

uint16_t dac_map(dac_t dev, int value, int min, int max)
Maps a integer value between interval min and max, to a valid integer to write onto the dac with respect to the selected precision.

uint16_t dac_mapf(dac_t dev, float value, float min, float max)
Maps a float value between interval min and max, to a valid integer to write onto the dac with respect to the selected precision.

PeterKietzmann · 2014-11-17T07:00:13Z

cpu/stm32f4/periph/dac.c

@@ -0,0 +1,149 @@
+/*
+ * Copyright (C) 2014 Freie Universität Berlin
+ *


I think it's more your copyright or maybe HAWs.

PeterKietzmann · 2014-11-17T07:11:54Z

Wow that PR came quick! Looks nice, I'll look and test in more detail later.

haukepetersen · 2014-11-17T17:11:05Z

boards/stm32f4discovery/include/periph_conf.h

+#define DAC_0_PORT_CLKEN()   (RCC->AHB1ENR |= RCC_AHB1ENR_GPIOAEN)     
+/* DAC 0 channel config */
+#define DAC_0_CH0_PIN        4
+#define DAC_0_CH1_PIN        5


We need to careful with these pins, so far they are configured for ADC (PA4) and SPI SCK (PA6). Have a look at this table and check if you can find other pins. If not we need to see if we can re-arrange the pin mapping for the f4discovery board

According to the stm32f4-family Reference Manual(Page 431)

http://www.st.com/web/en/resource/technical/document/reference_manual/DM00031020.pdf

Are the two DAC Channals hardwired to PortA Pin4 and 5. Not much possibility to choose anything else.

haukepetersen · 2014-11-17T17:17:41Z

Congrats to you first PR! Besides some minor newline issues it looks very good! We just have to think about the pin mapping. So far the strategy was, not to map pins twice...

PeterKietzmann · 2014-11-18T07:17:47Z

PA4 and PA5 are the only pins for the DAC. I suggest to:

Map ADC_0_CH1_PIN to PA0
Port SPI_0_* pins to B3-B5
Should we do this in a separate PR? I could do so...

PeterKietzmann · 2014-11-18T07:29:36Z

For completeness you should add periph_dac in RIOT/boards/stm32f4discovery/Makefile.features

brummer-simon · 2014-11-18T09:00:38Z

I added periph_dac. Would be great if you change ADC_0_CH1_PIN and SPI_0_*, because I can't test the results here(I got no SPI Device here).

haukepetersen · 2014-11-18T12:23:35Z

Ok, again some funny design by ST... For this case I would actually leave the pin config for SPI_1 and ADC_0 as it is. But I would suggest that we think of some kind of warning or similar that we can output, so that users are warned about pins that are assigned to more then 1 peripheral... Any ideas?

PeterKietzmann · 2014-11-18T12:47:12Z

Ok maybe these ideas are totally stupid and "tinkering" but just to have a start of discussion:

The user has to set the define in the periph_conf for the appropriate peripheral to zero if he does not want to use it. In some board-specific code (don't know where exactly) we check if ADC_0 and SPI_0 are enabled at the same time and throw a warning in this case.
If any application has the FEATURES_REQUIRED = periph_spi periph_dac and this code is compiled for e.g. stm32f4discovery board, we could maybe throw an error or warning in any (don't know where exactly) od RIOTs makefiles.

PeterKietzmann · 2014-11-19T11:09:37Z

@haukepetersen, how du you like this DAC interface? I think it's quite clear and in the style of your ADC interface. Just the return tyoes are a bit different.

PeterKietzmann · 2014-11-19T11:26:38Z

cpu/stm32f4/periph/dac.c

+int8_t dac_write(dac_t dev, uint8_t channel, uint16_t value) 
+{
+    DAC_TypeDef* dac = 0;
+    uint16_t val = 0;


Travis is unhappy:

cpu/stm32f4/periph/dac.c:86: style (unreadVariable): Variable 'dac' is assigned a value that is never used. cpu/stm32f4/periph/dac.c:87: style (unreadVariable): Variable 'val' is assigned a value that is never used.

Maybe you could try to just initialize them without assigning a value?

I'll try that to shutup Travis but i am sure thats considered bad style in c.

You could simply guard the file with #if DAC_0_EN instead of #if DAC_NUMOF

haukepetersen · 2014-11-20T12:54:15Z

The interface is fine. I miss one thing in the return values (and in the implementation): The given dac_t dev should always be checked if it exists, and I would return always -1 if the device does not exists. This would be consistent with (most of) our other interfaces...

PeterKietzmann · 2014-11-20T13:20:35Z

cpu/stm32f4/periph/dac.c

+            DAC_0_PORT->MODER |=  (3 << (DAC_0_CH0_PIN * 2) | 3 << (DAC_0_CH1_PIN * 2));
+            DAC_0_PORT->PUPDR &= ~(3 << (DAC_0_CH0_PIN * 2) | 3 << (DAC_0_CH1_PIN * 2));
+            break;
+#endif


add this like @haukepetersen commented:

default: return -1;

same for the other functions...

Changed return values and documentation

BytesGalore · 2014-11-21T08:42:16Z

Hi @brummer-simon, seems that you merged the master on-top of this branch.
That's the reason why you had like a ton of conflicts to solve.
(the same happend to me alot as I started using git)

To rebase on the current master you first need to update your local master branch:

switch to your master branch
apply git pull --rebase https://github.com/RIOT-OS/RIOT.git
push the rebased master (just git push)

This way you have your master updated to the same state as the RIOT master.

Now rebase your branch, i.e. devel-dac_driver on the "fresh" master:

switch to this branch
apply git rebase master
solve conflicts if happen
and if everything is done apply git push -f
note: if not using the -f (force) option git will complain your local branch diverges with your remote branch, and suggests you to first pull the changes (which would be not what you want)

To resolve your situation:

rebase your master as described above
change to this branch and go back to the commit before merging the master, which would be:
git reset --hard 6d785ed10e4c292e3d3d4af8528c4a7c0dbd9390
this is the sha of your last commit before merge.
now proceed with rebasing this branch on your "fresh" master.

I hope this helps a bit.

brummer-simon · 2014-11-21T12:13:46Z

Okay I rebased according to the upper comment.
Is there a Wikientry or something, that explains what exacly i did there, because I wanna know.

BytesGalore · 2014-11-21T12:33:09Z

@brummer-simon we have a git cheatsheet where we included a howto [1] for rebasing lately (literally today).
For detailed information [2] could be interesting. It is a comprehensive explanation on git procedures, and its chapter on branching [3] might give you detailed insights on what happend and how you resolved it.

[1] https://github.com/RIOT-OS/RIOT/wiki/Git-cheatsheet#how-to-rebase-your-master-on-a-current-riot-master
[2] http://git-scm.com/book/en/v2/
[3] http://git-scm.com/book/en/v2/Git-Branching-Rebasing

brummer-simon · 2014-11-24T08:41:29Z

Finally the Travis CI build went well. As far as is see the only open Issue is the conflicting Pin configration. Any Ideas on this topic?

PeterKietzmann · 2014-11-24T13:49:57Z

#2076 looks like a compromise for the pin-problem

haukepetersen · 2014-11-25T18:20:23Z

boards/stm32f4discovery/Makefile.features

@@ -1,2 +1 @@
-FEATURES_PROVIDED += periph_gpio periph_spi periph_i2c periph_pwm periph_random periph_adc cpp
-
+FEATURES_PROVIDED += periph_gpio periph_spi periph_pwm periph_random periph_adc periph_dac cpp


where did the i2c go?

haukepetersen · 2014-11-25T18:21:36Z

besides one question on the changed feature list I think we can merge this. If you could address this I give my ack

PeterKietzmann · 2014-11-25T18:29:57Z

Ok after this PR we can have a look at #2023 :-)

PeterKietzmann · 2014-11-25T18:33:24Z

Besides of the "pin-question" I'll give my ACK too but I think you should squash the commits

N8Fear · 2014-11-25T19:29:14Z

From the commit messages both commits actually do more or less the same: is there a reason not to squash further?

brummer-simon · 2014-11-25T19:31:00Z

Interactive Mode doens't let me squash further.

N8Fear · 2014-11-25T19:33:15Z

Try git rebase -i <master>/master ~~last commit on master - not part of this PR~~ - that should work (master is the main RIOT repo).

Edit: I had Brainlag - either the one above or git rebase -i <last commit not part of this PR>

brummer-simon · 2014-11-25T21:52:47Z

I managed to squash it to one commit after fiddeling around with git

PeterKietzmann · 2014-11-26T09:24:40Z

I'm happy that Travis is happy, finally! ACK ACK. Can someone please merge this and maybe also PR #2023 then?!

haukepetersen · 2014-11-26T16:52:35Z

and go.

Basic DAC Driver, implementation on stm32f4discovery

PeterKietzmann reviewed Nov 17, 2014
View reviewed changes

haukepetersen reviewed Nov 17, 2014
View reviewed changes

PeterKietzmann mentioned this pull request Nov 18, 2014

stm32f4discovery/periph_conf: Changing pin configuration #2022

Closed

OlegHahm assigned PeterKietzmann Nov 18, 2014

OlegHahm added Area: drivers Area: Device drivers Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Nov 18, 2014

PeterKietzmann mentioned this pull request Nov 18, 2014

test/periph_dac: adds a test for DACs #2023

Merged

PeterKietzmann reviewed Nov 19, 2014
View reviewed changes

PeterKietzmann reviewed Nov 20, 2014
View reviewed changes

haukepetersen mentioned this pull request Nov 21, 2014

RIOT port for the MKW22D512 SiP and Phytec PBA-D-01 PhyWave Evaluations-Board #2059

Merged

brummer-simon force-pushed the devel-dac_driver branch 2 times, most recently from 33a25a3 to 6d785ed Compare November 21, 2014 12:07

brummer-simon force-pushed the devel-dac_driver branch from 6d785ed to 9e993a1 Compare November 21, 2014 12:21

PeterKietzmann mentioned this pull request Nov 24, 2014

RIOT/Makefile.include: added indicating of possibe conflicting features on compile #2076

Merged

haukepetersen reviewed Nov 25, 2014
View reviewed changes

brummer-simon force-pushed the devel-dac_driver branch 4 times, most recently from 7decfee to 7296a87 Compare November 25, 2014 19:21

PeterKietzmann mentioned this pull request Nov 25, 2014

boards/stm32f4discovery: fix typo in comment for section #2088

Merged

Basic DAC Driver, implementation on stm32f4discovery

5a4b966

brummer-simon force-pushed the devel-dac_driver branch from 7296a87 to 5a4b966 Compare November 25, 2014 21:41

haukepetersen added a commit that referenced this pull request Nov 26, 2014

Merge pull request #2018 from brummer-simon/devel-dac_driver

f1fda21

Basic DAC Driver, implementation on stm32f4discovery

haukepetersen merged commit f1fda21 into RIOT-OS:master Nov 26, 2014

brummer-simon deleted the devel-dac_driver branch February 23, 2015 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic DAC Driver, implementation on stm32f4discovery #2018

Basic DAC Driver, implementation on stm32f4discovery #2018

brummer-simon commented Nov 16, 2014

PeterKietzmann Nov 17, 2014

PeterKietzmann commented Nov 17, 2014

haukepetersen Nov 17, 2014

brummer-simon Nov 17, 2014

haukepetersen commented Nov 17, 2014

PeterKietzmann commented Nov 18, 2014

PeterKietzmann commented Nov 18, 2014

brummer-simon commented Nov 18, 2014

haukepetersen commented Nov 18, 2014

PeterKietzmann commented Nov 18, 2014

PeterKietzmann commented Nov 19, 2014

PeterKietzmann Nov 19, 2014

brummer-simon Nov 19, 2014

N8Fear Nov 19, 2014

haukepetersen commented Nov 20, 2014

PeterKietzmann Nov 20, 2014

PeterKietzmann Nov 20, 2014

brummer-simon Nov 20, 2014

BytesGalore commented Nov 21, 2014

brummer-simon commented Nov 21, 2014

BytesGalore commented Nov 21, 2014

brummer-simon commented Nov 24, 2014

PeterKietzmann commented Nov 24, 2014

haukepetersen Nov 25, 2014

haukepetersen commented Nov 25, 2014

PeterKietzmann commented Nov 25, 2014

PeterKietzmann commented Nov 25, 2014

N8Fear commented Nov 25, 2014

brummer-simon commented Nov 25, 2014

N8Fear commented Nov 25, 2014

brummer-simon commented Nov 25, 2014

PeterKietzmann commented Nov 26, 2014

haukepetersen commented Nov 26, 2014

		@@ -1,2 +1 @@
		FEATURES_PROVIDED += periph_gpio periph_spi periph_i2c periph_pwm periph_random periph_adc cpp

		FEATURES_PROVIDED += periph_gpio periph_spi periph_pwm periph_random periph_adc periph_dac cpp

Basic DAC Driver, implementation on stm32f4discovery #2018

Basic DAC Driver, implementation on stm32f4discovery #2018

Conversation

brummer-simon commented Nov 16, 2014

Choose a reason for hiding this comment

PeterKietzmann commented Nov 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen commented Nov 17, 2014

PeterKietzmann commented Nov 18, 2014

PeterKietzmann commented Nov 18, 2014

brummer-simon commented Nov 18, 2014

haukepetersen commented Nov 18, 2014

PeterKietzmann commented Nov 18, 2014

PeterKietzmann commented Nov 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen commented Nov 20, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BytesGalore commented Nov 21, 2014

brummer-simon commented Nov 21, 2014

BytesGalore commented Nov 21, 2014

brummer-simon commented Nov 24, 2014

PeterKietzmann commented Nov 24, 2014

Choose a reason for hiding this comment

haukepetersen commented Nov 25, 2014

PeterKietzmann commented Nov 25, 2014

PeterKietzmann commented Nov 25, 2014

N8Fear commented Nov 25, 2014

brummer-simon commented Nov 25, 2014

N8Fear commented Nov 25, 2014

brummer-simon commented Nov 25, 2014

PeterKietzmann commented Nov 26, 2014

haukepetersen commented Nov 26, 2014