Fixed External audio output

[tomcombriat]

This is just a small tweak to make EXTERNAL_AUDIO_OUTPUT working. Tested crudefully with a digital output, will try to hook up a real DAC today.

Note: I did not get why I had to change #if (MBED_AUDIO_OUT_MODE == TIMEDPWM && EXTERNAL_AUDIO_OUTPUT != true) : I was initially thinking that, as I did not changed the MBED config, MBED_AUDIO_OUT_MODE == TIMEDPWM would be false. Shouldn't AudioConfigMBED.h be included? I did not go very deep yet.

Let me know if you think me making commits on this branch is not helpful!

[tomcombriat]

Ok, this does not work when trying to emulate I2S via SPI, the board crashes on the first audioOutput() call, when trying to call SPI1.transfer();

Interestingly, it works (with some glitches), when putting the SPI1.transfer() in updateAudio(). It also works when using digitalWrite as a crude DAC.

Edit: the reason of this bug seems to be the an SPI transfer cannot happen in a ticker callback, which kills a bit the interest of having an external audio output function. updateAudio() is not a callback, works there. Will see if there is a clean and easy workaround, would be too sad not to be able to hook a HiRes Dac on this platform!

[tomcombriat]

I think the good way to do it would be to add an option like AUDIO_OUTPUT_OUTSIDE_CALLBACK, that would change the behavior of MozziGuts.cpp. This activated, the callback would only raise a flag requesting audioOutput() to be called. This call could happen in audioHook() when a request has been issued. With new boards being more and more on Mbed, that would provide an option for that.

Until further opinions I'll stop my work here on EXTERNAL_AUDIO_OUTPUT, as this PR is not very useful atm I convert it to draft.

[tfry-git]

1. The actual content of this PR: That might have been due to my typo in MBED_AUDIOw_OUTPUT_MODE ?
2. The strategy for making the callbacks work: Yes, that sounds sane in principle. Could we perhaps simply use the LOOP_YIELD define for that purpose?

[tomcombriat]

The actual content of this PR: That might have been due to my typo in MBED_AUDIOw_OUTPUT_MODE ?

I am not sure, because I corrected this typo right away. Here are the outputs when changing only the typo, the timer and setting Mozzi on external_output mode:

In file included from /home/tom/Arduino_sketchbook/libraries/Mozzi/MozziGuts.cpp:72:0:
/home/tom/Arduino_sketchbook/libraries/Mozzi/MozziGuts_impl_MBED.hpp:169:42: error: 'AUDIO_CHANNEL_1_PIN' was not declared in this scope
 mbed::PwmOut pwmpin1(digitalPinToPinName(AUDIO_CHANNEL_1_PIN));
                                          ^~~~~~~~~~~~~~~~~~~
/home/tom/Arduino_sketchbook/libraries/Mozzi/MozziGuts_impl_MBED.hpp:169:42: note: suggested alternative: 'ADC_CHANNEL_1_SMP'
 mbed::PwmOut pwmpin1(digitalPinToPinName(AUDIO_CHANNEL_1_PIN));
                                          ^~~~~~~~~~~~~~~~~~~
                                          ADC_CHANNEL_1_SMP
/home/tom/Arduino_sketchbook/libraries/Mozzi/MozziGuts_impl_MBED.hpp: In function 'void startAudio()':
/home/tom/Arduino_sketchbook/libraries/Mozzi/MozziGuts_impl_MBED.hpp:187:21: error: 'US_PER_UPDATE' was not declared in this scope
   pwmpin1.period_us(US_PER_UPDATE);
                     ^~~~~~~~~~~~~
/home/tom/Arduino_sketchbook/libraries/Mozzi/MozziGuts_impl_MBED.hpp:187:21: note: suggested alternative: 'TIM_IT_UPDATE'
   pwmpin1.period_us(US_PER_UPDATE);
                     ^~~~~~~~~~~~~
                     TIM_IT_UPDATE

Using library Mozzi at version 1.1.0 in folder: /home/tom/Arduino_sketchbook/libraries/Mozzi 
exit status 1

Compilation error: exit status 1

Seems that startAudio is not aware of the config. By the way, I think it is safer to put the EXTERNAL_AUDIO there also to avoid starting the PWM channels to be started if the global config is set to use external_audio and MBED config is set to use PWM: we want only external_audio to be used in that case I think. Hopefully I did not make a stupid mistake somewhere.

---

Could we perhaps simply use the LOOP_YIELD define for that purpose?

Yes, that could be an option! But I won't try tonight… I have spent a bit of time fighting to get the ADC working for AUDIO_INPUT, to not avail. My strategy was to advance the ADC steps using a timer, but that crashes immediately. I suspect a similar problem actually: what you can and can't do in a callback is a bit different than previous architectures…

[tfry-git]

Feel free to rip out the entire TIMED_PWM for now, keeping only what is needed for EXTERNAL_AUDIO_OUTPUT, if you think it's cleaner. I still hope we'll have a workable idea on that, sooner or later, but it is certainly not going to look like the current state at all. (With or without PR).

As for analog, maybe the time has come to accept that we need to support a different approach on some boards. The giga is just another one that has hardware support for round-robin sampling (mozziAnalogRead()), and also for sampling a single pin at a fixed rate (getAudioInput()). A rough idea, how mozziAnalogRead() could be implemented on these boards:

