Fix up display artficacts and clean up unused features

2 files changed, 94 insertions, 144 deletions

diff --git a/src/drivers/display.cpp b/src/drivers/display.cpp
index 951a45eb..6ec82787 100644
--- a/src/drivers/display.cpp
+++ b/src/drivers/display.cpp
@@ -1,29 +1,31 @@
 #include "display.hpp"
 
-#include <atomic>
 #include <cstdint>
 #include <cstring>
 #include <memory>
-#include <mutex>
 
 #include "assert.h"
 #include "driver/gpio.h"
 #include "driver/spi_master.h"
 #include "esp_attr.h"
+#include "esp_err.h"
 #include "esp_heap_caps.h"
 #include "freertos/portable.h"
 #include "freertos/portmacro.h"
 #include "freertos/projdefs.h"
 #include "hal/gpio_types.h"
+#include "hal/lv_hal_disp.h"
 #include "hal/spi_types.h"
 #include "lvgl/lvgl.h"
 
 #include "display_init.hpp"
+#include "gpio_expander.hpp"
 
 static const char* kTag = "DISPLAY";
 
-static const uint8_t kDisplayWidth = 128;
-static const uint8_t kDisplayHeight = 160;
+// TODO(jacqueline): Encode width and height variations in the init data.
+static const uint8_t kDisplayWidth = 128 + 2;
+static const uint8_t kDisplayHeight = 160 + 1;
 static const uint8_t kTransactionQueueSize = 10;
 
 /*
@@ -37,43 +39,27 @@ static const int kDisplayBufferSize = (kDisplayWidth * kDisplayHeight) / 10;
 
 // Allocate both buffers in static memory to ensure that they're in DRAM, with
 // minimal fragmentation. We most cases we always need these buffers anyway, so
-// it's not a memory hit we can avoid anyway.
+// it's not a memory hit we can avoid.
 // Note: 128 * 160 / 10 * 2 bpp * 2 buffers = 8 KiB
 DMA_ATTR static lv_color_t sBuffer1[kDisplayBufferSize];
 DMA_ATTR static lv_color_t sBuffer2[kDisplayBufferSize];
 
 namespace drivers {
 
-// Static functions for interrop with the LVGL display driver API, which
-// requires a function pointer.
-namespace callback {
-static std::atomic<Display*> instance = nullptr;
-
-extern "C" void flush_cb(lv_disp_drv_t* disp_drv,
-                         const lv_area_t* area,
-                         lv_color_t* color_map) {
-  auto instance_unwrapped = instance.load();
-  if (instance_unwrapped == nullptr) {
-    ESP_LOGW(kTag, "uncaught flush callback");
-    return;
-  }
-  // TODO: what if a transaction comes in right now?
-  instance_unwrapped->Flush(disp_drv, area, color_map);
-}
-
-static void IRAM_ATTR post_cb(spi_transaction_t* transaction) {
-  auto instance_unwrapped = instance.load();
-  if (instance_unwrapped == nullptr) {
-    // Can't log in ISR.
-    return;
-  }
-  instance_unwrapped->PostTransaction(*transaction);
+/*
+ * Callback invoked by LVGL when there is new data to be written to the display.
+ */
+extern "C" void FlushDataCallback(lv_disp_drv_t* disp_drv,
+                                  const lv_area_t* area,
+                                  lv_color_t* color_map) {
+  Display* instance = static_cast<Display*>(disp_drv->user_data);
+  instance->OnLvglFlush(disp_drv, area, color_map);
 }
-}  // namespace callback
 
 auto Display::create(GpioExpander* expander,
                      const displays::InitialisationData& init_data)
