audiocore-mp3-decoder #12

Merged
matthias merged 6 commits from audiocore-mp3-decoder into main 2025-03-22 10:19:13 +00:00
Owner

Implement mp3 decoding in audiocore module on core1 using the helix_mp3 fixed point MP3 decoder library.
The audiocore module and C module should be complete, the higher-level mp3player module is still WIP.

Implement mp3 decoding in audiocore module on core1 using the helix_mp3 fixed point MP3 decoder library. The audiocore module and C module should be complete, the higher-level mp3player module is still WIP.
matthias self-assigned this 2025-03-17 20:15:20 +00:00
matthias added 4 commits 2025-03-17 20:15:21 +00:00
Rename the exising audiocore C module to _audiocore and create a new
Micropython wrapper module audiocore. This makes it easier to implement
async methods.
Add interrupt support to _audiocore that notifies core0 whenever data
has been consumed from the MP3 bitstream buffer.
Use this interrupt and an asyncio.ThreadSafeFlag to implement
audiocore.async_put which will play back the provided buffer, allowing
other async tasks to run while waiting for space in the bitstream
buffer.
wip: MP3Player module to manage mp3 playing
All checks were successful
Build RPi Pico firmware image / Build-Firmware (push) Successful in 2m56s
Check code formatting / Check-C-Format (push) Successful in 7s
Check code formatting / Check-Python-Flake8 (push) Successful in 8s
Check code formatting / Check-Bash-Shellcheck (push) Successful in 5s
Run unit tests on host / Run-Unit-Tests (push) Successful in 9s
da48e7546f
matthias requested review from stefank 2025-03-17 20:15:22 +00:00
matthias force-pushed audiocore-mp3-decoder from da48e7546f to 3a0784c863 2025-03-18 21:18:05 +00:00 Compare
matthias changed title from WIP: audiocore-mp3-decoder to audiocore-mp3-decoder 2025-03-18 21:18:24 +00:00
stefank approved these changes 2025-03-21 21:45:02 +00:00
stefank left a comment
Member

Just a few questions, and one irrelevant typo.

Just a few questions, and one irrelevant typo.
@@ -27,0 +55,4 @@
uint32_t *buf;
if (multicore_fifo_rvalid()) {
cmd = multicore_fifo_pop_blocking();
switch (cmd) {
Member

That#s not something we need to do now, but we definitely need to document the IPC protocol. I am getting lwIP vibes here ^^

That#s not something we need to do now, but we definitely need to document the IPC protocol. I am getting lwIP vibes here ^^
Author
Owner

There is some "documentation" in audiocore.h where the commands are defined.

There is some "documentation" in audiocore.h where the commands are defined.
@@ -27,0 +87,4 @@
i2s_play(samplerate);
playing = true;
}
volume_adjust((int16_t *)buf, 2304, current_volume);
Member

2304 should probably be a constant (I think this is the second time I saw it in our codebase)

2304 should probably be a constant (I think this is the second time I saw it in our codebase)
matthias marked this conversation as resolved
@@ -86,0 +89,4 @@
// FLUSH - signals end of file and stop decoding when buffer empty - no arguments - return 0 when decoding is finished
#define AUDIOCORE_CMD_FLUSH 0xdeadc0dd
#define AUDIOCORE_MAX_VOLUME 0x8000u
Member

Stumbled across this because of volume_adjust() and the 15 bit right shift.

I am only a little bit confused, so here's a stupid question: do we implement an efficient 'clamp' here?

Stumbled across this because of `volume_adjust()` and the 15 bit right shift. I am only a little bit confused, so here's a stupid question: do we implement an efficient 'clamp' here?
Author
Owner

The volume adjustment is a kind of fixed point multiplication right now. We multiply our signed 16 bit sample with a 16 bit multiplier and then discard the lower 15 bits. So AUDIOCORE_MAX_VOLUME is in effect a multiply by 1, the PCM samples stay the same. And any lower volume reduces the amplitude. The code can currently not increase the volume, which saves us the trouble of having to deal with clamping and such.
Do you think it is necessary to be able to increase the volume here?

The volume adjustment is a kind of fixed point multiplication right now. We multiply our signed 16 bit sample with a 16 bit multiplier and then discard the lower 15 bits. So AUDIOCORE_MAX_VOLUME is in effect a multiply by 1, the PCM samples stay the same. And any lower volume reduces the amplitude. The code can currently not increase the volume, which saves us the trouble of having to deal with clamping and such. Do you think it is necessary to be able to increase the volume here?
@@ -80,0 +147,4 @@
i2s_context.cur_playing = 0;
memset(i2s_context.dma_buf, 0, sizeof(i2s_context.dma_buf[0][0]) * AUDIO_BUFS * I2S_DMA_BUF_SIZE);
i2s_max98357_program_init(audiocore_pio, i2s_context.pio_sm, i2s_context.pio_program_offset, i2s_context.out_pin,
i2s_context.sideset_base, samplerate);
Member

Ah, I was already wondering why i2s needs to know about the samplerate. Check 👍

Ah, I was already wondering why i2s needs to know about the samplerate. Check 👍
matthias marked this conversation as resolved
@@ -81,3 +157,4 @@
{
pio_sm_set_enabled(audiocore_pio, i2s_context.pio_sm, false);
dma_channel_set_irq1_enabled(i2s_context.dma_ch, false);
dma_channel_abort(i2s_context.dma_ch);
Member

Shouldn't be dma_channel_abort() and irq_remove_handler() be the other way round because of E13?
Ah, bullshit, line 83 takes care about that... Check 👍

~~Shouldn't be dma_channel_abort() and irq_remove_handler() be the other way round because of E13?~~ Ah, bullshit, line 83 takes care about that... Check 👍
matthias marked this conversation as resolved
@@ -63,2 +99,3 @@
if (to_copy > 0) {
audiocore_audio_buffer_put(bufinfo.buf, to_copy);
audiocore_buffer_put(bufinfo.buf, to_copy);
__sev();
Member

Aaah, ARM Compiler manual says this is "Set Event" - not Schienenersatzverkehr...

Aaah, ARM Compiler manual says this is "Set Event" - not Schienenersatzverkehr...
matthias marked this conversation as resolved
@@ -95,0 +143,4 @@
if (volume < 0 || volume > 255)
mp_raise_ValueError("volume out of range");
multicore_fifo_push_blocking(AUDIOCORE_CMD_SET_VOLUME);
multicore_fifo_push_blocking(AUDIOCORE_MAX_VOLUME * volume / 255);
Member

Partly complements my question from above.

The volume setting is something I would like to know more about. I am a bit tired right now, let's talk about this some later...

Partly complements my question from above. The volume setting is something I would like to know more about. I am a bit tired right now, let's talk about this some later...
Author
Owner
See https://git.ka.blankertz.org/TonBERRY/tonberry-pico/pulls/12#issuecomment-151
matthias marked this conversation as resolved
@@ -0,0 +29,4 @@
}
}
static size_t __time_critical_func(mp3_get_continous_data)(unsigned char **readptr)
Member

continuous

continuous
matthias marked this conversation as resolved
matthias force-pushed audiocore-mp3-decoder from 3a0784c863 to 1385eee85c 2025-03-22 09:54:32 +00:00 Compare
matthias merged commit 72d5d97d46 into main 2025-03-22 10:19:13 +00:00
matthias deleted branch audiocore-mp3-decoder 2025-03-22 10:19:13 +00:00
Sign in to join this conversation.
No Reviewers
No Label
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: TonBERRY/tonberry-pico#12