• Keep a list of cached readings per analog pin, just as before.
• Add a flag, whether each pin has been requested in a mozziAnalogRead().
• After each iteration of updateControl(), start hardware round-robin sampling on all pins with the flag.
• Store their value in the cache, when conversion is done (or, should it be difficult to get an interrupt, before the next updateControl() is called).

Thoughts?

[tfry-git]

Small clarification:

a workable idea on that

that = TIMED_PWM

[tomcombriat]

has hardware support for round-robin sampling

I have to say that I really did not look into it, but when looking at the advanced_analog library it was not clear to me how to implement that without reserving all the analog pins for Mozzi, which I felt was not okay. But as I said I did not look much into other ways to access the ADCs.

For the audio input, the idea of having the possibility to bypass the audio_in buffer started to make its way into my mind. For this board, the situation for audio input is actually very symmetric to what you have done for the DAC.

[tfry-git]

I have to say that I really did not look into it, but when looking at the advanced_analog library it was not clear to me how to implement that without reserving all the analog pins for Mozzi

I believe this pull request should help with that: https://github.com/arduino-libraries/Arduino_AdvancedAnalog/pull/38

[tomcombriat]

This is a bit what I had in mind, unfortunately it does not work yet. The problem is that MozziGuts.cpp is not aware of AUDIO_OUTPUT_OUTSIDE_CALLBACK define at compile time because MozziGuts_impl_MBED.h is not included yet (nor the audio config for that matter, this file is not used when external audio output is activated).

I am not sure how to go around this, maybe moving the block of include before, and adding a forward declaration of defaultAudioOutput would be clean?

The forward declarations in MozziGuts.cpp would then to be removed, there might be a need to adapt other boards.

PS: I still see some problems with AdvancedAnalog: just adding SPI1.begin() in the setup trips it away…

[tomcombriat]

Seems to work like that. I moved forward the including of the MozziGuts_imp*.h files. This removes a few forward declarations, but adds a forward declaration of static void CACHED_FUNCTION_ATTR defaultAudioOutput();.

Seems to work here on both the built-in DAC, and an external DAC connected via SPI. Let me know if you comments or a cleaner idea for doing this!

Best,

[tfry-git]

I modified your code a bit, following the idea of LOOP_YIELD, which allows to keep most code out of MozziGuts.cpp itself. I have not tested anything fancy like SPI, but I think this still follows your logic.

(nor the audio config for that matter, this file is not used when external audio output is activated)

That might explain our initial confusion about how to write the #ifs. It's probably a bad design choice, EXTERNAL_AUDIO_OUTPUT should behave more like just another output mode. However, that isn't something to fix right now (probably rather for Mozzi 2.0).

PS: I still see some problems with AdvancedAnalog: just adding SPI1.begin() in the setup trips it away…

Note that the latest fixes there are not yet merged: https://github.com/arduino-libraries/Arduino_AdvancedAnalog/pull/36 . Are you sure you are using that?

[tomcombriat]

I modified your code a bit, following the idea of LOOP_YIELD, which allows to keep most code out of MozziGuts.cpp itself. I have not tested anything fancy like SPI, but I think this still follows your logic.

Seems to work great! My idea of putting it in MozziGuts.cpp was with the next Uno in mind, but yours seems to be more versatile while keeping the clutter out the Guts.

I might put a few questions in the review, but probably more for my own learning rather than suggestions!

I would nearly be for merging that: two output modes work (and the "natural" ones). Analog input (and audio in) will also need some work but I would be for having a separate PR for that, what do you think?

Note that the latest fixes there are not yet merged: https://github.com/arduino-libraries/Arduino_AdvancedAnalog/pull/36 . Are you sure you are using that?

Yes, I am using the zip version in the comments there (if arduino did not change it behind my back) though. A small delay after the SPI.begin solves it.

[tfry-git]

I would nearly be for merging that: two output modes work (and the "natural" ones). Analog input (and audio in) will also need some work but I would be for having a separate PR for that, what do you think?

Yes. Merged.

Yes, I am using the zip version in the comments there (if arduino did not change it behind my back) though. A small delay after the SPI.begin solves it.

Ok, we should probably still report that. If a simple delay fixes it, it's probably just a mutex that needs locking or something. Can you trigger the bug in one of the Advanced_Analog examples? That would make for the most useful test-case, I think.

[tfry-git]

PS: I still see some problems with AdvancedAnalog: just adding SPI1.begin() in the setup trips it away…

Seems to work for me, for some reason...

[tomcombriat]

Reproduced here but the conflict seems to come from Serial and SPI: the sinewave example sketch crashes when adding:

SPI1.begin();
Serial.begin(115200);

before startMozzi(); A delay resolves. I will not have much time to investigate (especially on the built-in examples) much in the following day :/.

[tomcombriat]

@tfry-git I think, we should think about merging devel/arduino_giga. Analog inputs are not there yet, nor Audio input but except for a bit of cosmetics and documentation, some nice outputs are already working. Some of the remaining work will probably need your PR on Advanced_Analog to be merged also.

I can take care of making this branch pretty enough for merging tomorrow if you want!

[tfry-git]

Yes, that would be cool!