arduino-libraries / Arduino_AdvancedAnalog

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

DMA DAC intialization is unstable #35

Closed tfry-git closed 1 year ago

tfry-git commented 1 year ago

While trying to port the Mozzi (Sound Synthesis) library to the Arduino Giga, I notice that DMA driven DAC seems to be quite fragile to set up, in some cases the DAC will simply fail to start.

Testcase: I can trip up the DAC_Sine_wave example by as little as uncommenting the Serial.begin() line. Now the very same sketch will work some but not all of the time (try resetting a couple of times).

After lots of trial and error, I think I have pinned this down to the double buffered dma setup around here:

https://github.com/arduino-libraries/Arduino_AdvancedAnalog/blob/80eeb61669f8c45105b973f8b400bb44de80db21/src/AdvancedDAC.cpp#L129

Quite obviously, first starting the DAC, then pausing it to enable double buffering, then restarting it seems like a hack (if justified) in the first place.

I managed to arrive at a robust sketch, by essentially copying the HAL_DAC_Start_DMA implementation from the HAL, but modifying it to set up double buffering right away (settings the correct flags, addresses, and starting DMA with HAL_DMAEx_MultiBufferStart_IT). Of course that makes for a very verbose solution. License-wise, I think it should be ok, as the HAL library appears to be BSD'ed.

Would the above make for an acceptable PR, do you have another idea on attacking the problem, or am I the only one to experience it?

aentinger commented 1 year ago

Hi @iabdalkader :coffee: :wave: could you please take a look at this one? :pray: :bow:

iabdalkader commented 1 year ago

Would the above make for an acceptable PR

The main problem with copying internal HAL code is that we have to maintain it for new HAL releases, and across different micro-controllers, but yes it would be acceptable if that dbm code is the main problem. Some other issues were reported recently, and I made some major fixes to library, and my testing shows it's very stable (DAC can be stopped/restarted every few buffers if needed). Could you please test the library attached in this comment and let me know if the issues you saw are fixed ?

https://github.com/arduino-libraries/Arduino_AdvancedAnalog/issues/32#issuecomment-1492482775

tfry-git commented 1 year ago

I had assumed the latest code to be in git.

I can confirm that this is definitely much more stable, and I have not seen this issue, again, yet. But as the problem was manifesting itself some, but not all of the time, I can't quite tell whether it's really gone. I'll keep you posted.

iabdalkader commented 1 year ago

I had assumed the latest code to be in git.

It will be very soon, I was just doing some local testing first, then will push all the changes in a PR. Let me know if you find any other issues, and thanks for testing.

iabdalkader commented 1 year ago

The latest code is now in this PR https://github.com/arduino-libraries/Arduino_AdvancedAnalog/pull/36 just as a precaution I now disable/enable the DMA IRQ before resetting the DBM bit. Note in the updated example there's a new command to stop/kill the DAC (k), you can stop/restart the DAC on demand, to restart the DAC just issue any other command. Note you can write multiple command like ks+ (which will execute kill, sine, increase freq).

jmdodd95682 commented 1 year ago

Thanks for fixing this. I was driving myself crazy trying to diagnose the behavior I was seeing on the DAC. It would hang, or keep sending the same buffer forever. Sometimes it would hang right away, sometimes a few milliseconds later. Anyway, I pulled the latest repo (I think it was #38), and all the weirdness is gone. DAC is very stable at 96KHz with 1024-sample buffers. I'm able to stream from the ADC at 96KHz, process the audio, and stream it out the DAC seemlessly. Very nice.

aentinger commented 1 year ago

I think we'll need a new release of this library, so that the fixed version is distributed to users automagically. Let me handle this.

aentinger commented 1 year ago

Here it is v1.2.0 -> https://github.com/arduino-libraries/Arduino_AdvancedAnalog/releases/tag/1.2.0 .