Repository navigation
Conversation
|
Using it here and is working well: https://github.com/hybridgroup/tinyplayer |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new stereo packing documentation reverses the channels produced by the implementation.
Review effort: Balanced
Findings: 1
What changed in this PR
Updates RP2 I2S playback to reduce underruns and correct audio timing.
Changes:
- Adds double-buffered DMA writes with FIFO fallback.
- Corrects sample-rate calculation.
- Duplicates mono samples across both channels.
| File | Description |
|---|---|
rp2-pio/piolib/i2s.go |
Implements DMA playback and corrects I2S timing and mono output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| // WriteStereo writes a stereo audio buffer to the I2S peripheral. | ||
| // Each value is one frame with the left sample in the low half. |
There was a problem hiding this comment.
The doc is correct. The program starts at instruction 7 and goes into bitloop1, which runs with LRCLK=1, so bits 31 to 16 are the right channel and bits 15 to 0 are the left. LRCLK changes one bit before each MSB as Philips I2S requires. I checked on a XIAO RP2350 with a PCM5102 using a WAV that says "left" only on the left channel and "right" only on the right, packed with left in the low half, and each word came out of the matching ear.
| func i2sWrite[T uint16 | uint32](i2s *I2S, b []T) (int, error) { | ||
| if len(b) == 0 { | ||
| return 0, nil | ||
| func i2sWrite[T uint16 | uint32](i2s *I2S, b []T, mono bool) (int, error) { |
There was a problem hiding this comment.
can replace mono bool with compile-time check
func i2sIsMono[T uint16| uint32]() {
return unsafe.Sizeof(T(0)) == 2
}There was a problem hiding this comment.
Done in c044cec, mono is now decided by the sample type.
Writes now copy into two buffers that DMA sends to the PIO in turn. Before, each frame was pushed into the 4 word TX FIFO by the CPU, so the FIFO ran empty while the caller prepared the next buffer and the audio was distorted. The TX FIFO is also joined to 8 words. If no DMA channel is free the old FIFO loop is used. SetSampleFrequency now uses 64 PIO cycles per frame. The program takes 2 instructions per bit, so audio played at half speed. WriteMono now plays each sample on both channels like machine.I2S does.
|
Thank you for contribution! |
tinygo-org/pio#64 is merged, so go.mod now uses that commit on the pio main branch instead of the PR branch commit, which is gone after the squash.
This fixes two problems with
piolib.I2Sthat show up when playing audio.Distorted audio. Each frame was pushed into the 4 word TX FIFO by the CPU. The FIFO only covers about 90 µs at 44.1 kHz, so it ran empty while the caller decoded the next buffer. Writes now copy into two 256 word buffers that DMA sends to the PIO in turn, and the TX FIFO is joined to 8 words. If no DMA channel is free the old FIFO loop is used.
Half speed playback.
SetSampleFrequencyused 32 PIO cycles per frame. The program takes 2 instructions per bit, so a frame is 64 cycles, the same as pico-extrasaudio_i2s.c.WriteMononow plays each sample on both channels, matchingmachine.I2Son nRF52 and ESP32.Tested on a XIAO RP2350 with a PCM5102 playing 8, 16 and 44.1 kHz WAV files. Counting TX stalls with
HasTxStalledbetween writes went from 87 to 0 for a 2 second 44.1 kHz stereo clip, and the clips now play at the right speed.