Isolate the SD card from the SPI bus when talking to the display (#176)

This should help significantly with https://codeberg.org/cool-tech-zone/tangara-fw/issues/121, which seems to be caused by some SD cards being very picky about being the only SPI device on their bus.

Co-authored-by: ailurux <ailuruxx@gmail.com>
Reviewed-on: https://codeberg.org/cool-tech-zone/tangara-fw/pulls/176
Co-authored-by: jacqueline <me@jacqueline.id.au>
Co-committed-by: jacqueline <me@jacqueline.id.au>

8 files changed, 79 insertions, 25 deletions

diff --git a/src/drivers/display.cpp b/src/drivers/display.cpp
index 15cd6c62..2fcb9dc0 100644
--- a/src/drivers/display.cpp
+++ b/src/drivers/display.cpp
@@ -179,6 +179,9 @@ auto Display::SetDisplayOn(bool enabled) -> void {
     return;
   }
 
+  spi_device_acquire_bus(handle_, portMAX_DELAY);
+  gpio_.SdMuxEnable(false);
+
   if (display_on_) {
     SendCommandWithData(displays::ST77XX_DISPON, nullptr, 0);
     vTaskDelay(pdMS_TO_TICKS(100));
@@ -191,6 +194,9 @@ auto Display::SetDisplayOn(bool enabled) -> void {
     vTaskDelay(pdMS_TO_TICKS(100));
     SendCommandWithData(displays::ST77XX_DISPOFF, nullptr, 0);
   }
+
+  gpio_.SdMuxEnable(true);
+  spi_device_release_bus(handle_);
 }
 
 auto Display::SetBrightness(uint_fast8_t percent) -> void {
@@ -222,6 +228,7 @@ void Display::SendInitialisationSequence(const uint8_t* data) {
   // finished may be increased by doing this, but a short boot with no feedback
   // feels worse than a longer boot that doesn't tell you anything.
   spi_device_acquire_bus(handle_, portMAX_DELAY);
+  gpio_.SdMuxEnable(false);
 
   // First byte of the data is the number of commands.
   for (int i = *(data++); i > 0; i--) {
@@ -243,6 +250,7 @@ void Display::SendInitialisationSequence(const uint8_t* data) {
     }
   }
 
+  gpio_.SdMuxEnable(true);
   spi_device_release_bus(handle_);
 }
 
@@ -250,24 +258,14 @@ IRAM_ATTR
 void Display::SendCommandWithData(uint8_t command,
                                   const uint8_t* data,
                                   size_t length) {
-  SendCmd(&command, 1);
-  SendData(data, length);
-}
-
-IRAM_ATTR
-void Display::SendCmd(const uint8_t* data, size_t length) {
-  SendTransaction(COMMAND, data, length);
-}
-
-IRAM_ATTR
-void Display::SendData(const uint8_t* data, size_t length) {
+  SendTransaction(COMMAND, &command, 1);
   SendTransaction(DATA, data, length);
 }
 
 IRAM_ATTR
 void Display::SendTransaction(TransactionType type,
-                                        const uint8_t* data,
-                                        size_t length) {
+                              const uint8_t* data,
+                              size_t length) {
   // TODO(jacqueline): What's sending this?
   if (length == 0) {
     return;
@@ -306,6 +304,7 @@ void Display::OnLvglFlush(const lv_area_t* area, uint8_t* color_map) {
   lv_draw_sw_rgb565_swap(color_map, size);
 
   spi_device_acquire_bus(handle_, portMAX_DELAY);
+  gpio_.SdMuxEnable(false);
 
   // First we need to specify the rectangle of the display we're writing into.
   uint16_t data[2] = {0, 0};
@@ -324,6 +323,7 @@ void Display::OnLvglFlush(const lv_area_t* area, uint8_t* color_map) {
   SendCommandWithData(displays::ST77XX_RAMWR,
                       reinterpret_cast<uint8_t*>(color_map), size * 2);
 
+  gpio_.SdMuxEnable(true);
   spi_device_release_bus(handle_);
 
   if (!first_flush_finished_ && lv_disp_flush_is_last(display_)) {
diff --git a/src/drivers/gpios.cpp b/src/drivers/gpios.cpp
index 52b2b874..cf7cf14e 100644
--- a/src/drivers/gpios.cpp
+++ b/src/drivers/gpios.cpp
@@ -95,6 +95,14 @@ auto Gpios::WriteSync(Pin pin, bool value) -> bool {
   return Flush();
 }
 
+auto Gpios::ShouldRead() -> bool {
+  if (!gpio_get_level(GPIO_NUM_34) || has_written_) {
+    has_written_ = false;
+    return true;
+  }
+  return false;
+}
+
 auto Gpios::Flush() -> bool {
   std::pair<uint8_t, uint8_t> ports_ab = unpack(ports_);
 
@@ -104,6 +112,7 @@ auto Gpios::Flush() -> bool {
       .write_ack(ports_ab.first, ports_ab.second)
       .stop();
 
+  has_written_ = true;
   return transaction.Execute() == ESP_OK;
 }
 
@@ -138,4 +147,32 @@ auto Gpios::Read() -> bool {
   return true;
 }
 
+auto Gpios::SdMuxEnable(bool en) -> void {
+  std::unique_lock<std::mutex> lock(mux_mutex_);
+  mux_en_ = en;
+  if (SdMuxTarget() == SD_MUX_ESP) {
+    WriteSync(Pin::kSdMuxDisable, !en);
+  }
+  // Don't touch the mux if it's pointed at the SAMD21. We'll write the new
+  // value when the mux points back at us again.
+}
+
+auto Gpios::SdMuxTarget(SdTarget target) -> void {
+  std::unique_lock<std::mutex> lock(mux_mutex_);
+  WriteBuffered(Pin::kSdMuxSwitch, target);
+  if (target == SD_MUX_ESP) {
+    WriteBuffered(Pin::kSdMuxDisable, !mux_en_);
+  } else {
+    // Mux is always enabled when it's pointing at the SAMD21, since it's the
+    // only thing on that SPI bus.
+    WriteBuffered(Pin::kSdMuxDisable, 0);
+  }
+  Flush();
+}
+
+auto Gpios::SdMuxTarget() -> SdTarget {
+  return static_cast<SdTarget>(
+      (ports_ & (1 << static_cast<int>(Pin::kSdMuxSwitch))) > 0);
+}
+
 }  // namespace drivers
diff --git a/src/drivers/include/drivers/display.hpp b/src/drivers/include/drivers/display.hpp
index 21ca3e2d..e5001c48 100644
--- a/src/drivers/include/drivers/display.hpp
+++ b/src/drivers/include/drivers/display.hpp
@@ -65,8 +65,6 @@ class Display {
   void SendInitialisationSequence(const uint8_t* data);
 
   void SendCommandWithData(uint8_t command, const uint8_t* data, size_t length);
-  void SendCmd(const uint8_t* data, size_t length);
-  void SendData(const uint8_t* data, size_t length);
 
   void SendTransaction(TransactionType type,
                        const uint8_t* data,
diff --git a/src/drivers/include/drivers/gpios.hpp b/src/drivers/include/drivers/gpios.hpp
index e27a3ade..5010e45f 100644
--- a/src/drivers/include/drivers/gpios.hpp
+++ b/src/drivers/include/drivers/gpios.hpp
@@ -61,7 +61,7 @@ class IGpios {
   };
 
   /* Nicer value names for use with kSdMuxSwitch. */
-  enum SdController {
+  enum SdTarget {
     SD_MUX_ESP = 0,
     SD_MUX_SAMD = 1,
   };
@@ -80,6 +80,15 @@ class IGpios {
   virtual auto Get(Pin) const -> bool = 0;
 
   virtual auto IsLocked() const -> bool = 0;
+
+  /*
+   * Enables or disable the SD mux. When the mux is disabled, the SD card is
+   * isolated from the rest of the SPI bus.
+   */
+  virtual auto SdMuxEnable(bool en) -> void = 0;
+
+  /* Switches whether the SD card is connected to the ESP32 or SAMD21. */
+  virtual auto SdMuxTarget(SdTarget target) -> void = 0;
 };
 
 class Gpios : public IGpios {
@@ -96,6 +105,8 @@ class Gpios : public IGpios {
    */
   auto WriteSync(Pin, bool) -> bool override;
 
+  auto ShouldRead() -> bool;
+
   virtual auto WriteBuffered(Pin, bool) -> void;
 
   /**
@@ -114,6 +125,11 @@ class Gpios : public IGpios {
    */
   auto Read(void) -> bool;
 
+  auto SdMuxEnable(bool en) -> void override;
+  auto SdMuxTarget(SdTarget target) -> void override;
+
+  auto SdMuxTarget() -> SdTarget;
+
   // Not copyable or movable. There should usually only ever be once instance
   // of this class, and that instance will likely have a static lifetime.
   Gpios(const Gpios&) = delete;
@@ -125,6 +141,10 @@ class Gpios : public IGpios {
   std::atomic<uint16_t> ports_;
   std::atomic<uint16_t> inputs_;
   const bool invert_lock_switch_;
+  bool has_written_;
+
+  std::mutex mux_mutex_;
+  bool mux_en_;
 };
 
 }  // namespace drivers
diff --git a/src/drivers/storage.cpp b/src/drivers/storage.cpp
index 09702a8c..ed6af1dd 100644
--- a/src/drivers/storage.cpp
+++ b/src/drivers/storage.cpp
@@ -35,8 +35,8 @@ const char* kStoragePath = "/sd";
 
 auto SdStorage::Create(IGpios& gpio) -> cpp::result<SdStorage*, Error> {
   gpio.WriteSync(IGpios::Pin::kSdPowerEnable, 1);
-  gpio.WriteSync(IGpios::Pin::kSdMuxSwitch, IGpios::SD_MUX_ESP);
-  gpio.WriteSync(IGpios::Pin::kSdMuxDisable, 0);
+  gpio.SdMuxTarget(IGpios::SD_MUX_ESP);
+  gpio.SdMuxEnable(true);
 
   sdspi_dev_handle_t handle;
   FATFS* fs = nullptr;
@@ -119,7 +119,7 @@ SdStorage::~SdStorage() {
   sdspi_host_remove_device(this->handle_);
   sdspi_host_deinit();
 
-  gpio_.WriteSync(IGpios::Pin::kSdMuxDisable, 1);
+  gpio_.SdMuxEnable(false);
   gpio_.WriteSync(IGpios::Pin::kSdPowerEnable, 0);
 }
 
diff --git a/src/tangara/system_fsm/booting.cpp b/src/tangara/system_fsm/booting.cpp
index 5253e4dd..b909e1ba 100644
--- a/src/tangara/system_fsm/booting.cpp
+++ b/src/tangara/system_fsm/booting.cpp
@@ -121,7 +121,7 @@ auto Booting::exit() -> void {
   });
 
   TimerHandle_t timer = xTimerCreate("INTERRUPTS", kInterruptCheckPeriod, true,
-                                     NULL, check_interrupts_cb);
+                                     sServices.get(), check_interrupts_cb);
   xTimerStart(timer, portMAX_DELAY);
 }
 
diff --git a/src/tangara/system_fsm/running.cpp b/src/tangara/system_fsm/running.cpp
index 227eac2c..cf820659 100644
--- a/src/tangara/system_fsm/running.cpp
+++ b/src/tangara/system_fsm/running.cpp
@@ -99,9 +99,7 @@ void Running::react(const SamdUsbMscChanged& ev) {
     // Set up the SD card for usage by the samd21.
     auto& gpios = sServices->gpios();
     gpios.WriteSync(drivers::IGpios::Pin::kSdPowerEnable, 1);
-    gpios.WriteSync(drivers::IGpios::Pin::kSdMuxSwitch,
-                    drivers::IGpios::SD_MUX_SAMD);
-    gpios.WriteSync(drivers::IGpios::Pin::kSdMuxDisable, 0);
+    gpios.SdMuxTarget(drivers::IGpios::SD_MUX_SAMD);
 
     // Off you go!
     sServices->samd().UsbMassStorage(true);
@@ -113,7 +111,7 @@ void Running::react(const SamdUsbMscChanged& ev) {
 
     auto& gpios = sServices->gpios();
     // No more writing, please!
-    gpios.WriteSync(drivers::IGpios::Pin::kSdMuxDisable, 1);
+    gpios.SdMuxTarget(drivers::IGpios::SD_MUX_ESP);
     vTaskDelay(pdMS_TO_TICKS(100));
 
     // Reboot the SD card so that it comes up in a consistent state.
diff --git a/src/tangara/system_fsm/system_fsm.cpp b/src/tangara/system_fsm/system_fsm.cpp
index 2e819569..d584d3f0 100644
--- a/src/tangara/system_fsm/system_fsm.cpp
+++ b/src/tangara/system_fsm/system_fsm.cpp
@@ -24,7 +24,8 @@ std::unique_ptr<drivers::SdStorage> SystemState::sStorage;
 console::AppConsole* SystemState::sAppConsole;
 
 void check_interrupts_cb(TimerHandle_t timer) {
-  if (!gpio_get_level(GPIO_NUM_34)) {
+  ServiceLocator* services = reinterpret_cast<ServiceLocator*>(pvTimerGetTimerID(timer));
+  if (services->gpios().ShouldRead()) {
     events::System().Dispatch(internal::GpioInterrupt{});
   }
   if (!gpio_get_level(GPIO_NUM_35)) {