Fixed External audio output #179

tomcombriat · 2023-04-04T08:26:25Z

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 · 2023-04-04T15:00:57Z

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 · 2023-04-04T16:33:59Z

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 · 2023-04-04T19:33:42Z

The actual content of this PR: That might have been due to my typo in MBED_AUDIOw_OUTPUT_MODE ?
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 · 2023-04-04T20:35:28Z

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 · 2023-04-05T08:16:26Z

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 · 2023-04-05T08:28:45Z

Small clarification:

a workable idea on that

that = TIMED_PWM

tomcombriat · 2023-04-05T09:02:51Z

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 · 2023-04-05T19:45:25Z

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: arduino-libraries/Arduino_AdvancedAnalog#38

tomcombriat · 2023-04-06T10:49:30Z

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…

…Output outside callback

tomcombriat · 2023-04-06T13:29:09Z

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 · 2023-04-07T10:26:26Z

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: arduino-libraries/Arduino_AdvancedAnalog#36 . Are you sure you are using that?

tomcombriat · 2023-04-07T16:02:32Z

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: arduino-libraries/Arduino_AdvancedAnalog#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.

tomcombriat · 2023-04-07T16:04:56Z

MozziGuts_impl_MBED.hpp

+}
+#define AUDIO_HOOK_HOOK { if (audio_output_requested) { audio_output_requested = false; defaultAudioOutput(); } }
+#else
+#define defaultAudioOutputCallback defaultAudioOutput


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

Copy-and-paste fail?

Oups, and an epic one! I wanted to ask if this was for forward compatibility for other output modes as defaultAudioOutput is never used (yet?) in this implementation.

You're right. It doesn't really make sense as long as there is no TIMED_PWM. I'll clean up (this and other bits) later.

tfry-git · 2023-04-07T17:58:09Z

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 · 2023-04-08T09:05:43Z

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 · 2023-04-08T09:35:41Z

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 · 2023-04-10T10:11:43Z

@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 · 2023-04-10T10:54:34Z

Yes, that would be cool!

@v4

* Added class MetaOscil, example of a s weep using it, and tables for band_limited square wave * Adapted comments at beginning of example * Corrected spurious parenthesis * Implemented variadic constructor and variadic initializer for the frequencies * Added comments and (hopefully) doxygen entries * Added BandLimited tables for tri, square and saw waves * Updated example * Removed some old test tables * Added keywords for MetaOscil * Readded standard way of feeding a MetaOsc with Oscil in order to allow initialization in the setup via a for loop for polyphonic purposes * Less ambiguous name * All names should be changed... * Removed unneeded functions, implemented dichotomic search * Updated keywords * Removed addOscil from MetaOscil, improved example with explanations * remove old docs * remove old docs * regenerate docs, check some typos * move multilines to multiLine branch * add .vscode * .vscode not *.vscode * remove cogl_sqrti.h, as Thomas' suggestion in file * Delete License.txt duplicate of LICENSE.TXT * regen after some removals * add rel 1.1.0 news * Slightly update NEWS.txt * Initial port to Arduino Giga. Crude, unfinished, and not tested on hardware, yet. * This actually allows to play the Sinewave example on the Giga's inbuilt DAC. Somehow the code is extremly touchy about the correct amount of buffers to use. Smells buggy. * Clean up initial dac output mode. This is not currently reliable, however, due to arduino-libraries/Arduino_AdvancedAnalog#35 * Some more bits * Rewrite platform detection macros to avoid flood of silly clang warnings. * Create compile_examples.yml Experimental workflow for automated build testing * Update compile_examples.yml * Fix theremin example * Limit example to compile (avoid ones that require a specific configuration; list not yet complete) * Include PinChangeInt library for automated builds * Next attempt at proper dependencies for compiling examples * Further limit what gets compiled for now * Modernize PinChangeInterupt example * Fix capitalization * Next attempt at covering sensors examples * Update for fixed Arduino_AdvancedAnalog * Add esp8266 to compilation matrix (excluding AVR only sketch) * Avoid broken triangle_warm_8192 table for now * Give up on exclusion matrix for now * Add autobuilds for Giga * Add more auto-checks (sensorium#180) Arduino lint and report size deltas (for PRs only) * Fixed External audio output (sensorium#179) * Fixed External audio output on Arduino Giga Co-authored-by: Thomas Friedrichsmeier <thomas.friedrichsmeier@kdemail.net> * Add required lib for Giga * Remove broken TIMED_PWM mode, add PDM_VIA_SERIAL, instead. Also, some cleanups. * Fix lib name * Re-add forward declarations (needed on AVR) * Hopefully more robust define for IS_STM32 * Experimental support for STM32 boards with the stmduino.com core. This is completely untested, as I'm struggling to find out how to upload using the core. If it works, it should allow to support many more boards, however, and also the core seems actively maintained (in contrast to the maple based cores) * Basics working * Document and auto-build new STM32duino-port * Prettier file names * Next attempt at telling the cores apart, reliably * Compilation fix for libmaple based STM32 core * Fix on more url * Implement analog reads for the STM32duino core. Note that this also utilizes the AUDIO_HOOK_HOOK that we are going to need in the Arduino Giga, too. * Add missing file * Document and auto-build SAMD21 port (sensorium#182) * Tweak platform define * Added documentation for Arduino Giga, reverted default output pin to have mono mode on the jack's tip * A litte more cleanup and attribution * Added documentation on AUDIO_HOOK_HOOK * Fixed triangle_warm8192 wavetable * Added github CI workflow for Pico * Morphed the giga port into MBED port, updated doc accordingly * Fix analog reads for STM32F411 And document the board as tested * Update dependency * Add recommended dependabot check * Fix compilation * Trying to modularize a bit more Audio_Input * Starting implementing Audio_Input for giga * First try at implementing audio input on giga using new modularized paradigm * Fixed audio input for giga * Don't nag on failures of report job Of course a proper fix would be better... * Documentation in MozziGuts.cpp * Cleaned up the modularization for AUDIO_INPUT, allowing legacy (async ADC) or custom modes * Fix for stereo, less nesting in #if'S * Formatting * Cleanups * Fixed compilation for EXTERNAL_AUDIO_OUTPUT for "new" STM32duino * Better fix for EXTERNAL_AUDIO_OUTPUT and STM32duino * Correct fix for STM32duino and EXTERNAL_AUDIO * Creating a devel branch for Arduino R4 * Correct hardware define for Uno R4 * fix number types * Revert to recommending Roger Clark's core In Stev Strong's core, pwmWrite() appears to be bugged (at least with the STM32F1 blue pill board) * Use a board with reasonable amount of flash for auto-builds * Barebones of Arduino R4 implementation * Correct audioConfig using built-in DAC, first attempt on IRQ for R4 New attempt at setting correctly the timer Timer config seems ok. Moved tables to RAM. Crude output (but PWM to slow?) * EXTERNAL_AUDIO should work. Moving to analogWave.h for standard output * First try at branching R4 output straight on the buffer data Set the buffer size as a define for easy future changes First try at hooking mozzi buffers with in-built analog generation Untested More low-level implementation for the dac, just an IRQ conflict remains * Add a second timer for synchronised remove of the buffer samples * Added documentation * samplerate at audio rate * tidy * Changed hardware define for Arduino Uno R4 Fig pgmspace for move to Renesas name * More modularization for Renesas * Modularization of Renesas Analog and documentation * Fix IntegerType.h * Bump actions/checkout from 3 to 4 in /.github/workflows Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3...v4) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> * Initial implementation of async ADC for Uno R4 * Cleanup and documentation * Added explanation on ADC callback * Updated readme * Updated version number * Fix frequency offset for AVR * Improved readability * Github workflow: Revert version increas in checkout action To test, whether this fixes the "report" stage on PR, or whether that breakage is due to something else. * Improved readability Added documentation for tweak on AVR timer frequency * Fix of mozziAnalogRead for Renesas * Updated version number * typos in docs * Replaced unsigned long into uint32_t for Oscil. Should optimize slightly for 32bitters * mozzi_midi as header only * Made midiToFreq table private * Inlining all functions of mozzi_midi.h * Add Uno R4 to automated builds * Attempt to reduce number of duplicate workflow builds * Update to upload/download-artifact @v4 * Silence warning * Fix workflow de-duplication logic * Fix deduplication logic once more... * Added comments to midi note table --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: tomcombriat <tomcombriat@live.fr> Co-authored-by: mr.sensorium <faveflave@gmail.com> Co-authored-by: Thomas Friedrichsmeier <thomas.friedrichsmeier@kdemail.net> Co-authored-by: Thomas Friedrichsmeier <tfry-git@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Fixed External audio output

8e6b509

tomcombriat marked this pull request as draft April 4, 2023 16:34

tomcombriat added 3 commits April 6, 2023 12:32

Added audioOutput outside callback functionnality, NON WORKING

47afeea

Removed spurious space

4cce91f

No need for the flag when not needed

8998427

Moved MozziGuts_imp.h forward in MozziGuts.cpp to allow to call audio…

da79133

…Output outside callback

tomcombriat marked this pull request as ready for review April 6, 2023 13:29

Move most of the logic into the implementation file

882a986

tomcombriat commented Apr 7, 2023

View reviewed changes

tfry-git merged commit bc221a7 into sensorium:devel/arduino_giga Apr 7, 2023

tomcombriat deleted the giga2 branch April 11, 2023 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixed External audio output #179

Fixed External audio output #179

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fixed External audio output #179

Fixed External audio output #179

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!