Devel/giga audio input

[tomcombriat]

Hi! This is an initial trial in making audio input working on the giga. It is actually a bit more as, because of the way ADCs work on MBED, it is possible to skip completely the input_buffer. Basically, I tried to "modularize" more the AUDIO_INPUT feature, allowing to work on a Mozzi buffer or on an hardware implemented one. In the end it is becoming very symmetric to what is implemented for outputting audio which I personally find very satisfying. Note that this would probably ease the implementation of other ways of inputting audio with timed protocols (I'm thinking of I2S, which can be used for both input and output). This actually should probably have been the subject of a separate PR… I am happy to get suggestions on that!

But,

This does not work well on the giga yet. The sampling rate seems to be way off (from the scope, seems to be around 1kHz). I guess the problem comes from a wrong use of DMA in the implementation, but my brain is too fried now to investigate it further today. I tried to mimic the dac output that @tfry-git implemented, but opposite, but it is still not satisfying. I hope that he might have an illumination on that…

Note that I did not test (yet) if the modularization broke the INPUT_AUDIO using Mozzi buffers. For this reason, and basically because it is not working yet, I am putting this PR as draft.

[tfry-git]

I'll be a bit busy the upcoming days, probably no comments before early next week.

Modularization sounds good, though ;-)

--

BTW, while twiddling with analog input on the new STM32 port (an infuriating experience, but apparently successful in the end), I realized that it should be quite possible to implement a fallback implementation for ports that still lack an asynchronous one: Essentially we could call plain analogRead() right at the place of AUDIO_HOOK_HOOK. That way analog reads would still be unnecessarily wasteful, but they would automatically be limited to a rate that prevents buffer underruns. So, something like that will be upcoming, too...

[tomcombriat]

I'll be a bit busy the upcoming days, probably no comments before early next week.

Of course no problem, I was just putting that there while it was fresh and, who knows, I might make it work in the meantime!

---

Essentially we could call plain analogRead() right at the place of AUDIO_HOOK_HOOK

You are talking about audio_input right? In that case I guess this would be more or less equivalent to putting in updateAudio() but I always had concerns about this: what guarantees that, if the buffer runs a bit short at some point (say, because of a slow thing in updateControl()) that there will be a burst of analogReads, took at a rate way faster than AUDIO_RATE afterward. Similarly, during the delay, the sampling rate will be way slower than AUDIO_RATE (0?). So, in averaged, the sampling frequency would be correct but not locally. This is probably not a problem if the buffers never get close to emptiness. Am I missing something?

[tfry-git]

You are talking about audio_input right?

No, analog reads in general. And in fact, for AUDIO_INPUT, your concerns apply.

[tomcombriat]

Ok, thanks for your precision!!

If I understand well, I guess that the analogReads should not be blocking if they are in HOOK_HOOK in order not to slow down the audioHook() function, so that would be a good approach for the MBED for instance. Otherwise, mozziAnalogRead() falling to analogRead() like it is now would limit the blocking time.

[tfry-git]

Ah, skipped part of the explanation. The idea is really to call this, if, and only if, the output buffer is actually full. In that case -> not much of a problem to fall back to a single blocking analogRead().

I do see, this may not hold for mbed+EXTERNAL_AUDIO_OUTPUT. But then, that sort of mess is just what we have to deal with in the port implementations, anyway. It should still allow an easy fallback for ESP8266, ESP32, and SAMD21 that will work, even if user code tried to mozziAnalogRead() a lot of pins.

[tomcombriat]

Ok, found the problem.

Basically, one needs to read the next input sample at the correct rate, otherwise, as this adc gives a full buffer, the samples are exhausted way to fast, leading to a hang of the adc (while it is sampling the next batch).

But now, I am curious about the way audio_input was working before. I guess the underlying assumption was the ADC was always slower than the AUDIO_RATE? Or that we could afford to waste some samples sometimes?

In all cases, the "legacy" audio_input is untested for now. I also want to do a bit of cleanup.

Suggestions are welcome in the meantime!

[tfry-git]

Ok, found the problem.

Great!

But now, I am curious about the way audio_input was working before. I guess the underlying assumption was the ADC was always slower than the AUDIO_RATE? Or that we could afford to waste some samples sometimes?

