Discussion:
Re: Multiple ADC channels
(too old to reply)
max.kriegleder@yahoo.com [nuttx]
2015-07-23 11:57:39 UTC
Permalink
What would it take to implement the DMA feature (I have not dealt with DMA in the nuttx context yet)? Also, Greg, I think you mentioned in another thread that several people have done this before but unfortunately did not commit the changes upstream. Any chance that the code is open-source somewhere?

Anyways, I am also interested in getting a proper multi-channel ADC running.

Thanks,
Max
spudarnia@yahoo.com [nuttx]
2015-07-23 13:32:29 UTC
Permalink
Hi, Max,

Actually, the job should be easy. Since that has been an issue plaguing STM32 for some time, let me give you a very detailed response.

It should be easy because DMA is highly modularized and you really do not have to write any low-level code at all; just plug the DMA calls into the drivers. The DMA module interface is defined in arch/arm/src/stm32/stm32_dma.h.

The only other important file is the DMA header file in chip/ sub-directory. Let's say for discussion that you are using an STM32 F429, then that would be chip/stm32f42xxx_dma.h. The only thing that is important in those files are the DMA stream definitions at the bottom of that file:

$ grep -n ADC chip/stm32f42xxx_dma.h
478:#define DMAMAP_ADC1_1 STM32_DMA_MAP(DMA2,DMA_STREAM0,DMA_CHAN0)
482:#define DMAMAP_ADC1_2 STM32_DMA_MAP(DMA2,DMA_STREAM4,DMA_CHAN0)
488:#define DMAMAP_ADC2_1 STM32_DMA_MAP(DMA2,DMA_STREAM2,DMA_CHAN1)
489:#define DMAMAP_ADC2_2 STM32_DMA_MAP(DMA2,DMA_STREAM3,DMA_CHAN1)
494:#define DMAMAP_ADC3_1 STM32_DMA_MAP(DMA2,DMA_STREAM0,DMA_CHAN2)
495:#define DMAMAP_ADC3_2 STM32_DMA_MAP(DMA2,DMA_STREAM1,DMA_CHAN2)

Notice that there are multiple options for each DMA stream. You have to disambiguate that stream numbers in your board.h header file, just as you have to do for pin multiplexing options (the F429 refers to these as streams, but a lot of the older F1 channel terminology is used).

Notice also that all ADC DMA is on DMA2 which you will have to enable in the configuration.

I see that the DAC driver, stm32_dac.c, supports DMA. The direction is in the wrong direction and, as a write only device, it is much simpler, but should be a good example for DMA.

Here is a step by step:

1. Add the DMA stream disambiguation to to the board.h header file.

2. Add an option to the STM32/Kconfig file to enable per ADC DMA operations. This option should depend on DMA2 being enabled. Let's say CONFIG_STM32_ADC_DMA.

Everything else would be done in stm32_adc.c:

3. Add two new fields to struct stm32_dev_s (both conditioned on CONFIG_STM32_ADC_DMA).

- A uint8_t field to hold the DMA stream number
- A DMA_HANDLE type stream handle
- Whatever else you need to manage the DMA

See line 337 of stm32_dac.c. These are the dmachan and dma fields in stm32_chan_s.

4. Add an initializer for the DMA stream number in g_adcpriv1, g_adcpriv2, and g_adcpriv3.

See line 394 of stm32_dac.c.

5. In stm32_adcinitialize(), call stm32_dmachannel() to allocate a DMA stream and save it in the device structure.

See line 965 of stm32_dac.c

6. Then when ADC is enabled, call stm32_dmasetup() using the handle, the address of the ADC data register, and the address in memory to receive the DMA data, the number of transfers, and the value of the DMA CCR register to use.

See line 713 of stm32_dac.c

This last part is the trickiest, CCR was an F1 DMA register but the name has stuck. For the F4, it is the SCR register. You will have to look at the chip/stm32f42xxx_dma.h file for SCR register settings and also look at other RX transfer examples in other C files (grep for SCR in stm32/ and you will see many). It is just a magic number that will require some thought.

7. Then call stm32_dmastart(). You have to provide a callback to this function. When the DMA completes, you will get a callback from the DMA completion interrupt handler with a code to tell you the DMA completed successfully.

