Support synchronized ADC capture #55

jmdodd95682 · 2024-02-06T00:59:16Z

The begin() function takes about 106us to execute. If you're trying to start two ADCs, this will mean the first ADC will already be capturing for at least 106us when you execute the ADC.begin() on the second one.

The changes proposed here allow the following semantics:

adc1.ready(uint32_t resolution, uint32_t sample_rate, size_t n_samples, size_t n_buffers);
adc2.ready(uint32_t resolution, uint32_t sample_rate, size_t n_samples, size_t n_buffers);

adc1.start(sample_rate);
adc2.start(sample_rate);

This reduces the skew between the two captures to about 4us.

ready() and start() are just the begin() function split into two pieces so you can setup the DMA and ADC without starting until later.

ready() and start() are just the begin() function split into two pieces. ready() does everything except start the ADC capture. clear() is calling the DMABufferPool routine to flush out anything in the DMA buffers.

CLAassistant · 2024-02-06T00:59:23Z

All committers have signed the CLA.

github-actions · 2024-02-06T01:12:22Z

Memory usage change @ a76e0e4

Board	flash	%	RAM for global variables	%
`arduino:mbed_giga:giga`	0 - 0	0.0 - 0.0	0 - 0	0.0 - 0.0

Click for full report table

Board	`examples/Advanced/ADC_Multi` flash	%	`examples/Advanced/ADC_Multi` RAM for global variables	%	`examples/Advanced/ADC_Multi_Channel` flash	%	`examples/Advanced/ADC_Multi_Channel` RAM for global variables	%	`examples/Advanced/ADC_Multi_Channel_Dynamic` flash	%	`examples/Advanced/ADC_Multi_Channel_Dynamic` RAM for global variables	%	`examples/Advanced/ADC_Multi_To_DAC` flash	%	`examples/Advanced/ADC_Multi_To_DAC` RAM for global variables	%	`examples/Advanced/ADC_Serial_Plotter` flash	%	`examples/Advanced/ADC_Serial_Plotter` RAM for global variables	%	`examples/Advanced/ADC_To_DAC` flash	%	`examples/Advanced/ADC_To_DAC` RAM for global variables	%	`examples/Advanced/DAC_One_Channel` flash	%	`examples/Advanced/DAC_One_Channel` RAM for global variables	%	`examples/Advanced/DAC_Sine_wave` flash	%	`examples/Advanced/DAC_Sine_wave` RAM for global variables	%	`examples/Advanced/DAC_Two_Channels` flash	%	`examples/Advanced/DAC_Two_Channels` RAM for global variables	%	`examples/Beginner/Audio_Playback` flash	%	`examples/Beginner/Audio_Playback` RAM for global variables	%	`examples/Beginner/Waveform_Generator` flash	%	`examples/Beginner/Waveform_Generator` RAM for global variables	%
`arduino:mbed_giga:giga`	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0

Click for full report CSV

Board,examples/Advanced/ADC_Multi<br>flash,%,examples/Advanced/ADC_Multi<br>RAM for global variables,%,examples/Advanced/ADC_Multi_Channel<br>flash,%,examples/Advanced/ADC_Multi_Channel<br>RAM for global variables,%,examples/Advanced/ADC_Multi_Channel_Dynamic<br>flash,%,examples/Advanced/ADC_Multi_Channel_Dynamic<br>RAM for global variables,%,examples/Advanced/ADC_Multi_To_DAC<br>flash,%,examples/Advanced/ADC_Multi_To_DAC<br>RAM for global variables,%,examples/Advanced/ADC_Serial_Plotter<br>flash,%,examples/Advanced/ADC_Serial_Plotter<br>RAM for global variables,%,examples/Advanced/ADC_To_DAC<br>flash,%,examples/Advanced/ADC_To_DAC<br>RAM for global variables,%,examples/Advanced/DAC_One_Channel<br>flash,%,examples/Advanced/DAC_One_Channel<br>RAM for global variables,%,examples/Advanced/DAC_Sine_wave<br>flash,%,examples/Advanced/DAC_Sine_wave<br>RAM for global variables,%,examples/Advanced/DAC_Two_Channels<br>flash,%,examples/Advanced/DAC_Two_Channels<br>RAM for global variables,%,examples/Beginner/Audio_Playback<br>flash,%,examples/Beginner/Audio_Playback<br>RAM for global variables,%,examples/Beginner/Waveform_Generator<br>flash,%,examples/Beginner/Waveform_Generator<br>RAM for global variables,%
arduino:mbed_giga:giga,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0

facchinm · 2024-02-06T08:21:02Z

Hi @jmdodd95682 ,
if start() and then ready() is equivalent to begin() I'd change the PR to do that in order to avoid code duplication.

facchinm

I'd change begin() to call start() and then ready() (if equivalent)

leonardocavagnis · 2024-02-06T10:17:55Z

I'd change begin() to call start() and then ready() (if equivalent)

My proposal:

