sensorium / Mozzi

sound synthesis library for Arduino
https://sensorium.github.io/Mozzi/
GNU Lesser General Public License v2.1
1.07k stars 186 forks source link

Split out (most) hardware dependent code into dedicated file #145

Closed tfry-git closed 2 years ago

tfry-git commented 2 years ago

See #135 , see #125 .

With all the new platform ports added on by one, MozziGuts.cpp has become quite a noodley soup of #ifs . A nightmare to read and maintain. This patch, although barley readable in itself, attempts to restore a readable code base, and make it easy to add new implementations, cleanly.

I also merged most of mozzi_analog.cpp into either MozziGuts.cpp or the new MozziGuts_impl_XYZ.hpp, as this is a) also heavily hardware dependent, and b) already strongly interwoven with the existing MozziGuts.cpp. Having it all in one place makes it much easier to follow, IMO.

For similar reasons I merged the hardware dependent bits from AudioOutput into the corresponding MozziGuts_impl_XYZ.hpp .

Finally, I added a MozziGuts_impl_template.hpp, which is - well - a template for adding support for new hardware.

Some platforms are starting to look almost ridiculously simple, now (esp. Teensy3/4), so I think this is very much the right thing to do.

Testing status: There is still a lot of testing left to do, and I hope you'll help me with that (esp. I don't have any Teensy3/4 or SAMD-boards, everything else will help, too). Here's a table of testing done so far "C" is for "compiles", "W" is for "works":

Config / Example / Feature -- Arch AVR ESP8266 ESP32 SAMD STM32 Teensy3/4 RP2040
defaults / Sinewave W W W C W W
defaults / Sinewave + mozziAnalogRead W W (1) W (1) C W W
HIFI / Sinewave C W (2) W (2) N/A C N/A
defaults / USE_AUDIO_INPUT C N/A N/A C W W
defaults / EXTERNAL_AUDIO_OUTPUT C C C C W

(1) With the known limitation that ADC reads are not asynchronous (and on the ESP8266 there is only one ADC pin, anyway). (2) ESP8266/ESP32 do not have a "HIFI" mode. Instead I tested the alternate PDM_VIA_I2S mode, on these.

tfry-git commented 2 years ago

I will add that the diff is near impossible to read. I rather suggest looking at the separate files, for a first impression, i.e. importantly:

https://github.com/sensorium/Mozzi/blob/work/split_hardware_implementations/MozziGuts.cpp https://github.com/sensorium/Mozzi/blob/work/split_hardware_implementations/MozziGuts_impl_template.hpp and some of the actual implementations, e.g.: https://github.com/sensorium/Mozzi/blob/work/split_hardware_implementations/MozziGuts_impl_STM32.hpp

tfry-git commented 2 years ago

Ok, so I couldn't resist, and added initial RP2040 (aka Raspberry Pi Pico) support on top of this (no documentation, yet).

I know, I should rather be testing than adding new stuff, but in a way, this also was a test that the framework is flexible enough to allow new ports, cleanly.

tfry-git commented 2 years ago

@tomcombriat @adrianfreed : I'm pretty happy with the outcome of my testing, and do not expect any difficult issues (if I did break anything, I rather expect it to show up at compilation stage). Nonetheless, I'd like to get some more confirmation before merging a patch of this size (see the testing status table in the main post). Esp. in the Teensy column, but perhaps also another "W" in the EXTERNAL_AUDIO_OUTPUT row would be nice. Could you help with that?

tomcombriat commented 2 years ago

@tfry-git That's a big baby, and a lot of work! Looking forward to implement new things using this new framework (native I2S on Teensy4 has been in the back of my head for some time now…)!

I can help with both EXTERNAL_AUDIO_OUTPUT and Teensys, I tinkered a lot trying these two things to work on Mozzi and all the hardware is around. Please allow a few days.

Best,

tomcombriat commented 2 years ago

I had time to make a few tests this morning. This is not extensive yet, hence, the following table will be updated (on this same comment) as I progress, with the planned tests. I have split out Teensy 3 and 4 to be on the safe side.

Config / Example / Feature -- Arch AVR Teensy 3 Teensy 4 STM32
defaults / Sinewave W W (1) C (2)
defaults / Sinewave + mozziAnalogRead W W (1)
HIFI / Sinewave N/A N/A
defaults / USE_AUDIO_INPUT W W (1)
defaults / EXTERNAL_AUDIO_OUTPUT W W W (1)

Best, (1) Working with #146 (2) Compiles with #147 EDIT: external audio output (in stereo!) confirmed working on AVR

tfry-git commented 2 years ago

@tomcombriat : Not sure, whether you intended to fill all of that table. I'll be happy to wait another while, but if this it for the time being, I think we can risk sorting out any remaining issues after merging (after all, that last STM32 problem wasn't even specific to this PR).

tomcombriat commented 2 years ago

Hi @tfry-git , My initial intention was to complete your table as much as possible, but not necessarily to complete the one in my comment entirely. I concentrated my efforts on teensy and external audio output for which I have most of my Mozzi experience. I have a hardware running on bluepill with external dac so this will also be treated at some point by I cannot guarantee when.

Considering the results of the tests, I agree that it sounds safe to merge it now, the two small fixes were extremely minor and very quickly fixed (and not necessarily related to this pr). If any remains they should be of the same order.