See line 718 and line 662 of stm32_dac.c

The DAC version of the DMA callback does not anything. You will probably need logic to buffer the the ADC data that you read and to wake up any application waiting for the availability of ADC data.

So that is it: On new configuration setting, on new handle variable, on new function, and three new function calls.

Greg
max.kriegleder@yahoo.com [nuttx]
2015-07-28 10:09:10 UTC
Permalink
Hi Greg,

I have looked into this and I think I figured most of it out. However, I believe that in combination with the existing upper-half driver, I will not get the desired behavior - maybe you can give me some input here.

Let's say I have two ADC devices at two different channels and I would like to read their values at given time instances (one after the other). Generally, I understand how this can be accomplished with DMA and I could configure the ADC to send me an interrupt either after each conversion or after the full sequence of both channels has been converted. In both cases I could grab the data from the DMA buffer and hand it to the upper-half driver with adc_receive.

But here is my problem with the upper-half - when I call read I would like to get the current value of the ADC, so either call read - convert one channel - provide data to read - reconfigure the ADC - call read - convert the other channel - provide data to read or call read - convert one channel, convert the other channel - provide data.

In the current implementation, read wakes up whenever adc_receive is called from the lower-half and thus if multiple channels are read, the data can be quite outdated. I feel like the upper-half read should trigger an ADC conversion (or a sequence of conversion for multiple channels) - I know I can just implement something like this for my own purposes, but my question is would this go against the POSIX/NuttX way of things?

An aside (to help me better understand NuttX): In the upper-half driver I see

return_with_irqdisabled:
irqrestore(flags);

Doesn't irqrestore re-enable interrupts? So, shouldn't the label read return_with_irqenabled?

Max
spudarnia@yahoo.com [nuttx]
2015-07-28 13:46:35 UTC
Permalink
spudarnia@yahoo.com [nuttx]
2015-07-28 13:53:08 UTC
Permalink
Hi, Max,
Post by ***@yahoo.com [nuttx]
Let's say I have two ADC devices at two different channels and I would like to read their values at given time instances (one after the other). Generally, I understand how this can be accomplished with DMA and I could configure the ADC to send me an interrupt either after each conversion or after the full sequence of both channels has been converted. In both cases I could grab the data from the DMA buffer and hand it to the upper-half driver with adc_receive.
But here is my problem with the upper-half - when I call read I would like to get the current value of the ADC, so either call read - convert one channel - provide data to read - reconfigure the ADC - call read - convert the other channel - provide data to read or call read - convert one channel, convert the other channel - provide data.
Open the driver non-blocking. You can flush only data by reading and discarding. If you are using a manual trigger, nothing should ever be buffered unless you ask for it with the ANIOC_TRIGGER ioctl all. See apps/example/adc/adc.c.

I don't see any problem. I believe that you have all of the tools that you need. I have even used apps/examples/adc with an STM32 and multiple channels. It works great (other than the data overrun).
Post by ***@yahoo.com [nuttx]
In the current implementation, read wakes up whenever adc_receive is called from the lower-half and thus if multiple channels are read, the data can be quite outdated. I feel like the upper-half read should trigger an ADC conversion (or a sequence of conversion for multiple channels) - I know I can just implement something like this for my own purposes, but my question is would this go against the POSIX/NuttX way of things?
I don't believe that the data can outdated. I think you don't understand the software triggered usage model. Look at apps/examples/adc. It works fine.
Post by ***@yahoo.com [nuttx]
An aside (to help me better understand NuttX): In the upper-half driver I see
irqrestore(flags);
Doesn't irqrestore re-enable interrupts? So, shouldn't the label read return_with_irqenabled?
No, you are thinking about it from a different point of view. The goto looks like:

goto return_with_irqdisable;

Meaning to under under the condition where the IRQ is disabled -- meaning that you should re-enable interrupts before returning. Not that you should disable interrupts again. All of the labels follow that naming convention: The condition at the time for the error, not what to do before returning.

So that is correct and won't be changed.

I don't think there is any problem in anything here.
Greg
max.kriegleder@yahoo.com [nuttx]
2015-07-28 14:56:53 UTC
Permalink
Hi Greg,

