I've added a bi-directional mode to the I2S library using INPUT_PULLUP as the value of direction for the constructor. Certain resources of the object have been split into input and output variants (eg: pins, flags, audio buffers, and callbacks). Some minor API changes have been made to allow separate access to these resources as needed, but there shouldn't be any breaking changes (besides a slight change in constructor pin arguments). The documentation has been updated to reflect these changes. A simple loopback example has also been included.
Notes:
If doing too much computation in the loop which copies data between the input and output, audible stutters will occur. This can be mitigated by adjusting buffer sizes and potentially running audio loop on the second core.
Twice as much memory is consumed if running in bi-directional mode due to the separate audio buffer managers for input and output.
If there is a better direction value than INPUT_PULLUP to use here, I'd love the input. I think it'd be best avoid creating a specific enum for this unless it could be applied to other audio libraries.
I've added a bi-directional mode to the I2S library using `INPUT_PULLUP` as the value of direction for the constructor. Certain resources of the object have been split into input and output variants (eg: pins, flags, audio buffers, and callbacks). Some minor API changes have been made to allow separate access to these resources as needed, but there shouldn't be any breaking changes (besides a slight change in constructor pin arguments). The documentation has been updated to reflect these changes. A simple loopback example has also been included.
Notes:
- If doing too much computation in the loop which copies data between the input and output, audible stutters will occur. This can be mitigated by adjusting buffer sizes and potentially running audio loop on the second core.
- Twice as much memory is consumed if running in bi-directional mode due to the separate audio buffer managers for input and output.
- If there is a better direction value than `INPUT_PULLUP` to use here, I'd love the input. I think it'd be best avoid creating a specific enum for this unless it could be applied to other audio libraries.
Nice, will look at it later today. However, looks like a Python cache file made it into the PR. Can you git rm tools/__pycache__/uf2conv.cpython-313.pyc?
Nice, will look at it later today. However, looks like a Python cache file made it into the PR. Can you `git rm tools/__pycache__/uf2conv.cpython-313.pyc`?
Nice, will look at it later today. However, looks like a Python cache file made it into the PR. Can you git rm tools/__pycache__/uf2conv.cpython-313.pyc?
My bad! That one definitely slipped through.
I've been doing some playing around with this update and an RP2040 with a WM8960 codec. Results are quite good so far! I've been able to process the audio signal with simple integer-based processing and get clean output. However, once I throw in any floating point logic, the output begins to distort. I'm thinking I either need to look deeper into the buffer settings or just switch over to an RP2350 board. But I don't think that consideration has much of an effect on this PR except to potentially update the example.
> Nice, will look at it later today. However, looks like a Python cache file made it into the PR. Can you `git rm tools/__pycache__/uf2conv.cpython-313.pyc`?
My bad! That one definitely slipped through.
I've been doing some playing around with this update and an RP2040 with a WM8960 codec. Results are quite good so far! I've been able to process the audio signal with simple integer-based processing and get clean output. However, once I throw in any floating point logic, the output begins to distort. I'm thinking I either need to look deeper into the buffer settings or just switch over to an RP2350 board. But I don't think that consideration has much of an effect on this PR except to potentially update the example.
@relic-se Looks interesting. The RP2040 has a hardware integer division unit, so fixed point would be a better choice than floating point - e.g. represent a 16 bit sample as a 32 bit number, with the fractional part as the lower 16 bits. That way you can do everything with integer operations.
The main thing is to always do any division as the last operation in a calculation, where possible, to keep as much precision as possible. E.g. don't do
a = (b/c)*d in left to right order, do
a= b*d/c instead.
@relic-se Looks interesting. The RP2040 has a hardware integer division unit, so fixed point would be a better choice than floating point - e.g. represent a 16 bit sample as a 32 bit number, with the fractional part as the lower 16 bits. That way you can do everything with integer operations.
The main thing is to always do any division as the last operation in a calculation, where possible, to keep as much precision as possible. E.g. don't do
```
a = (b/c)*d in left to right order, do
a= b*d/c instead.
```
Re: performance, there's only soft-FP on the RP2040 so anything that's doing floating point will be pretty slow. Fixed point math is your friend there, though, and it's not hard to work with 17.15 (easier saturating math) or 16.16 (easier normalizing) numbers on a 32b device. Alternatively, look at the callback routines. You'll be delivered a block of bufsize l/r samples you can iterate over all at once. There's a lot of overhead doing individual 16bit read and writes plus you have the potential for cache spillage. You also have the 2nd core sitting there, unused...it's possible to grab data from the I2S running on core 0 and push work down to core1 for processing and re-output.
But a Pico2 is only $1.00 more and gives 2x flash and ~2x performance plus has single-precision FP in HW. Choices... 😆
FWIW, when I did block operations in BackgroundAudio (instead of per-sample ones like in ESP8266Audio) I went from an unusable webradio streamer (jitter, skips, just out of CPU oomph) to something that's rock solid and has ~30% free processing power left over each loop even while serving web pages and decoding MP3.
Re: performance, there's only soft-FP on the RP2040 so anything that's doing floating point will be pretty slow. Fixed point math is your friend there, though, and it's not hard to work with 17.15 (easier saturating math) or 16.16 (easier normalizing) numbers on a 32b device. Alternatively, look at the callback routines. You'll be delivered a block of `bufsize` l/r samples you can iterate over all at once. There's a lot of overhead doing individual 16bit read and writes plus you have the potential for cache spillage. You also have the 2nd core sitting there, unused...it's possible to grab data from the I2S running on core 0 and push work down to core1 for processing and re-output.
But a Pico2 is only $1.00 more and gives 2x flash and ~2x performance plus has single-precision FP in HW. Choices... :laughing:
FWIW, when I did block operations in [BackgroundAudio](https://github.com/earlephilhower/BackgroundAudio) (instead of per-sample ones like in [ESP8266Audio](https://github.com/earlephilhower/ESP8266Audio)) I went from an unusable webradio streamer (jitter, skips, just out of CPU oomph) to something that's rock solid and has ~30% free processing power left over each `loop` even while serving web pages and decoding MP3.
earlephilhower
(Migrated from github.com)
left a comment
Copy Link
Copy Source
This is great work and very much appreciated. Been something sitting out there since #1055 over 2 years ago, and it'll be awesome to get it into the core.
Very minor nomenclature and DWIM changes and I think we're set.
I'll have to wire up a Pico (everything's stuck on about 8 different ESP32 boards now for another library I'm writing) and make sure it doesn't break anything existing that shouldn't be broken. Visually, seems solid!
Thx again!
This is great work and very much appreciated. Been something sitting out there since #1055 over 2 years ago, and it'll be awesome to get it into the core.
Very minor nomenclature and DWIM changes and I think we're set.
I'll have to wire up a Pico (everything's stuck on about 8 different ESP32 boards now for another library I'm writing) and make sure it doesn't break anything existing that shouldn't be broken. Visually, seems solid!
Thx again!
I appreciate shoehorning this into an existing enum has tradeoffs. We can leave this for now, but I think for my own sanity I'll just add a BIDIR or INPUT_OUTPUT enum in the ArduinoCoreAPI. We already added enums for the multiple output drive strengths, and one more is no biggie...
I appreciate shoehorning this into an existing `enum` has tradeoffs. We can leave this for now, but I think for my own sanity I'll just add a `BIDIR` or `INPUT_OUTPUT` enum in the ArduinoCoreAPI. We already added enums for the multiple output drive strengths, and one more is no biggie...
The original method was getOverUnderflow because it would be either Overflow (for input) or Underflow (for output). Probably not the best design choice but naming things is hard!
But now that there really is a separately tracked Overflow and Underflow state, can we use getOverflow and getUnderflow? All these OverUnders make me feel like I'm in Vegas. :)
The original method was `getOverUnderflow` because it would be either Overflow (for input) or Underflow (for output). Probably not the best design choice but naming things is hard!
But now that there really is a separately tracked Overflow and Underflow state, can we use `getOverflow` and `getUnderflow`? All these `OverUnder`s make me feel like I'm in Vegas. :)
Since we can do bidir, then this should really just be available (and availableForWrite) no? That's the canonical definition/name for this in Stream.
Since we can do bidir, then this should really just be `available` (and `availableForWrite`) no? That's the canonical definition/name for this in `Stream`.
Maybe also add || (_isOutput && _isInput) to the condition? Should error if they're calling it on a bidir I2S since it's ambiguous and they need to use the 2 calls you added below.
Maybe also add `|| (_isOutput && _isInput)` to the condition? Should error if they're calling it on a bidir I2S since it's ambiguous and they need to use the 2 calls you added below.
You;re bending over backwards now to make broken code work. I think auto avail = _isInput ? _arbInput->available() : 0; covers things. And my comment next line should be 4 bytes, not 4 samples per 32b.
You;re bending over backwards now to make broken code work. I think `auto avail = _isInput ? _arbInput->available() : 0;` covers things. And my comment next line should be 4 *bytes*, not 4 *samples* per 32b.
I get the need to allow multiple pins, but I think we need 2 constructors here not one because in the common, unidirectional case you'd need to put in a dummy out pin on an INPUT
I2S(PinMode direction = OUTPUT, pin_size_t bclk = 26, pin_size_t data = 28, pin_size_t mclk = 25);
and I2S(PinMode direction, pin_size_t bclk, pin_size_t data_out, pin_size_t data_in, pin_size_t mclk = 25);
(probably need a little munging w/removing some default args to allos C++ to disambiguate which one you mean)...
The dataconstructor would assign the internal pinDIN or pinDOUT as appropriate for direction probably by calling the other constructor with one pin_size_t set to -1 or something dummy...
I get the need to allow multiple pins, but I think we need 2 constructors here not one because in the common, unidirectional case you'd need to put in a dummy `out` pin on an `INPUT`
`I2S(PinMode direction = OUTPUT, pin_size_t bclk = 26, pin_size_t data = 28, pin_size_t mclk = 25);`
and
` I2S(PinMode direction, pin_size_t bclk, pin_size_t data_out, pin_size_t data_in, pin_size_t mclk = 25);`
(probably need a little munging w/removing some default args to allos C++ to disambiguate which one you mean)...
The `data`constructor would assign the internal `pinDIN` or `pinDOUT` as appropriate for `direction` probably by calling the other constructor with one `pin_size_t` set to -1 or something dummy...
I think you might have an older version of pioasm. Dropping the (new) version member of the structure isn't really a problem, I think, but it'll cause diffs when we have to open the .pio file up later on....
I think you might have an older version of `pioasm`. Dropping the (new) version member of the structure isn't really a problem, I think, but it'll cause diffs when we have to open the `.pio` file up later on....
My initial instinct was to add INPUT_OUTPUT, but I wasn't sure if there would be any issues down the line in AudioCore-API. I can create a separate PR on that repo if you'd like me to go forward with the change.
My initial instinct was to add `INPUT_OUTPUT`, but I wasn't sure if there would be any issues down the line in AudioCore-API. I can create a separate PR on that repo if you'd like me to go forward with the change.
I did use that naming convention in a previous commit (https://github.com/earlephilhower/arduino-pico/pull/2775/commits/7dcdad69a2447dbc2fcca30819a54c3a98a6184a), but it conflicted with `AudioOutputBase`. I wasn't too familiar with the specific use of that method, so I changed it again to avoid the override. From my searching, it doesn't look like it will be a problem.
Instead, I've got the order of the arguments worked out so that it's compatible with existing code. The only issue is that it will be a little awkward when initializing a bi-directional I2S bus without the need for mclk in which case you would use -1 or something similar as you alluded.
I don't think I'll be able to use multiple constructors in this case because of the types and defaults of the argument list. Ie:
```
I2S(PinMode direction, pin_size_t bclk, pin_size_t data_out, pin_size_t data_in);
I2S(PinMode direction, pin_size_t bclk, pin_size_t data, pin_size_t mclk);
```
Instead, I've got the order of the arguments worked out so that it's compatible with existing code. The only issue is that it will be a little awkward when initializing a bi-directional I2S bus without the need for mclk in which case you would use -1 or something similar as you alluded.
```
I2S(PinMode direction, pin_size_t bclk, pin_size_t data, pin_size_t mclk, pin_size_t data_rx);
```
I think "broken code" is a bit of an overstatement. :P I've instead just added the check at the start of the function. The only comment I have is that I left the output buffer manager in case some users were using I2S::available() instead of I2S::availableForWrite() for an output-only bus. If you think that's a possibility, I can change this back.
I think "broken code" is a bit of an overstatement. :P I've instead just added the check at the start of the function. The only comment I have is that I left the output buffer manager in case some users were using `I2S::available()` instead of `I2S::availableForWrite()` for an output-only bus. If you think that's a possibility, I can change this back.
I actually used the online pioasm tool (https://wokwi.com/tools/pioasm), but it looks like they must be on an outdated version. It seems that all of the other pio headers are compiled using v0 rather than v1. So, I've recompiled using v0 locally.
I actually used the online pioasm tool (https://wokwi.com/tools/pioasm), but it looks like they must be on an outdated version. It seems that all of the other pio headers are compiled using v0 rather than v1. So, I've recompiled using v0 locally.
@Andy2No @earlephilhower Just a quick comment on the floating point debate. I am aware of the architectural differences between RP2040 and RP2350. In the case of the RP2040, my understanding is that pico-sdk implemented integer calculations which approximate their floating point counterparts.
When experimenting with this feature with a basic loop I2S::available() check then I2S::read16() and I2S::write16, even basic calculations were causing output underflows (stutters in the audio output). Here's an example of this:
const float volume = 0.75;
void loop() {
int16_t l, r;
while (i2s.available()) {
if (!i2s.read16(&l, &r)) {
break;
}
l *= volume;
r *= volume;
i2s.write16(l, r);
}
}
I'm sure an RP2350 with proper FPU would fix this, but I think better buffer management might also do the trick (with size_t I2S::write(const uint8_t *buffer, size_t size)). I'll report back if I have any success there. Otherwise, I don't think it's critical to this PR.
@Andy2No @earlephilhower Just a quick comment on the floating point debate. I am aware of the architectural differences between RP2040 and RP2350. In the case of the RP2040, my understanding is that pico-sdk implemented integer calculations which approximate their floating point counterparts.
When experimenting with this feature with a basic loop `I2S::available()` check then `I2S::read16()` and `I2S::write16`, even basic calculations were causing output underflows (stutters in the audio output). Here's an example of this:
```
const float volume = 0.75;
void loop() {
int16_t l, r;
while (i2s.available()) {
if (!i2s.read16(&l, &r)) {
break;
}
l *= volume;
r *= volume;
i2s.write16(l, r);
}
}
```
I'm sure an RP2350 with proper FPU would fix this, but I think better buffer management might also do the trick (with `size_t I2S::write(const uint8_t *buffer, size_t size)`). I'll report back if I have any success there. Otherwise, I don't think it's critical to this PR.
The RP2040 has some helper FP functions in ROM so they're 1-ROM read cycle access (vs. XIP cache loads) (not 1 cycle in operation!). They're supposed to be optimized for speed not accuracy. But only some functions are in ROM and it's still doing floating point in software. Each iteration of your loop has to go through an integer->double(I *think...might only be a single) conversion, a FP multiply, and a FP->integer conversion to go back to int16_t (x2 for stereo). You might have enough CPU on average, but I bet there's enough jitter due to USB IRQs, XIP cache misses, etc., that you're missing some deadlines.
A pure integer version would be (top of my head so sorry for typos)
Doing a read(buffer, count)<process-a-bunch-in-situ>write(buffer,count) instead of 1-sample at a time would save a lot of overhead, too, in here and in the ABM.
The RP2040 has some helper FP functions in ROM so they're 1-ROM read cycle access (vs. XIP cache loads) (not 1 cycle in operation!). They're supposed to be optimized for speed not accuracy. But only some functions are in ROM and it's still doing floating point in software. Each iteration of your loop has to go through an integer->double(I *think...might only be a single) conversion, a FP multiply, and a FP->integer conversion to go back to int16_t (x2 for stereo). You might have enough CPU on average, but I bet there's enough jitter due to USB IRQs, XIP cache misses, etc., that you're missing some deadlines.
A pure integer version would be (top of my head so sorry for typos)
````
const float volume = 0.75;
const int32_t sf = (1<<16 ) * volume;
void loop() {
int16_t l, r;
while (i2s.available()) {
if (!i2s.read16(&l, &r)) {
break;
}
int32_t ll = l;
ll *= sf;
l = ll >> 16;
int32_t rr = r;
rr *= sf;
r = rr >> 16;
i2s.write16(l, r);
}
}
````
Doing a `read(buffer, count)` `<process-a-bunch-in-situ>` `write(buffer,count)` instead of 1-sample at a time would save a lot of overhead, too, in here and in the ABM.
earlephilhower
(Migrated from github.com)
left a comment
Copy Link
Copy Source
LGTM now! Good compromise with the constructor. I've done some simple tests with other libraries that use I2S output and not seen any compile or function issues so I'm happy to get this merged it. Thx again!
LGTM now! Good compromise with the constructor. I've done some simple tests with other libraries that use I2S output and not seen any compile or function issues so I'm happy to get this merged it. Thx again!
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
I've added a bi-directional mode to the I2S library using
INPUT_PULLUPas the value of direction for the constructor. Certain resources of the object have been split into input and output variants (eg: pins, flags, audio buffers, and callbacks). Some minor API changes have been made to allow separate access to these resources as needed, but there shouldn't be any breaking changes (besides a slight change in constructor pin arguments). The documentation has been updated to reflect these changes. A simple loopback example has also been included.Notes:
INPUT_PULLUPto use here, I'd love the input. I think it'd be best avoid creating a specific enum for this unless it could be applied to other audio libraries.Nice, will look at it later today. However, looks like a Python cache file made it into the PR. Can you
git rm tools/__pycache__/uf2conv.cpython-313.pyc?My bad! That one definitely slipped through.
I've been doing some playing around with this update and an RP2040 with a WM8960 codec. Results are quite good so far! I've been able to process the audio signal with simple integer-based processing and get clean output. However, once I throw in any floating point logic, the output begins to distort. I'm thinking I either need to look deeper into the buffer settings or just switch over to an RP2350 board. But I don't think that consideration has much of an effect on this PR except to potentially update the example.
@relic-se Looks interesting. The RP2040 has a hardware integer division unit, so fixed point would be a better choice than floating point - e.g. represent a 16 bit sample as a 32 bit number, with the fractional part as the lower 16 bits. That way you can do everything with integer operations.
The main thing is to always do any division as the last operation in a calculation, where possible, to keep as much precision as possible. E.g. don't do
Re: performance, there's only soft-FP on the RP2040 so anything that's doing floating point will be pretty slow. Fixed point math is your friend there, though, and it's not hard to work with 17.15 (easier saturating math) or 16.16 (easier normalizing) numbers on a 32b device. Alternatively, look at the callback routines. You'll be delivered a block of
bufsizel/r samples you can iterate over all at once. There's a lot of overhead doing individual 16bit read and writes plus you have the potential for cache spillage. You also have the 2nd core sitting there, unused...it's possible to grab data from the I2S running on core 0 and push work down to core1 for processing and re-output.But a Pico2 is only $1.00 more and gives 2x flash and ~2x performance plus has single-precision FP in HW. Choices... 😆
FWIW, when I did block operations in BackgroundAudio (instead of per-sample ones like in ESP8266Audio) I went from an unusable webradio streamer (jitter, skips, just out of CPU oomph) to something that's rock solid and has ~30% free processing power left over each
loopeven while serving web pages and decoding MP3.This is great work and very much appreciated. Been something sitting out there since #1055 over 2 years ago, and it'll be awesome to get it into the core.
Very minor nomenclature and DWIM changes and I think we're set.
I'll have to wire up a Pico (everything's stuck on about 8 different ESP32 boards now for another library I'm writing) and make sure it doesn't break anything existing that shouldn't be broken. Visually, seems solid!
Thx again!
@@ -30,6 +30,12 @@ I2S(INPUT)Creates an I2S input port. Needs to be connected up to thedesired pins (see below) and started before any input can happen.I2S(INPUT_PULLUP)I appreciate shoehorning this into an existing
enumhas tradeoffs. We can leave this for now, but I think for my own sanity I'll just add aBIDIRorINPUT_OUTPUTenum in the ArduinoCoreAPI. We already added enums for the multiple output drive strengths, and one more is no biggie...The original method was
getOverUnderflowbecause it would be either Overflow (for input) or Underflow (for output). Probably not the best design choice but naming things is hard!But now that there really is a separately tracked Overflow and Underflow state, can we use
getOverflowandgetUnderflow? All theseOverUnders make me feel like I'm in Vegas. :)Since we can do bidir, then this should really just be
available(andavailableForWrite) no? That's the canonical definition/name for this inStream.Maybe also add
|| (_isOutput && _isInput)to the condition? Should error if they're calling it on a bidir I2S since it's ambiguous and they need to use the 2 calls you added below.@@ -303,12 +362,12 @@ bool I2S::end() {}int I2S::available() {You;re bending over backwards now to make broken code work. I think
auto avail = _isInput ? _arbInput->available() : 0;covers things. And my comment next line should be 4 bytes, not 4 samples per 32b.This is actually
availableand I tihnk we don't need this method here at all...This is probably originally my comment, but it's wrong here and below.
// 4 bytes per 32-bit sample. Nothing I2S runs 8-bit samples...I get the need to allow multiple pins, but I think we need 2 constructors here not one because in the common, unidirectional case you'd need to put in a dummy
outpin on anINPUTI2S(PinMode direction = OUTPUT, pin_size_t bclk = 26, pin_size_t data = 28, pin_size_t mclk = 25);and
I2S(PinMode direction, pin_size_t bclk, pin_size_t data_out, pin_size_t data_in, pin_size_t mclk = 25);(probably need a little munging w/removing some default args to allos C++ to disambiguate which one you mean)...
The
dataconstructor would assign the internalpinDINorpinDOUTas appropriate fordirectionprobably by calling the other constructor with onepin_size_tset to -1 or something dummy...@@ -72,3 +71,3 @@return false;} else {return _arb->getOverUnderflow();return _isOutput ? _arbOutput->getOverUnderflow() : _arbInput->getOverUnderflow();getOverflow()
getUnderflow()
I think you might have an older version of
pioasm. Dropping the (new) version member of the structure isn't really a problem, I think, but it'll cause diffs when we have to open the.piofile up later on....@@ -30,6 +30,12 @@ I2S(INPUT)Creates an I2S input port. Needs to be connected up to thedesired pins (see below) and started before any input can happen.I2S(INPUT_PULLUP)My initial instinct was to add
INPUT_OUTPUT, but I wasn't sure if there would be any issues down the line in AudioCore-API. I can create a separate PR on that repo if you'd like me to go forward with the change.I did use that naming convention in a previous commit (https://github.com/earlephilhower/arduino-pico/pull/2775/commits/7dcdad69a2447dbc2fcca30819a54c3a98a6184a), but it conflicted with
AudioOutputBase. I wasn't too familiar with the specific use of that method, so I changed it again to avoid the override. From my searching, it doesn't look like it will be a problem.Agreed. It's been removed.
I don't think I'll be able to use multiple constructors in this case because of the types and defaults of the argument list. Ie:
Instead, I've got the order of the arguments worked out so that it's compatible with existing code. The only issue is that it will be a little awkward when initializing a bi-directional I2S bus without the need for mclk in which case you would use -1 or something similar as you alluded.
Good call. I've added a note to the documentation as well.
@@ -303,12 +362,12 @@ bool I2S::end() {}int I2S::available() {I think "broken code" is a bit of an overstatement. :P I've instead just added the check at the start of the function. The only comment I have is that I left the output buffer manager in case some users were using
I2S::available()instead ofI2S::availableForWrite()for an output-only bus. If you think that's a possibility, I can change this back.I actually used the online pioasm tool (https://wokwi.com/tools/pioasm), but it looks like they must be on an outdated version. It seems that all of the other pio headers are compiled using v0 rather than v1. So, I've recompiled using v0 locally.
@Andy2No @earlephilhower Just a quick comment on the floating point debate. I am aware of the architectural differences between RP2040 and RP2350. In the case of the RP2040, my understanding is that pico-sdk implemented integer calculations which approximate their floating point counterparts.
When experimenting with this feature with a basic loop
I2S::available()check thenI2S::read16()andI2S::write16, even basic calculations were causing output underflows (stutters in the audio output). Here's an example of this:I'm sure an RP2350 with proper FPU would fix this, but I think better buffer management might also do the trick (with
size_t I2S::write(const uint8_t *buffer, size_t size)). I'll report back if I have any success there. Otherwise, I don't think it's critical to this PR.The RP2040 has some helper FP functions in ROM so they're 1-ROM read cycle access (vs. XIP cache loads) (not 1 cycle in operation!). They're supposed to be optimized for speed not accuracy. But only some functions are in ROM and it's still doing floating point in software. Each iteration of your loop has to go through an integer->double(I *think...might only be a single) conversion, a FP multiply, and a FP->integer conversion to go back to int16_t (x2 for stereo). You might have enough CPU on average, but I bet there's enough jitter due to USB IRQs, XIP cache misses, etc., that you're missing some deadlines.
A pure integer version would be (top of my head so sorry for typos)
Doing a
read(buffer, count)<process-a-bunch-in-situ>write(buffer,count)instead of 1-sample at a time would save a lot of overhead, too, in here and in the ABM.@@ -30,6 +30,12 @@ I2S(INPUT)Creates an I2S input port. Needs to be connected up to thedesired pins (see below) and started before any input can happen.I2S(INPUT_PULLUP)No worries. We'll leave this for now as documented and in your example. The fewer upstream changes the easier it is to move to newer revisions.
Thanks for the insight! I'll definitely give some of that a whirl.
LGTM now! Good compromise with the constructor. I've done some simple tests with other libraries that use I2S output and not seen any compile or function issues so I'm happy to get this merged it. Thx again!