Hm, this did indeed look wrong, and your fix looks exactly right. Of course (on the existing ports) new input readings would always have been triggered from within defaultAudioOutput(), so they could never actually outpace the output, anyway. That actually makes some sense, too, as far as keeping the rate in sync, although we wouldn't really have needed a 256 sample input buffer, then...

I guess AUDIO_INPUT isn't quite one of the best-tested areas in Mozzi...

[tomcombriat]

I guess AUDIO_INPUT isn't quite one of the best-tested areas in Mozzi...

That's a cool feature, but I never actually used it… Anyway, the fix should not break the previous port we will try to check that soon!

[tomcombriat]

Alright, except documenting a bit not much changes. (I changed a comment regarding the setting of ADC with was misleading in MozziGuts.cpp also).

Tested with success on Pico, Teensy, Giga/MBED.

Tested with mitigation on Genuino Mega 2560: the audio input seems to hang from time to time. That being said:

• not sure my board is up to spec
• compiled on Os
  • setting the ADCs to FASTEST_ADC does not resolve the issue.
• it also does it using sensorium/master branch… I never tried extensively this function, maybe someone with a good board can give it a try?

I tried to document the changes in MozziGuts.cpp, main idea is to make the situation more symmetric between inputs and outputs. As said in comments, this is needed because of the way ADCs work on the Arduino Giga, hence more changes on MozziGuts will probably needed in the future for mozziAnalogRead. A smart and clear separated case might appear then but for now I think that this symmetric situation is quite satisfying!

Comments are welcome (I might not have a lot of time in the following days, do not hesitate do modify if it seems fit)

[tfry-git]

My main worry is stability. We talked about that strange for loop in DAC output. Looking at Arduino_AdanvcedAnalog, they are already taking caching issues into account, however. I do not see, what should be missing.

Testing gives quite mixed results, however:

• Compiling a simple example without USE_AUDIO_INPUT, and in mono, I find that DAC output works highly reliably both with and without the extra loop.
• As soon as I add either USE_AUDIO_INPUT, or stereo into the mix, my success rate is much worse, and annoyingly, the same sketch will sometimes work, and sometimes it will not. I.e. simply pressing the reset button a couple of times may "fix" or "trigger" the issue.
• The extra loop may help increase stability a little, or it may not. But I've definitely seen both success and failure with both the loop present or removed.

This is giving me quite a headache, and I'm giving up for today. Fact is, we are having a stability problem, however. It may or may not be related to audio input. Can you confirm the (lack of a) pattern?

[tomcombriat]

Can you confirm the (lack of a) pattern?

Here, using the git version of Arduino_AdvancedAnalog (it has been updated not so long ago), keeping the AUDIO_INPUT as it is now in this PR:

• memcpy(dmabuf.data(), buffer, CHUNKSIZE*sizeof(Sample)); fails 10 out of 10, or works for a few samples (hard to see on an analog scope ;) )
• for (unsigned int i=0;i<CHUNKSIZE;i++) memcpy(dmabuf.data(), buffer, CHUNKSIZE*sizeof(Sample)); succeed 10 out of 10
• for (unsigned int i=0;i<CHUNKSIZE;i++) dmabuf[i]=buffer[i]; succeed 10 out of 10.

I think that the last is more satisfying than the penultimate, just because we do not copy the same thing over and over hoping for the CPU to look what happened in RAM…

That being said, there are still some mysteries, why is stable (even if the results are not the expected one) here? Why does it work for ADC and not for DAC?

---

I will try to modify this PR for hiding more things in the hood, hopefully tomorrow or might be a bit longer (week or so), anyway, this is still pending this cache issue…

[tfry-git]

Hm, we're seeing different things then (and yes, I am using the most recent git version). However, I am more and more getting the impression, that this is sometimes simply very prone to minor unrelated changes. Adding or removing some Serial debug code, for instance, seems to have quite a bit of impact. I've toying with a lot of small changes, and some seemed to help a little, but the whole thing just remains way too touchy, IMO.

Anyway, to make sure we a are somewhat on the same page, I have now settled on this basic testing setup: USE_AUDIO_INPUT true, AUDIO_CHANNELS STEREO, and then the Sinewave sketch with the following mods:

