Palmerr23 i2 s mclk

[palmerr23]

Sorry about the messy PR, I updated the arduino-pico board files after developing this and had to update some of your I2S.cpp code by hand.

Note also the change to setFrequency(long) - simply to match the documentation.

I'll add a PR for the doco later.

Main changes:
Add MCLK support - enabled by setting the MCLK pin.
Sample rate to MCLK multiplier can be independently set to multiples of 64.
Changing the setting of the PIO dividers from float mode to 16:8 mode and restricting the clock dividers to 16:0 only, so that MCLK can be reliably synchronised.
Add SYSCLK to sample rate optimisation - by changing the processor frequency.

[palmerr23]

Hold off on this Earle - the update to the board files has broken a few things.

[palmerr23]

I think everything is OK now. Multiple copy and pastes to align my PR with the 3.3.0 release that occurred midway in development of this code.

[palmerr23]

Sorry Linus,

I'll fix it.

[palmerr23]

Reinserted #1548 code from Linus.

[earlephilhower]

Can you run tools/restyle.sh to fix the formatting issues?

Also, it looks like that maybe there are some old/missing files:

/home/runner/arduino_ide/hardware/pico/rp2040/libraries/I2S/src/I2S.cpp: In member function 'void I2S::MCLKbegin()':
/home/runner/arduino_ide/hardware/pico/rp2040/libraries/I2S/src/I2S.cpp:180:32: error: 'pio_i2s_mclk_program' was not declared in this scope; did you mean 'pio_i2s_in_program'?
  180 |     _i2sMCLK = new PIOProgram(&pio_i2s_mclk_program);
      |                                ^~~~~~~~~~~~~~~~~~~~
      |                                pio_i2s_in_program
/home/runner/arduino_ide/hardware/pico/rp2040/libraries/I2S/src/I2S.cpp:182:5: error: 'pio_i2s_MCLK_program_init' was not declared in this scope; did you mean 'pio_i2s_in_program_init'?
  182 |     pio_i2s_MCLK_program_init(_pioMCLK, _smMCLK, off, _pinMCLK);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~
      |     pio_i2s_in_program_init
Using library I2S at version 2.0 in folder: /home/runner/arduino_ide/hardware/pico/rp2040/libraries/I2S 
Using library AudioBufferManager at version 1.0.0 in folder: /home/runner/arduino_ide/hardware/pico/rp2040/libraries/AudioBufferManager 
exit status 1

[palmerr23]

Will do, probably tomorrow.

[jfrey-xx]

Since I'm also using MCLK I'm eagerly following the PR ^^

To stay consistent with current code, shouldn't you also stop the MCLK PIO upon I2S.end()? I have a use-case where I switch back and forth between I2S and PWM, repeatedly calling I2S.begin/end.

[jfrey-xx]

Also, maybe I missed it, but there is no default value for _pinMCLK?

[palmerr23]

Jeremy,

That is correct. As this is an independent PIO program and not using multiple pins, any legitimate pin can be assigned as for DIN/DOUT.

[palmerr23]

Jfrey,

Yes. I'll add code to terminate the MCLK PIO program on I2S exit.

[jfrey-xx]

Jeremy,

That is correct. As this is an independent PIO program and not using multiple pins, any legitimate pin can be assigned as for DIN/DOUT.

_pinDOUT is set to 28 in the constructor, I am just wondering what would happen here with _pinMCLK, if setMCLK() is not called upon start -- it defaults to 0 and not a random value?

[palmerr23]

Good point. I think PIN 27 makes a more sensible default than PIN 0.

[earlephilhower]

Good point. I think PIN 27 makes a more sensible default than PIN 0.

I would suggest that, by default, no pin is used since most codecs don't require it. Set the default pin to 255 or something and check and only if the app tries to set a pin should we start driving an MCLK..

[palmerr23]

Oops! make that pin 25, pin 27 is LRCLK.

[palmerr23]

