Add a little wrapper for I2C things

main/CMakeLists.txt

@@ -1,4 +1,4 @@
 idf_component_register(
-  SRCS "gay-ipod-fw.cpp" "dac.cpp" "gpio-expander.cpp" "battery.cpp" "storage.cpp"
+  SRCS "gay-ipod-fw.cpp" "dac.cpp" "gpio-expander.cpp" "battery.cpp" "storage.cpp" "i2c.cpp"
   INCLUDE_DIRS "."
   REQUIRES "esp_adc_cal" "fatfs")

main/dac.cpp

@@ -1,5 +1,6 @@
 #include "dac.h"
 
+#include "i2c.h"
 #include "esp_log.h"
 #include "assert.h"
 #include "driver/i2c.h"
@@ -11,18 +12,6 @@ namespace gay_ipod {
 
 static const char* TAG = "AUDIODAC";
 
-/**
- * Utility method for writing to a register on the PCM5122. Writes two bytes:
- * first the address for the register that we're writing to, and then the value
- * to write.
- *
- * Note this function assumes that the correct page has already been selected.
- */
-static void set_register(i2c_cmd_handle_t handle, uint8_t reg, uint8_t val) {
-    i2c_master_write_byte(handle, reg, true);
-    i2c_master_write_byte(handle, val, true);
-}
-
 AudioDac::AudioDac(GpioExpander *gpio) {
   this->gpio_ = gpio;
 };
@@ -132,51 +121,44 @@ void AudioDac::Start(SampleRate sample_rate, BitDepth bit_depth) {
 
   // We've calculated all of our data. Now assemble the big list of registers
   // that we need to configure.
-  i2c_cmd_handle_t handle = i2c_cmd_link_create();
+  I2CTransaction transaction;
-  if (handle == NULL) {
+  transaction
-    return;
+    .start()
-  }
+    .write_addr(kPCM5122Address, I2C_MASTER_WRITE)
-
-  i2c_master_start(handle);
-  i2c_master_write_byte(handle, (kPCM5122Address << 1 | I2C_MASTER_WRITE), true);
-
     // All our registers are on the first page.
-  set_register(handle, Register::PAGE_SELECT, 0);
+    .write_ack(Register::PAGE_SELECT, 0)
-
     // Disable clock autoset and ignore SCK detection
-  set_register(handle, Register::IGNORE_ERRORS, 0x1A);
+    .write_ack(Register::IGNORE_ERRORS, 0x1A)
     // Set PLL clock source to BCK
-  set_register(handle, Register::PLL_CLOCK_SOURCE, 0x10);
+    .write_ack(Register::PLL_CLOCK_SOURCE, 0x10)
     // Set DAC clock source to PLL output
-  set_register(handle, Register::DAC_CLOCK_SOURCE, 0x10);
+    .write_ack(Register::DAC_CLOCK_SOURCE, 0x10)
 
     // Configure PLL
-  set_register(handle, Register::PLL_P, p - 1);
+    .write_ack(Register::PLL_P, p - 1)
-  set_register(handle, Register::PLL_J, j);
+    .write_ack(Register::PLL_J, j)
-  set_register(handle, Register::PLL_D_MSB, (d >> 8) & 0x3F);
+    .write_ack(Register::PLL_D_MSB, (d >> 8) & 0x3F)
-  set_register(handle, Register::PLL_D_LSB, d & 0xFF);
+    .write_ack(Register::PLL_D_LSB, d & 0xFF)
-  set_register(handle, Register::PLL_R, r - 1);
+    .write_ack(Register::PLL_R, r - 1)
 
     // Clock dividers
-  set_register(handle, Register::DSP_CLOCK_DIV, nmac - 1);
+    .write_ack(Register::DSP_CLOCK_DIV, nmac - 1)
-  set_register(handle, Register::DAC_CLOCK_DIV, ndac - 1);
+    .write_ack(Register::DAC_CLOCK_DIV, ndac - 1)
-  set_register(handle, Register::NCP_CLOCK_DIV, ncp - 1);
+    .write_ack(Register::NCP_CLOCK_DIV, ncp - 1)
-  set_register(handle, Register::OSR_CLOCK_DIV, dosr - 1);
+    .write_ack(Register::OSR_CLOCK_DIV, dosr - 1)
 
     // IDAC (nb of DSP clock cycles per sample)
-  set_register(handle, Register::IDAC_MSB, (idac >> 8) & 0xFF);
+    .write_ack(Register::IDAC_MSB, (idac >> 8) & 0xFF)
-  set_register(handle, Register::IDAC_LSB, idac & 0xFF);
+    .write_ack(Register::IDAC_LSB, idac & 0xFF)
 
-  set_register(handle, Register::FS_SPEED_MODE, speedMode);
+    .write_ack(Register::FS_SPEED_MODE, speedMode)
-  set_register(handle, Register::I2S_FORMAT, i2s_format);
+    .write_ack(Register::I2S_FORMAT, i2s_format)
 
-  i2c_master_stop(handle);
+    .stop();
 
   ESP_LOGI(TAG, "Configuring DAC");
   // TODO: Handle this gracefully.
-  ESP_ERROR_CHECK(i2c_master_cmd_begin(I2C_NUM_0, handle, kPCM5122Timeout));
+  ESP_ERROR_CHECK(transaction.Execute());
-
-  i2c_cmd_link_delete(handle);
 
   // The DAC takes a moment to reconfigure itself. Give it some time before we
   // start asking for its state.
@@ -205,45 +187,33 @@ void AudioDac::Start(SampleRate sample_rate, BitDepth bit_depth) {
 }
 
 uint8_t AudioDac::ReadPowerState() {
-  i2c_cmd_handle_t handle = i2c_cmd_link_create();
-  if (handle == NULL) {
-    return 0;
-  }
-
   uint8_t result = 0;
 
-  i2c_master_start(handle);
+  I2CTransaction transaction;
-  i2c_master_write_byte(handle, (kPCM5122Address << 1 | I2C_MASTER_WRITE), true);
+  transaction
-  i2c_master_write_byte(handle, DSP_BOOT_POWER_STATE, true);
+    .start()
-  i2c_master_start(handle);
+    .write_addr(kPCM5122Address, I2C_MASTER_WRITE)
-  i2c_master_write_byte(handle, (kPCM5122Address << 1 | I2C_MASTER_READ), true);
+    .write_ack(DSP_BOOT_POWER_STATE)
-  i2c_master_read_byte(handle, &result, I2C_MASTER_NACK);
+    .start()
-  i2c_master_stop(handle);
+    .write_addr(kPCM5122Address, I2C_MASTER_READ)
-
+    .read(&result, I2C_MASTER_NACK)
-  ESP_ERROR_CHECK(i2c_master_cmd_begin(I2C_NUM_0, handle, kPCM5122Timeout));
+    .stop();
 
-  i2c_cmd_link_delete(handle);
+  ESP_ERROR_CHECK(transaction.Execute());
 
   return result;
 }
 
 void AudioDac::WriteVolume(uint8_t volume) {
-  i2c_cmd_handle_t handle = i2c_cmd_link_create();
+  I2CTransaction transaction;
-  if (handle == NULL) {
+  transaction
-    return;
+    .start()
-  }
+    .write_addr(kPCM5122Address, I2C_MASTER_WRITE)
-
+    .write_ack(Register::DIGITAL_VOLUME_L, volume)
-  i2c_master_start(handle);
+    .write_ack(Register::DIGITAL_VOLUME_R, volume)
-  i2c_master_write_byte(handle, (kPCM5122Address << 1 | I2C_MASTER_WRITE), true);
+    .stop();
-
+
-  set_register(handle, Register::DIGITAL_VOLUME_L, volume);
+  ESP_ERROR_CHECK(transaction.Execute());
-  set_register(handle, Register::DIGITAL_VOLUME_R, volume);
-
-  i2c_master_stop(handle);
-
-  i2c_master_cmd_begin(I2C_NUM_0, handle, 50);
-
-  i2c_cmd_link_delete(handle);
 }
 
 void AudioDac::WritePowerMode(PowerMode mode) {

main/gpio-expander.cpp

@@ -1,4 +1,7 @@
 #include "gpio-expander.h"
+
+#include "i2c.h"
+
 #include <cstdint>
 
 namespace gay_ipod {
@@ -27,40 +30,26 @@ esp_err_t GpioExpander::Write() {
 
   std::pair<uint8_t, uint8_t> ports_ab = unpack(ports());
 
-  // Technically enqueuing these commands could fail, but we don't worry about
+  I2CTransaction transaction;
-  // it because that would indicate some really very badly wrong more generally.
+  transaction.start()
-  i2c_master_start(handle);
+    .write_addr(kPca8575Address, I2C_MASTER_WRITE)
-  i2c_master_write_byte(handle, (kPca8575Address << 1 | I2C_MASTER_WRITE), true);
+    .write_ack(ports_ab.first, ports_ab.second)
-  i2c_master_write_byte(handle, ports_ab.first, true);
+    .stop();
-  i2c_master_write_byte(handle, ports_ab.second, true);
-  i2c_master_stop(handle);
-
-  esp_err_t ret = i2c_master_cmd_begin(I2C_NUM_0, handle, kPca8575Timeout);
 
-  i2c_cmd_link_delete(handle);
+  return transaction.Execute();
-  return ret;
 }
 
 esp_err_t GpioExpander::Read() {
-  i2c_cmd_handle_t handle = i2c_cmd_link_create();
-  if (handle == NULL) {
-    return ESP_ERR_NO_MEM;
-  }
-
   uint8_t input_a, input_b;
 
-  // Technically enqueuing these commands could fail, but we don't worry about
+  I2CTransaction transaction;
-  // it because that would indicate some really very badly wrong more generally.
+  transaction.start()
-  i2c_master_start(handle);
+    .write_addr(kPca8575Address, I2C_MASTER_READ)
-  i2c_master_write_byte(handle, (kPca8575Address << 1 | I2C_MASTER_READ), true);
+    .read(&input_a, I2C_MASTER_ACK)
-  i2c_master_read_byte(handle, &input_a, I2C_MASTER_ACK);
+    .read(&input_b, I2C_MASTER_LAST_NACK)
-  i2c_master_read_byte(handle, &input_b, I2C_MASTER_LAST_NACK);
+    .stop();
-  i2c_master_stop(handle);
-
-  esp_err_t ret = i2c_master_cmd_begin(I2C_NUM_0, handle, kPca8575Timeout);
-
-  i2c_cmd_link_delete(handle);
 
+  esp_err_t ret = transaction.Execute();
   inputs_ = pack(input_a, input_b);
   return ret;
 }

main/i2c.cpp

@@ -0,0 +1,44 @@
+#include "i2c.h"
+#include "assert.h"
+
+namespace gay_ipod {
+
+I2CTransaction::I2CTransaction() {
+  handle_ = i2c_cmd_link_create();
+  assert(handle_ != NULL && "failed to create command link");
+}
+
+I2CTransaction::~I2CTransaction() {
+  i2c_cmd_link_delete(handle_);
+}
+
+esp_err_t I2CTransaction::Execute() {
+  return i2c_master_cmd_begin(I2C_NUM_0, handle_, kI2CTimeout);
+}
+
+I2CTransaction& I2CTransaction::start() {
+  ESP_ERROR_CHECK(i2c_master_start(handle_));
+  return *this;
+}
+
+I2CTransaction& I2CTransaction::stop() {
+  ESP_ERROR_CHECK(i2c_master_stop(handle_));
+  return *this;
+}
+
+I2CTransaction& I2CTransaction::write_addr(uint8_t addr, uint8_t op) {
+  write_ack(addr << 1 | op);
+  return *this;
+}
+
+I2CTransaction& I2CTransaction::write_ack(uint8_t data) {
+  ESP_ERROR_CHECK(i2c_master_write_byte(handle_, data, true));
+  return *this;
+}
+
+I2CTransaction& I2CTransaction::read(uint8_t *dest, i2c_ack_type_t ack) {
+  ESP_ERROR_CHECK(i2c_master_read_byte(handle_, dest, ack));
+  return *this;
+}
+
+} // namespace gay_ipod

main/i2c.h

@@ -0,0 +1,80 @@
+#pragma once
+
+#include "driver/i2c.h"
+#include "hal/i2c_types.h"
+#include <cstdint>
+
+namespace gay_ipod {
+
+/*
+ * Convenience wrapper for performing an I2C transaction with a reasonable
+ * preconfigured timeout, automatic management of a heap-based command buffer,
+ * and a terser API for enqueuing bytes.
+ *
+ * Any error codes from the underlying ESP IDF are treated as fatal, since they
+ * typically represent invalid arguments or OOMs.
+ */
+class I2CTransaction {
+  public:
+    static const uint8_t kI2CTimeout = 100 / portTICK_RATE_MS;
+
+    I2CTransaction();
+    ~I2CTransaction();
+
+    /*
+     * Executes all enqueued commands, returning the result code. Possible error
+     * codes, per the ESP-IDF docs:
+     *
+     * ESP_OK Success
+     * ESP_ERR_INVALID_ARG Parameter error
+     * ESP_FAIL Sending command error, slave doesn’t ACK the transfer.
+     * ESP_ERR_INVALID_STATE I2C driver not installed or not in master mode.
+     * ESP_ERR_TIMEOUT Operation timeout because the bus is busy.
+     */
+    esp_err_t Execute();
+
+    /*
+     * Enqueues a start condition. May also be used for repeated start conditions.
+     */
+    I2CTransaction& start();
+    /* Enqueues a stop condition. */
+    I2CTransaction& stop();
+
+    /*
+     * Enqueues writing the given 7 bit address, followed by one bit indicating
+     * whether this is a read or write request.
+     *
+     * This command will expect an ACK before continuing.
+     */
+    I2CTransaction& write_addr(uint8_t addr, uint8_t op);
+
+    /*
+     * Enqueues one or more bytes to be written. The transaction will wait for
+     * an ACK to be returned before writing the next byte.
+     */
+    I2CTransaction& write_ack(uint8_t data);
+    template <typename ...More>
+    I2CTransaction& write_ack(uint8_t data, More... more) {
+      write_ack(data);
+      write_ack(more...);
+      return *this;
+    }
+
+    /*
+     * Enqueues a read of one byte into the given uint8. Responds with the given
+     * ACK/NACK type.
+     */
+    I2CTransaction& read(uint8_t *dest, i2c_ack_type_t ack);
+
+    /* Returns the underlying command buffer. */
+    i2c_cmd_handle_t handle() { return handle_; }
+
+    // Cannot be moved or copied, since doing so is probably an error. Pass a
+    // reference instead.
+    I2CTransaction(const I2CTransaction&) = delete;
+    I2CTransaction& operator=(const I2CTransaction&) = delete;
+  private:
+    i2c_cmd_handle_t handle_;
+};
+
+} // namespace gay_ipod