Neaten up the gpio api, use RAII, make it thread safe

main/gay-ipod-fw.cpp

@@ -92,14 +92,12 @@ extern "C" void app_main(void)
   init_i2c();
   init_spi();
 
-  ESP_LOGI(TAG, "Setting default GPIO state");
+  ESP_LOGI(TAG, "Init GPIOs");
   gay_ipod::GpioExpander expander;
 
   // for debugging usb ic
   //expander.set_sd_mux(gay_ipod::GpioExpander::USB);
 
-  ESP_ERROR_CHECK(expander.Write());
-
   ESP_LOGI(TAG, "Init ADC");
   ESP_ERROR_CHECK(gay_ipod::init_adc());
 
@@ -125,6 +123,8 @@ extern "C" void app_main(void)
   ESP_LOGI(TAG, "Looks okay? Let's list some files!");
   vTaskDelay(pdMS_TO_TICKS(1000));
 
+  {
+    auto lock = expander.AcquireSpiBus(gay_ipod::GpioExpander::SD_CARD);
     DIR *d;
     struct dirent *dir;
     d = opendir(gay_ipod::kStoragePath);
@@ -136,6 +136,7 @@ extern "C" void app_main(void)
     } else {
       ESP_LOGI(TAG, "nope!");
     }
+  }
 
   vTaskDelay(pdMS_TO_TICKS(1000));
   ESP_LOGI(TAG, "Time to deinit.");

main/gpio-expander.cpp

@@ -1,11 +1,22 @@
 #include "gpio-expander.h"
