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

Teensy4x port #138

Closed tomcombriat closed 2 years ago

tomcombriat commented 2 years ago

This adds supports for Teensy4.

As the Teensy4 do not have any DAC, the default output is PWM on pin A8 (chosen for the lack anything useful on this pin for audio applications, who needs CAN anyway ;) ). Obviously, because of the capacity of this chip (800MHz base frequency, more than 1GHz on overclock!) it is a bit sad to use it with PWM but that gives a default "bare chip" output capacity.

Output has been tested, a few things remain to be tested before even considering merging:

I am personally looking forward for the sounds that can created with that much CPU power!

Best,

tomcombriat commented 2 years ago

Hi again,

I have faced some difficulties with AudioInput because of a change in the ADC.h library of Teensyduino. Weird thing, in order for things to compile I had to declare the ISR in mozzi_analog.h (see last commit #68d7edf), even thought it was not the case before… @tfry-git you were the author of the original Teensy port, would you have an idea why this change was needed? It looks logic for me that, as the ISR is needed to be declared for the compilation of MozziGuts.cpp or mozzi_analog.cpp (depending if the input audio is wanted or not) but then I do not understand while it was compiling before.

I have to say that it is the first time I use this feature on Mozzi, but even mozziAnalogRead would not work on Teensy before this declaration (I never used mozziAnalogRead on Teensy before either actually…). Worth mentionning that, I just updated my Arduino to 1.8.16 whereas I was at 1.8.5 before.

That said, it is maybe only needed for the Teensy and the declaration in mozzi_analog.h could be put only for the Teensys.

Some tests are still needed to see if that mess up something for AVR/STM etc, but also for the external_audio_output.

Best,

tomcombriat commented 2 years ago

Ok, after thinking about it, I think it actually makes perfect sense when seeing the changes made to the Teensy ADC library.

As there is no DAC on Teensy I also added a default (PWM) pin, and adapted AudioOuptut to accommodate stereo mode for this new Teensy.

Still require a bit of testing but I think it starts to look good!

tfry-git commented 2 years ago

Glad you figured it out (I had no idea), but for the record, @sensorium is the original author of the Teensy port.

tomcombriat commented 2 years ago

Oups, sorry @sensorium! That was actually thanks to this initial port that I discovered the Teensys (and the STM32 some time after…).

Ok, I changed this PR to "ready" because it is working now perfectly on Teensy 4 (mono, audio input, stereo, external audio and the two later together).

I had to adapt the examples for the external audio because the SPI on this chip is so fast that the DAC could not follow (for the other platform the maximum was less than the maximum of the chip so no problem).

I did not do extensive tests on the other platforms (I tested a few sketches on AVR without any problems though) because they should not be affected by that. I also tested that the initial Teensy3 port still worked as it was the most likely to have been affected by mistake.

The Teensy4 would be, for now, the most powerful chip supported by Mozzi (and it has an FPU and DSP), allowing for very advanced sound synthesis. Please let me know if you have any question or suggestion,

Best,

sensorium commented 2 years ago

Great work, @tomcombriat!

It must be time to update/regenerate the html documentation to reflect recent developments. Is there anything you want to add to the code documentation before I regenerate with doxygen, or changes to the main page?

Maybe this should be a different thread, but to me, it seems ironic to use Mozzi on more powerful platforms since the main challenge initially was optimising it for 8 bit processing. Most of the synthesis classes are so simple I have often wondered if it would be easier to rewrite from scratch for better boards rather than keep adapting with the limitations of the 8 bit version.

I don't have much time to spend on Mozzi these days, but I am slowly trying to change the license to GPL3 (I did the editing a while ago but got lost while keeping up with the recent changes!) The GPL license could make the code more useful to others.

tomcombriat commented 2 years ago

Thanks @sensorium , I like Mozzi and I am happy to try to contribute to it!

Since the last update of the documentation a lot of things have been modified but principally under the hood (#98 and the followings). However, I think that the main page should at least reflect that it is now possible to use additional hardware to output the sound thanks to EXTERNAL_AUDIO_OUTPUT (#87 and #97 ) allowing for audio quality output if needed. There are a few open PR also that maybe should or should not be included before regenerating the docs.

it seems ironic to use Mozzi on more powerful platforms since the main challenge initially was optimising it for 8 bit processing.

Indeed most of the PR these days are actually about keeping that optimization for AVR while not making it a limitation for more powerful platforms, sometimes through less optimized solutions. Actually that is what I really like about Mozzi, it can be used on a lot bunch of different platforms and output very different qualities of sound depending on what is the intended use, it is very flexible which is great! Obviously the bad side effect become clear when opening the guts of Mozzi but that is becoming easier and easier thanks to the modulation work of @tfry-git . Maybe this discussion should be in a new thread as you said. That would be interesting to have a place to discuss future developments and ideas for Mozzi!

tomcombriat commented 2 years ago

This seems to resolve the compilation issue for Teensy 3.2/LC and 4. However it seems from the latest issue ( #139 ) that LC is hanged upon call of the ISR for mozziAnalogRead(). I think this should be resolved before merging and will commit myself to that. I admit that I have troubles understanding what is the difference with 3.2 except that it has one ADC converter but this is taken into account in the Teensy ADC library…

tomcombriat commented 2 years ago

Okaaaaaaay, I got a Teensy LC for testing! At first everything seemed to work alright, even mozziAnalogRead

… until I tried to read more than one pin. Then, indeed, problems arise. This might also be the cases with the others actually, I have to say that testing several analogRead was not part of my testing procedure. Even before this commit I never used mozziAnalogRead on a teensy…

Will dig more and - hopefully - come with a fix soon.

tomcombriat commented 2 years ago

I put my hand on something fuzzy:

For the Teensy (LC at least), NUM_ANALOG_INPUTS is equal to 13. However, if I try to read the pin A4, the affected channel (current_channel) is equal to 18. This leads to an overflow of the analog_readings stacks (which is of size 13), because the pin number is directly used on the Teensy (current_channel = pin).

This overflow does not lead to any problem in most cases - at least here - when only one value is read: the value is written in the wrong place, read in the wrong place, if nothing else is there no problem. With more pin read, this overflow is getting worse and start reading, or in very wrong places.

A quick fix would be to increase the sizes of these two stacks. But it might be cleaner to actually use the adcPinToChannelNum function already there for arduino to shift the pin number only, to use a stack of minimal size.

I'll first check if this is affecting all Teensys, and come with a fix using hopefully the second solution soon. Let me know if you have any comments!

tomcombriat commented 2 years ago

The last commit should - in theory - solves the ADC issues for all Teensys supported (3.X, 4.X). The main problem with mozziAnalogRead on Teensy was that the pin number was used as index for the analog_readings array, even thought the smallest pin number was not 0. This led to overflows with more or less dramatic consequences. To fix that I made a map of the pins to give them a valid index in the expected range.

This has been tested on Teensy LC, soon to be on 3.2 and 4.0.

Happy to take comments if there is a more intelligent way to do this map :) !

Best,

tomcombriat commented 2 years ago

Alright, the previous proposition solved the overflow in analog_readings but another problem occurred during testing. Some of these chips (basically all of them except the Teensy LC) have two internal ADC. The problem is that, each analogInput pin is not necessary connected to the two ADC, sometimes only to one.

Henceforth, a test needs to be made when the chip has two ADC in order to know which one to use, the last commits 0c96d3 and so on are implementing that. Tested on Teensy 3.2 and Teensy 4.0. Teensy LC should not have been affected by the last commits as it has only one ADC but will try to test it soon too… After this last test I'll try to update the news for the front page which should be coherent with this update.

Every time I try to modify "something simple" in Mozzi it ends up to be a big story, sorry for that.

PS: on teensy 4.0, the readings on A8 are not stable at all, looking like a square… I do not understand why. This is the only pin where this happens and even forcing one ADC or the other do not fix the problem. I think this is minor and can be solved by using a plain analogRead for this pin but prefer to mention it. It might also be my chip.

Best,

tomcombriat commented 2 years ago

Hello!

This is now also tested on Teensy LC with no problem. I also tried to update the Readme to reflect the latest changes. I was not very inspired so feel free to modify it! I put my name for the Teensy4 port, feel also free to remove it, this was mainly a modification of the original Teensy3 port, most of my commits were actually to fix other small related problems.

I think this is now in final state, or very close to it! Best,

tomcombriat commented 2 years ago

@tfry-git Everytime I learn new things… I just successfully squashed my commits into three. This was my first time ;).

I'll soon start a device using that port, if a minor bug escaped my and your vigilance it should be spotted fairly quickly.

Thanks for your help!

tfry-git commented 2 years ago

Great!

So you're saying, we'll wait another short while before merging, so you can do some final testing. Right?

tomcombriat commented 2 years ago

Hei,

Not really, I think this is mergeable state now, ADC problems for all Teensys are solved, alongside the slight out-of-tune of the output, plus the Teensy4 support. If something went underlooked it has to be fairly small as it went through tests. I was more meaning that these small details (if there are some) will probably be spotted in the future, by me or someone else (like the ADC issue), but this is basically working now.