void setup(){
  pinMode(LED_BUILTIN, OUTPUT);
  startMozzi(CONTROL_RATE); // :)
  aSin.setFreq(440); // set the frequency
}

void updateControl(){}

AudioOutput_t updateAudio(){
  return StereoOutput::from8Bit(aSin.next(), getAudioInput()/16-128);
}

void loop(){
  audioHook();
  static uint32_t dummy;
  digitalWrite(LED_BUILTIN, (++dummy / 100000) % 10);
}

Hardware setup: A13 hooked to A0, monitoring output on A12. BTW, when this works, it works beautifully.

--

Anyway - while this did pop up now, and - for me - seems to be more pronounced with USE_AUDIO_INPUT and STEREO, I tend to think that it is not actually related to this PR, after all. So I would propose to clean up, here, and merge, and then continue the hunt.

As this is very likely a bug in Arduino_AdvancedAnalog, if we manage to create a limited testcase (i.e. without bringing Mozzi) to showcase the problem that would probably help a lot.

[tfry-git]

I had forgotten to also share some basic observations from my testcase. Can you confirm those?

• If the DAC is failing, it is failing right away. I.e. there is no short sound and then silence, or something, it seems to fail to start.
• The "heartbeat" that I added in loop() shows that the MCU keeps running in either case. Not locked up inside some infinite loop or something.
• Most of the time - independent of DAC output or not - the heartbeat is at around 2 per second. Sometimes, however it is much slower (1 per 20 seconds or so). What the ...?
• If there is output on A13 (dac1), the rest of the setup seems to work, too. I.e. it doesn't happen that dac1 is running, but not dac2.

[tomcombriat]

Ok, I have cleaned things a bit up. Seems to work but I will be away from breadboard (AFB) until next week, so not fully tested. I hope I understand correctly your suggestion.

I will put a few comments in there where I would be happy to have your point of view (in particular, I think the previous version was breaking mozziAnalogRead, see below).

Looking at this, I think some parts of MozziGuts.cpp might change soon, in particular to accommodate ADCs that does trigger an ISR at the end of a reading (MBED?). A smarter set of #if will probably be more likely but I have not made beforehand room for that.

A step further, more into modularization, this calls a lot to allow different input modes to be defined and configured (I'm again thinking of I2S input). We are not there yet but it might be good to think of it as a AudioConfigInput_impl.h I think.

And I remember saying that this port would probably be easy…

[tomcombriat]

PS: will try to confirm you test cases as soon as I get to the breadboard again ;)

Edit: hopefully I did things correctly and the comments will show up

Edit2: there is still a bit of garbage left from my previous implementation, I prefer things to be tested fully before getting rid of them.

[tfry-git]

I have committed some formatting changes, and a small simplification of the #if-layout.

Tested again, on the Giga, and found no - new - problems.

I have also tested this PR with an Arduino Uno: Both getAudioInput() and mozziAnalogRead() seem to work just fine. (Quality of the audio input is not quite as smooth as on the giga, but then that isn't to be expected, either. To make extra sure, I tested the same with Mozzi v1.0.0 and got the same result).

If that's ok with you, I'll clean up the remaining comments and merge.

--

While testing on the Uno, I also tried simply taking away the input_buffer (writing to audio_input, directly, instead), and that seemed to work just fine. But then I have not tested anything more complex, and I propose not to touch this bit for the moment.

[tomcombriat]

Hi!

I have committed some formatting changes, and a small simplification of the #if-layout.

Indeed this looks simpler than my complex and interleaved #if… That is the drawback of trying to optimize too much runtime branching, this is pointless here. I also saw that you corrected a confusion on my side between AudioOutput_t and AudioOutputStorage_t.

I am okay with merging soon. I guess a proper solution for mozziAnalogRead will still be on the todo list but this PR has no reason to be hanged by that.

---

As per the audio input buffer, it might be useful to have one in some situations if it is actually used. Let's keep as it it for now and see with time if a nice solution arise, with or without a buffer.

Thanks!

NB: I have added one comment as per the final choice for the memcpy line for audio output.

[tomcombriat]

(@tfry-git Deeply sorry to let you clean after my mess :/). Can try to test it tomorrow if you deem it necessary!