esp32,stm32/i2s: I2S protocol support for Pyboards and ESP32. #7183
Conversation
docs/library/machine.I2S.rst
Outdated
| "buf" byte ordering is little-endian. For Stereo format, left channel sample precedes right channel sample. | ||
| Returns number of bytes written | ||
|
|
||
| .. method:: I2S.callback(handler) |
There was a problem hiding this comment.
To match other classes (eg Pin, BLE), I'd suggest renaming this method to I2S.irq(handler).
There was a problem hiding this comment.
Thanks for flagging that. I tried to keep consistency with other modules, but I missed that one. I'll update the PR.
There was a problem hiding this comment.
I added a new PR commit that brings consistency to the callback naming.
|
Fantastic work, thank you! I especially like the "3 modes of operation" part. I'll need to find some time (and I2S hardware) to test this. |
| self, | ||
| I2S_TASK_PRIORITY, | ||
| (TaskHandle_t *)&self->non_blocking_mode_task, | ||
| MP_TASK_COREID) != pdPASS) { |
There was a problem hiding this comment.
I wonder if there is a possibility to allow this task to run on the PRO esp32 core?
Unlike _thread which pins new threads to the APP core there is no sharing of Micropython data and all inter task communication goes through the FreeRTOS queue.
I read some more above and my description is not quite accurate. The mp_buffer_into_t passes in from the micropython script and the non_blocking_descriptor_t contains a pointer to it. When reads or writes happen in the non blocking task they touch the buffer data.
But if the callbacks are used then it reads like it should be safe because they are invoked after processing on that buffer is done.
I'll see what happens if the core is changed and report back.
Maybe its too niche and I should just patch this in my firmware but if it works it might be a useful switch to use where you have too much processing to do and also sample gap-less audio on one esp32 core.
There was a problem hiding this comment.
I'm interested to see what happens as well. Thanks for volunteering to try this out.
There was a problem hiding this comment.
It took me a few days to upgrade my firmware to use this new version.
I'm trying to run the tensorflow micro_speech example and because of the machine learning model, working area, etc I don't have enough RAM to allocate 40k + 10k to buffers like your record to SD card example.
It looks like its fast enough when the task is running on the APP core for my use-case.
I'm using 320 x 5 x 4 (6400) bytes for the i2s dma buffer and 320 x 2 (640) bytes read into buffer.
The x 4 on the dma buffer is because your comments say it samples 32 bit integers and then throws out non-applicable byes to scale the i2s dma samples into the read into buffer.
But I also changed the task to pin to core 1 (PRO core) and it seems to run faster.
I need to do more analysis to quantify the speedup.
When running I'm having trouble getting it to stop consistently using control-c.
If the code running is micropython then it can get caught by the except block and stop.
caught exception KeyboardInterrupt
Done
Traceback (most recent call last):
File "main.py", line 396, in <module>
File "main.py", line 361, in runModel
File "main.py", line 86, in input_callback
File "micro_speech.py", line 80, in setInputTensorValues
KeyboardInterrupt:
MicroPython v1.15-66-g7aa713ace-dirty on 2021-05-04; ESP32 module with ESP32
Type "help()" for more information.
>>>
But about half the time I see this instead and the result is a soft reboot which starts up the program again.
caught exception KeyboardInterrupt
Done
/opt/esp/idf/components/freertos/queue.c:764 (xQueueGenericSend)- assert failed!
abort() was called at PC 0x40093d4c on core 0
Backtrace:0x40092ca2:0x3ffce5e0 0x400933e9:0x3ffce600 0x40098db2:0x3ffce620 0x40093d4c:0x3ffce690 0x40132789:0x3ffce6d0 0x4020876b:0x3ffce710 0x4010868d:0x3ffce740 0x40101655:0x3ffce770 0x40107321:0x3ffce7a0 0x4010744e:0x3ffce7c0 0x401098a9:0x3ffce7e0 0x401016d4:0x3ffce880 0x40107321:0x3ffce8b0 0x40107409:0x3ffce8d0 0x4012fd04:0x3ffce900 0x4012fdf0:0x3ffce980 0x4012fe55:0x3ffce9a0 0x4013175f:0x3ffce9c0 0x40131a39:0x3ffcea50 0x40131a9d:0x3ffcea70 0x4011a0f0:0x3ffcea90
ELF file SHA256: 4d5c617ffc8e19ad
Rebooting...
ets Jun 8 2016 00:22:57
rst:0xc (SW_CPU_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT)
configsip: 0, SPIWP:0xee
clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00
mode:DIO, clock div:2
load:0x3fff0030,len:5516
ho 0 tail 12 room 4
load:0x40078000,len:13800
ho 0 tail 12 room 4
load:0x40080400,len:3388
entry 0x40080634
Fatal exception (29): StoreProhibited
epc1=0x40079fd8, epc2=0x00000000, epc3=0x00000000, excvaddr=0xa2f40000, depc=0x00000000
ets Jun 8 2016 00:22:57
rst:0x10 (RTCWDT_RTC_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT)
configsip: 0, SPIWP:0xee
clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00
mode:DIO, clock div:2
load:0x3fff0030,len:5516
ho 0 tail 12 room 4
load:0x40078000,len:13800
ho 0 tail 12 room 4
load:0x40080400,len:3388
entry 0x40080634
There was a problem hiding this comment.
It took me a few days to upgrade my firmware to use this new version.
Awesome ! Which version of ESP-IDF did you use?
your record to SD card example
Which example are you using? e.g. blocking, non-blocking, uasyncio
The x 4 on the dma buffer is because your comments say it samples 32 bit integers and then throws out non-applicable byes to scale the i2s dma samples into the read into buffer.
There is no need to be concerned with buffer ratios in the MicroPython code. For example, you could define an internal buffer of 10k and a readbuffer of 1k, or an internal buffer of 8k and a readbuffer of 20k. The C code implementation shields that complexity from the user. It was accidental that the C code comment uses 10k and 40k (which is the same as the MicroPython examples). I will change the 10k and 40k in the C code comment to eliminate any confusion on this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I setup on your i2s branch and built using the latest 4.3 idf. I'm building using the espressif docker image: espressif/idf:release-v4.3.
I modified the non-blocking example:
https://github.com/miketeachman/micropython-i2s-examples/blob/master/examples/record-mic-to-sdcard-non-blocking.py
Thanks for working on this pull request for so long. I think it will open up a lot of opportunities for driving LED's and various other projects that previously could only be done using C or C++.
| } | ||
| } else { // Master Rx | ||
| // read stereo frames for all I2S hardware | ||
| return I2S_CHANNEL_FMT_RIGHT_LEFT; |
There was a problem hiding this comment.
This is great work.
Can you explain why you always read both left and right channels? It's quite unusual for people to have two microphones connected.
There was a problem hiding this comment.
You bring up a good point. I need to take a second look at this implementation. Valuable DMA buffer space is wasted for 16-bit and 32-bit Mono modes. Thank you for reviewing this code and making suggestions. I appreciate it !
There was a problem hiding this comment.
No problem - I'm really glad I found the PR and samples.
| // in the I2S constructor. e.g. 16-bit mono | ||
| STATIC const int8_t i2s_frame_map[NUM_I2S_USER_FORMATS][I2S_RX_FRAME_SIZE_IN_BYTES] = { | ||
| { 6, 7, -1, -1, -1, -1, -1, -1 }, // Mono, 16-bits | ||
| { 4, 5, 6, 7, -1, -1, -1, -1 }, // Mono, 32-bits |
There was a problem hiding this comment.
I think this assumes that Mono always means left - this is probably true in most cases as most microphones will default to left. But it might be worth calling this out in the documentation.
There was a problem hiding this comment.
That is right - Mono Rx mode only uses the Left channel audio data. Great idea to mention this in the MicroPython I2s documentation - I will make this improvement.
| // the left and right channels | ||
| // https://github.com/espressif/esp-idf/issues/6625 | ||
| REG_SET_BIT(I2S_CONF_REG(self->port), I2S_TX_MSB_RIGHT); | ||
| REG_SET_BIT(I2S_CONF_REG(self->port), I2S_RX_MSB_RIGHT); |
There was a problem hiding this comment.
Is this still required - I think it might have been fixed on esspressif's side of things.
There was a problem hiding this comment.
Yes - the workaround is still needed. This bug still exists in the versions of ESP-IDF used by MicroPython. I have not seen any indication that Espressif has fixed the bug in new versions of the ESP-IDF.
|
I'm not sure if it would belong in this PR or as an extension to the work, but some higher-level abstractions would be really helpful for people - particularly people just starting out. Being able to stream audio directly to and from WAV files (either to flash or SD Cards) would be really nice. e.g. something along these lines, where you could kick off recording or playback in a non-blocking way. For the playback, adding volume adjustments would be a really nice additional feature. |
|
Would being able to use the internal ADC and DAC for the esp32 version be hard to add? It could be useful for some people if they don't want an external I2S device. |
|
I'm already using your old i2s esp32 (#4471) implementation with success with an ics43432 mic. Thank you very much @miketeachman 💐 One thing which begs me: currently there is no way to detect an overflow when reading right? |
This is a really good idea. For example, I can imagine creating a new class It might work as follows |
I think it could be added to MicroPython, but it might be best implemented in the The |
Sorry, but I am having some difficulty to understand the question. Is it possible to provide an example? thanks! |
This change will need to be put into a separate commit. I'm happy to do that if needed. |
I am mostly interested in the ADC side of it where I can sample audio using the internal ADC using the I2S system, because the normal way of using the ADC on esp32 can't get very high sample rates, let alone once you stuff Micropython between you and the ADC. And having it in the correct machine class for the ADC would make sense but that means when using the ADC connected over I2S you would have the machine class for the ADC touching the I2S driver stuff that this class uses possibly messing up state info. |
This is a very good point. Both the internal ADCs and internal DAC can be used via I2S - quite a few people use this for recording and playing back audio. |
|
Do you mean that the ADC can be used as an input source to the I2S peripheral, and the DAC and output source? So it's:
|
|
That’s correct. I have some sample code for both cases.
…On Sat, May 29, 2021 at 23:03, Damien George ***@***.***> wrote:
Both the internal ADCs and internal DAC can be used via I2S
Do you mean that the ADC can be used as an input source to the I2S
peripheral, and the DAC and output source? So it's:
- CPU <- I2S <- ADC <- microphone
- CPU -> I2S -> DAC -> speaker
?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#7183 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFMTAA5QA6GNDYGFNJX2SDTQFQDNANCNFSM43YCOT4Q>
.
|
Yes please! We'd need to add docs and a test for it as well. It's not in CPython but it sounds like they will eventually add it: https://bugs.python.org/issue41305 |
Then that should be a relatively simple enhancement to the API here. Maybe something like |
|
There's a bit of funkiness on the processing the samples from the ADC inputs - sample code here should show how to do it - https://github.com/atomic14/esp32_sdcard_audio |
I can imagine a new
Good point. This situation would need careful consideration if the two I2S peripherals on the ESP32 become shared between machine module classes, such as |
This is very true because also you can have the issue the other way, say the I2S class adds using the ADC or DAC as input and output, you have to worry about the ADC or DAC class trying to use that same peripheral without knowing that its in use by the I2S peripheral. I feel like some sort of shared state would be needed to tell each what device is truly in use with I2S. |
For me, the preferred approach is to extend the |
You mean like stm32 has Whatever the way it's done, that should not affect this I2S API which seems pretty solid. |
New name is shorter a EED3 nd allows for the future option of supplying internal buffer memory in the constructor.
I rebased against the master. I didn't squash the 3 PR commits. I could do that step as well, but at this point I thought it might be better to see the individual commits.
I didn't see anything in the API either that should be an issue. It's only in the C code and commit comments. |
| }; | ||
|
|
||
| // This is used by the DMA callback functions to access the I2S object | ||
| STATIC machine_i2s_obj_t *machine_i2s_obj[MAX_I2S_STM32]; |
There was a problem hiding this comment.
This will probably need to go in the root pointer section, eg like pyb_uart_obj_all. Then access it via MP_STATE_PORT(machine_i2s_obj).
There was a problem hiding this comment.
I held off on this change as I don't understand the issue or risk. I had considered adding this array to the root pointer section, but the array is holding references to I2S objects, which are allocated in the recommended way, and should not be prematurely GC'd. I can see a different situation for "stranded" heap objects that are allocated using m_new() and held inside the C code, and not connected to any structure that the GC can see - in that case I understand the need to declare a root pointer, so the "stranded" object is not GC'd.
There was a problem hiding this comment.
You are right that if the users code holds a reference to the object then they will not be GC'd. The issue comes about if the user creates an object, registers an IRQ callback, then no longer holds on to it. Then it can get GC'd and the IRQ will run with invalid data. This is pretty easy to do with Pin and Timer IRQ callbacks. It's probably also possible to do with an I2S object, but if you think it's ok as-is then let's leave it, and possibly change it later if the need arises.
There was a problem hiding this comment.
I hadn't considered the possibility of a race condition. I can see that possibility in the I2S implementation. I'll definitely make the root pointer fix that you recommend, in both the STM32 and ESP32 ports. There is nothing more heinous than a non-reproducible crash.
There was a problem hiding this comment.
I started work on the root pointer fix, but it seems there are some tricky aspects to adding root pointers to esp32/mpconfigport.h.
For example, in the root pointer section I need to add a line for I2S - shown below. To compile this I need a way to declare the macro I2S_NUM_MAX . Unfortunately adding the relevant header file #include driver/i2s.h causes many compile errors. Substituting I2S_NUM_MAX with the magic number 2 gets everything compiling, but that's not a very maintainable solution.
Rather than continue reverse engineering the code, I thought it best to ask for help ...
#define MICROPY_PORT_ROOT_POINTERS \
const char *readline_hist[8]; \
mp_obj_t machine_pin_irq_handler[40]; \
struct _machine_timer_obj_t *machine_timer_obj_head; \
struct _machine_i2s_obj_t *machine_i2s_obj[I2S_NUM_MAX]; \
MICROPY_PORT_ROOT_POINTER_BLUETOOTH_NIMBLE
There was a problem hiding this comment.
Ok, interesting... some options:
- make
machine_i2s_objavoid*and ini2s_init0()allocate (on the GC heap) an array of the right size and type and point the root pointer to it #define MP_I2S_MAX 2inmpconfigport.hand static assert that it's less than or equal toI2S_NUM_MAXsomewhere inmachine_i2s.c
There was a problem hiding this comment.
Thanks. Using a compile-time assert is a good fit for this problem. I'll move forward with option 2.
There was a problem hiding this comment.
I made a commit adding machine_i2s_obj[] to the root pointer section. Builds and testing on my local machine worked, but two builds on the server failed after commit. I took quick look and one is easy to fix (nucleo build), but the other build (ESP32/IDF) shows a non-obvious failure. Unfortunately, I don't have any time before holidays to fix these issues. I decided to back out the commit.
I can fix these build problems when I return from holidays in 2 weeks and a bit. The root pointer concern doesn't seem like a showstopper. Maybe the PR can be merged and I'll commit a new PR on the root pointer fix when I get back? Or, just wait...
There was a problem hiding this comment.
Let's merge. We can fix it later.
|
I did not test this on hardware but am confident it works 😄 The only outstanding discussion point would be about the io mode: blocking vs non-blocking vs uasyncio. Here we have I2S automatically configuring itself based on:
For comparison:
My feeling is that it would be nice if I2S fit in either of the existing models for blocking/non-blocking... would it be possible to make non-blocking and uasyncio IO modes the same, and then add |
changes resulting from code review comments:
- make object allocation consistent with other peripherals by
reusing the existing I2S object when any other I2S objects are
instantiated using the same I2S peripheral
- misc: change "busses" to "buses"
fix function scope errors (global->file scope)
|
Your summary of the operational modes is bang on. My goal with this I2S module is to have an API that is easy to master and "just works". Also working identically across the ports was paramount. The latter goal took more effort than I'd like to admit. I'd like to keep these two goals when changes to the operation of the module are considered.
Thanks for sharing these examples of existing models. I'll take a look at the two examples and see if there is a fit for this I2S PR. One thing ... I'm on summer vacation starting in a few days so any meaningful re-work won't start for about 3 weeks. |
That's a good goal, and matches how most of the other machine classes are (although none are as complex as I2S in terms of blocking/non-blocking streams). For comparison, the
Yes that is definitely the number 1 goal.
Ok. I know how much work has already been put into this PR (thank you!), and it has also been getting some real-world use. So I'd be happy to merge it as-is -- so it doesn't linger too long here -- but merged with an understanding that there could be some tweaks to the API in the future. Is that reasonable? |
I think the time is right to get this module into wider use, get feedback, fix bugs, then tune the implementation and API based on user feedback and observed patterns of use. What if the PR was merged as-is (with the addition of the root pointer fix that I'll make later today), but the I2S docs flag the module as "preliminary" or "technology preview"? This approach would advertise that the API might change in the future. We could leave the module in a preview form for a couple of releases. Even though I've put a lot of time into the module I'm definitely not resistant to future changes. For example, a desirable outcome might be reducing the volume of C code, adding python wrappers, while maintaining functionality, performance, and usability. I imagine there are a lot more developers who are skilled in python rather than embedded C. Less C code would seem like a win. MicroPython has enabled a lot of my embedded projects and I teach a MicroPython course at our local Victoria Makerspace. I see future support of I2S as one way of giving back to the MicroPython community. |
Yes, let's do it!
Fantastic! Thanks for the feedback, always great to hear about how it's being used. |
I submitted a commit that identifies the I2S class as a Technology Preview |
|
Squashed and merged in 8a5bfe4! Thank you @miketeachman for the huge effort to get this over the line. |
|
The docs are confusing this old codger. Should sdin=sdin_pin read sd=sdin_pin? This would conform with the list of constructor args. Secondly the setups call for sdin_pin whereas only sdout_pin is declared and sdin_pin seems unexpected for an output. As a more general point, I'm using the hardware designed by @blmorris who did the initial work on I2S. This worked within the limits of his software. I started out by reading a WAV file off the Pyboard's SD card. I'm pretty sure I'm reading the data segment OK, but the playback is noise not music. I'm using the blocking writer but I suspect that the rate I can read data from the disk may be inadequate. Do you have any general comments on how best to play substantial files on I2S/Pyboard? In the meantime I'll try much smaller samples and input. |
|
Another query, this time on the nonblocking docs which read:
Presumably More importantly, the callback appears to offer the possibility of the user implementing a double buffering scheme where the callback issues .readinto after switching buffers and flags the user code to process the data just read. This poses the question of whether this can happen fast enough to avoid data loss. Should the callback be issued before the buffer is completely full? There would then need to be some way of knowing how much data it held at the instant of buffer switching - I can't help thinking there is still potential for data loss. Or should buffer switching be done in the C code? In practice I intend to use the uasyncio interface but I am rather puzzled over how (or if) nonblocking mode can be used to acquire longer sequences. |
|
I'm making no headway here. I've written a script which creates a single cycle of a sinewave comprising 44 samples and saves it to a file. The following script attempts to play it back 10,000 times at 44Ksps using the blocking driver. It blocks for 10s as expected, but the result is silence. Obviously I can't expect you to debug the hardware setup but it did make some noise in my earlier tests. I'd be grateful if you could point out anything daft that I'm doing. Hardware is a Pyboard 1.1 running today's daily build. The relevant part of the script is: sck_pin = Pin("Y6") # Serial clock output
ws_pin = Pin("Y5") # Word clock output
sdout_pin = Pin("Y8") # Serial data output
audio_out = I2S(2,
sck=sck_pin, ws=ws_pin, sd=sdout_pin,
mode=I2S.TX,
bits=16,
format=I2S.STEREO,
rate=44100,
ibuf=20000)
hwsetup() # This sets up @blmorris hardware for stream output
with open('sin', 'rb') as f:
buf = f.read()
for x in range(0, len(buf), 4):
l, r = struct.unpack('<hh', buf[x: x+4])
print(l, r)
for cycles in range(10000):
audio_out.write(buf)and the printout of left and right channels is |
Yes I think you're right. I've raised #7617 to fix this. But otherwise I don't have any hardware at the moment to test this. |
|
Thanks for confirming. Hopefully @miketeachman will be able to comment on my code. It would be good if there was a recommended "official" hardware audio board and a couple of demo scripts for testing input and output. Then we'd have a good starting point for developing applications. Incidentally the I2S functionality is present in daily builds, but if I build my own firmware it is absent from machine. |
|
@peterhinch Are you aware of this repo with the comprehensive examples on how to use this pull request? https://github.com/miketeachman/micropython-i2s-examples It says 3 boards were tested:
I don't think the blmorris board is one of these. That page also has the recommended pins to use per board but yours look to be correct (the ones from the sine example). I'm setup and working on esp32 with an INMP441 mic in mono mode. For the esp32 implementation I got no data(silence/static) when the mic was setup in right channel mode as mono was expecting the mic to be in left channel mode. I don't have experience with the transmit code but on the receive code there is a translation process between the hardware sampling and the bytes put into the user_buffer which match the bit size and channels configured in machine.I2S. One suggestion I have would be to try different mono/stereo and bit rate combinations to see if anything works. Or pick up one of the read wav from sd card examples and see if that will transmit properly. I used a variant of this to validate my hardware setup: https://github.com/miketeachman/micropython-i2s-examples/blob/master/examples/record-mic-to-sdcard-uasyncio.py There was a prior version that would read from the mic and save to the esp32 flash filesystem. I was able to record at 8000 and 16000 hertz. I would record the sampled data to a wav file. Download and listen in windows media player to verify. For the non-blocking mode you need to be sure to trigger the next i2s.readinto(user_buffer) within the callback. When I first tried to implement I only called from the main program body and only read one buffer's full of data instead of getting a stream of data. I think the i2s.readinto(user_buf) is a no-op in the listener but causes the registration of the user_buf to receive the data eventually (when available in the dma buffer). In my testing the irq callback is invoked when the user_buf has been filled (or partially filled) and the num_read global will have the total bytes just read. |
|
Thank you very much. I'll order some suitable hardware. |
|
I've got a very simple test program you can try that you can flash using Arduino if that helps - at least to confirm your hardware and pins are correct - https://github.com/atomic14/esp32-i2s-mic-test |
|
I found the details for the @blmorris audio board: https://github.com/blmorris/uPy_AudioCodec I wonder if there is some special handling within @blmorris's i2s driver that was removed from the final code in this pull request. current : https://github.com/micropython/micropython/blob/master/ports/stm32/machine_i2s.c original: There do seem to be some special syncing logic but for when running in SLAVE mode; like this one: https://github.com/blmorris/micropython/blob/cd5556909883e8083a770c22f72b7908a430d95d/stmhal/i2s.c#L597 |
|
Thanks all. I've ordered a SPH0645LM4H and a MAX98357A from Adafruit as @miketeachman has tested with these. With his demos working I'll test with uasyncio which is my real interest. I worked with the @blmorris hardware as the only kit available - I may have a go at getting it working once I'm happy with the code. |
|
[EDITED] tl;dr |
This change eliminates the risk of the IRQ callback accessing invalid data. Discussed here: #7183 (comment) Signed-off-by: Mike Teachman <mike.teachman@gmail.com>
This change eliminates the risk of the IRQ callback accessing invalid data. Discussed here: micropython#7183 (comment) Signed-off-by: Mike Teachman <mike.teachman@gmail.com>
This change eliminates the risk of the IRQ callback accessing invalid data. Discussed here: micropython#7183 (comment) Signed-off-by: Mike Teachman <mike.teachman@gmail.com>
This PR adds I2S protocol support for STM32 and ESP32 ports: