arduino-libraries / Arduino_AdvancedAnalog

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

Added DualMode capability #60

Closed jmdodd95682 closed 6 months ago

jmdodd95682 commented 6 months ago

I added Dual Mode ADC capability to the Arduino_Advanced_Analog library.

Dual mode allows you to trigger ADC1 and ADC2 simultaneously, allowing for synchronized capture of data from them.

It works like this:

After declaring an each instance of AdvancedADC for dualmode, call the new method (setADCDualMode(true);). Next, you call begin() for both instances with two new defaulted parameters. The start parameter should be set to false because we cannot auto-start the ADC since we have to setup/config both instances. The fixed_adc parameter indicates which ADCthe instance will use. This is necessary since dual-mode will only work on ADC1 and ADC2. One instance (called the pimary) will use ADC1 (fixed_adc=1) and the secondary will use ADC2 (fixed_adc=2).

Finally you call start on the primary instance. This will automatically call the new method enableDualMode to configure the HW for synchronized start for ADC1 and ADC2. The output will be available through the dma_buffers for each instance.

I've tested it very simply.

github-actions[bot] commented 6 months ago

Memory usage change @ df7d73dcc86121a431c423bf085f279b37a744cd

Board flash % RAM for global variables %
arduino:mbed_giga:giga :small_red_triangle: 0 - +320 0.0 - +0.02 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`|256|0.01|0|0.0|320|0.02|0|0.0|256|0.01|0|0.0|320|0.02|0|0.0|256|0.01|0|0.0|320|0.02|0|0.0|0|0.0|0|0.0|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,256,0.01,0,0.0,320,0.02,0,0.0,256,0.01,0,0.0,320,0.02,0,0.0,256,0.01,0,0.0,320,0.02,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0 ```
github-actions[bot] commented 6 months ago

Memory usage change @ 11a4e773b6996fee216ec79b696f2ef26186d1fc