Earle,

Yes that's the default behaviour: MCLK is not driven unless the MCLK pin is set.

There is no harm, however in Jeremy's suggestion that a default be set (and not assigned to anything active) in the constructor.

I've also added a default of 256 for _multMCLK

I'll run the tidy-up routine and update the pull, once I've tested the additions.

[palmerr23]

Updates now restyled with restyle.sh

Additions of default pins and stopping MCKL in i2s.end() now tested.

[palmerr23]

I wrote some code to test all possible sysclk and PIO divider permutations. There may be more at higher sysclks, but I stopped checking at 200MHz.

The two I have suggested in the doco are:
44.1kHz (etc) 135.6 MHz. Sample rate error = 0.092%
48kHz (etc) 147.6 MHz. Sample rate error = 0.098%

I found two further 'good' sysclks:
44.1kHz (etc) 158 MHz. Sample rate error = 0.034%
48kHz (etc) 172 MHz. Sample rate error = 0.019%

While these are even closer to the desired sample rates, they are more overclocked. Should I include them in the doco for completeness?

[earlephilhower]

Took some time, but looks like it's building OK finally! Because this is a little big, I'll need a few days to look it over.

If anyone watching the PR could give it a try in their application that would be great, too.

[earlephilhower]

Very nice work and thanks for the effort in packaging it up. Some really minor niggles and I think we're all set!

[earlephilhower]

Oh, and double thanks for actually adding documentation for your changes!!!

[palmerr23]

Thanks Earle and Linus,

I'll get to all the minor issues - in which you are both correct in all cases.

It might take a few days, as my fork seems to be out of date with the base branch. I rarely use git in community contribution mode and I'm not sure how to bring my fork into line without overwriting all my changes, as it did last time when I hit the "update branch" button. I'd rather abandon the pull than go through that pain again!

For the one below, I was intending to release the pin, rather than set it to false. My bad, not having written a lot of PIO code. Clearly, once the PIO code abort, it will no longer try to change the pin value, and PIO to pin mappings are not exclusive.

void I2S::end() {
    if (_MCLKenabled && _running) {
        pio_sm_set_enabled(_pioMCLK, _smMCLK, false);
        _MCLKenabled = false;

Richard

[earlephilhower]

Don't worry about the update, just do your changes and push. The online update branch button has never given me any trouble (and if there are any conflicts they can be edited online by me if needed).

FWIW I normally just do the update branch from the web interface and then just git pull while in my editing branch and things generally "just work."

[palmerr23]

:-) Thanks

…

_________________________ Richard Palmer 0417 514 473

On Mon, 3 Jul 2023 at 13:01, Earle F. Philhower, III < ***@***.***> wrote: Don't worry about the update, just do your changes and push. The online update branch button has never given me any trouble (and if there are any conflicts they can be edited online by me if needed). FWIW I normally just do the update branch from the web interface and then just git pull while in my editing branch and things generally "just work." — Reply to this email directly, view it on GitHub <#1555 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADRLBMIJE4JXX6WCB335WBDXOIYZBANCNFSM6AAAAAAZPSIKDM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[palmerr23]

I've made all the changes suggested, with the exception of removing #include <pico/stdlib.h> which is required for the call to set_sys_clock_khz().

The doco file is full of CR/LF sequences, however you don't seem to care about these. All other changes have been restyled.

[earlephilhower]

Thanks, looks good now!

I fixed some merge weirdness (had <<<old ..... >>> new .... ==== section) and the I2S::end() lost the MCLK stop bit completely. Re-added that and deleted the PIOProgram to avoid a memory leak. Other very minor things scattered around hence the flurry of small change emails you probably just received.

[earlephilhower]

Thanks again! Know it was a long slog, but we do really appreciate the efforts!

[LinusHeu]

I think #1548 is still/again accidentally overwritten and should be restored

[earlephilhower]

Huh, did not notice that. Good catch! See #1582 .