The current begin() function has been removed.
The ready() function will be renamed to begin().
The start() function has been added. There is no need to pass sample_rate as input; it could become a private object variable initialized by the begin() function.

With these changes, you would use begin() to configure the ADC and start() to initiate the conversion process.

@facchinm @iabdalkader Sounds good for you?
Please, @iabdalkader could you take a look and give a spin?

Thanks

iabdalkader · 2024-02-06T10:56:57Z

I can take a look at the issue and PR next week. In the meantime @jmdodd95682 please sign the CLA.

jmdodd95682 · 2024-02-06T17:10:48Z

Signed the CLA.

I agree with not having code duplication. I didn't want to just remove begin() if there was some other purpose (e.g. ease of use).

So we just have start() followed by begin() to do ADC capture. Do you want me to make those changes to my branch?

jmdodd95682 · 2024-02-07T02:44:51Z

I had another thought. Rather than getting rid of the existing begin() we should just keep it for backward compatibility with people's existing code. Instead, add a new bool flag to begin with a default of false. When this flag is set true, it does all the setup, but does not start. If the flag is false, it does the legacy behavior of doing the setup and start.

To avoid duplicate code, the begin() will call the new start() routine when the flag is false. So, we get full backward compatibility and get the new functionality with no duplicated code.

leonardocavagnis · 2024-02-07T09:06:44Z

I had another thought. Rather than getting rid of the existing begin() we should just keep it for backward compatibility with people's existing code. Instead, add a new bool flag to begin with a default of false. When this flag is set true, it does all the setup, but does not start. If the flag is false, it does the legacy behavior of doing the setup and start.

To avoid duplicate code, the begin() will call the new start() routine when the flag is false. So, we get full backward compatibility and get the new functionality with no duplicated code.

Your proposal sounds good!
Do the changes and add commits to this branch

Added bool noStart with default of false to allow backward compatibility with existing code.

jmdodd95682 · 2024-02-07T18:12:11Z

Made proposed changes. Tested on my application, works as expected.

github-actions · 2024-02-07T19:14:30Z

Memory usage change @ 13e9f0e

Board	flash	%	RAM for global variables	%
`arduino:mbed_giga:giga`	❔ -64 - +64	-0.0 - 0.0	0 - 0	0.0 - 0.0

Click for full report table

Board	`examples/Advanced/ADC_Multi` flash	%	`examples/Advanced/ADC_Multi` RAM for global variables	%	`examples/Advanced/ADC_Multi_Channel` flash	%	`examples/Advanced/ADC_Multi_Channel` RAM for global variables	%	`examples/Advanced/ADC_Multi_Channel_Dynamic` flash	%	`examples/Advanced/ADC_Multi_Channel_Dynamic` RAM for global variables	%	`examples/Advanced/ADC_Multi_To_DAC` flash	%	`examples/Advanced/ADC_Multi_To_DAC` RAM for global variables	%	`examples/Advanced/ADC_Serial_Plotter` flash	%	`examples/Advanced/ADC_Serial_Plotter` RAM for global variables	%	`examples/Advanced/ADC_To_DAC` flash	%	`examples/Advanced/ADC_To_DAC` RAM for global variables	%	`examples/Advanced/DAC_One_Channel` flash	%	`examples/Advanced/DAC_One_Channel` RAM for global variables	%	`examples/Advanced/DAC_Sine_wave` flash	%	`examples/Advanced/DAC_Sine_wave` RAM for global variables	%	`examples/Advanced/DAC_Two_Channels` flash	%	`examples/Advanced/DAC_Two_Channels` RAM for global variables	%	`examples/Beginner/Audio_Playback` flash	%	`examples/Beginner/Audio_Playback` RAM for global variables	%	`examples/Beginner/Waveform_Generator` flash	%	`examples/Beginner/Waveform_Generator` RAM for global variables	%
`arduino:mbed_giga:giga`	-64	-0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	64	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0	0	0.0

Click for full report CSV

Board,examples/Advanced/ADC_Multi<br>flash,%,examples/Advanced/ADC_Multi<br>RAM for global variables,%,examples/Advanced/ADC_Multi_Channel<br>flash,%,examples/Advanced/ADC_Multi_Channel<br>RAM for global variables,%,examples/Advanced/ADC_Multi_Channel_Dynamic<br>flash,%,examples/Advanced/ADC_Multi_Channel_Dynamic<br>RAM for global variables,%,examples/Advanced/ADC_Multi_To_DAC<br>flash,%,examples/Advanced/ADC_Multi_To_DAC<br>RAM for global variables,%,examples/Advanced/ADC_Serial_Plotter<br>flash,%,examples/Advanced/ADC_Serial_Plotter<br>RAM for global variables,%,examples/Advanced/ADC_To_DAC<br>flash,%,examples/Advanced/ADC_To_DAC<br>RAM for global variables,%,examples/Advanced/DAC_One_Channel<br>flash,%,examples/Advanced/DAC_One_Channel<br>RAM for global variables,%,examples/Advanced/DAC_Sine_wave<br>flash,%,examples/Advanced/DAC_Sine_wave<br>RAM for global variables,%,examples/Advanced/DAC_Two_Channels<br>flash,%,examples/Advanced/DAC_Two_Channels<br>RAM for global variables,%,examples/Beginner/Audio_Playback<br>flash,%,examples/Beginner/Audio_Playback<br>RAM for global variables,%,examples/Beginner/Waveform_Generator<br>flash,%,examples/Beginner/Waveform_Generator<br>RAM for global variables,%
arduino:mbed_giga:giga,-64,-0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,64,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0

