Add RP2 PIO background read #9659

timchinowsky · 2024-09-24T22:25:58Z

This PR adds a background_read method to rp2pio.StateMachine to complement the existing background_write.

jepler

Thanks for this, I'm excited!

I did have some review comments & requested changes. I also didn't actually test the code. Any suggestion on something that would be useful to try/test if I don't have a TAC5xxx codec?

jepler · 2024-09-25T02:07:07Z

ports/raspberrypi/bindings/rp2pio/StateMachine.c

+static void fill_buf_info_read(sm_buf_info *info, mp_obj_t obj, size_t *stride_in_bytes) {
+    if (obj != mp_const_none) {
+        info->obj = obj;
+        mp_get_buffer_raise(obj, &info->info, MP_BUFFER_WRITE);
+        size_t stride = mp_binary_get_size('@', info->info.typecode, NULL);
+        if (stride > 4) {
+            mp_raise_ValueError(MP_ERROR_TEXT("Buffer elements must be 4 bytes long or less"));
+        }
+        if (*stride_in_bytes && stride != *stride_in_bytes) {
+            mp_raise_ValueError(MP_ERROR_TEXT("Mismatched data size"));
+        }
+        *stride_in_bytes = stride;
+    } else {
+        memset(info, 0, sizeof(*info));
+    }
+}


Can you please combine the fill_buf_info_{read,write} routines? I notice you've added detection of mismatched stride between the "once" and "loop" variants, which should probably apply to write as well. I think you'd just have to add the access type parameter and pass MP_BUFFER_{READ,WRITE} in.

Or is there a reason the new checking you added should not also have been done for the background write case?

jepler · 2024-09-25T02:07:52Z

ports/raspberrypi/bindings/rp2pio/StateMachine.c

 //|     pending: int
+//|     pending_write: int


Please document that the pending property is a deprecated alias for pending_write and will be removed in a future version of CircuitPython.

jepler · 2024-09-25T02:10:07Z

ports/raspberrypi/bindings/rp2pio/StateMachine.c