+#include <cstdint>
 
 namespace gay_ipod {
 
 GpioExpander::GpioExpander() {
+  ports_ = pack(kPortADefault, kPortBDefault);
+  // Read and write initial values on initialisation so that we do not have a
+  // strange partially-initialised state.
+  // TODO: log or abort if these error; it's really bad!
+  Write();
+  Read();
 }
 
-GpioExpander::~GpioExpander() {
+GpioExpander::~GpioExpander() {}
+
+void GpioExpander::with(std::function<void(GpioExpander&)> f) {
+  f(*this);
+  Write();
 }
 
 esp_err_t GpioExpander::Write() {
@@ -14,15 +25,17 @@ esp_err_t GpioExpander::Write() {
     return ESP_ERR_NO_MEM;
   }
 
+  std::pair<uint8_t, uint8_t> ports_ab = unpack(ports());
+
   // Technically enqueuing these commands could fail, but we don't worry about
   // it because that would indicate some really very badly wrong more generally.
   i2c_master_start(handle);
-  i2c_master_write_byte(handle, (PCA8575_ADDRESS << 1 | I2C_MASTER_WRITE), true);
+  i2c_master_write_byte(handle, (kPca8575Address << 1 | I2C_MASTER_WRITE), true);
-  i2c_master_write_byte(handle, port_a_, true);
+  i2c_master_write_byte(handle, ports_ab.first, true);
-  i2c_master_write_byte(handle, port_b_, true);
+  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, PCA8575_TIMEOUT);
+  esp_err_t ret = i2c_master_cmd_begin(I2C_NUM_0, handle, kPca8575Timeout);
 
   i2c_cmd_link_delete(handle);
   return ret;
@@ -34,55 +47,55 @@ esp_err_t GpioExpander::Read() {
     return ESP_ERR_NO_MEM;
   }
 
+  uint8_t input_a, input_b;
+
   // Technically enqueuing these commands could fail, but we don't worry about
   // it because that would indicate some really very badly wrong more generally.
   i2c_master_start(handle);
-  i2c_master_write_byte(handle, (PCA8575_ADDRESS << 1 | I2C_MASTER_READ), true);
+  i2c_master_write_byte(handle, (kPca8575Address << 1 | I2C_MASTER_READ), true);
-  i2c_master_read_byte(handle, &input_a_, I2C_MASTER_ACK);
+  i2c_master_read_byte(handle, &input_a, I2C_MASTER_ACK);
-  i2c_master_read_byte(handle, &input_b_, I2C_MASTER_LAST_NACK);
+  i2c_master_read_byte(handle, &input_b, I2C_MASTER_LAST_NACK);
   i2c_master_stop(handle);
 
-  esp_err_t ret = i2c_master_cmd_begin(I2C_NUM_0, handle, PCA8575_TIMEOUT);
+  esp_err_t ret = i2c_master_cmd_begin(I2C_NUM_0, handle, kPca8575Timeout);
 
   i2c_cmd_link_delete(handle);
+
+  inputs_ = pack(input_a, input_b);
   return ret;
 }
 
-bool GpioExpander::charge_power_ok(void) const {
+void GpioExpander::set_pin(ChipSelect cs, bool value) {
-  // Active-low.
+  set_pin((Pin) cs, value);
-  return (input_a_ & (1 << 4)) == 0;
 }
 
-bool GpioExpander::headphone_detect(void) const {
+void GpioExpander::set_pin(Pin pin, bool value) {
-  return (input_b_ & (1 << 0));
+  if (value) {
+    ports_ |= (1 << pin);
+  } else {
+    ports_ &= ~(1 << pin);
+  }
 }
 
-uint8_t GpioExpander::key_states(void) const {
+bool GpioExpander::get_input(Pin pin) const {
-  return input_b_ & 0b00111111;
+  return (inputs_ & (1 << pin)) > 0;
 }
 
-void GpioExpander::set_sd_mux(SdMuxController controller) {
+GpioExpander::SpiLock GpioExpander::AcquireSpiBus(ChipSelect cs) {
-  if (controller == USB) {
+  return SpiLock(*this, cs);
-    port_a_ |= (1 << 5);
-  } else {
-    port_a_ &= ~(1 << 5);
-  }
 }
 
-void GpioExpander::set_sd_cs(bool high) {
+GpioExpander::SpiLock::SpiLock(GpioExpander& gpio, ChipSelect cs)
-  if (high) {
+  : lock_(gpio.cs_mutex_), gpio_(gpio), cs_(cs) {
-    port_a_ |= (1 << 6);
+  gpio_.with([&](auto& gpio) {
-  } else {
+      gpio.set_pin(cs_, 0);
-    port_a_ &= ~(1 << 6);
+  });
-  }
 }
 
-void GpioExpander::set_display_cs(bool high) {
+GpioExpander::SpiLock::~SpiLock() { 
-  if (high) {
+  gpio_.with([&](auto& gpio) {
-    port_a_ |= (1 << 7);
+      gpio.set_pin(cs_, 1);
-  } else {
+  });
-    port_a_ &= ~(1 << 7);
-  }
 }
 
 } // namespace gay_ipod

main/gpio-expander.h

@@ -1,46 +1,38 @@
 #pragma once
 
+#include <atomic>
+#include <functional>
+#include <mutex>
 #include <stdint.h>
+#include <tuple>
+#include <utility>
 
 #include "esp_check.h"
+#include "esp_log.h"
 #include "esp_err.h"
 #include "driver/i2c.h"
-
+#include "freertos/FreeRTOS.h"
-#define PCA8575_ADDRESS (0x20)
-#define PCA8575_TIMEOUT (5)
 
 namespace gay_ipod {
 
 /**
- * PCA8575
+ * Wrapper for interfacing with the PCA8575 GPIO expander. Includes basic
+ * low-level pin setting methods, as well as higher level convenience functions
+ * for reading, writing, and atomically interacting with the SPI chip select
+ * pins.
+ *
+ * Each method of this class can be called safely from any thread, and all
+ * updates are guaranteed to be atomic. Any access to chip select related pins
+ * should be done whilst holding `cs_lock` (preferably via the helper methods).
  */
 class GpioExpander {
   public:
     GpioExpander();
     ~GpioExpander();
 
-    esp_err_t Write(void);
+    static const uint8_t kPca8575Address = 0x20;
-    esp_err_t Read(void);
+    static const uint8_t kPca8575Timeout = 100 / portTICK_RATE_MS;
-
-    bool charge_power_ok(void) const;
-    bool headphone_detect(void) const;
-    uint8_t key_states(void) const;
-
-    enum SdMuxController {
-      ESP,
-      USB
-    };
-
-    void set_sd_mux(SdMuxController controller);
-    void set_sd_cs(bool high);
-    void set_display_cs(bool high);
 
-    // Not copyable or movable.
-    // TODO: maybe this could be movable?
-    GpioExpander(const GpioExpander&) = delete;
-    GpioExpander& operator=(const GpioExpander&) = delete;
-
-  private:
     // Port A:
     // 0 - audio power enable
     // 1 - usb interface power enable
@@ -51,7 +43,7 @@ class GpioExpander {
     // 6 - sd chip select
     // 7 - display chip select
     // All power switches low, chip selects high, active-low charge power high
-    uint8_t port_a_ = 0b11010001;
+    static const uint8_t kPortADefault = 0b11010001;
 
     // Port B:
     // 0 - 3.5mm jack detect (active low)
@@ -63,10 +55,155 @@ class GpioExpander {
     // 6 - GPIO
     // 7 - GPIO
     // DAC mute output low, everything else is active-low inputs.
-    uint8_t port_b_ = 0b11111101;
+    static const uint8_t kPortBDefault = 0b11111101;
+
+    /*
+     * Convenience mehod for packing the port a and b bytes into a single 16 bit
+     * value.
+     */
+    static uint16_t pack(uint8_t a, uint8_t b) {
+      return ((uint16_t) b) << 8 | a;
+    }
+
+    /*
+     * Convenience mehod for unpacking the result of `pack` back into two single
+     * byte port datas.
+     */
+    static std::pair<uint8_t, uint8_t> unpack(uint16_t ba) {
+      return std::pair((uint8_t) ba, (uint8_t) (ba >> 8));
+    }
+
+    /*
+     * Convenience function for running some arbitrary pin writing code, then
+     * flushing a `Write()` to the expander. Example usage:
+     *
+     * ```
+     * gpio_.with([&](auto& gpio) {
+     *	gpio.set_pin(AUDIO_POWER_ENABLE, true);
+     * });
+     * ```
+     */
+    void with(std::function<void(GpioExpander&)> f);
+
+    /**
+     * Sets the ports on the GPIO expander to the values currently represented
+     * in `ports`.
+     */
+    esp_err_t Write(void);
+
+    /**
+     * Reads from the GPIO expander, populating `inputs` with the most recent
+     * values.
+     */
+    esp_err_t Read(void);
+
+    /* Maps each pin of the expander to its number in a `pack`ed uint16. */
+    enum Pin {
+      // Port A
+      AUDIO_POWER_ENABLE = 0,
+      USB_INTERFACE_POWER_ENABLE = 1,
+      DISPLAY_POWER_ENABLE = 2,
+      SD_CARD_POWER_ENABLE = 3,
+      CHARGE_POWER_OK = 4, // Active-low input
+      SD_MUX_SWITCH = 5,
+      SD_CHIP_SELECT = 6,
+      DISPLAY_CHIP_SELECT = 7,
+
+      // Port B
+      PHONE_DETECT = 8, // Active-high input
+      DAC_MUTE = 9,
+      GPIO_1 = 10,
+      GPIO_2 = 11,
+      GPIO_3 = 12,
+      GPIO_4 = 13,
+      GPIO_5 = 14,
+      GPIO_6 = 15,
+    };
 
-    uint8_t input_a_ = 0;
+    /* Pins whose access should be guarded by `cs_lock`. */
-    uint8_t input_b_ = 0;
+    enum ChipSelect {
+      SD_CARD = SD_CHIP_SELECT,
+      DISPLAY = DISPLAY_CHIP_SELECT,
+    };
+
+    /* Nicer value names for use with the SD_MUX_SWITCH pin. */
+    enum SdController {
+      SD_MUX_ESP = 0,
+      SD_MUX_USB = 1,
+    };
+
+    /**
+     * Returns the current driven status of each of the ports. The first byte is
+     * port a, and the second byte is port b.
+     */
+    std::atomic<uint16_t>& ports() { return ports_; }
+
+    /*
+     * Sets a single specific pin to the given value. `true` corresponds to
+     * HIGH, and `false` corresponds to LOW.
+     *
+     * Calls to this method will be buffered in memory until a call to `Write()`
+     * is made.
+     */
+    void set_pin(Pin pin, bool value);
+    void set_pin(ChipSelect cs, bool value);
+    
+    /**
+     * Returns the input status of each of the ports. The first byte is port a,
+     * and the second byte is port b.
+     */
+    const std::atomic<uint16_t>& inputs() const { return inputs_; }
+
+    /* Returns the most recently cached value of the given pin. Only valid for
+     * pins used as inputs; to check what value we're driving a pin, use
+     * `ports()`.
+     */
+    bool get_input(Pin pin) const;
+
+    /* Returns the mutex that must be held whilst pulling a CS pin low. */
+    std::mutex& cs_mutex() { return cs_mutex_; }
+
+    /*
+     * Helper class containing an active `cs_mutex` lock. When an instance of
+     * this class is destroyed (usually by falling out of scope), the associated
+     * CS pin will be driven high before the lock is released.
+     */
+    class SpiLock {
+      public:
+	SpiLock(GpioExpander &gpio, ChipSelect cs);
+	~SpiLock();
+
+	SpiLock(const SpiLock&) = delete;
+      private:
+	std::scoped_lock<std::mutex> lock_;
+	GpioExpander &gpio_;
+	ChipSelect cs_;
+    };
+
+    /*
+     * Pulls the given CS pin low to signal that we are about to communicate
+     * with a particular device, after acquiring a lock on `cs_mutex`. The
+     * recommended way to safely interact with devices on the SPI bus is to have
+     * a self-contained block like so:
+     *
+     * ```
+     * {
+     *	auto lock = AcquireSpiBus(WHATEVER);
+     *  // Do some cool things here.
+     * }
+     * ```
+     */
+    SpiLock AcquireSpiBus(ChipSelect cs);
+    
+    // Not copyable or movable. There should usually only ever be once instance
+    // of this class, and that instance will likely have a static lifetime.
+    GpioExpander(const GpioExpander&) = delete;
+    GpioExpander& operator=(const GpioExpander&) = delete;
+
+  private:
+    std::mutex cs_mutex_;
+    std::atomic<uint16_t> ports_;
+    std::atomic<uint16_t> inputs_;
 };
 
 } // namespace gay_ipod

main/storage.cpp

@@ -1,7 +1,10 @@
 #include "storage.h"
 
+#include <mutex>
+
 #include "diskio_impl.h"
 #include "diskio_sdmmc.h"
+#include "driver/gpio.h"
 #include "driver/sdspi_host.h"
 #include "esp_check.h"
 #include "esp_err.h"
@@ -23,10 +26,9 @@ SdStorage::SdStorage(GpioExpander *gpio) {
 SdStorage::~SdStorage() {}
 
 SdStorage::Error SdStorage::Acquire(void) {
-  // First switch to this device, and pull CS.
+  // Acquiring the bus will also flush the mux switch change.
-  gpio_->set_sd_mux(GpioExpander::ESP);
+  gpio_->set_pin(GpioExpander::SD_MUX_SWITCH, GpioExpander::SD_MUX_ESP);
-  gpio_->set_sd_cs(false);
+  auto lock = gpio_->AcquireSpiBus(GpioExpander::SD_CARD);
-  gpio_->Write();
 
   // Now we can init the driver and set up the SD card into SPI mode.
   sdspi_host_init();
@@ -47,8 +49,6 @@ SdStorage::Error SdStorage::Acquire(void) {
   esp_err_t err = sdmmc_card_init(&host_, &card_);
   if (err != ESP_OK) {
     ESP_LOGW(TAG, "Failed to read, err: %d", err);
-    gpio_->set_sd_cs(true);
-    gpio_->Write();
     return Error::FAILED_TO_READ;
   }
 
@@ -62,16 +62,11 @@ SdStorage::Error SdStorage::Acquire(void) {
     return Error::FAILED_TO_MOUNT;
   }
 
-  // We're done chatting for now.
-  //gpio_->set_sd_cs(true);
-  //gpio_->Write();
-
   return Error::OK;
 }
 
 void SdStorage::Release(void) {
-  gpio_->set_sd_cs(false);
+  auto lock = gpio_->AcquireSpiBus(GpioExpander::SD_CARD);
-  gpio_->Write();
 
   // Unmount and unregister the filesystem
   f_unmount("");
@@ -82,10 +77,6 @@ void SdStorage::Release(void) {
   // Uninstall the SPI driver
   sdspi_host_remove_device(this->handle_);
   sdspi_host_deinit();
-
-  gpio_->set_sd_mux(GpioExpander::USB);
-  gpio_->set_sd_cs(true);
-  gpio_->Write();
 }
 
 } // namespace gay_ipod