audio_i2sin module for espressif and raspberrypi#10990
Conversation
tannewt
left a comment
tannewt
left a comment
There was a problem hiding this comment.
Code seems fine. Do we want it to be audio_i2sin instead of audioi2sin? I think the latter is more Python-style and more what we've done in the core. Python reference: https://peps.python.org/pep-0008/#package-and-module-names
|
Starting to play around with this module. I see there's no option to support signed samples. The audio codec I'm testing with only supports signed samples (or I haven't implemented it in the driver library). I think it'd be beneficial to add that option to the constructor rather than require the user to perform the conversion within Python (which I haven't figured out how best to do yet). An example of a Here's the code I'm working with right now with a Pimoroni Pico Plus 2, TLV320AIC3204 audio codec, and SPW2430 microphone. import array
from audioi2sin import I2SIn
import board
import ulab.numpy as np
import relic_tlv320aic3204
# Initialize codec
codec = relic_tlv320aic3204.TLV320AIC3204(
i2c=board.STEMMA_I2C(),
mclk=board.GP17,
rst=board.GP16,
)
codec.sample_rate = 44100
codec.bit_depth = 16
# Initialize I2S bus
i2sin = I2SIn(
bit_clock=board.GP18,
word_select=board.GP19,
data=board.GP21,
sample_rate=codec.sample_rate,
bit_depth=codec.bit_depth,
mono=True,
)
# Connect IN1L to Left MICPGA and IN1R to Right MICPGA
codec.connect_input(relic_tlv320aic3204.INPUT_1, relic_tlv320aic3204.IMPEDANCE_20K)
codec.input_gain = 6.0 # dB
# Setup ADC Input
codec.dac_enabled = True # BUG: DAC must be enabled for ADC functionality
codec.adc_volume = 0.0 # dB
codec.adc_enabled = True
codec.adc_muted = False
# Setup buffer
buf = array.array("h", [0] * (codec.sample_rate // 16))
while True:
# Record to buffer
i2sin.record(buf, len(buf))
# Determine maximum level
print(np.max(np.array(buf, dtype=np.int16)))Right now, the level is all wonky because of the issue with signedness. |
|
@relic-se The latest commit adds |
|
@tannewt this is refactored to |
|
I was beginning to do a deeper review of the recent |
|
@relic-se I believe that assuming signed data was the original behavior. Handling unsigned data was only added with this most recent commit and is only achieved by explicitly passing I can remove that new argument if it's not necessary to handle unsigned data. I don't have much experience with these (or really any) mics yet, so I am not about what is normal or what alternates are used. |
|
I believe my confusion stems from the use of I initially thought your original implementation only handled unsigned data which is why I suggested the Unless I am mistaken on something, the data should always be signed and the user should not be concerned with fixing the signedness of the data. Hence, we should be using the formats |
|
So, I can now see some benefit of the conversion implemented in I still think that the following changes should be implemented:
Currently, the module requires unsigned typecodes regardless of I can provide a full code review if that is beneficial. |
|
@relic-se yeah, your review would be appreciated.. I'll look into the typecodes thing that you mentioned and see about changing that as suggested. |
|
I'll do a final review after @relic-se does theirs. |
|
@FoamyGuy Is |
|
@relic-se my primary goal was to unlock support for mics built-in to Sparkle Motion devices and this other breakout: https://www.adafruit.com/product/6049. Aiming to support audio reacitive neopixel stuff and recording audio for allowing a project like these walkie talkies: https://learn.adafruit.com/esp-now-walkie-talkies to work under CircuitPython. I'm not opposed to ultimately having more functionality but enabling those things is was the main focus of the current implementation. I'm not sure how bit depth works exactly, would using bit depth 8 require specific microphone hardware? or any mic can support it? |
I am not aware of any I2S microphones which are limited to or support 8-bit data. I only know of some audio codecs which support that format. I am only this question asking because the espressif implementation has 8-bit support while the raspberrypi hal does not. I wasn't sure if that lack of functionality on one platform was a concern. I could be added by including another set of PIO programs and making some code alterations, but I am not opposed to leaving that support omitted. This is especially true if flash size is a concern on raspberrypi.
I think that scope is correct for this module. I do have an ICS-43434 module and plan on conducting some tests with that device once we've resolved these array format issues. |
relic-se
left a comment
relic-se
left a comment
There was a problem hiding this comment.
Most of my comments are related to bit depth and signedness. Currently, I think there is more work to be done to make it so that it doesn't have to be fixed within user code. All users should have to worry about is ensuring they have the correct bit_depth set for their microphone.
On that note, I could see an output_bit_depth option being added which could enable the core to handle conversion between 24-bit and 16-bit samples (for instance) and just give the user back their data in an "h" array where they can write it straight to WAV or output to an I2S speaker, etc. A default value of output_bit_depth=None would respect the provided bit_depth property.
|
|
||
| // I2S delivers signed PCM. When samples_signed is false, XOR each sample with | ||
| // the sign bit for its width to convert to unsigned PCM (WAV convention). | ||
| static void i2sin_convert_to_unsigned(void *buffer, uint32_t samples, |
There was a problem hiding this comment.
For better performance, only process as 32-bit words rather than casting buffer to relevant word size. See shared-module/audiomixer/Mixer.c for reference.
There was a problem hiding this comment.
updated to use the words approach instead of samples in the latest commit.
There was a problem hiding this comment.
Nice catch on processing the tail end of the data, but any thoughts on requiring buffer lengths that are multiples of 2 for 16-bit and 4 for 8-bit so that you wouldn't have to deal with that?
|
@relic-se Thank you! I think I have made all of the requested changes. Let me know if I have missed anything or changes don't align with what you had in mind. I successfully re-tested the reactive neopixels on both RP2350 and esp32s3 and the sdcard recording on the esp32s3 after these changes. |
Looking good so far! Any thoughts on the
My audio codec example commented above is now working more as expected. I've got more testing to do to validate audio quality, but so far so good! |
|
In the following program, I'm take a 128 frame sample of data recording at 16-bit 44.1kHz after a second of delay (to warm up the bus). (Note: the environment I'm in is relatively quiet.) mic = I2SIn(sample_rate=44100, bit_depth=16, mono=True) # excludes pins
time.sleep(1)
buf = array.array("h", [0] * 128)
mic.record(buf, len(buf))
print(", ".join([str(x) for x in buf]))With that, I'm getting the following data: The same demo but using the The codec I'm using also supports 32-bit data. With that, I'm getting this result: And using Looks like |
|
This may be a future consideration but was there any thought to take the I2S input right to an output? I'm thinking about like a kid's karaoke machine that they sing into and it adds echo or distortion and plays it out right away. Maybe going to the buffer and feeding that into an audio.play object would work. Just a random thought. |
Yes, I think the next step is to implement the audiocore API. I've already made some progress on |
|
@relic-se the latest commits add I tested with the new example on esp32s3 and the neopixel reactive examples on rp2350 and found all appear to work as expected. I did not test all possible output depths. And agree'd that it would be nice to ultimately have the ability for this to be hooked up to audiocore for applying an effect and then playing it back out. |
@FoamyGuy I'll need to do some review and testing on my own, but that definitely simplifies the the WAV example. Heck, with the |
relic-se
left a comment
relic-se
left a comment
There was a problem hiding this comment.
Just a small error text request here.
|
|
||
| #: ports/espressif/common-hal/audiobusio/PDMIn.c | ||
| #: ports/espressif/common-hal/audioi2sin/I2SIn.c | ||
| msgid "bit_depth must be 8, 16, 24, or 32." |
There was a problem hiding this comment.
If we were to change this to "%q must be 8, 16, 24, or 32" within PDMIn, it would save a bit of space.
| bool mono, bool left_justified, bool samples_signed) { | ||
|
|
||
| if (bit_depth != 8 && bit_depth != 16 && bit_depth != 24 && bit_depth != 32) { | ||
| mp_raise_ValueError(MP_ERROR_TEXT("bit_depth must be 8, 16, 24, or 32.")); |
There was a problem hiding this comment.
Change to:
mp_raise_ValueError_varg(MP_ERROR_TEXT("%q must be 8, 16, 24, or 32"), MP_QSTR_bit_depth);
There was a problem hiding this comment.
I think this validation check can be removed from the espressif common-hal implementation.
My understanding is validation should occur in shared-bindings and the implementation can assume valid values. The exception in this case is the lack of current support for 8 bit_depth in raspberrypi implementation so it has to check for that and raise.
|
I've recently discovered that circuitpython/shared-module/audiocore/__init__.h Lines 94 to 116 in 66001d0 Rather than maintaining separate platform implementations within |
|
Here are my results from the logic analyzer. You can see the samples dropped from the left channel. I think they're related to a timing issue with BCLK. The last clock cycle of the left word after LRCLK transitions to high to indicate the right word is 500ns when it should be 700ns. This causes the bus to run closer to 44.6kHz rather than 44.1kHz. My audio codec (which has a 15 MHz MCLK driven from the pico) might be more sensitive to errors in the I2S timing than the I2S mics. 
I'm going to take a closer look at your PIO code and see if I can pinpoint the error. |
There was a problem hiding this comment.
Each PIO program was missing a delay of 2 cycles on the first set instruction. Adding the delay in results in more stable and accurate I2S timing. I'm still getting some dropped sample frames, but not as frequently. I'll continue to diagnose this, but I can't see anything wrong with the BCLK and LRCLK signals at this time.
Btw, sorry for the repetitive comments. Just being thorough! 😝
Update: The only sample rate that doesn't produce dropped samples with my codec is sample_rate=8000. Progress, but I'll keep looking for timing issues.
| // Master-mode RX, regular pin order (BCLK = WS - 1), Philips alignment. | ||
| static const uint16_t i2sin_program[] = { | ||
| // .wrap_target | ||
| 0xf04e, // 0: set y, 14 side 2 |
| // Master-mode RX, regular pin order, left-justified. | ||
| static const uint16_t i2sin_program_left_justified[] = { | ||
| // .wrap_target | ||
| 0xe04e, // 0: set y, 14 side 0 |
| // Master-mode RX, swapped pin order (BCLK = WS + 1), Philips alignment. | ||
| static const uint16_t i2sin_program_swap[] = { | ||
| // .wrap_target | ||
| 0xe84e, // 0: set y, 14 side 1 |
| // Master-mode RX, swapped pin order, left-justified. | ||
| static const uint16_t i2sin_program_left_justified_swap[] = { | ||
| // .wrap_target | ||
| 0xe04e, // 0: set y, 14 side 0 |
| // outside the loop = 32 in's per channel). | ||
| static const uint16_t i2sin_program_32[] = { | ||
| // .wrap_target | ||
| 0xf05e, // 0: set y, 30 side 2 |
| ; /--- LRCLK | ||
| ; |/-- BCLK | ||
| ; || | ||
| set y 30 side 0b00 |
| ; /--- BCLK | ||
| ; |/-- LRCLK | ||
| ; || | ||
| set y 14 side 0b01 |
| ; /--- BCLK | ||
| ; |/-- LRCLK | ||
| ; || | ||
| set y 30 side 0b01 |
| ; /--- BCLK | ||
| ; |/-- LRCLK | ||
| ; || | ||
| set y 14 side 0b00 |
| ; /--- BCLK | ||
| ; |/-- LRCLK | ||
| ; || | ||
| set y 30 side 0b00 |
4 participants
Adds
audio_i2sin.I2SInclass. Only enabled/implemented for espressif and raspberrypi ports currently.Two new manual test scripts are included in the PR that were used to verify the functionality of the module. Recording to an SDCard, and sound reactive neopixels.
Sparkle Motion board def is updated to include the pins that the I2S mic is connected to.
Testing:
sdcardio. Might be worth testingsdio?