+}
+MP_DEFINE_CONST_FUN_OBJ_1(rp2pio_statemachine_get_pending_write_obj, rp2pio_statemachine_obj_get_pending_write);
+
+const mp_obj_property_t rp2pio_statemachine_pending_write_obj = {


Please use the MP_PROPERTY_GETTER macro here, even though it wasn't used in the code whose style you were emultaing elsewhere in this file. We should tidy away the direct uses of const mp_obj_property_t that have not previously been done, like the other ones in this file.

jepler · 2024-09-25T02:14:00Z

ports/raspberrypi/common-hal/rp2pio/StateMachine.c

@@ -1023,121 +1047,241 @@ uint8_t rp2pio_statemachine_program_offset(rp2pio_statemachine_obj_t *self) {
    return _current_program_offset[pio_index][sm];
 }

-bool common_hal_rp2pio_statemachine_background_write(rp2pio_statemachine_obj_t *self, const sm_buf_info *once, const sm_buf_info *loop, uint8_t stride_in_bytes, bool swap) {
+bool common_hal_rp2pio_statemachine_background_write(rp2pio_statemachine_obj_t *self, const sm_buf_info *once_write, const sm_buf_info *loop_write, uint8_t stride_in_bytes, bool swap) {


I wouldn't personally have changed the parameter and local variables names but this is OK.

I wanted to make it clearer that the parameters are buffers not flags, as loop is used for instance in https://github.com/adafruit/circuitpython/blob/main/ports/raspberrypi/audio_dma.c

jepler · 2024-09-25T02:14:51Z

ports/raspberrypi/common-hal/rp2pio/StateMachine.h

    int background_stride_in_bytes;
-    bool dma_completed, byteswap;
+    bool dma_completed_write, byteswap;


hmm are two byteswap values (one for read, one for write) needed?

timchinowsky · 2024-09-25T16:30:35Z

Thanks for this, I'm excited!

I did have some review comments & requested changes. I also didn't actually test the code. Any suggestion on something that would be useful to try/test if I don't have a TAC5xxx codec?

For testing with no codec, I'm getting together a loopback example similar to https://github.com/adafruit/Adafruit_CircuitPython_PIOASM/blob/main/examples/pioasm_i2s_codec.py that will send a background_write to a background_read by connecting output and input data pins. If you're interested in messing with the codec itself, I've got some extras I could send you, let me know.

timchinowsky · 2024-09-26T21:50:14Z

Any suggestion on something that would be useful to try/test if I don't have a TAC5xxx codec?

I've added a loopback example at https://github.com/timchinowsky/tac5 which you should be able to run as is on a Feather RP2040 if you connect DOUT (D9) to DIN (D10).

timchinowsky · 2024-10-24T22:22:43Z

The last two commits add double buffering and a last_read property which (a) returns the buffer which was last completely filled by background reads and (b) self-clears, so that if the property is read again before the next buffer swap, it will return an empty buffer. This allows data read in the background to be recorded to SD with code as simple as

with open(filename, 'w') as f:
    while True:
        f.write(self.pcm.pio.last_read)

With the double-buffering and last_read property there are a lot of sm_buf_info's and mp_obj_t buffer objects hanging around, which I'm hoping to improve, but it seems to work. In the saved file I can see

The four-line sections starting with F7 and F6 are written from alternating buffers, and have the expected content.

add double-buffering for background read and write

timchinowsky · 2024-11-07T22:59:43Z

Background playback of 8-channel audio from SD card to a 4-codec TAC5212 setup now working! Details at https://github.com/timchinowsky/tac5.

relic-se · 2024-11-07T23:04:24Z

Hey @timchinowsky ! I've been loving this new feature and have recently implemented in a simple general purpose I2S library: https://github.com/relic-se/CircuitPython_I2SInOut. So far so good, but if you need any assistance with reviewing these updates further, let me know how I can help.

timchinowsky · 2024-11-07T23:07:21Z

@relic-se that's great, thanks for letting me know!

relic-se · 2024-11-13T00:16:41Z

I'm not certain exactly what your goals in the recent updates are, but rather than building bespoke audio DSP processes for the rp2pio module, it would likely be better to find a way to integrate it into the existing audio systems of CircuitPython using the audiosample API. That way the existing processing modules could be used with custom PIO systems (ie: audiomixer, audiofilters, audiodelays, + future additions).

Alternatively, a separate audiobusio.I2Sin class could be created with the sole purpose of being an audio source in these existing modules (see audiocore.RawSample as an example). I've been working on this particular addition on the side, but it's not yet ready to share.

relic-se · 2024-11-13T00:20:05Z

It's also possible to feed data from rp2pio into an audiocore.RawSample buffer when double-buffering is enabled using the background_read functionality. See the "effect" example in the library I shared earlier for a demonstration of this: https://github.com/relic-se/CircuitPython_I2SInOut/blob/master/examples/i2sinout_effect.py.

timchinowsky · 2024-11-13T17:11:31Z

I'm not certain exactly what your goals in the recent updates are, but rather than building bespoke audio DSP processes for the rp2pio module, it would likely be better to find a way to integrate it into the existing audio systems of CircuitPython using the audiosample API.

Yes, totally. I was thinking when I started this that I would be leaning more on audiocore, but its limitations (<=16-bit, <=2 channels) made it difficult to explore the full capabilities of the codec I am working with. Now that I've got the codec working well with rp2pio I'm going to see what can be done as far as unification. For example, suppose that an 8-channel ADC could present as 8 mono rawsample-like objects or 4 stereo objects, etc., and that an 8-channel DAC could present as 4 stereo I2SOut objects, and so forth.

That said, I also like the idea of enabling ulab-like procedural processing of signals, to complement synthio's declarative style. I first got into CircuitPython as a tool for teaching electrical engineering lab classes, and making the signal processing more explicit and transparent would be great for that application.

I'm also planning to continue filling out the API for the TAC5212 codec. For instance, the part has built in biquad filters on both ADCs and DACs (up to 3 per channel) and optional loopback between ADC and DAC, so the part can be used to implement 6-stage biquad analog-in/analog out filtering without any microcontroller involvement at all.

relic-se · 2024-11-13T20:39:03Z

Yes, totally. I was thinking when I started this that I would be leaning more on audiocore, but its limitations (<=16-bit, <=2 channels) made it difficult to explore the full capabilities of the codec I am working with. Now that I've got the codec working well with rp2pio I'm going to see what can be done as far as unification. For example, suppose that an 8-channel ADC could present as 8 mono rawsample-like objects or 4 stereo objects, etc., and that an 8-channel DAC could present as 4 stereo I2SOut objects, and so forth.

Okay, I see where you're coming from here. Multi-channel output is fairly niche, so I don't think it's at the forefront of development in those areas. Yet, I don't see why one could operate with multiple mono/stereo outputs simultaneously as you've described for the time being.

As I've been playing around more with the API, I do think it's possible to connect an object like audiocore.RawSample to a StateMachine input and audioio.AudioOut as an output. For multi-channels, a few parameters like channel_spacing (total number of channels), channel_offset (starting channel index), and channel_count (mono/stereo) could be used to iterate through the appropriate buffers.

That said, I also like the idea of enabling ulab-like procedural processing of signals, to complement synthio's declarative style. I first got into CircuitPython as a tool for teaching electrical engineering lab classes, and making the signal processing more explicit and transparent would be great for that application.

Currently, the new process method feels out of scope for rp2pio. I think it would make much more sense to provide a separate DSP library that acts like ulab that you could operate directly on WriteableBuffer objects as you've described.

I'd really like to see the new background_read additions be added to the core. I think it might be best to save these new additions for another PR rather combine them with this one.

timchinowsky · 2024-11-14T16:49:50Z

SGTM 🚀

timchinowsky mentioned this pull request Sep 24, 2024

audio: continuous DMA from AudioIn, real-time audio filtering #2676

Open

jepler requested changes Sep 25, 2024

View reviewed changes

timchinowsky requested a review from jepler October 24, 2024 22:23

timchinowsky added 9 commits November 7, 2024 14:20

add pio background read,

fad4727

add double-buffering for background read and write

address review comments, add beginning of api for double-buffering

6b9fcb4

fix docs

9e52138

add double buffering

6ff3fe8

add last_read property

1e8f045

address conflicts

62061c8

address conficts

4aaa867

add last_write property

0ef45ca

remove whitespace

10dfea4

timchinowsky force-pushed the add-pio-background-read branch from b5019a4 to 10dfea4 Compare November 7, 2024 22:34

timchinowsky added 2 commits November 11, 2024 12:55

remove redundant buffer object storage, start processing function

540dcfc

add DSP demos

a01b336

remove DSP demo

75501d4

timchinowsky marked this pull request as ready for review November 14, 2024 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RP2 PIO background read #9659

Add RP2 PIO background read #9659

timchinowsky commented Sep 24, 2024

jepler left a comment

jepler Sep 25, 2024

jepler Sep 25, 2024

jepler Sep 25, 2024

jepler Sep 25, 2024

timchinowsky Oct 1, 2024

jepler Sep 25, 2024

timchinowsky commented Sep 25, 2024

timchinowsky commented Sep 26, 2024

timchinowsky commented Oct 24, 2024 •

edited

Loading

timchinowsky commented Nov 7, 2024

relic-se commented Nov 7, 2024

timchinowsky commented Nov 7, 2024 •

edited

Loading

relic-se commented Nov 13, 2024

relic-se commented Nov 13, 2024

timchinowsky commented Nov 13, 2024

relic-se commented Nov 13, 2024

timchinowsky commented Nov 14, 2024

Add RP2 PIO background read #9659

Are you sure you want to change the base?

Add RP2 PIO background read #9659

Conversation

timchinowsky commented Sep 24, 2024

jepler left a comment

Choose a reason for hiding this comment

jepler Sep 25, 2024

Choose a reason for hiding this comment

jepler Sep 25, 2024

Choose a reason for hiding this comment

jepler Sep 25, 2024

Choose a reason for hiding this comment

jepler Sep 25, 2024

Choose a reason for hiding this comment

timchinowsky Oct 1, 2024

Choose a reason for hiding this comment

jepler Sep 25, 2024

Choose a reason for hiding this comment

timchinowsky commented Sep 25, 2024

timchinowsky commented Sep 26, 2024

timchinowsky commented Oct 24, 2024 • edited Loading

timchinowsky commented Nov 7, 2024

relic-se commented Nov 7, 2024

timchinowsky commented Nov 7, 2024 • edited Loading

relic-se commented Nov 13, 2024

relic-se commented Nov 13, 2024

timchinowsky commented Nov 13, 2024

relic-se commented Nov 13, 2024

timchinowsky commented Nov 14, 2024

timchinowsky commented Oct 24, 2024 •

edited

Loading

timchinowsky commented Nov 7, 2024 •

edited

Loading