8000
Skip to content

[speaker, i2s_audio] I2S Speaker implementation using a ring buffer - #7605

Merged
jesserockz merged 14 commits into
esphome:devfrom
kahrendt:i2s-speaker-ring-buffer
Oct 16, 2024
Merged

[speaker, i2s_audio] I2S Speaker implementation using a ring buffer#7605
jesserockz merged 14 commits into
esphome:devfrom
kahrendt:i2s-speaker-ring-buffer

Conversation

@kahrendt
@kahrendt kahrendt commented Oct 14, 2024
Copy link
Copy Markdown
Contributor

What does this implement/fix?

This is a near complete rewrite of the i2s_audio speaker component with improvements for simplification, stability, and supporting different audio sample rates and bits per sample. It adds to the speaker platform to support these new features while avoiding breaking changes. It adds a new audio component that is auto-loaded to handle passing different audio stream settings. Other audio components can auto-load it in the future if needed. Closes #7137, as this implements the core changes (the ESPHome ring buffer uses xStreamBuffers).

  • Speaker platform supports writing audio with a FreeRTOS ticks_to_wait parameter for flexibility when using a separate task to write to the speaker.
  • Speaker platform supports setting the volume with a new action. Each child speaker component needs to implement its own implementation.
    • The i2s_audio speaker component implements this in software.
  • Transfers all audio data via a ring buffer instead of using a FreeRTOS queue.
    • The ring buffer deallocates when stopping, so there isn't an increase in memory use when idle.
  • Uses a FreeRTOS Event Group for communicating between the speaker task and the main loop.
    • Simplifies the state and error reporting and commands.
    • Used to lock writing to the ring buffer, as FreeRTOS Stream buffers should only have 1 task writing at a time. Also ensures the task doesn't deallocate the ring buffer while a write is in progress.
    • Implements the existing stop logic: stop() ends it immediately, finish() ends the task after the ring buffer is emptied, and it will end the task with the configured timeout if no audio has been read after that time.
  • Increases the default timeout to 500 ms to avoid occasional issues with delayed TTS responses.
  • Simplifies the logic for writing to the I2S bus.
    • Uses i2s_write_expand for converting between lower bits per sample to higher bits per sample.
    • Uses i2s_set_clk for handling mono and stereo audio
  • Adds support for reconfiguring the I2S settings by the component sending audio with the set_audio_stream_info function.
    • This enables support for audio with more than 16 bits per sample, a sample rate higher than 16000 Hz, and more than 1 channel - the previous component always assumed that particular format.
    • Should give an error if the sending component tries to set an incompatible audio stream instead of playing distorted audio, though a component needs to implement this call if it is sending non-default setting audio.
  • Documents the various new (and old) functions in detail.

The component should be much more resilient to errors and should hopefully avoid ever getting in a stuck state that locks the device. It should stop the task and unlock the I2S port if it can't recover, but future playback should work without needing to reboot. My ATOM Echo is much more reliable and stable with this implementation. I additionally tested the changes to the parent speaker component with an S3 Box Lite using the esp_adf speaker with no issues. I would appreciate anyone able to test this on other hardware!

I'm keeping as a draft until I add documentation for the speaker.volume_set action.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Related issue or feature (if applicable): not applicable

Pull request in esphome-docs with documentation (if applicable): esphome/esphome.io#4343

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040
  • BK72xx
  • RTL87xx

Example entry for config.yaml:

# Example config.yaml

esphome:
  on_boot:
    - speaker.volume_set: 0.9


speaker:
  - platform: i2s_audio
    id: echo_speaker
    i2s_dout_pin: GPIO22
    dac_type: external
    bits_per_sample: 32bit
    channel: right
    timeout: 500ms

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@kahrendt kahrendt changed the title [i2s_audio] I2S Speaker implementation using a ring buffer [speaker, i2s_audio] I2S Speaker implementation using a ring buffer Oct 14, 2024
@codecov-commenter
codecov-commenter commented Oct 14, 2024
Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.05%. Comparing base (4d8b5ed) to head (1944c47).
Report is 1452 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #7605      +/-   ##
==========================================
+ Coverage   53.70%   54.05%   +0.34%     
==========================================
  Files          50       50              
  Lines        9408     9698     +290     
  Branches     1654     1344     -310     
==========================================
+ Hits         5053     5242     +189     
- Misses       4056     4130      +74     
- Partials      299      326      +27     

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

@kahrendt
kahrendt force-pushed the i2s-speaker-ring-buffer branch from c43a2c3 to 9ce705d Compare October 14, 2024 21:21
@nielsnl68 nielsnl68 left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @kahrendt,

I like the new approach of the speaker component.
I added a couple of suggestions below.

Anyway it looks much better then what i tried to do with it.

Comment thread esphome/components/i2s_audio/speaker/i2s_audio_speaker.cpp
Comment thread esphome/components/i2s_audio/speaker/i2s_audio_speaker.cpp
Comment thread esphome/components/i2s_audio/speaker/i2s_audio_speaker.cpp Outdated
Comment thread esphome/components/i2s_audio/speaker/i2s_audio_speaker.cpp
Comment thread esphome/components/i2s_audio/speaker/i2s_audio_speaker.cpp Outdated
Comment thread esphome/components/i2s_audio/speaker/i2s_audio_speaker.cpp
Comment thread esphome/components/i2s_audio/speaker/i2s_audio_speaker.cpp
@probot-esphome
Copy link
Copy Markdown

Hey there @kahrendt,
Thanks for submitting this pull request! Can you add yourself as a codeowner for this integration? This way we can notify you if a bug report for this integration is reported.
In __init__.py of the integration, please add:

CODEOWNERS = ["@kahrendt"]

And run script/build_codeowners.py

(message by NeedsCodeownersLabel)

@nielsnl68
Copy link
Copy Markdown
Contributor

@kahrendt could you show how to use the new futures in CPP components as well?

@kahrendt
kahrendt commented Oct 15, 2024
Copy link
Copy Markdown
Contributor Author

@kahrendt could you show how to use the new futures in CPP components as well?

I don't have an easy example readily available, but the nabu component in the kahrendt-i2s-audio-approach branch home-assistant-voice-pe calls the new play function in audio_mixer.cpp in the audio_mixer_task function. The set_audio_stream_info function is called in the nabu_media_player.cpp file in the same branch.

Thanks for all the feedback!

Comment thread esphome/components/audio/audio.h
@kahrendt
kahrendt marked this pull request as ready for review October 15, 2024 14:46
@kahrendt
kahrendt requested a review from jesserockz as a code owner October 15, 2024 14:46
@probot-esphome
Copy link
Copy Markdown

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

@nielsnl68
Copy link
Copy Markdown
Contributor

Keven, about the speaker_task(), what you could do is the following:

class i2s_audio_speaker .... {
  static void speaker_task(void *params);
protected:
  void run_speaker_task_();
}

}

static void i2s_audio_speaker::speaker_task(void *params) {
  I2SAudioSpeaker *this_speaker = (I2SAudioSpeaker *) params;
  this_speaker->run_speaker_task_();
}

This work very well on my espnow component.

@jesserockz
Copy link
Copy Markdown
Member

Keven, about the speaker_task(), what you could do is the following

I actually think it is better to keep the task running code in the static function, it reminds you that by using this_speaker instead of just this that you are in a task and shouldn't do unsafe operations.

@jesserockz
jesserockz merged commit 1c845e0 into esphome:dev Oct 16, 2024
@nielsnl68
Copy link
Copy Markdown
Contributor

Keven, about the speaker_task(), what you could do is the following

I actually think it is better to keep the task running code in the static function, it reminds you that by using this_speaker instead of just this that you are in a task and shouldn't do unsafe operations.

Okay, then i will update my espnow component as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

0