Add initial DatanoiseTV PicoADK Board Support #169
Conversation
There was a problem hiding this comment.
Hei,
I have made a few comments. None of them is completely definitive, let us see if the others have some.
In the general way, I think that we do not want, nor I think this is desirable, to put to much details about the different boards for one arch inside Mozzi. First this will become quite easily a clutter (and we cannot test all of them) and also some designer might not actually put them in, leading to a mixed solution where some boards are defined until the end and some needs some customization by the user. I am more in favor of giving the user the config files (mozzi_config.h and AudioConfigRP2040.h in this case) needed to run on a specific board. Once more, let's see what the other say.
Finally, you have a lot of commits for not that many changes, some of them being irrelevant (one of them is because you started editing on an "old" version of Mozzi). I suggest creating a specific branch in your fork with only the relevant commits.
Best,
| #if IS_RP2040() | ||
| #define AUDIO_RATE_PLATFORM_DEFAULT 32768 | ||
| #endif | ||
|
|
There was a problem hiding this comment.
I do not think this is needed as the RP2040 is not AVR so the test on line 35 will be false and the AUDIO_RATE was already at 32768.
There was a problem hiding this comment.
Have you checked this?
| #elif IS_RP2040() | ||
| uint32_t rand_seed; | ||
| for (int i = 0; i < 32; i++) | ||
| { | ||
| bool randomBit = rosc_hw->randombit; | ||
| rand_seed = rand_seed | (randomBit << i); | ||
| } | ||
| x = random (rand_seed); | ||
| y = random (rand_seed); | ||
| z = random (rand_seed); | ||
| #else |
There was a problem hiding this comment.
Haven't tested but look good to me.
MozziGuts_impl_RP2040.hpp
Outdated
| #if (ARDUINO_DATANOISETV_PICOADK) | ||
| // Soft mute and de-emphasis for audio codec | ||
| pinMode(25, OUTPUT); | ||
| digitalWrite(25, HIGH); | ||
| pinMode(23, OUTPUT); | ||
| digitalWrite(23, LOW); | ||
| #endif |
There was a problem hiding this comment.
I do not think it is desirable that the details of every board created are in Mozzi. Architectures yes, but if we start to "pre-config" every board that goes out this is a never ending story. Especially as what is done here can easily be done in the sketch.
AudioConfigRP2040.h
Outdated
| // The arduino-pico SDK has I2S Pins predefined for some boards. | ||
| #if defined(ARDUINO_ARCH_RP2040) | ||
|
|
||
| #if defined(PIN_I2S_BCLK) | ||
| #define BCLK_PIN PIN_I2S_BCLK | ||
| #define WS_PIN (PIN_I2S_BCLK+) | ||
| #endif | ||
|
|
||
| #if defined(PIN_I2S_DOUT) | ||
| #define DOUT_PIN PIN_I2S_DOUT | ||
| #endif | ||
|
|
||
| #endif | ||
|
|
There was a problem hiding this comment.
Is this erasing the config for all cases? When is this test going true? I would be against putting these pins at the default values of the arduino-pico library, with no easy change if that is the case. I think that (alongside the comment on the support of ARDUINO_DATANOISETV_PICOADK) this kind of config should be given by the board designer and not be hardcoded in Mozzi.
|
I tend to agree that this PR looks too specific to just one board, overall. It's hard enough to maintain code for many different platforms. Providing specifics for individual boards just will not scale. Sure, if this board gains the traction of an Arduino Uno, we would have to re-evaluate, but short of that, I do not think, any specific pin mappings should end up in the library. In addition, let's not rule out the case that a user of that board may still want to use a custom separate SPI DAC with Mozzi. It may of course be worth documenting instructions for working with specific boards in a dedicated section of the Readme. The random seeding is definitely a candidate for merging, however. Could you split that into a separate PR? |
So far, there are around 150 around the world. Maybe using the I2S_PIN_* as used in the arduino-pico might make more sense if present for now?
I can manage that next week. |
|
I would be favorable in setting the Pins defaults as per the library or board definition if that does not restrain the user to simply change to a custom configuration (without heavily editing |
No description provided.