esp-rs / esp-hal

no_std Hardware Abstraction Layers for ESP32 microcontrollers
https://docs.esp-rs.org/esp-hal/
Apache License 2.0
678 stars 189 forks source link

I2S write broken #1832

Closed bjoernQ closed 1 month ago

bjoernQ commented 1 month ago

Apparently i2s-write is broken - luckily released 0.19.0 is not affected

❯ git bisect good
6fd9443c138bc3eb46da5dead333ccf6d424cec7 is the first bad commit
commit 6fd9443c138bc3eb46da5dead333ccf6d424cec7
Author: Dominic Fischer <14130965+Dominaezzz@users.noreply.github.com>
Date:   Tue Jul 16 09:52:48 2024 +0100

    Allow users to easily name DMA channels (#1771)

    * Introduce public DmaChannel trait

    * Use DmaChannel trait in all peripherals

    * Simplify ChannelTypes trait

    * changelog

    * Rename things

    * missed a spot

    ---------

    Co-authored-by: Dominic Fischer <git@dominicfischer.me>

 esp-hal/CHANGELOG.md             |   1 +
 esp-hal/src/aes/mod.rs           |  24 +++---
 esp-hal/src/dma/gdma.rs          |  33 +++++---
 esp-hal/src/dma/mod.rs           | 168 ++++++++++++++++++---------------------
 esp-hal/src/dma/pdma.rs          |  20 +++--
 esp-hal/src/i2s.rs               |  88 ++++++++++----------
 esp-hal/src/lcd_cam/cam.rs       |  25 +++---
 esp-hal/src/lcd_cam/lcd/i8080.rs |  29 +++----
 esp-hal/src/parl_io.rs           |  84 ++++++++++----------
 esp-hal/src/spi/master.rs        |  68 ++++++++--------
 esp-hal/src/spi/slave.rs         |  36 +++++----
 11 files changed, 291 insertions(+), 285 deletions(-)

Easy to reproduce via cargo xtask run-example examples esp32c6 embassy_i2s_sound - after that commit it doesn't do anything anymore

This is especially bad since the hil-test didn't catch this 😢

bjoernQ commented 1 month ago

Ok makes sense the test didn't catch this since it's only broken for async (which doesn't make a lot of sense somehow)

bjoernQ commented 1 month ago

I guess I found it: https://github.com/esp-rs/esp-hal/blob/bc7c53f6680dc194262a451e3c9bf71c2730c9a5/esp-hal/src/dma/mod.rs#L1455

That should be CH::Tx::waker()

Dominaezzz commented 1 month ago

Whoops! sorry about that 😬

bjoernQ commented 1 month ago

Whoops! sorry about that 😬

No problem - should have noticed it during the review 😃