working isr-based sink, but still grainy

src/audio/audio_playback.cpp

@@ -41,7 +41,7 @@ AudioPlayback::AudioPlayback(std::unique_ptr<I2SAudioOutput> output)
   pipeline->AddInput(file_source_.get());
 
   task::StartPipeline(pipeline, i2s_output_.get());
-  task::StartDrain(i2s_output_.get());
+  // task::StartDrain(i2s_output_.get());
 }
 
 AudioPlayback::~AudioPlayback() {}

src/audio/audio_task.cpp

@@ -58,7 +58,7 @@ auto StartDrain(IAudioSink* sink) -> void {
 
   ESP_LOGI(kTag, "starting audio drain task");
   xTaskCreate(&AudioDrainMain, "drain", kDrainStackSize, drain_args,
-                          kTaskPriorityAudioDrain, NULL);
+              kTaskPriorityAudioDrain, NULL);
 }
 
 void AudioTaskMain(void* args) {
@@ -134,7 +134,7 @@ void AudioTaskMain(void* args) {
           // The format of the stream within the sink stream has changed. We
           // need to reconfigure the sink, but shouldn't do so until we've fully
           // drained the current buffer.
-          if (xStreamBufferIsEmpty(*sink->buffer())) {
+          if (xStreamBufferIsEmpty(sink->buffer())) {
             ESP_LOGI(kTag, "reconfiguring dac");
             output_format = sink_stream.info().format;
             sink->Configure(*output_format);
@@ -149,7 +149,7 @@ void AudioTaskMain(void* args) {
           // throttle this task's CPU time. Maybe also hold off on the pipeline
           // if the buffer is already close to full?
           std::size_t sent = xStreamBufferSend(
-              *sink->buffer(), sink_stream.data().data(),
+              sink->buffer(), sink_stream.data().data(),
               sink_stream.data().size_bytes(), pdMS_TO_TICKS(10));
           sink_stream.consume(sent);
         }
@@ -172,7 +172,7 @@ void AudioDrainMain(void* args) {
 
     // TODO(jacqueline): implement PAUSE without busy-waiting.
     while (*command != QUIT) {
-      std::size_t len = xStreamBufferReceive(*sink->buffer(), sDrainBuf,
+      std::size_t len = xStreamBufferReceive(sink->buffer(), sDrainBuf,
                                              sizeof(sDrainBuf), portMAX_DELAY);
       if (len > 0) {
         sink->Send({sDrainBuf, len});

src/audio/i2s_audio_output.cpp

@@ -39,8 +39,8 @@ auto I2SAudioOutput::create(drivers::GpioExpander* expander)
 I2SAudioOutput::I2SAudioOutput(drivers::GpioExpander* expander,
                                std::unique_ptr<drivers::AudioDac> dac)
     : expander_(expander), dac_(std::move(dac)), current_config_() {
-      //dac_->SetSource(buffer());
+  dac_->SetSource(buffer());
-    }
+}
 
 I2SAudioOutput::~I2SAudioOutput() {
   dac_->SetSource(nullptr);

src/audio/include/audio_sink.hpp

@@ -11,22 +11,26 @@ class IAudioSink {
  private:
   // TODO: tune. at least about 12KiB seems right for mp3
   static const std::size_t kDrainBufferSize = 24 * 1024;
-  uint8_t *buffer_;
+  uint8_t* buffer_;
-  StaticStreamBuffer_t *metadata_;
+  StaticStreamBuffer_t* metadata_;
-  StreamBufferHandle_t *handle_;
+  StreamBufferHandle_t handle_;
 
  public:
-  IAudioSink() :
+  IAudioSink()
-    buffer_(reinterpret_cast<uint8_t*>(heap_caps_malloc(kDrainBufferSize, MALLOC_CAP_DMA))),
+      : buffer_(reinterpret_cast<uint8_t*>(
-    metadata_(reinterpret_cast<StaticStreamBuffer_t*>(heap_caps_malloc(sizeof(StaticStreamBuffer_t), MALLOC_CAP_DMA))),
+            heap_caps_malloc(kDrainBufferSize,
-    handle_(reinterpret_cast<StreamBufferHandle_t*>(heap_caps_malloc(sizeof(StreamBufferHandle_t), MALLOC_CAP_DMA))) {
+                             MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT))),
-      *handle_ = xStreamBufferCreateStatic(kDrainBufferSize, 1, buffer_, metadata_);
+        metadata_(reinterpret_cast<StaticStreamBuffer_t*>(
-    }
+            heap_caps_malloc(sizeof(StaticStreamBuffer_t),
+                             MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT))),
+        handle_(xStreamBufferCreateStatic(kDrainBufferSize,
+                                          1,
+                                          buffer_,
+                                          metadata_)) {}
 
   virtual ~IAudioSink() {
-    vStreamBufferDelete(*handle_);
+    vStreamBufferDelete(handle_);
     free(buffer_);
-    free(handle_);
     free(metadata_);
   }
 
@@ -34,7 +38,7 @@ class IAudioSink {
   virtual auto Send(const cpp::span<std::byte>& data) -> void = 0;
   virtual auto Log() -> void {}
 
-  auto buffer() -> StreamBufferHandle_t* { return handle_; }
+  auto buffer() -> StreamBufferHandle_t { return handle_; }
 };
 
 }  // namespace audio

src/drivers/dac.cpp

@@ -33,7 +33,7 @@ auto AudioDac::create(GpioExpander* expander)
   i2s_chan_handle_t i2s_handle;
   i2s_chan_config_t channel_config =
       I2S_CHANNEL_DEFAULT_CONFIG(kI2SPort, I2S_ROLE_MASTER);
-  channel_config.auto_clear = true;
+  // channel_config.auto_clear = true;
 
   ESP_ERROR_CHECK(i2s_new_channel(&channel_config, &i2s_handle, NULL));
   //
@@ -115,7 +115,7 @@ AudioDac::AudioDac(GpioExpander* gpio, i2s_chan_handle_t i2s_handle)
       i2s_active_(false),
       active_page_(),
       clock_config_(I2S_STD_CLK_DEFAULT_CONFIG(44100)),
-      slot_config_(I2S_STD_MSB_SLOT_DEFAULT_CONFIG(I2S_DATA_BIT_WIDTH_16BIT,
+      slot_config_(I2S_STD_PHILIPS_SLOT_DEFAULT_CONFIG(I2S_DATA_BIT_WIDTH_16BIT,
                                                        I2S_SLOT_MODE_STEREO)) {
   clock_config_.clk_src = I2S_CLK_SRC_PLL_160M;
   gpio_->set_pin(GpioExpander::AMP_EN, true);
@@ -192,8 +192,8 @@ auto AudioDac::Reconfigure(BitsPerSample bps, SampleRate rate) -> void {
       bps == BPS_24 ? I2S_MCLK_MULTIPLE_384 : I2S_MCLK_MULTIPLE_256;
   ESP_ERROR_CHECK(i2s_channel_reconfig_std_clock(i2s_handle_, &clock_config_));
 
-  WriteRegister(pcm512x::I2S_1, (0b11 << 4) | bps_bits);
+  WriteRegister(pcm512x::I2S_1, bps_bits);
-  WriteRegister(pcm512x::I2S_2, 0);
+  // WriteRegister(pcm512x::I2S_2, 0);
 
   // Configuration is all done, so we can now bring the DAC and I2S stream back
   // up. I2S first, since otherwise the DAC will see that there's no clocks and
@@ -212,31 +212,35 @@ auto AudioDac::WriteData(const cpp::span<const std::byte>& data) -> void {
   }
 }
 
-IRAM_ATTR auto callback(i2s_chan_handle_t handle, i2s_event_data_t *event, void *user_ctx) -> bool {
+extern "C" IRAM_ATTR auto callback(i2s_chan_handle_t handle,
+                                   i2s_event_data_t* event,
+                                   void* user_ctx) -> bool {
   if (event == nullptr || user_ctx == nullptr) {
     return false;
   }
   if (event->data == nullptr || event->size == 0) {
     return false;
   }
-  StreamBufferHandle_t *src = reinterpret_cast<StreamBufferHandle_t*>(user_ctx);
+  uint8_t** buf = reinterpret_cast<uint8_t**>(event->data);
+  StreamBufferHandle_t src = reinterpret_cast<StreamBufferHandle_t>(user_ctx);
   BaseType_t ret = false;
-  std::size_t bytes_received = xStreamBufferReceiveFromISR(*src, event->data, event->size, &ret);
+  std::size_t bytes_received =
+      xStreamBufferReceiveFromISR(src, *buf, event->size, &ret);
   if (bytes_received < event->size) {
-    // TODO(jacqueline): zero-pad.
+    memset(*buf + bytes_received, 0, event->size - bytes_received);
   }
   return ret;
 }
 
-auto AudioDac::SetSource(StreamBufferHandle_t *buffer) -> void {
+auto AudioDac::SetSource(StreamBufferHandle_t buffer) -> void {
   if (i2s_active_) {
     ESP_ERROR_CHECK(i2s_channel_disable(i2s_handle_));
   }
-  i2s_event_callbacks_t callbacks {
+  i2s_event_callbacks_t callbacks{
-    .on_recv = NULL,
+      .on_recv = NULL,
-    .on_recv_q_ovf = NULL,
+      .on_recv_q_ovf = NULL,
-    .on_sent = NULL,
+      .on_sent = NULL,
-    .on_send_q_ovf = NULL,
+      .on_send_q_ovf = NULL,
   };
   if (buffer != nullptr) {
     callbacks.on_sent = &callback;

src/drivers/include/dac.hpp

@@ -158,7 +158,7 @@ class AudioDac {
   auto Reconfigure(BitsPerSample bps, SampleRate rate) -> void;
 
   auto WriteData(const cpp::span<const std::byte>& data) -> void;
-  auto SetSource(StreamBufferHandle_t *buffer) -> void;
+  auto SetSource(StreamBufferHandle_t buffer) -> void;
 
   auto Stop() -> void;
   auto LogStatus() -> void;