From 4e643baf5f6af1c65e08295bab4ab4f55c3feaf4 Mon Sep 17 00:00:00 2001 From: jacqueline Date: Thu, 6 Oct 2022 21:20:04 +1100 Subject: [PATCH] Neaten up the gpio api, use RAII, make it thread safe --- main/gay-ipod-fw.cpp | 25 +++--- main/gpio-expander.cpp | 81 ++++++++++------- main/gpio-expander.h | 195 +++++++++++++++++++++++++++++++++++------ main/storage.cpp | 23 ++--- 4 files changed, 233 insertions(+), 91 deletions(-) diff --git a/main/gay-ipod-fw.cpp b/main/gay-ipod-fw.cpp index bc03a32b..f9ae00b0 100644 --- a/main/gay-ipod-fw.cpp +++ b/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,16 +123,19 @@ extern "C" void app_main(void) ESP_LOGI(TAG, "Looks okay? Let's list some files!"); vTaskDelay(pdMS_TO_TICKS(1000)); - DIR *d; - struct dirent *dir; - d = opendir(gay_ipod::kStoragePath); - if (d) { - while ((dir = readdir(d)) != NULL) { - ESP_LOGI(TAG, "file! %s", dir->d_name); + { + auto lock = expander.AcquireSpiBus(gay_ipod::GpioExpander::SD_CARD); + DIR *d; + struct dirent *dir; + d = opendir(gay_ipod::kStoragePath); + if (d) { + while ((dir = readdir(d)) != NULL) { + ESP_LOGI(TAG, "file! %s", dir->d_name); + } + closedir(d); + } else { + ESP_LOGI(TAG, "nope!"); } - closedir(d); - } else { - ESP_LOGI(TAG, "nope!"); } vTaskDelay(pdMS_TO_TICKS(1000)); diff --git a/main/gpio-expander.cpp b/main/gpio-expander.cpp index aedfbf2b..1320057e 100644 --- a/main/gpio-expander.cpp +++ b/main/gpio-expander.cpp @@ -1,11 +1,22 @@ #include "gpio-expander.h" +#include 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 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 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, port_a_, true); - i2c_master_write_byte(handle, port_b_, true); + i2c_master_write_byte(handle, (kPca8575Address << 1 | I2C_MASTER_WRITE), true); + i2c_master_write_byte(handle, ports_ab.first, 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_read_byte(handle, &input_a_, I2C_MASTER_ACK); - i2c_master_read_byte(handle, &input_b_, I2C_MASTER_LAST_NACK); + 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_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 { - // Active-low. - return (input_a_ & (1 << 4)) == 0; +void GpioExpander::set_pin(ChipSelect cs, bool value) { + set_pin((Pin) cs, value); } -bool GpioExpander::headphone_detect(void) const { - return (input_b_ & (1 << 0)); +void GpioExpander::set_pin(Pin pin, bool value) { + if (value) { + ports_ |= (1 << pin); + } else { + ports_ &= ~(1 << pin); + } } -uint8_t GpioExpander::key_states(void) const { - return input_b_ & 0b00111111; +bool GpioExpander::get_input(Pin pin) const { + return (inputs_ & (1 << pin)) > 0; } -void GpioExpander::set_sd_mux(SdMuxController controller) { - if (controller == USB) { - port_a_ |= (1 << 5); - } else { - port_a_ &= ~(1 << 5); - } +GpioExpander::SpiLock GpioExpander::AcquireSpiBus(ChipSelect cs) { + return SpiLock(*this, cs); } -void GpioExpander::set_sd_cs(bool high) { - if (high) { - port_a_ |= (1 << 6); - } else { - port_a_ &= ~(1 << 6); - } +GpioExpander::SpiLock::SpiLock(GpioExpander& gpio, ChipSelect cs) + : lock_(gpio.cs_mutex_), gpio_(gpio), cs_(cs) { + gpio_.with([&](auto& gpio) { + gpio.set_pin(cs_, 0); + }); } -void GpioExpander::set_display_cs(bool high) { - if (high) { - port_a_ |= (1 << 7); - } else { - port_a_ &= ~(1 << 7); - } +GpioExpander::SpiLock::~SpiLock() { + gpio_.with([&](auto& gpio) { + gpio.set_pin(cs_, 1); + }); } } // namespace gay_ipod diff --git a/main/gpio-expander.h b/main/gpio-expander.h index c1003f9d..93e2d3e0 100644 --- a/main/gpio-expander.h +++ b/main/gpio-expander.h @@ -1,46 +1,38 @@ #pragma once +#include +#include +#include #include +#include +#include #include "esp_check.h" +#include "esp_log.h" #include "esp_err.h" #include "driver/i2c.h" - -#define PCA8575_ADDRESS (0x20) -#define PCA8575_TIMEOUT (5) +#include "freertos/FreeRTOS.h" 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); - esp_err_t Read(void); - - 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; + static const uint8_t kPca8575Address = 0x20; + static const uint8_t kPca8575Timeout = 100 / portTICK_RATE_MS; - 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 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 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, - uint8_t input_a_ = 0; - uint8_t input_b_ = 0; + // 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, + }; + + /* Pins whose access should be guarded by `cs_lock`. */ + 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& 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& 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 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 ports_; + std::atomic inputs_; }; } // namespace gay_ipod diff --git a/main/storage.cpp b/main/storage.cpp index 54829302..e1f6f386 100644 --- a/main/storage.cpp +++ b/main/storage.cpp @@ -1,7 +1,10 @@ #include "storage.h" +#include + #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. - gpio_->set_sd_mux(GpioExpander::ESP); - gpio_->set_sd_cs(false); - gpio_->Write(); + // Acquiring the bus will also flush the mux switch change. + gpio_->set_pin(GpioExpander::SD_MUX_SWITCH, GpioExpander::SD_MUX_ESP); + auto lock = gpio_->AcquireSpiBus(GpioExpander::SD_CARD); // 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); - gpio_->Write(); + auto lock = gpio_->AcquireSpiBus(GpioExpander::SD_CARD); // 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