esphome / issues

Issue Tracker for ESPHome
https://esphome.io/
290 stars 34 forks source link

I2S Media Audio Component - L and R channels are swapped #3409

Open chunkysteveo opened 2 years ago

chunkysteveo commented 2 years ago

The problem

Testing the new I2S Audio Player component with an ESP32 and I2S DAC (PCM5102A) - I noticed that the channels are swapped when using stereo - i.e. left is coming out right, and right is left.

Installed Squeezelite-esp32 firmware and configured the default settings to match the same I2C DAC pins and their setup - the channels are CORRECT - Left is left, Right is right. Revert the ESP back to ESPHome firmware and the channels are swapped again to their incorrect sides.

Which version of ESPHome has the issue?

2022.6.2

What type of installation are you using?

Home Assistant Add-on

Which version of Home Assistant has the issue?

2022.6.7

What platform are you using?

ESP32

Board

TinyPico V1 + PCM5102A Stereo I2C DAC

Component causing the issue

I2S Audio

Example YAML snippet

media_player:
  - platform: i2s_audio
    name: ESPHome I2S Media Player
    dac_type: external
    i2s_lrclk_pin: GPIO33
    i2s_dout_pin: GPIO22
    i2s_bclk_pin: GPIO19
    mode: stereo

Anything in the logs that might be useful for us?

https://open.spotify.com/track/5ezjAnO0uuGL10qvOe1tCT?si=57ab27cde4de4724 is a good stereo track example from Spotify that can be used to show if your Left is your Left, or not?!

Additional information

From what I have read up - I think this is an issue with the Arduino implementation of I2S audio, and also possible the master Espressif esp-idf. I managed to find some solutions about the data being in the high position, and thus the bits need to be a 1, rather than a 0 in the code. Ref - https://github.com/redchenjs/esp-idf/commit/3040a4d7341846cb34420393df96c5d8cf9478f7 file components/driver/i2s.c

Scanning through Squeezelite firmware files, they have a patch for the same code in their files (components/services/i2s.c ). Looks like the had same issue and corrected it - tx_msb_right = 1, rx_msb_right = 1 - although they have a check for 32 bits per sample.

This is where I am stumped and don't know where/how to fix? I see i2s->conf.tx_msb_right = 1; but that's for I2S FastLED, not sure if that's also used for audio?

Would be great to get the ESPHome I2S Audio channels outputting correctly!

nagyrobi commented 2 years ago

Would be nice to also have an option in config swap_stereo_channels: true.

chunkysteveo commented 2 years ago

Would be nice to also have an option in config swap_stereo_channels: true.

That would be a valuable config option.

nagyrobi commented 2 years ago

Just thinking: how about:

i2s_lrclk_pin: 
  number: GPIO33
  inverted: true

Edit: D'oh, fails with This variable only supports pin numbers, not full pin schemas (with inverted and mode). A bit misleading that doc points to Pin Schema config, which doesn't seem to be supported here.

probot-esphome[bot] commented 2 years ago

Hey there @jesserockz, mind taking a look at this issue as it has been labeled with an integration (i2s_audio) you are listed as a code owner for? Thanks! (message by CodeOwnersMention)

chunkysteveo commented 2 years ago

A bit misleading that doc points to Pin Schema config, which doesn't seem to be supported here.

The I2S audio integration is very new in ESPHome, everything still needs bedding in and documents correcting. We can forgive them for now! 😄

nagyrobi commented 2 years ago

Sure, sure, I was just trying to follow the docs. I even tried to fork the code and adapt it a little to accept the full schema but I run into:

Compiling /data/wt32-eth01-media/.pioenvs/wt32-eth01-media/src/esphome/components/i2s_audio/i2s_audio_media_player.cpp.o
src/esphome/components/i2s_audio/i2s_audio_media_player.cpp: In member function 'virtual void esphome::i2s_audio::I2SAudioMediaPlayer::setup()':
src/esphome/components/i2s_audio/i2s_audio_media_player.cpp:111:52: error: invalid conversion from 'esphome::GPIOPin*' to 'uint8_t {aka unsigned char}' [-fpermissive]
     this->audio_->setPinout(this->bclk_pin_, this->lrclk_pin_, this->dout_pin_);
                                                    ^
In file included from src/esphome/components/i2s_audio/i2s_audio_media_player.h:10:0,
                 from src/esphome/components/i2s_audio/i2s_audio_media_player.cpp:1:
/piolibs/ESP32-audioI2S/src/Audio.h:174:10: note:   initializing argument 2 of 'bool Audio::setPinout(uint8_t, uint8_t, uint8_t, int8_t)'
     bool setPinout(uint8_t BCLK, uint8_t LRC, uint8_t DOUT, int8_t DIN=I2S_PIN_NO_CHANGE);
          ^
*** [/data/wt32-eth01-media/.pioenvs/wt32-eth01-media/src/esphome/components/i2s_audio/i2s_audio_media_player.cpp.o] Error 1
Compiling /data/wt32-eth01-media/.pioenvs/wt32-eth01-media/lib64d/WiFi/WiFi.cpp.o
========================== [FAILED] Took 4.08 seconds ==========================
chunkysteveo commented 2 years ago

You're smarter than me to try to figure this out!!

nagyrobi commented 2 years ago

Not enough, to understand where to look for to find where to go forward.

My main idea was just to simply invert the lrclk as that's the one telling to the DAC which data packets belong to L and which to R channel. So just inverting the GPIO output should be enough - in theory. And it could be kept as configurable as one might want to swap those channels deliberately for any special reason (say - fixed cabling installation done above the ceiling, but turns out later it needs to be swapped).

nagyrobi commented 2 years ago

Made some Stereo Test files just for ESPHome: https://nagyrobi.github.io/assets/audio/esphome_stereo_test.mp3 https://nagyrobi.github.io/assets/audio/esphome_stereo_test.ogg https://nagyrobi.github.io/assets/audio/esphome_stereo_test.wav

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

elik745i commented 1 year ago

Stereo mode does not work, I'm using two MAX98357A boards and both sounds the same.