Thank you for the clarification regarding the goto - I was indeed reading it literally and that's why it confused me that IRQs were actually re-enabled (as they should be).

Regarding the ADC, I definitely have all the tools available (especially thanks to your previous detailed answer). I just feel like that the upper-half driver in the current implementation would deliver outdated data either way (and it is likely that I am just missing something). Ideally, I would like to see the flaw in my thinking and not have to tamper with the upper-half part. Here is my reasoning:

Let's say I am using the software trigger and the read in non blocking mode, trying to read two channels. So, only if there is a correct delay between the trigger and the read, read will return with exactly the result of the two channels. If the read is issued immediately after the trigger, most likely read will return with no data.

If on the other hand I am using the software trigger and the read in blocking mode, and the read is issued immediately after the trigger, read will return most likely immediately after the first channel has been converted.

So in either case, I believe if there is not a correct amount of delay between ioctl and read, read will either return with no data/ not all data / or outdated data.

Max
spudarnia@yahoo.com [nuttx]
2015-07-28 15:56:50 UTC
Permalink
Post by ***@yahoo.com [nuttx]
Let's say I am using the software trigger and the read in non blocking mode, trying to read two channels. So, only if there is a correct delay between the trigger and the read, read will return with exactly the result of the two channels. If the read is issued immediately after the trigger, most likely read will return with no data.
A normal blocking read will not return until the data is available -- i.e., until the ADC DMA completes. Then it will return immediately with all of the data from all of the channels. It is immediate and complete and cannot fail because of all of the handshaking.


If you use non-blocking read, then yes, it can return with no data. But that is the only case, otherwise it will return with the fresh data immediately after the DMA completes.


You could also you poll which will wait asynchronously until the DMA complete.
Post by ***@yahoo.com [nuttx]
If on the other hand I am using the software trigger and the read in blocking mode, and the read is issued immediately after the trigger, read will return most likely immediately after the first channel has been converted.
No, the read will not complete until the DMA is finished. All of the channel samples will be available at the same time.
Post by ***@yahoo.com [nuttx]
So in either case, I believe if there is not a correct amount of delay between ioctl and read, read will either return with no data/ not all data / or outdated data.
I don't agree.


Greg
max.kriegleder@yahoo.com [nuttx]
2015-07-28 22:30:00 UTC
Permalink
Hi Greg,

I focused first on the DMA part before worrying about the upper-half driver and how to get all the DMA data into it at once. I have followed the steps you described and it works in principle, but now I am stuck.

To my understanding dmasetup and dmastart should be called for every sequence of conversions (at least this is how I read it from dac_send, which I use as a reference). So I added the two functions to adc_startconv, which is called before each read from the upper-half through ioctl. After the second conversion, the code gets stuck in stm32f40xxx_dma:625, so it appears the transfer does never finish. The callback set in dmastart is also never triggered, however the DMA transfer seems to have worked at least once, because the memory allocated to the DMA has changed to reasonable values.

Here the debug output:
nsh> adc -n 2
adc_main: Initializing external ADC device
stm32_adcinitialize: intf: 1 nchannels: 2
stm32_adcinitialize: ADC1 Selected
adc_main: g_adcstate.count: 2
adc_main: Hardware initialized. Opening the ADC device: /dev/adc0
adc_reset: intf: ADC1
adc_enable: enable: 1
adc_startconv: enable: 1
stm32_dmasetup: paddr: 4001204c maddr: 200003d8 ntransfers: 2 scr: 00002c00
stm32_dmacapable: stm32_dmacapable: 0x200003d8/2 0x00002c00
stm32_dmacapable: stm32_dmacapable: transfer OK
adc_reset: SR: 0x00000010 CR1: 0x008001e8 CR2: 0x10000101
adc_reset: SQR1: 0x00100000 SQR2: 0x00000000 SQR3: 0x00000128
adc_reset: CCR: 0x00000000
adc_setup: Enable the ADC interrupt: irq=34
adc_rxint: intf: 1 enable: 1
adc_startconv: enable: 1
stm32_dmasetup: paddr: 4001204c maddr: 200003d8 ntransfers: 2 scr: 00002c00
stm32_dmacapable: stm32_dmacapable: 0x200003d8/2 0x00002c00
stm32_dmacapable: stm32_dmacapable: transfer OK
adc_read: buflen: 20
adc_read: Returning: 5
Sample:
1: channel: 8 value: 3945
adc_startconv: enable: 1
stm32_dmasetup: paddr: 4001204c maddr: 200003d8 ntransfers: 2 scr: 00002c00
stm32_dmacapable: stm32_dmacapable: 0x200003d8/2 0x00002c00
stm32_dmacapable: stm32_dmacapable: transfer OK

This is the code I added to adc_startconv:
uint32_t ccr =
DMA_SCR_MSIZE_16BITS | /* Memory size */
DMA_SCR_PSIZE_16BITS | /* Peripheral size */
DMA_SCR_MINC | /* Memory increment mode */
DMA_SCR_DIR_P2M; /* Read from peripheral */

stm32_dmasetup(priv->dma,
STM32_ADC1_DR,
(uint32_t)priv->dmabuffer,
priv->nchannels,
ccr);

stm32_dmastart(priv->dma, callback, priv, false);

Do you have any comment, where I could be making a mistake?

Max
'Juha Niskanen (Haltian)' juha.niskanen@haltian.com [nuttx]
2015-07-29 11:56:38 UTC
Permalink
Hi Max,


"Also, Greg, I think you mentioned in another thread that several people have done this before but unfortunately did not commit the changes upstream. Any chance that the code is open-source somewhere?"


You might want to compare notes with our implementation. Attached is Thingsee project's ADC implementation for STM32L1. We use IO_START_CONV ioctl() to select channel to use.


Hope we did not break anything for the F-series chips. Our internal commit history for this feature got too convoluted to create a clean set of patches from original commits so I just took a quick diff between our and Greg's tree + manual tweaks, so an alpha/preliminary quality patch. Would appreciate review by others using ADC for STM32.


Best Regards,

Juha


________________________________
From: ***@yahoogroups.com <***@yahoogroups.com> on behalf of ***@yahoo.com [nuttx] <***@yahoogroups.com>
Sent: Wednesday, July 29, 2015 1:30 AM
To: ***@yahoogroups.com
Subject: [nuttx] Re: Multiple ADC channels



Hi Greg,

I focused first on the DMA part before worrying about the upper-half driver and how to get all the DMA data into it at once. I have followed the steps you described and it works in principle, but now I am stuck.

To my understanding dmasetup and dmastart should be called for every sequence of conversions (at least this is how I read it from dac_send, which I use as a reference). So I added the two functions to adc_startconv, which is called before each read from the upper-half through ioctl. After the second conversion, the code gets stuck in stm32f40xxx_dma:625, so it appears the transfer does never finish. The callback set in dmastart is also never triggered, however the DMA transfer seems to have worked at least once, because the memory allocated to the DMA has changed to reasonable values.

Here the debug output:
nsh> adc -n 2
adc_main: Initializing external ADC device
stm32_adcinitialize: intf: 1 nchannels: 2
stm32_adcinitialize: ADC1 Selected
adc_main: g_adcstate.count: 2
adc_main: Hardware initialized. Opening the ADC device: /dev/adc0
adc_reset: intf: ADC1
adc_enable: enable: 1
adc_startconv: enable: 1
stm32_dmasetup: paddr: 4001204c maddr: 200003d8 ntransfers: 2 scr: 00002c00
stm32_dmacapable: stm32_dmacapable: 0x200003d8/2 0x00002c00
stm32_dmacapable: stm32_dmacapable: transfer OK
adc_reset: SR: 0x00000010 CR1: 0x008001e8 CR2: 0x10000101
adc_reset: SQR1: 0x00100000 SQR2: 0x00000000 SQR3: 0x00000128
adc_reset: CCR: 0x00000000
adc_setup: Enable the ADC interrupt: irq=34
adc_rxint: intf: 1 enable: 1
adc_startconv: enable: 1
stm32_dmasetup: paddr: 4001204c maddr: 200003d8 ntransfers: 2 scr: 00002c00
stm32_dmacapable: stm32_dmacapable: 0x200003d8/2 0x00002c00
stm32_dmacapable: stm32_dmacapable: transfer OK
adc_read: buflen: 20
adc_read: Returning: 5
Sample:
1: channel: 8 value: 3945
adc_startconv: enable: 1
stm32_dmasetup: paddr: 4001204c maddr: 200003d8 ntransfers: 2 scr: 00002c00
stm32_dmacapable: stm32_dmacapable: 0x200003d8/2 0x00002c00
stm32_dmacapable: stm32_dmacapable: transfer OK