Board flash % RAM for global variables %
arduino:mbed_giga:giga :small_red_triangle: 0 - +320 0.0 - +0.02 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`|256|0.01|0|0.0|320|0.02|0|0.0|256|0.01|0|0.0|320|0.02|0|0.0|256|0.01|0|0.0|320|0.02|0|0.0|0|0.0|0|0.0|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,256,0.01,0,0.0,320,0.02,0,0.0,256,0.01,0,0.0,320,0.02,0,0.0,256,0.01,0,0.0,320,0.02,0,0.0,0,0.0,0,0.0,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 6 months ago

@jmdodd95682 Thank you for sending this, this is an excellent feature that we should definitely have. I'll review this as soon as possible. In the meantime, I think this supersedes #55 should we close it ?

I've also been thinking about how to implement this in the background, and had an idea to keep the ADC constructor simple. What if we add a new class called AdvancedADCDual, for example, that accepts two initialized (but not started) ADCs and then links them together and starts them ? Would that be easier to use ? You can do the error checking in AdvancedADCDual, for example check ADC numbers, check they're not started etc..

jmdodd95682 commented 6 months ago

You are welcome. I am not happy with the code changes because it seems very clumsy to utilize the Dual mode, but I cannot see a simpler way to implement it. You are fighting three problems. 1) You MUST use ADC1 and ADC2. 2) You MUST have ADC2 completely configured (including DMA etc.) except for a timer, before starting ADC1 and 3) you MUST know up front that you're going to enable Dual Mode.

What I did was stick to the idea from #55 to have a separate begin() and start() and have begin() be backward compatible. It does supersede #55, but it also has all the functionality of #55. So you can also NOT enable Dual mode, but have to ADC's be somewhat synchronized together (within 4us).

iabdalkader commented 6 months ago

but I cannot see a simpler way to implement it

Just some rough ideas, I haven't had time to look at this yet, but we could do something like:

AdvancedADCDual adc_dual(AdvancedADC(<id>, A0...), AdvancedADC(<id>, A1...));

void setup() {
   adc_dual.begin(AN_RESOLUTION_16, 8000, 32, 64); // do error checking, initialize ADC with no start, link, start.
}

Or

AdvancedADC adc1(<id>, A0...);
AdvancedADC adc2(<id>, A1...);
AdvancedADCSync adcsync;

void setup() {
    adc1.begin(AN_RESOLUTION_16, 8000, 32, 64, false);  // No start
    adc2.begin(AN_RESOLUTION_16, 8000, 32, 64, false)  // No start
    ....
   adcsync.start(adc1, adc2); // do error checking, link, start.
}

It does supersede #55, but it also has all the functionality of #55.

I'll close #55 and let's focus on this one.

jmdodd95682 commented 6 months ago

Actually, I like that. I think the AdvancedADCSync adcsync; is a great idea. Now you can explicitly enable Dual Mode when you want it, and make the start cleaner. It remains backward compatible too.

I would prefer AdvancedADCSync to a new AdvancedADCDual class because AdvancedADCDual means you need to know ahead of time that you want dual mode. There may be applications where you want to switch between Dual Mode and Independent mode. But either is better than my original proposal.

iabdalkader commented 6 months ago

It's a tough choice, I'm leaning towards the first one, the dual API:

AdvancedADC adc1(<id>, A0...);
AdvancedADC adc2(<id>, A1...);
AdvancedADCDual adc_dual(adc1, adc2);

void setup() {
    adc_dual.begin(AN_RESOLUTION_16, 8000, 32, 64);
}

Is there a need for different settings for each ADC ? Don't they need to have the same config ?

There may be applications where you want to switch between Dual Mode and Independent mode.

You should still be able to stop and restart ADCs individually with the above, no ?

jmdodd95682 commented 6 months ago

I thought about it more, and I think this is probably the best approach since you are forced to use ADC0 and ADC1 for dual mode. It's easier and cleaner to have a different API. Plus, we can also add interleaved mode to this once we figure out how to make it work.

jmdodd95682 commented 6 months ago

To your question...I think the intent is that both ADCs operate identically in DUAL mode (sample sample rate, same resolution etc.). So they should be programmed identically. But, when in Dual Mode, ADC1 is simply sharing some of the control bits from ADC0. There's also the question of whether we can just program ADC0 for some or all of it. I don't know. Probably safer just to program both identically (which is what I did).

I did find one weird thing. I had to link the DMA and ADC2 prior to programming the DUAL field in the ADC1/2_CCR. If I set the DUAL field first, it seemed to ignore my linking of the DMA and ADC1. And obviously you only program and start the timer for ADC1 since ADC2 is controlled via ADC1 start.

jmdodd95682 commented 6 months ago

Sorry...typo in the previous comment. Repeating here with the correct ADC #'s.

To your question...I think the intent is that both ADCs operate identically in DUAL mode (sample sample rate, same resolution etc.). So they should be programmed identically. But, when in Dual Mode, ADC2 is simply sharing some of the control bits from ADC1. There's also the question of whether we can just program ADC1 for some or all of it. I don't know. Probably safer just to program both identically (which is what I did).

I did find one weird thing. I had to link the DMA and ADC2 prior to programming the DUAL field in the ADC1/2_CCR. If I set the DUAL field first, it seemed to ignore my linking of the DMA and ADC2. And obviously you only program and start the timer for ADC1 since ADC2 is controlled via ADC1 start.

iabdalkader commented 6 months ago

I thought about it more, and I think this is probably the best approach since you are forced to use ADC0 and ADC1 for dual mode.

Okay great, let's go with AdvancedADCDual. Do you want to give a try or do you want me to implement it ?

Note I don't think we need to specify the ADC number, which should make things easier. The first pin always determines the ADC instance, and the rest of the pins must map to the same ADC (this code is already implemented). The instances/descriptors are also allocated in order, so you'll get ADC1 first, then ADC2, then ADC3. The AdvancedADCDual can also check if they are the correct ADCs or not.

Also note that it might be enough to stop/start the timer to start() and stop() the ADC. Note that stop() currently deallocates the pool, so calling start() again won't restart the ADC. We didn't have a start() before so that wasn't a problem, but if you add one now, then stop() should disable the timer.

iabdalkader commented 6 months ago

I did find one weird thing. I had to link the DMA and ADC2 prior to programming the DUAL field in the ADC1/2_CCR. If

See my comment above, if we try to use the timer for start/stop, then begin() should do all of the DMA setup, as it does right now, and in start() we just enable the timer. This should fix that, right ?

So it should be something like this:

-int AdvancedADC::begin(uint32_t resolution, uint32_t sample_rate, size_t n_samples, size_t n_buffers) {
+int AdvancedADC::begin(uint32_t resolution, uint32_t sample_rate, size_t n_samples, size_t n_buffers, bool start) {
     ADCName instance = ADC_NP;

     // Sanity checks.
@@ -214,21 +214,25 @@ int AdvancedADC::begin(uint32_t resolution, uint32_t sample_rate, size_t n_sampl

     // Init, config and start the ADC timer.
     hal_tim_config(&descr->tim, sample_rate);
+    if (start) {
+        start();
+    }
+    return 1;
+}
+
+int AdvancedADC::start() {
     if (HAL_TIM_Base_Start(&descr->tim) != HAL_OK) {
         return 0;
     }
-    
     return 1;
 }

-int AdvancedADC::stop()
-{
-    dac_descr_deinit(descr, true);
+int AdvancedADC::stop() {
+    HAL_TIM_Base_Stop(&descr->tim);
     return 1;
 }

-AdvancedADC::~AdvancedADC()
-{
+AdvancedADC::~AdvancedADC() {
     dac_descr_deinit(descr, true);
 }

And then in AdvancedADCDual, do the following:

jmdodd95682 commented 6 months ago

Yes. I agree. We no longer care how much time it takes prior to starting since both ADCs will start synchronously. This makes sense.

I can take a try at it. Might take me a couple of days, but I can then test it out with my oscilloscope app.