search

arduino-libraries / Arduino_AdvancedAnalog

Advanced Analog Library
GNU Lesser General Public License v2.1
7 stars 5 forks source link

Support synchronized ADC capture #55

Closed jmdodd95682 closed 4 months ago

jmdodd95682 commented 5 months ago

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.

CLAassistant commented 5 months ago

CLA assistant check
All committers have signed the CLA.

github-actions[bot] commented 5 months ago

Memory usage change @ a76e0e49ecd18838cecc69ef1912b78144075a12

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
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 ```
facchinm commented 5 months ago

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.

leonardocavagnis commented 5 months ago

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

My proposal:

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 commented 5 months ago

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

jmdodd95682 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

jmdodd95682 commented 5 months ago

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

github-actions[bot] commented 5 months ago

Memory usage change @ 13e9f0e9981e15b2aee6ebf8a60ee85c693d21f5

Board flash % RAM for global variables %
arduino:mbed_giga:giga :grey_question: -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
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 ```
iabdalkader commented 5 months ago

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 commented 5 months ago

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:

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

per1234 commented 4 months ago

we should look into supporting "Dual Mode".

This is now tracked at https://github.com/arduino-libraries/Arduino_AdvancedAnalog/issues/59 (thanks @jmdodd95682!).

iabdalkader commented 4 months ago

Superseded by https://github.com/arduino-libraries/Arduino_AdvancedAnalog/pull/60.