IS2 transfers using 32-bit data to and from AIC

[chipaudette]

Currently, the Tympan library uses 16-bit resolution for audio samples as they're transfered between the audio codec and the main processor. This behavior was inherited from the Teensy Audio library.

With good PDM mics and with good AICs, we might be limited by the 96dB dynamic range inherent with 16-bit transfers.

Is it time to upgrade our code to support 32-bit data transfers?

Work to do:

• Alter the I2S configuration for the I2S input and output classes
• Alter the DMA configuration in those same classes
• Alter the ISR() and update() methods in those same classes

Considerations:

• Do we enable the classes to be configured for either 16-bit or 32 but? Or do we fully transition to 32-bit and exclude 16-bit
• Do we also enable this functionality for older Tympans (Teensy 3.6 based Tympan Rev D) or just the newer Tympans (Teensy 4.1 based Tympan Rev E and F)?
• Do we do all variants of the I2S classes (stereo, quad, hex) or just a subset?

Tag: @cab-creare-com , @eyuan-creare

[chipaudette]

Branch started for mods to quad I2S classes:

https://github.com/Tympan/Tympan_Library/tree/feature/32bit_quad_i2s

We think that the I2S setup and DMA setup has been modified. We are still working on the ISR and update() methods.

Currently, we're taking a very narrow approach:

• this code only supports 32-bit mode (there is no software switch to go back to 16-bit mode).
• I think that this is only for Rev E and F...it probably breaks on Rev D (which is not acceptable)
• We're currently only doing the I2S quad classes

[cab-creare-com]

Do we enable the classes to be configured for either 16-bit or 32-bit? Or do we fully transition to 32-bit and exclude 16-bit
Do we also enable this functionality for older Tympans (Teensy 3.6 based Tympan Rev D) or just the newer Tympans (Teensy 4.1 based Tympan Rev E and F)?
Do we do all variants of the I2S classes (stereo, quad, hex) or just a subset? 

Considerations:

• The I2S BCLK and WCLK frequencies were already specified to allow for 32-bit comms (at least for Quad I2S on Tympan Rev F)
• There is a small memory increase in shifting to 32-bit. (Increase is 2 * 2 bytes * MAX_AUDIO_BLOCK_SAMPLES_F32 * NUM_CHANNELS.)

My preference:

• Let classes be configured for either 16-bit or 32-bit using a #define. (Candidate: AIC_BITS = 16 or 32, as in control_aic3206.cpp.)
• We should make this change for all variants of the I2S classes.
• We do not enable the functionality for older Tympans until/unless we have a use case.

[chipaudette]

I have now added the 32-bit transfers to AudioInputI2SQuad_F32. As it currently stands within the branch, this only works on Tympan Rev E or F (ie, Teensy 4.1). It breaks when compiling for Tympan Rev D (ie, Teensy 3.6).

[chipaudette]

My preference:

• Let classes be configured for either 16-bit or 32-bit using a #define. (Candidate: AIC_BITS = 16 or 32, as in control_aic3206.cpp.)

@cab-creare-com , If I understand you correctly, there would be a #define at the top of each header file for each I2S class? If so, I don't think that I agree with this approach as it requires the user to edit their library differently if a given project needs 16 or 32 bit transfers. If we're going to allow the user to choose between 16-bit and 32-bit transfers, I think that I would prefer it to be an optional argument in the constructor.

Thoughts?

[chipaudette]

@cab-creare-com , as you probably remember, we have that other development effort where we use the AudioInputI2SHex_F32 for the I2S input and the AudioOutputI2SQuad_F32 for the I2S output. Due to the need for common I2S clocking (?), I'm guessing that both need to be 16-bit or both need to be 32-bit?

If so, this is another incompatibility that we'll need to fix before merging the 32-bit I2S stuff into the main branch.

[cab-creare-com]

My preference:

• Let classes be configured for either 16-bit or 32-bit using a #define. (Candidate: AIC_BITS = 16 or 32, as in control_aic3206.cpp.)

@cab-creare-com , If I understand you correctly, there would be a #define at the top of each header file for each I2S class? If so, I don't think that I agree with this approach as it requires the user to edit their library differently if a given project needs 16 or 32 bit transfers. If we're going to allow the user to choose between 16-bit and 32-bit transfers, I think that I would prefer it to be an optional argument in the constructor.

Thoughts?

@chipaudette I don't actually think that anyone will want to use 16 bits, unless they're also changing the sampling rate to be higher than 96kHz, which makes it an edge case.

I was thinking that the #define would be wrapped in an #ifndef, so it could be specified as a compiler argument:

#ifndef AIC_BITS
#define AIC_BITS    32
#endif

For the PlatformIO-compiled code, I could add -D'AIC_BITS=(16)' to the build flags, if I needed to force 16 bits. But I'm less familiar with the Arduino IDE build process. Is there a way to specify build arguments in the Arduino IDE?

[cab-creare-com]

@cab-creare-com , as you probably remember, we have that other development effort where we use the AudioInputI2SHex_F32 for the I2S input and the AudioOutputI2SQuad_F32 for the I2S output. Due to the need for common I2S clocking (?), I'm guessing that both need to be 16-bit or both need to be 32-bit?

If so, this is another incompatibility that we'll need to fix before merging the 32-bit I2S stuff into the main branch.

Yes. That's definitely something to test before merging.

[eyuan-creare]

Is there a way to specify build arguments in the Arduino IDE?

Perhaps create a new "board" in the Arduino IDE?

• Reference: https://arduino.github.io/arduino-cli/1.2/platform-specification/#boardstxt

Attached is the teensy boards.txt found at: \AppData\Local\Arduino15\packages\teensy\hardware\avr\1.59.0\

teensy41.build.flags.defs=-D__IMXRT1062__ -DTEENSYDUINO=159

-D name=definition
The contents of definition are tokenized and processed as if they appeared during translation phase three in a ‘#define’ directive. In particular, the definition is truncated by embedded newline characters.

[chipaudette]

@cab-creare-com and I just chatted. We agree that a good path forward is to enable 32-bit mode in all of the I2S classes. This will require a small amount of time for him to update the DMA settings and a modestly longer time for me to update the isr() and update() functions.

The main wrinkle with this plan is whether we should allow a 16-bit mode to be available to the user as an option. This might be needed if:

• The AIC3212 or PCMD3140 are somehow incompatible with our implementation of 32-bit I2S transfers
• The PDM clock rates generated by the AIC3206 / AIC3212 / PCMD3140 when in 32-bit mode is somehow incompatible with the specific PDM mics that we're using

To answer these two questions, we should:

• Test using the existing Auren hardware, which would confirm that the PDM clocking is fine for the Auren's mics via the AIC3206
• Extend the AudioInputI2SHex_F32 class for 32-bit and then test with our other hardware to confirm both the AIC3212 and the PCMD3140.

If we pass both tests, we can move forward with confidence on the AudioInputI2S_F32, AudioOutputI2S_F32, AudioOutputI2SHex_F32.

We do still need to talk about make it compatible with RevD. For that, we either need to make 32-bit switchable back to 16-bit, or we need to update all the processor-specific I2S and DMA settings for RevD's processor.

[eyuan-creare]

@chipaudette this branch renames this:

Tympan_Library/src/SDWriter.h

Line 30 in 7548322

#define maxBufferLengthBytes 150000 //size of big memroy buffer to smooth out slow SD write operations

to this:

Tympan_Library/src/SDWriter.h

Line 30 in 2c78f83

#define SDWRITER_MAX_BUFFER_LENGTH_BYTES 150000 //size of big memroy buffer to smooth out slow SD write operations

which breaks the external Open Hearing project, AudioManager.cpp ln 400

resp["maxBufferLengthBytes"] = maxBufferLengthBytes;

Should we change it back? Or update it in Open Hearing?
@cab-creare-com

[cab-creare-com]

@eyuan I already the Open Hearing feature/32-bit-audio branch.

[chipaudette]

@cab-creare-com , I believe that I've gotten all I2S classes working with 32-bits for Teensy4 platforms and with 16-bis for Teensy3 platforms.

As before, I've tested AudioPassThru in the following:

• RevE vs RevD
• Stereo vs Quad vs Hex
• 44100 and 96000 kHz along with different block sizes

Caveats:

• My tests have only run for ~10 seconds, not minutes or hours
• While I did test the inputs and outputs, I cannot confirm that the output loudness produced by the RevD and RevE is the same, which means that I cannot confirm the out-going scaling (in the AudioOutputI2Sxxx classes) is consistent.

On this last point, my ears thought they heard a difference in loudness between RevD and RevE. But, pure tones can be very tricky to assess subjectively. So, while I don't really think that there's a problem, you and @eyuan-creare could keep an eye out for it. Specifically, when you're working to calibrate the output of Rev E / Rev F, you might wish to compare back to results from earlier firmware and see if the loudness calibration is consistent between then and now.

Be aware that the paragraph above is only regarding the output levels from the system. Regarding the inputs, I was able to test the inputs of of the system by recording the in-coming signal to the SD and measuring the RMS of the WAV files. This testing confirmed that both RevE and RevD see the same signal level coming in. Therefore, I have confirmd that the in-coming scaling (in the AudioInputI2Sxxx classes) appears to be just fine. Great.

@cab-creare-com , from my perspective, the collection of revisions in this branch look good. Presumably, you'll integrate this code back into the OpenHearing firmware. If your work with this code also shows that everything is working fine, I think that we can go ahead and merge the changes into the main branch of the Tympan_Library.

When you are also content that everything is stable, please accept the pull request so that it merges. Or, you can tell me that you're content and I'll do the merge.

Thanks for the good work on this!

[chipaudette]

As discussed with @cab-creare-com , I've now unified the I2S classes:

• All InputI2S classes use a common isr() and a common update() method via inheriting from AudioInputI2SBase_F32
• All OutputI2S classes use a common isr() and a common update() method via inheriting from AudioOutputI2SBase_F32
• Some methods used by both Input and Output classes now use a central implementation in AudioI2SBase

The individual classes (AudioInputI2S_F32, AudioInputI2SQuad_F32, etc) are now much reduced. Basically, they're only holding the I2S and DMA setup commands. That's it. Even in this modes amount of remaining code, there is more code that could gathering together and moved under the base class, but I did not do so. I had too many questions. We can pursue this in the future, if we want.

My updated code has been merged into the 32-bit I2S branch being debated in this pull request. This pull request is the gate keeper to merging all of the 32-bit code (including the 32-bit WAV files) into the develop branch. Whenever @cab-creare-com feels comfortable with this collection of changes, please accept the pull request to do the merge!