src/AdvancedADC.cpp

src/AdvancedADC.h

src/AdvancedADC.cpp

Co-authored-by: Ibrahim Abdelkader <[email protected]>

src/AdvancedADC.h

src/AdvancedADC.cpp

leonardocavagnis

@jmdodd95682
I removed sample_rate parameter from start() to avoid duplication.

Please, apply these changes:

add the private variable uint32_t sampling_rate; here:

Arduino_AdvancedAnalog/src/AdvancedADC.h

Lines 30 to 34 in f7d081c

    
           private: 
        
               size_t n_channels; 
        
               adc_descr_t *descr; 
        
               PinName adc_pins[AN_MAX_ADC_CHANNELS];

modify this line in: hal_tim_config(&descr->tim, sampling_rate); https://github.com/jmdodd95682/Arduino_AdvancedAnalog/blob/0d2b69c152cd8925f64205b94c842d4edde0ae24/src/AdvancedADC.cpp#L224

iabdalkader · 2024-02-14T14:30:34Z

This change is fine on its own, and it probably wouldn't hurt to merge it (after the issues are fixed) but it doesn't really fix the issue, the 2 ADCs will still be out of sync. I think, if we have time and if this is high priority, we should look into supporting "Dual Mode". In dual mode, one ADC controls the other and sampling is triggered simultaneously. I haven't tested it before but it sounds exactly like what's needed here.

jmdodd95682 · 2024-02-14T17:17:33Z

I agree. This was a quick fix and gets them with about 4us of each other. Not great, but okay as a short-term fix.
I read about the Dual-Mode as well. Adc1 and Adc2 can be bound together, with Adc1 being the master. This seems like a longer effort, but probably worth it. I thought about doing that enhancement, but I don't have enough experience with the HAL code.

The applications where it would be useful is things like:

Capturing two analog signals and using one as a capture trigger (e.g. capture scope application)
Recording two correlated signals for low-frequency radar or lidar.

per1234 · 2024-03-14T21:35:30Z

we should look into supporting "Dual Mode".

This is now tracked at #59 (thanks @jmdodd95682!).

iabdalkader · 2024-03-15T16:52:13Z

Superseded by #60.

jmdodd95682 added 2 commits February 5, 2024 16:29

Declared new member functions ready(), start(), and clear()

7a77b31

ready() and start() are just the begin() function split into two pieces so you can setup the DMA and ADC without starting until later.

Added member functions ready(), start() and clear()

a76e0e4

ready() and start() are just the begin() function split into two pieces. ready() does everything except start the ADC capture. clear() is calling the DMABufferPool routine to flush out anything in the DMA buffers.

facchinm requested changes Feb 6, 2024

View reviewed changes

per1234 changed the title ~~Jmdodd95682 patch 1~~ Support synchronized ADC capture Feb 6, 2024

per1234 added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Feb 6, 2024

per1234 linked an issue Feb 6, 2024 that may be closed by this pull request

Synchronized ADC Capture #54

Closed

jmdodd95682 added 2 commits February 7, 2024 10:07

Changed implementation to re-use begin() for backward compatibility

d30bcea

Added bool noStart with default of false to allow backward compatibility with existing code.

Changed implementation to reuse begin() with option noStart

6e9040b

jmdodd95682 added 2 commits February 7, 2024 11:06

Fixed compile bug with defaulted parameters

563bf02

Fixed compile bug with defaulted parameters

13e9f0e

iabdalkader requested changes Feb 7, 2024

View reviewed changes

leonardocavagnis and others added 7 commits February 8, 2024 09:57

Update src/AdvancedADC.cpp

bf6113c

Co-authored-by: Ibrahim Abdelkader <[email protected]>

Update src/AdvancedADC.cpp

a3bc455

Co-authored-by: Ibrahim Abdelkader <[email protected]>

Update src/AdvancedADC.cpp

0a854f1

Co-authored-by: Ibrahim Abdelkader <[email protected]>

Update src/AdvancedADC.cpp

1341867

Co-authored-by: Ibrahim Abdelkader <[email protected]>

Update src/AdvancedADC.h

8cb517d

Co-authored-by: Ibrahim Abdelkader <[email protected]>

Update src/AdvancedADC.h

db817ed

Co-authored-by: Ibrahim Abdelkader <[email protected]>

Update src/AdvancedADC.cpp

0d2b69c

Co-authored-by: Ibrahim Abdelkader <[email protected]>