This is the code I added to adc_startconv:
uint32_t ccr =
DMA_SCR_MSIZE_16BITS | /* Memory size */
DMA_SCR_PSIZE_16BITS | /* Peripheral size */
DMA_SCR_MINC | /* Memory increment mode */
DMA_SCR_DIR_P2M; /* Read from peripheral */

stm32_dmasetup(priv->dma,
STM32_ADC1_DR,
(uint32_t)priv->dmabuffer,
priv->nchannels,
ccr);

stm32_dmastart(priv->dma, callback, priv, false);

Do you have any comment, where I could be making a mistake?

Max
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2015-07-29 13:04:25 UTC
Permalink
Hello,

I'm ready to test, but I am interested by an implementation for the stm32f429,
not the L series.

If the hardware is compatible, I can tell you how it comes on our board. There
are 3 voltages to be monitored, as well as vbat.

Sébastien Lorquet
[Attachment(s) <#TopText> from Juha Niskanen (Haltian) included below]
Hi Max,
"Also, Greg, I think you mentioned in another thread that several people have
done this before but unfortunately did not commit the changes upstream. Any
chance that the code is open-source somewhere?"
You might want to compare notes with our implementation. Attached is Thingsee
project's ADC implementation for STM32L1. We use IO_START_CONV ioctl() to select
channel to use.
Hope we did not break anything for the F-series chips. Our internal commit
history for this feature got too convoluted to create a clean set of patches
from original commits so I just took a quick diff between our and Greg's tree +
manual tweaks, so an alpha/preliminary quality patch. Would appreciate review by
others using ADC for STM32.
Best Regards,
Juha
--------------------------------------------------------------------------------
*Sent:* Wednesday, July 29, 2015 1:30 AM
*Subject:* [nuttx] Re: Multiple ADC channels
Hi Greg,
I focused first on the DMA part before worrying about the upper-half driver and
how to get all the DMA data into it at once. I have followed the steps you
described and it works in principle, but now I am stuck.
To my understanding dmasetup and dmastart should be called for every sequence of
conversions (at least this is how I read it from dac_send, which I use as a
reference). So I added the two functions to adc_startconv, which is called
before each read from the upper-half through ioctl. After the second conversion,
the code gets stuck in stm32f40xxx_dma:625, so it appears the transfer does
never finish. The callback set in dmastart is also never triggered, however the
DMA transfer seems to have worked at least once, because the memory allocated to
the DMA has changed to reasonable values.
nsh> adc -n 2
adc_main: Initializing external ADC device
stm32_adcinitialize: intf: 1 nchannels: 2
stm32_adcinitialize: ADC1 Selected
adc_main: g_adcstate.count: 2
adc_main: Hardware initialized. Opening the ADC device: /dev/adc0
adc_reset: intf: ADC1
adc_enable: enable: 1
adc_startconv: enable: 1
stm32_dmasetup: paddr: 4001204c maddr: 200003d8 ntransfers: 2 scr: 00002c00
stm32_dmacapable: stm32_dmacapable: 0x200003d8/2 0x00002c00
stm32_dmacapable: stm32_dmacapable: transfer OK
adc_reset: SR: 0x00000010 CR1: 0x008001e8 CR2: 0x10000101
adc_reset: SQR1: 0x00100000 SQR2: 0x00000000 SQR3: 0x00000128
adc_reset: CCR: 0x00000000
adc_setup: Enable the ADC interrupt: irq=34
adc_rxint: intf: 1 enable: 1
adc_startconv: enable: 1
stm32_dmasetup: paddr: 4001204c maddr: 200003d8 ntransfers: 2 scr: 00002c00
stm32_dmacapable: stm32_dmacapable: 0x200003d8/2 0x00002c00
stm32_dmacapable: stm32_dmacapable: transfer OK
adc_read: buflen: 20
adc_read: Returning: 5
1: channel: 8 value: 3945
adc_startconv: enable: 1
stm32_dmasetup: paddr: 4001204c maddr: 200003d8 ntransfers: 2 scr: 00002c00
stm32_dmacapable: stm32_dmacapable: 0x200003d8/2 0x00002c00
stm32_dmacapable: stm32_dmacapable: transfer OK
uint32_t ccr =
DMA_SCR_MSIZE_16BITS | /* Memory size */
DMA_SCR_PSIZE_16BITS | /* Peripheral size */
DMA_SCR_MINC | /* Memory increment mode */
DMA_SCR_DIR_P2M; /* Read from peripheral */
stm32_dmasetup(priv->dma,
STM32_ADC1_DR,
(uint32_t)priv->dmabuffer,
priv->nchannels,
ccr);
stm32_dmastart(priv->dma, callback, priv, false);
Do you have any comment, where I could be making a mistake?
Max
------------------------------------

------------------------------------


------------------------------------

Yahoo Groups Links

<*> To visit your group on the web, go to:
http://groups.yahoo.com/group/nuttx/

<*> Your email settings:
Individual Email | Traditional

<*> To change settings online go to:
http://groups.yahoo.com/group/nuttx/join
(Yahoo! ID required)

<*> To change settings via email:
nuttx-***@yahoogroups.com
nuttx-***@yahoogroups.com

<*> To unsubscribe from this group, send an email to:
nuttx-***@yahoogroups.com

<*> Your use of Yahoo Groups is subject to:
https://info.yahoo.com/legal/us/yahoo/utos/terms/
max.kriegleder@yahoo.com [nuttx]
2015-07-29 14:13:58 UTC
Permalink
Hi Sebastien,

I finally got it running (the CR magic for the ADC was actually tricker than expected). If you give me another hour, I can send you a patch and it would be great if you could test it.

Max
spudarnia@yahoo.com [nuttx]
2015-07-29 14:46:11 UTC
Permalink
Great news. DMA code is easy but, yes, debugging it is tricky. I am glad that you have success.


I just checked in Juha's big modification for STM32L ADC and reviewed all of the ADC-related files. So your code now will probably not diff well with the current repository content.


My proposal is that (1) you finish up your changes, (2) patch against the previous ADC code and send that to me, (3) I will integrate that into the current repository content, and (4) you can pull the merged code and retest.


Or you can merge in yourself. I am just offering to minimize your work. Your choice.


Also, how portable are your DMA changes? Do you expect it to work on all STM32's? The DMA setup is different for the F1 family (CCR vs. SCR for one thing; streams vs. channels for another). The DMA feature should be conditioned on something like CONFIG_STM32_ADC_DMA=y and perhaps we could make that depend on (CONFIG_EXPERIMENTAL || CONFIG_STM32_what_you_tested_on).


Greg
spudarnia@yahoo.com [nuttx]
2015-07-29 14:38:50 UTC
Permalink
Hi, Juha,


All of your changes (the last three patches) have been review and committed. Based on your commits, I suppose it needs some regression testing on other STM32 platforms.


Thanks,
Greg
spudarnia@yahoo.com [nuttx]
2015-07-29 23:48:34 UTC
Permalink
I am doing some build testing now using tools/testbuil.sh. These the ADC changes do break some other build configurations. I did this conditional logic reversion to get things to build:

https://bitbucket.org/nuttx/arch/commits/a496ef8cbf1a2ca1f510e9e319ecc499294027b9 https://bitbucket.org/nuttx/arch/commits/a496ef8cbf1a2ca1f510e9e319ecc499294027b9

I do get these warnings. I would like to get rid of them but the conditional logic is not obvious to me:

chip/stm32_adc.c:2102:12: warning: 'adc_set_ch' defined but not used [-Wunused-function]
static int adc_set_ch(FAR struct adc_dev_s *dev, uint8_t ch)
^
chip/stm32_adc.c:923:13: warning: 'adc_startconv' defined but not used [-Wunused-function]
static void adc_startconv(struct stm32_dev_s *priv, bool enable)
^

This is why I am a big, big fan of code replication. My preference would be to have a separate stm32_adc.c file for each significantly different architecture rather than one big, complex entangled file for all architectures with unfathomable conditional compilation. With those complex files, it is pretty much impossible to make a change to one part and not have the change ripple through and break everything else.

Also... I am not testing anything... just verifying that all configurations still build. I have not idea if ADC is still functional on these other platforms.

Greg
'Juha Niskanen (Haltian)' juha.niskanen@haltian.com [nuttx]
2015-07-30 07:27:34 UTC
Permalink
Hi Greg,


Thanks for taking time with this! Our code was originally developed with minimal considerations for upstreaming - in thingsee some parts of it were in wrong places like include/nuttx/analog/adc.h and I had to do some last minute code motion when converting it to patches. Not a big surprise that there are problems on F-family of STM32, sorry about that.


I think cleanest way to get rid of those warnings is to extend adc_ioctl() for all other supported chips. That, specifically the IO_START_CONV cmd, pulls those two symbols. I don't want to do that blindly, as I have zero experience with the F-family ADC and adding unused ioctl cmds prevents linker from removing the referenced symbols. Maybe F-family users are already porting the multi-channelness of L1 to their chips?


The CONFIG_ADC_NO_STARTUP_CONV Kconfig was missing from patches, here is it:


diff --git a/nuttx/drivers/analog/Kconfig b/nuttx/drivers/analog/Kconfig
index e442ef4..0a342e6 100644
--- a/nuttx/drivers/analog/Kconfig
+++ b/nuttx/drivers/analog/Kconfig
@@ -43,6 +43,13 @@ config ADC_TOTAL_CHANNELS
This variable defines the amount of configured channels. Channels are
configured for ADC conversion when ADC is initialized.

+config ADC_NO_STARTUP_CONV
+ bool "Do not start conversion when opening ADC device"
+ default n
+ depends on ARCH_CHIP_STM32
+ ---help---
+ Do not start conversion when opening ADC device.
+
config ADC_ADS125X
bool "TI ADS1255/ADS1256 support"
default n


and if that option is wanted for STM32F10XX, then I think the #ifdefs should be like in attached diff.txt (totally untested).


Gcc and Keil compilers support __attribute__((unused)) for functions. Maybe add #define for it to nuttx/compiler.h?


- Juha

________________________________
From: ***@yahoogroups.com <***@yahoogroups.com> on behalf of ***@yahoo.com [nuttx] <***@yahoogroups.com>
Sent: Thursday, July 30, 2015 2:48 AM
To: ***@yahoogroups.com
Subject: Re: [PATCH] Re: [nuttx] Re: Multiple ADC channels



I am doing some build testing now using tools/testbuil.sh. These the ADC changes do break some other build configurations. I did this conditional logic reversion to get things to build:

https://bitbucket.org/nuttx/arch/commits/a496ef8cbf1a2ca1f510e9e319ecc499294027b9

I do get these warnings. I would like to get rid of them but the conditional logic is not obvious to me:

chip/stm32_adc.c:2102:12: warning: 'adc_set_ch' defined but not used [-Wunused-function]
static int adc_set_ch(FAR struct adc_dev_s *dev, uint8_t ch)
^
chip/stm32_adc.c:923:13: warning: 'adc_startconv' defined but not used [-Wunused-function]
static void adc_startconv(struct stm32_dev_s *priv, bool enable)
^

This is why I am a big, big fan of code replication. My preference would be to have a separate stm32_adc.c file for each significantly different architecture rather than one big, complex entangled file for all architectures with unfathomable conditional compilation. With those complex files, it is pretty much impossible to make a change to one part and not have the change ripple through and break everything else.

Also... I am not testing anything... just verifying that all configurations still build. I have not idea if ADC is still functional on these other platforms.

Greg
max.kriegleder@yahoo.com [nuttx]
2015-07-30 12:00:52 UTC
Permalink
Hi Greg,

Thank you for the offer - I made the changes to the code before you included these other changes.

My code works fine for the STM32F40XX architecture, but I could only test ADC1. For the STM32F10XX architecture I am not sure, but I looked in the datasheet and it seems the for example ADC2 does not support DMA. However, I included conditional statements such that DMA will only enabled for the STM32F40XX architecture.

On a separate note, I agree that each significantly other architecture should have its separate stm32_... file. If you remember the I2C issue that would only occur on the STM32F1 - now there is stm32_i2c and stm32_i2c_alt, whereas it would be much nicer if it was stm32_i2c_f4 and stm32_i2c_f1 without all the conditional stuff.

Max
spudarnia@yahoo.com [nuttx]
2015-07-30 14:49:44 UTC
Permalink
Max


I hand integrated your DMA changes with Juha's changes. I don't even know if the resulting chimera even compiles. You (and Juha) need to retest and see how much damage has been done.


https://bitbucket.org/nuttx/arch/commits/49469c3de808256b93b4fcfc5a88753c1d86c6f2 https://bitbucket.org/nuttx/arch/commits/49469c3de808256b93b4fcfc5a88753c1d86c6f2


Greg
spudarnia@yahoo.com [nuttx]
2015-07-30 14:50:57 UTC
Permalink
Hi, Juha,


I did incorporate your change. Hopefully some one using STM32 ADC on other STM32 parts can let us know if anything is broken.


Greg
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2015-07-30 15:53:02 UTC
Permalink
Hello

I will test that as soon as I get rid of the parasitic work I have been forced
to complete (sigh).

Thanks for contributing this code.

Sébastien Lorquet
Post by ***@yahoo.com [nuttx]
Hi, Juha,
I did incorporate your change. Hopefully some one using STM32 ADC on other
STM32 parts can let us know if anything is broken.
Greg
max.kriegleder@yahoo.com [nuttx]
2015-08-04 10:08:58 UTC
Permalink
Hi Greg,

I tested your code, which compiled immediately. However, there is an issue with the newly introduced cchannels in the device structure for my part of the code. Due to your changes nchannels is not set anywhere, but used often.

So for my part, changing stm32_adc.c:2831 to

priv->nchannels = cchannels;

solves the problem. Of course, priv->cchannels is now not set anymore, but as I did not dig into why it was introduced in the first place, I think somebody else needs to check where it should be set.

Also I think the Kconfig:3117 should be

depends on STM32_ADC1 || STM32_ADC2 || STM32_ADC3

Max
spudarnia@yahoo.com [nuttx]
2015-08-04 12:34:28 UTC
Permalink
Hi, Max,
Post by ***@yahoo.com [nuttx]
I tested your code, which compiled immediately. However, there is an issue with the newly introduced cchannels in the device structure for my part of the code. Due to your changes nchannels is not set anywhere, but used often.
Its not exactly "my code". I dont' really understand the changes either.
Post by ***@yahoo.com [nuttx]
So for my part, changing stm32_adc.c:2831 to
priv->nchannels = cchannels;
solves the problem. Of course, priv->cchannels is now not set anymore, but as I did not dig into why it was introduced in the first place, I think somebody else needs to check where it should be set.
I changed the code so that nchannels is set to cchannels for all architectures, in the #else part aftter the #ifdef CONFIG_STM32_STM32L15XX (line 2287)



https://bitbucket.org/nuttx/arch/commits/a23db9776052c2fb239368eff7c2017eecd403d1 https://bitbucket.org/nuttx/arch/commits/a23db9776052c2fb239368eff7c2017eecd403d1



But I don't understand what I am doing since I don't understand the changes and I did not test the change since I am not using STM32 ADC.
Post by ***@yahoo.com [nuttx]
Also I think the Kconfig:3117 should be
depends on STM32_ADC1 || STM32_ADC2 || STM32_ADC3
Also fixed except I used 'depends on STM32_ADC' which should be the same thing.


Greg
max.kriegleder@yahoo.com [nuttx]
2015-08-04 13:59:06 UTC
Permalink
Hi Greg,

Your changes work for me (ADC1, software trigger).

Max
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2015-09-07 15:34:14 UTC
Permalink
Hello,

I also confirm that multiple ADC channels with DMA and sw trigger works for me.

Thank you!

Sébastien Lorquet
Post by ***@yahoo.com [nuttx]
Hi Greg,
Your changes work for me (ADC1, software trigger).
Max
Continue reading on narkive:
Loading...