Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RP2 PIO background read #9659

Merged
merged 7 commits into from
Nov 20, 2024
Merged

Conversation

timchinowsky
Copy link

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

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

ports/raspberrypi/bindings/rp2pio/StateMachine.c Outdated Show resolved Hide resolved
ports/raspberrypi/bindings/rp2pio/StateMachine.c Outdated Show resolved Hide resolved
ports/raspberrypi/bindings/rp2pio/StateMachine.c Outdated Show resolved Hide resolved
ports/raspberrypi/common-hal/rp2pio/StateMachine.c Outdated Show resolved Hide resolved
@timchinowsky
Copy link
Author

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
Copy link
Author

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
Copy link
Author

timchinowsky commented Oct 24, 2024

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

Screenshot from 2024-10-24 15-17-36

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

@timchinowsky
Copy link
Author

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
Copy link

relic-se commented Nov 7, 2024

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
Copy link
Author

timchinowsky commented Nov 7, 2024

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

@relic-se
Copy link

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
Copy link

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
Copy link
Author

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
Copy link

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
Copy link
Author

SGTM 🚀

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

Not sure why the espressif builds are failing, any suggestions @relic-se @jepler? Thanks! Perhaps related, my submodule understanding is fuzzy, do I need to do something get rid of the git warnings

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   ports/espressif/esp-idf (new commits)
	modified:   ports/raspberrypi/sdk (new commits)

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are three regressions in submodule versions. See the first three entries in "Files changed". There shouldn't be any submodules listed in there for this PR> You can update submodules locally and then push again. The submodules don't get updated automatically necessarily when you merge from upstream.

@timchinowsky
Copy link
Author

There are three regressions in submodule versions. See the first three entries in "Files changed". There shouldn't be any submodules listed in there for this PR> You can update submodules locally and then push again. The submodules don't get updated automatically necessarily when you merge from upstream.

Thanks @dhalbert! I can update locally with make fetch-all-submodules but then how do I push the submodules? The various advice I am seeing on stackoverflow is not giving me a warm feeling.

@dhalbert
Copy link
Collaborator

I can update locally with make fetch-all-submodules but then how do I push the submodules?

You commit the changed commits for the submodules.

@timchinowsky timchinowsky reopened this Nov 20, 2024
@timchinowsky
Copy link
Author

I can update locally with make fetch-all-submodules but then how do I push the submodules?

You commit the changed commits for the submodules.

@dhalbert thanks, but we would have had to get into details to debug what I was seeing. I was trying to do as you say, but things were not acting as expected. For now, I short-cut the process by cloning a fresh copy of main and re-applying the changes.

Now it looks like the build is too large for some boards?

@dhalbert
Copy link
Collaborator

I update submodules using this alias (for convenience):

alias gitsubupdate='git submodule sync --quiet --recursive && git submodule update --init --filter=blob:none'

Yes, the Wiznet boards were very close to full. Let's turn off USB host on those two boards. Add this to mpconfigboard.mk for those two:

CIRCUITPY_USB_HOST = 0

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some review comments. Thanks you in advance for addressing them!

@dhalbert
Copy link
Collaborator

There is trailing whitespace on line 148 of StateMachine.c

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, that addressed my requested changes.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

submodule skew is fixed

@jepler
Copy link
Member

jepler commented Nov 20, 2024

I did a quick test of adafruit_neopxl8 which uses the old single buffered background write and it still functioned properly with Adafruit CircuitPython 9.2.0-38-g8d561a0b0e-dirty (this PR)

@jepler jepler merged commit 254f8ef into adafruit:main Nov 20, 2024
141 checks passed
@jepler
Copy link
Member

jepler commented Nov 20, 2024

just testing the built in neopixel on rp2350:

>>> import adafruit_neopxl8 
>>> import board
>>> n = adafruit_neopxl8.NeoPxl8(board.NEOPIXEL, 1, num_strands=1)    
>>> n.fill(0)    
>>> n.fill(0xff00)    

@timchinowsky
Copy link
Author

Thanks for the test @jepler!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants