Skip to content

esp32/machine_i2s: Integrate new I2S IDF driver. - #13727

Merged
dpgeorge merged 1 commit into
micropython:masterfrom
miketeachman:i2s-esp-mono-fix-pr-dec-2023
Mar 8, 2024
Merged

esp32/machine_i2s: Integrate new I2S IDF driver.#13727
dpgeorge merged 1 commit into
micropython:masterfrom
miketeachman:i2s-esp-mono-fix-pr-dec-2023

Conversation

@miketeachman

@miketeachman miketeachman commented Feb 22, 2024

Copy link
Copy Markdown
Contributor

The legacy I2S "shim" is removed and replaced by the new I2S driver. The new driver fixes a bug where mono audio plays only in one channel.
Application code size is reduced by 2672 bytes with this change. Tested on ESP32, ESP32+spiram, ESP32-S3 using example code from https://github.com/miketeachman/micropython-i2s-examples

@codecov

codecov Bot commented Feb 22, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (4dc262c) to head (0b145fd).

Additional details and impacted files 
@@           Coverage Diff           @@
##           master   #13727   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         161      161           
  Lines       21078    21078           
=======================================
  Hits        20739    20739           
  Misses        339      339

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions

github-actions Bot commented Feb 22, 2024

Copy link
Copy Markdown

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@ricksorensen

ricksorensen commented Feb 22, 2024

Copy link
Copy Markdown
Contributor

Thanks -

Just tried this with SEEED XIAO ESP32-C3 - and it works. I ran your easy_wav_player using your mono and stereo wav files pulled from the local memory (so only 16-bit versions). I used pcm5102 for I2S to audio out.

@miketeachman

miketeachman commented Feb 23, 2024

Copy link
Copy Markdown
Contributor Author

@ricksorensen Fantastic ! Thank you for testing this PR.

dpgeorge
dpgeorge reviewed Feb 26, 2024
View reviewed changes
Comment thread extmod/machine_i2s.c Outdated

@dpgeorge dpgeorge Feb 26, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this blank line can be removed, to keep coding style consistent (eg with the "if" part of this statement).

Comment thread extmod/machine_i2s.c Outdated

@dpgeorge dpgeorge Feb 26, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This blank line can go.

Comment thread extmod/machine_i2s.c Outdated

@dpgeorge dpgeorge Feb 26, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove blank line.

Comment thread extmod/machine_i2s.c Outdated

@dpgeorge dpgeorge Feb 26, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove blank line.

Comment thread extmod/machine_i2s.c Outdated

@dpgeorge dpgeorge Feb 26, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove blank line.

Comment thread ports/esp32/machine_i2s.c Outdated

@dpgeorge dpgeorge Feb 26, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

only one blank line needed here (for consistency with the rest of this file)

Comment thread ports/esp32/machine_i2s.c Outdated

@dpgeorge dpgeorge Feb 26, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

blank line not needed

@dpgeorge

dpgeorge commented Feb 26, 2024

Copy link
Copy Markdown
Member

Thanks for this, it looks good!

I think the line CONFIG_I2S_SUPPRESS_DEPRECATE_WARN can now be removed from esp32/boards/sdkconfig.base.

@miketeachman

miketeachman commented Feb 26, 2024

Copy link
Copy Markdown
Contributor Author

@dpgeorge thanks for reviewing this PR. I removed all the unnecessary blank lines. The macro could be removed after I included the header file for the new driver.

@Gadgetoid Gadgetoid mentioned this pull request Feb 29, 2024
Merged
@projectgus

projectgus commented Mar 7, 2024

Copy link
Copy Markdown
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

@dpgeorge

dpgeorge commented Mar 8, 2024

Copy link
Copy Markdown
Member

I'm currently rebasing this on latest master and about to merge it.

@miketeachman @dpgeorge
esp32/machine_i2s: Integrate new I2S IDF driver.
0b145fd 
The legacy I2S "shim" is removed and replaced by the new I2S driver.  The
new driver fixes a bug where mono audio plays only in one channel.

Application code size is reduced by 2672 bytes with this change.  Tested on
ESP32, ESP32+spiram, ESP32-S3 using example code from
https://github.com/miketeachman/micropython-i2s-examples

Signed-off-by: Mike Teachman <[email protected]>
@dpgeorge
dpgeorge force-pushed the i2s-esp-mono-fix-pr-dec-2023 branch from 7ed5b34 to 0b145fd Compare March 8, 2024 02:33
@dpgeorge
dpgeorge merged commit 0b145fd into micropython:master Mar 8, 2024
@ricksorensen ricksorensen mentioned this pull request May 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dpgeorge dpgeorge dpgeorge left review comments

Assignees

No one assigned

Labels

port-esp32

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@miketeachman @ricksorensen @dpgeorge @projectgus @ma261065