-    -> cpp::result<std::unique_ptr<Display>, Error> {
+    -> std::unique_ptr<Display> {
+  // First, turn on the LED backlight.
   expander->set_pin(GpioExpander::DISPLAY_LED, 0);
   expander->set_pin(GpioExpander::DISPLAY_POWER_ENABLE, 1);
   expander->Write();
@@ -94,16 +80,15 @@ auto Display::create(GpioExpander* expander,
       .flags = 0,
       .queue_size = kTransactionQueueSize,
       .pre_cb = NULL,
-      .post_cb = &callback::post_cb,
+      .post_cb = NULL,
   };
   spi_device_handle_t handle;
   spi_bus_add_device(VSPI_HOST, &spi_cfg, &handle);
 
-  // TODO: ideally create this later? a bit awkward rn.
   auto display = std::make_unique<Display>(expander, handle);
 
   // Now we reset the display into a known state, then configure it
-  // TODO: set rotatoin
+  // TODO(jacqueline): set rotation
   ESP_LOGI(kTag, "Sending init sequences");
   for (int i = 0; i < init_data.num_sequences; i++) {
     display->SendInitialisationSequence(init_data.sequences[i]);
@@ -118,7 +103,8 @@ auto Display::create(GpioExpander* expander,
   display->driver_.draw_buf = &display->buffers_;
   display->driver_.hor_res = kDisplayWidth;
   display->driver_.ver_res = kDisplayHeight;
-  display->driver_.flush_cb = &callback::flush_cb;
+  display->driver_.flush_cb = &FlushDataCallback;
+  display->driver_.user_data = display.get();
 
   ESP_LOGI(kTag, "Registering driver");
   display->display_ = lv_disp_drv_register(&display->driver_);
@@ -127,23 +113,19 @@ auto Display::create(GpioExpander* expander,
 }
 
 Display::Display(GpioExpander* gpio, spi_device_handle_t handle)
-    : gpio_(gpio), handle_(handle) {
-  callback::instance = this;
-}
+    : gpio_(gpio), handle_(handle) {}
 
-Display::~Display() {
-  callback::instance = nullptr;
-  // TODO.
-}
+Display::~Display() {}
 
 void Display::SendInitialisationSequence(const uint8_t* data) {
-  uint8_t command, num_args;
-  uint16_t sleep_duration_ms;
+  // Hold onto the bus for the entire sequence so that we're not interrupted
+  // part way through.
+  spi_device_acquire_bus(handle_, portMAX_DELAY);
 
   // First byte of the data is the number of commands.
   for (int i = *(data++); i > 0; i--) {
-    command = *(data++);
-    num_args = *(data++);
+    uint8_t command = *(data++);
+    uint8_t num_args = *(data++);
     bool has_delay = (num_args & displays::kDelayBit) > 0;
     num_args &= ~displays::kDelayBit;
 
@@ -151,123 +133,100 @@ void Display::SendInitialisationSequence(const uint8_t* data) {
 
     data += num_args;
     if (has_delay) {
-      sleep_duration_ms = *(data++);
+      uint16_t sleep_duration_ms = *(data++);
       if (sleep_duration_ms == 0xFF) {
         sleep_duration_ms = 500;
       }
+
+      // Avoid hanging on to the bus whilst delaying.
+      spi_device_release_bus(handle_);
       vTaskDelay(pdMS_TO_TICKS(sleep_duration_ms));
+      spi_device_acquire_bus(handle_, portMAX_DELAY);
     }
   }
+
+  spi_device_release_bus(handle_);
 }
 
 void Display::SendCommandWithData(uint8_t command,
                                   const uint8_t* data,
-                                  size_t length,
-                                  uintptr_t flags) {
-  SendCmd(&command, 1, flags);
-  SendData(data, length, flags);
+                                  size_t length) {
+  SendCmd(&command, 1);
+  SendData(data, length);
 }
 
-void Display::SendCmd(const uint8_t* data, size_t length, uintptr_t flags) {
-  SendTransaction(COMMAND, data, length, flags);
+void Display::SendCmd(const uint8_t* data, size_t length) {
+  SendTransaction(COMMAND, data, length);
 }
 
-void Display::SendData(const uint8_t* data, size_t length, uintptr_t flags) {
-  SendTransaction(DATA, data, length, flags);
+void Display::SendData(const uint8_t* data, size_t length) {
+  SendTransaction(DATA, data, length);
 }
 
 void Display::SendTransaction(TransactionType type,
                               const uint8_t* data,
-                              size_t length,
-                              uint32_t flags) {
+                              size_t length) {
+  // TODO(jacqueline): What's sending this?
   if (length == 0) {
     return;
   }
 
-  // TODO: Use a memory pool for these.
-  spi_transaction_t* transaction = (spi_transaction_t*)heap_caps_calloc(
-      1, sizeof(spi_transaction_t), MALLOC_CAP_DMA);
+  spi_transaction_t transaction;
+  memset(&transaction, 0, sizeof(transaction));
 
-  transaction->rx_buffer = NULL;
+  transaction.rx_buffer = NULL;
   // Length is in bits, so multiply by 8.
-  transaction->length = length * 8;
-  transaction->rxlength = 0;  // Match `length` value.
+  transaction.length = length * 8;
+  transaction.rxlength = 0;  // Match `length` value.
 
   // If the data to transmit is very short, then we can fit it directly
   // inside the transaction struct.
-  if (length * 8 <= 32) {
-    transaction->flags = SPI_TRANS_USE_TXDATA;
-    std::memcpy(&transaction->tx_data, data, length);
+  if (transaction.length <= 32) {
+    transaction.flags = SPI_TRANS_USE_TXDATA;
+    std::memcpy(&transaction.tx_data, data, length);
   } else {
-    // TODO: copy data to a DMA-capable transaction buffer
-    transaction->tx_buffer = const_cast<uint8_t*>(data);
+    // Note: LVGL's buffers are in DMA-accessible memory, so whatever pointer
+    // it handed us should be DMA-accessible already. No need to copy.
+    transaction.tx_buffer = data;
   }
 
-  transaction->user = reinterpret_cast<void*>(flags);
-
-  // TODO: acquire the bus first? Or in an outer scope?
-  // TODO: fail gracefully
-  // ESP_ERROR_CHECK(spi_device_queue_trans(handle_, transaction,
-  // portMAX_DELAY));
-  //
+  // TODO(jacqueline): Move this to an on-board GPIO for speed.
+  gpio_->set_pin(GpioExpander::DISPLAY_DR, type);
+  gpio_->Write();
 
-  ServiceTransactions();
-
-  gpio_->with(
-      [&](auto& gpio) { gpio.set_pin(GpioExpander::DISPLAY_DR, type); });
-
-  ESP_ERROR_CHECK(spi_device_polling_transmit(handle_, transaction));
-
-  free(transaction);
+  // TODO(jacqueline): Handle these errors.
+  esp_err_t ret = spi_device_polling_transmit(handle_, &transaction);
+  ESP_ERROR_CHECK(ret);
 }
 
-void Display::Flush(lv_disp_drv_t* disp_drv,
-                    const lv_area_t* area,
-                    lv_color_t* color_map) {
+void Display::OnLvglFlush(lv_disp_drv_t* disp_drv,
+                          const lv_area_t* area,
+                          lv_color_t* color_map) {
+  // Ideally we want to complete a single flush as quickly as possible, so grab
+  // the bus for this entire transaction sequence.
+  spi_device_acquire_bus(handle_, portMAX_DELAY);
+
+  // First we need to specify the rectangle of the display we're writing into.
   uint16_t data[2] = {0, 0};
 
   data[0] = SPI_SWAP_DATA_TX(area->x1, 16);
   data[1] = SPI_SWAP_DATA_TX(area->x2, 16);
-  SendCommandWithData(displays::ST77XX_CASET, (uint8_t*)data, 4);
+  SendCommandWithData(displays::ST77XX_CASET, reinterpret_cast<uint8_t*>(data),
+                      4);
 
   data[0] = SPI_SWAP_DATA_TX(area->y1, 16);
   data[1] = SPI_SWAP_DATA_TX(area->y2, 16);
-  SendCommandWithData(displays::ST77XX_RASET, (uint8_t*)data, 4);
+  SendCommandWithData(displays::ST77XX_RASET, reinterpret_cast<uint8_t*>(data),
+                      4);
 
+  // Now send the pixels for this region.
   uint32_t size = lv_area_get_width(area) * lv_area_get_height(area);
-  SendCommandWithData(displays::ST77XX_RAMWR, (uint8_t*)color_map, size * 2,
-                      LVGL_FLUSH);
-
-  // ESP_LOGI(kTag, "finished flush.");
-  // lv_disp_flush_ready(&driver_);
-}
+  SendCommandWithData(displays::ST77XX_RAMWR,
+                      reinterpret_cast<uint8_t*>(color_map), size * 2);
 
-void Display::PostTransaction(const spi_transaction_t& transaction) {
-  if (reinterpret_cast<uintptr_t>(transaction.user) & LVGL_FLUSH) {
-    lv_disp_flush_ready(&driver_);
-  }
-}
-
-void Display::ServiceTransactions() {
-  // todo
-  if (1)
-    return;
-  spi_transaction_t* transaction = nullptr;
-  // TODO: just wait '1' here, provide mechanism to wait for sure (poll?)
-  while (spi_device_get_trans_result(handle_, &transaction, pdMS_TO_TICKS(1)) !=
-         ESP_ERR_TIMEOUT) {
-    ESP_LOGI(kTag, "cleaning up finished transaction");
-
-    // TODO: a bit dodge lmao
-    // TODO: also this should happen in the post callback instead i guess?
-    if (transaction->length > 1000) {
-      ESP_LOGI(kTag, "finished flush.");
-      lv_disp_flush_ready(&driver_);
-    }
+  spi_device_release_bus(handle_);
 
-    // TODO: place back into pool.
-    free(transaction);
-  }
+  lv_disp_flush_ready(&driver_);
 }
 
 }  // namespace drivers
diff --git a/src/drivers/include/display.hpp b/src/drivers/include/display.hpp
index 5f6d6f58..8157c3a5 100644
--- a/src/drivers/include/display.hpp
+++ b/src/drivers/include/display.hpp
@@ -1,6 +1,7 @@
 #pragma once
 
 #include <cstdint>
+#include <memory>
 
 #include "driver/spi_master.h"
 #include "lvgl/lvgl.h"
@@ -8,32 +9,30 @@
 
 #include "display_init.hpp"
 #include "gpio_expander.hpp"
-#include "sys/_stdint.h"
 
 namespace drivers {
 
 /*
- * Display driver for LVGL.
+ * LVGL display driver for ST77XX family displays.
  */
 class Display {
  public:
-  enum Error {};
+  /*
+   * Creates the display driver, and resets and reinitialises the display
+   * over SPI. This never fails, since unfortunately these display don't give
+   * us back any kind of signal to tell us we're actually using them correctly.
+   */
   static auto create(GpioExpander* expander,
                      const displays::InitialisationData& init_data)
-      -> cpp::result<std::unique_ptr<Display>, Error>;
+      -> std::unique_ptr<Display>;
 
   Display(GpioExpander* gpio, spi_device_handle_t handle);
   ~Display();
 
-  void WriteData();
-
-  void Flush(lv_disp_drv_t* disp_drv,
-             const lv_area_t* area,
-             lv_color_t* color_map);
-
-  void IRAM_ATTR PostTransaction(const spi_transaction_t& transaction);
-
-  void ServiceTransactions();
+  /* Driver callback invoked by LVGL when there is new data to display. */
+  void OnLvglFlush(lv_disp_drv_t* disp_drv,
+                   const lv_area_t* area,
+                   lv_color_t* color_map);
 
  private:
   GpioExpander* gpio_;
@@ -48,23 +47,15 @@ class Display {
     DATA = 1,
   };
 
-  enum TransactionFlags {
-    LVGL_FLUSH = 1,
-  };
-
   void SendInitialisationSequence(const uint8_t* data);
 
-  void SendCommandWithData(uint8_t command,
-                           const uint8_t* data,
-                           size_t length,
-                           uintptr_t flags = 0);
+  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 SendCmd(const uint8_t* data, size_t length, uintptr_t flags = 0);
-  void SendData(const uint8_t* data, size_t length, uintptr_t flags = 0);
   void SendTransaction(TransactionType type,
                        const uint8_t* data,
-                       size_t length,
-                       uint32_t flags = 0);
+                       size_t length);
 };
 
 }  // namespace drivers