From 122306d619ae583bbc2d30c39cf98479f83530f5 Mon Sep 17 00:00:00 2001
From: jacqueline <me@jacqueline.id.au>
Date: Wed, 5 Oct 2022 11:01:02 +1100
Subject: [PATCH] Clean up new storage bits

---
 main/gay-ipod-fw.cpp | 10 +++++++---
 main/storage.cpp     | 42 +++++++++++++++++-------------------------
 main/storage.h       | 29 ++++++++++++++++++++++++-----
 3 files changed, 48 insertions(+), 33 deletions(-)

diff --git a/main/gay-ipod-fw.cpp b/main/gay-ipod-fw.cpp
index ed35c469..84a5b007 100644
--- a/main/gay-ipod-fw.cpp
+++ b/main/gay-ipod-fw.cpp
@@ -107,13 +107,17 @@ extern "C" void app_main(void)
   ESP_LOGI(TAG, "Trying to init SD card");
   gay_ipod::SdStorage storage(&expander);
 
-  ESP_ERROR_CHECK(storage.Acquire());
+  gay_ipod::SdStorage::Error err = storage.Acquire();
+  if (err != gay_ipod::SdStorage::Error::OK) {
+    ESP_LOGE(TAG, "Failed to acquire storage!");
+    return;
+  }
   ESP_LOGI(TAG, "Looks okay? Let's list some files!");
   vTaskDelay(pdMS_TO_TICKS(1000));
 
   DIR *d;
   struct dirent *dir;
-  d = opendir(gay_ipod::STORAGE_PATH);
+  d = opendir(gay_ipod::kStoragePath);
   if (d) {
     while ((dir = readdir(d)) != NULL) {
       ESP_LOGI(TAG, "file! %s", dir->d_name);
@@ -125,7 +129,7 @@ extern "C" void app_main(void)
 
   vTaskDelay(pdMS_TO_TICKS(1000));
   ESP_LOGI(TAG, "Time to deinit.");
-  ESP_ERROR_CHECK(storage.Release());
+  storage.Release();
 
   ESP_LOGI(TAG, "Hooray!");
   vTaskDelay(pdMS_TO_TICKS(1000));
diff --git a/main/storage.cpp b/main/storage.cpp
index 378bc3cf..54829302 100644
--- a/main/storage.cpp
+++ b/main/storage.cpp
@@ -14,15 +14,15 @@
 
 namespace gay_ipod {
 
+static const char* TAG = "SDSTORAGE";
+
 SdStorage::SdStorage(GpioExpander *gpio) {
   this->gpio_ = gpio;
 }
 
-SdStorage::~SdStorage() {
-  ESP_ERROR_CHECK(sdspi_host_deinit());
-}
+SdStorage::~SdStorage() {}
 
-esp_err_t SdStorage::Acquire(void) {
+SdStorage::Error SdStorage::Acquire(void) {
   // First switch to this device, and pull CS.
   gpio_->set_sd_mux(GpioExpander::ESP);
   gpio_->set_sd_cs(false);
@@ -39,50 +39,44 @@ esp_err_t SdStorage::Acquire(void) {
     .gpio_wp = SDSPI_SLOT_NO_WP,
     .gpio_int = GPIO_NUM_NC,
   };
-  esp_err_t ret = sdspi_host_init_device(&config, &handle_);
-  if (ret != ESP_OK) {
-    gpio_->set_sd_cs(true);
-    gpio_->Write();
-    return ret;
-  }
+  ESP_ERROR_CHECK(sdspi_host_init_device(&config, &handle_));
 
   host_ = sdmmc_host_t SDSPI_HOST_DEFAULT();
   host_.slot = handle_;
   // Will return ESP_ERR_INVALID_RESPONSE if there is no card
-  // TODO: use our own error code
-  ret = sdmmc_card_init(&host_, &card_);
-  if (ret != ESP_OK) {
+  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 ret;
+    return Error::FAILED_TO_READ;
   }
 
-  ESP_ERROR_CHECK(
-      esp_vfs_fat_register(STORAGE_PATH, "", MAX_OPEN_FILES, &fs_));
+  ESP_ERROR_CHECK(esp_vfs_fat_register(kStoragePath, "", kMaxOpenFiles, &fs_));
   ff_diskio_register_sdmmc(fs_->pdrv, &card_);
 
   // Mount right now, not on first operation.
-  FRESULT fret = f_mount(fs_, "", 1);
-  if (fret != FR_OK) {
-    // TODO: Proper error handling
-    return ESP_ERR_INVALID_MAC;
+  FRESULT ferr = f_mount(fs_, "", 1);
+  if (ferr != FR_OK) {
+    ESP_LOGW(TAG, "Failed to mount, err: %d", ferr);
+    return Error::FAILED_TO_MOUNT;
   }
 
   // We're done chatting for now.
   //gpio_->set_sd_cs(true);
   //gpio_->Write();
 
-  return ret;
+  return Error::OK;
 }
 
-esp_err_t SdStorage::Release(void) {
+void SdStorage::Release(void) {
   gpio_->set_sd_cs(false);
   gpio_->Write();
 
   // Unmount and unregister the filesystem
   f_unmount("");
   ff_diskio_register(fs_->pdrv, NULL);
-  esp_vfs_fat_unregister_path(STORAGE_PATH);
+  esp_vfs_fat_unregister_path(kStoragePath);
   fs_ = nullptr;
 
   // Uninstall the SPI driver
@@ -92,8 +86,6 @@ esp_err_t SdStorage::Release(void) {
   gpio_->set_sd_mux(GpioExpander::USB);
   gpio_->set_sd_cs(true);
   gpio_->Write();
-
-  return ESP_OK;
 }
 
 } // namespace gay_ipod
diff --git a/main/storage.h b/main/storage.h
index 514457f3..cffeb3fc 100644
--- a/main/storage.h
+++ b/main/storage.h
@@ -7,19 +7,38 @@
 #include "esp_vfs_fat.h"
 #include "gpio-expander.h"
 
-#define MAX_OPEN_FILES (8)
-
 namespace gay_ipod {
 
-static const char *STORAGE_PATH = "/sd";
+static const char *kStoragePath = "/sd";
+static const uint8_t kMaxOpenFiles = 8;
 
 class SdStorage {
   public:
     SdStorage(GpioExpander *gpio);
     ~SdStorage();
 
-    esp_err_t Acquire(void);
-    esp_err_t Release(void);
+    enum Error {
+      OK,
+      /** We couldn't interact with the SD card at all. Is it missing? */
+      FAILED_TO_READ,
+      /** We couldn't mount the SD card. Is it formatted? */
+      FAILED_TO_MOUNT,
+    };
+
+    // FIXME: these methods should also handling powering the SD card up and
+    // down once we have that capability.
+
+    /**
+     * Initialises the SDSPI driver and mounts the SD card for reading and
+     * writing. This must be called before any interactions with the underlying
+     * storage.
+     */
+    Error Acquire(void);
+
+    /**
+     * Unmounts the SD card and frees memory associated with the SDSPI driver.
+     */
+    void Release(void);
 
     // Not copyable or movable.
     // TODO: maybe this could be movable?