Ignore comments within playlist files

Includes a general cleanup+restructure of playlist.cpp, and fixing the
tests and benchmarks

src/drivers/test/test_samd.cpp

@@ -4,6 +4,7 @@
  * SPDX-License-Identifier: GPL-3.0-only
  */
 
+#include "drivers/nvs.hpp"
 #include "drivers/samd.hpp"
 
 #include <cstdint>
@@ -16,7 +17,8 @@ namespace drivers {
 
 TEST_CASE("samd21 interface", "[integration]") {
   I2CFixture i2c;
-  auto samd = std::make_unique<Samd>();
+  std::unique_ptr<drivers::NvsStorage> nvs{drivers::NvsStorage::OpenSync()};
+  auto samd = std::make_unique<Samd>(*nvs);
 
   REQUIRE(samd);
 

src/tangara/audio/playlist.cpp

@@ -5,14 +5,16 @@
  */
 #include "playlist.hpp"
 
-#include <string.h>
+#include <string>
 
-#include "audio/playlist.hpp"
-#include "database/database.hpp"
 #include "esp_log.h"
 #include "ff.h"
 
+#include "audio/playlist.hpp"
+#include "database/database.hpp"
+
 namespace audio {
+
 [[maybe_unused]] static constexpr char kTag[] = "playlist";
 
 Playlist::Playlist(const std::string& playlistFilepath)
@@ -20,190 +22,281 @@ Playlist::Playlist(const std::string& playlistFilepath)
       mutex_(),
       total_size_(0),
       pos_(-1),
+      file_open_(false),
+      file_error_(false),
       offset_cache_(&memory::kSpiRamResource),
       sample_size_(50) {}
 
 auto Playlist::open() -> bool {
+  std::unique_lock<std::mutex> lock(mutex_);
+  if (file_open_) {
+    return true;
+  }
+
   FRESULT res =
       f_open(&file_, filepath_.c_str(), FA_READ | FA_WRITE | FA_OPEN_ALWAYS);
   if (res != FR_OK) {
     ESP_LOGE(kTag, "failed to open file! res: %i", res);
     return false;
   }
-  // Count all entries
-  consumeAndCount(-1);
-  // Grab the first one
-  skipTo(0);
-  return true;
+  file_open_ = true;
+  file_error_ = false;
+
+  // Count the playlist size and build our offset cache.
+  countItems();
+  // Advance to the first item.
+  skipToWithoutCache(0);
+
+  return !file_error_;
 }
 
 Playlist::~Playlist() {
+  if (file_open_) {
     f_close(&file_);
   }
+}
+
+auto Playlist::filepath() const -> std::string {
+  return filepath_;
+}
 
 auto Playlist::currentPosition() const -> size_t {
-  return pos_;
+  std::unique_lock<std::mutex> lock(mutex_);
+  return pos_ < 0 ? 0 : pos_;
 }
 
 auto Playlist::size() const -> size_t {
+  std::unique_lock<std::mutex> lock(mutex_);
   return total_size_;
 }
 
-auto MutablePlaylist::append(Item i) -> void {
+auto Playlist::value() const -> std::string {
   std::unique_lock<std::mutex> lock(mutex_);
-  auto offset = f_tell(&file_);
-  bool first_entry = current_value_.empty();
-  // Seek to end and append
-  auto end = f_size(&file_);
-  auto res = f_lseek(&file_, end);
-  if (res != FR_OK) {
-    ESP_LOGE(kTag, "Seek to end of file failed? Error %d", res);
-    return;
-  }
-  // TODO: Resolve paths for track id, etc
-  std::string path;
-  if (std::holds_alternative<std::string>(i)) {
-    path = std::get<std::string>(i);
-    f_printf(&file_, "%s\n", path.c_str());
-    if (total_size_ % sample_size_ == 0) {
-      offset_cache_.push_back(end);
+  return current_value_;
 }
-    if (first_entry) {
-      current_value_ = path;
+
+auto Playlist::atEnd() const -> bool {
+  std::unique_lock<std::mutex> lock(mutex_);
+  return pos_ + 1 >= total_size_;
 }
-    total_size_++;
+
+auto Playlist::next() -> void {
+  std::unique_lock<std::mutex> lock(mutex_);
+  if (pos_ + 1 < total_size_ && !file_error_) {
+    advanceBy(1);
   }
-  // Restore position
-  res = f_lseek(&file_, offset);
-    if (res != FR_OK) {
-      ESP_LOGE(kTag, "Failed to restore file position after append?");
-      return;
 }
-  res = f_sync(&file_);
-  if (res != FR_OK) {
-    ESP_LOGE(kTag, "Failed to sync playlist file after append");
-    return;
+
+auto Playlist::prev() -> void {
+  std::unique_lock<std::mutex> lock(mutex_);
+  if (!file_error_) {
+    // Naive approach to see how that goes for now
+    skipToLocked(pos_ - 1);
   }
 }
 
 auto Playlist::skipTo(size_t position) -> void {
   std::unique_lock<std::mutex> lock(mutex_);
+  skipToLocked(position);
+}
+
+auto Playlist::skipToLocked(size_t position) -> void {
+  if (!file_open_ || file_error_) {
+    return;
+  }
+
   // Check our cache and go to nearest entry
-  pos_ = position;
   auto remainder = position % sample_size_;
   auto quotient = (position - remainder) / sample_size_;
   if (offset_cache_.size() <= quotient) {
-    // Fall back case
-    ESP_LOGW(kTag, "File offset cache failed, falling back...");
-    f_rewind(&file_);
-    advanceBy(pos_);
+    skipToWithoutCache(position);
     return;
   }
-  auto entry = offset_cache_.at(quotient);
+
   // Go to byte offset
+  auto entry = offset_cache_.at(quotient);
   auto res = f_lseek(&file_, entry);
   if (res != FR_OK) {
-    ESP_LOGW(kTag, "Error going to byte offset %llu for playlist entry index %d", entry, pos_);
+    ESP_LOGW(kTag, "error seeking %u", res);
+    file_error_ = true;
+    return;
   }
-  // Count ahead entries
+
+  // Count ahead entries.
   advanceBy(remainder + 1);
 }
 
-auto Playlist::next() -> void {
-  if (!atEnd()) {
+auto Playlist::skipToWithoutCache(size_t position) -> void {
+  if (position >= pos_) {
+    advanceBy(position - pos_);
+  } else {
+    pos_ = -1;
+    FRESULT res = f_rewind(&file_);
+    if (res != FR_OK) {
+      ESP_LOGW(kTag, "error rewinding %u", res);
+      file_error_ = true;
+      return;
+    }
+    advanceBy(position + 1);
+  }
+}
+
+auto Playlist::countItems() -> void {
+  TCHAR buff[512];
+
+  for (;;) {
+    auto offset = f_tell(&file_);
+    auto next_item = nextItem(buff);
+    if (!next_item) {
+      break;
+    }
+    if (total_size_ % sample_size_ == 0) {
+      offset_cache_.push_back(offset);
+    }
+    total_size_++;
+  }
+
+  f_rewind(&file_);
+}
+
+auto Playlist::advanceBy(ssize_t amt) -> bool {
+  TCHAR buff[512];
+  std::optional<std::string_view> item;
+
+  while (amt > 0) {
+    item = nextItem(buff);
+    if (!item) {
+      break;
+    }
     pos_++;
-    skipTo(pos_);
+    amt--;
   }
+
+  if (item) {
+    current_value_ = *item;
   }
 
-auto Playlist::prev() -> void {
-  // Naive approach to see how that goes for now
-  pos_--;
-  skipTo(pos_);
+  return amt == 0;
 }
 
-auto Playlist::value() const -> std::string {
-  return current_value_;
+auto Playlist::nextItem(std::span<TCHAR> buf)
+    -> std::optional<std::string_view> {
+  while (file_open_ && !file_error_ && !f_eof(&file_)) {
+    // FIXME: f_gets is quite slow (it does several very small reads instead of
+    // grabbing a whole sector at a time), and it doesn't work well for very
+    // long lines. We should do something smarter here.
+    TCHAR* str = f_gets(buf.data(), buf.size(), &file_);
+    if (str == NULL) {
+      ESP_LOGW(kTag, "Error consuming playlist file at offset %llu",
+               f_tell(&file_));
+      file_error_ = true;
+      return {};
+    }
+
+    std::string_view line{str};
+    if (line.starts_with("#")) {
+      continue;
+    }
+    if (line.ends_with('\n')) {
+      line = line.substr(0, line.size() - 1);
+    }
+    return line;
+  }
+
+  // Got to EOF without reading a valid line.
+  return {};
 }
 
-MutablePlaylist::MutablePlaylist(const std::string& playlistFilepath) : Playlist(playlistFilepath) {}
+MutablePlaylist::MutablePlaylist(const std::string& playlistFilepath)
+    : Playlist(playlistFilepath) {}
 
 auto MutablePlaylist::clear() -> bool {
   std::unique_lock<std::mutex> lock(mutex_);
-  auto res = f_close(&file_);
+
+  // Try to recover from any IO errors.
+  if (file_error_ && file_open_) {
+    file_error_ = false;
+    file_open_ = false;
+    f_close(&file_);
+  }
+
+  FRESULT res;
+  if (file_open_) {
+    res = f_rewind(&file_);
     if (res != FR_OK) {
+      ESP_LOGE(kTag, "error rewinding %u", res);
+      file_error_ = true;
       return false;
     }
-  res =
-      f_open(&file_, filepath_.c_str(), FA_READ | FA_WRITE | FA_CREATE_ALWAYS);
+    res = f_truncate(&file_);
     if (res != FR_OK) {
+      ESP_LOGE(kTag, "error truncating %u", res);
+      file_error_ = true;
       return false;
     }
+  } else {
+    res = f_open(&file_, filepath_.c_str(),
+                 FA_READ | FA_WRITE | FA_CREATE_ALWAYS);
+    if (res != FR_OK) {
+      ESP_LOGE(kTag, "error opening file %u", res);
+      file_error_ = true;
+      return false;
+    }
+    file_open_ = true;
+  }
+
   total_size_ = 0;
   current_value_.clear();
   offset_cache_.clear();
-  pos_ = 0;
+  pos_ = -1;
   return true;
 }
 
-auto Playlist::atEnd() -> bool {
-  return pos_ + 1 >= total_size_;
-}
-
-auto Playlist::filepath() -> std::string {
-  return filepath_;
+auto MutablePlaylist::append(Item i) -> void {
+  std::unique_lock<std::mutex> lock(mutex_);
+  if (!file_open_ || file_error_) {
+    return;
   }
 
-auto Playlist::consumeAndCount(ssize_t upto) -> bool {
-  std::unique_lock<std::mutex> lock(mutex_);
-  TCHAR buff[512];
-  size_t count = 0;
-  f_rewind(&file_);
-  while (!f_eof(&file_)) {
   auto offset = f_tell(&file_);
-    // TODO: Correctly handle lines longer than this
-    // TODO: Also correctly handle the case where the last entry doesn't end in
-    // \n
-    auto res = f_gets(buff, 512, &file_);
-    if (res == NULL) {
-      ESP_LOGW(kTag, "Error consuming playlist file at line %d", count);
-      return false;
-    }
-    if (count % sample_size_ == 0) {
-      offset_cache_.push_back(offset);
-    }
-    count++;
+  bool first_entry = current_value_.empty();
 
-    if (upto >= 0 && count > upto) {
-      size_t len = strlen(buff);
-      current_value_.assign(buff, len - 1);
-      break;
+  // Seek to end and append
+  auto end = f_size(&file_);
+  auto res = f_lseek(&file_, end);
+  if (res != FR_OK) {
+    ESP_LOGE(kTag, "Seek to end of file failed? Error %d", res);
+    file_error_ = true;
+    return;
   }
+
+  // TODO: Resolve paths for track id, etc
+  std::string path;
+  if (std::holds_alternative<std::string>(i)) {
+    path = std::get<std::string>(i);
+    f_printf(&file_, "%s\n", path.c_str());
+    if (total_size_ % sample_size_ == 0) {
+      offset_cache_.push_back(end);
     }
-  if (upto < 0) {
-    total_size_ = count;
-    f_rewind(&file_);
+    if (first_entry) {
+      current_value_ = path;
     }
-  return true;
+    total_size_++;
   }
 
-auto Playlist::advanceBy(ssize_t amt) -> bool {
-  TCHAR buff[512];
-  size_t count = 0;
-  while (!f_eof(&file_)) {
-    auto res = f_gets(buff, 512, &file_);
-    if (res == NULL) {
-      ESP_LOGW(kTag, "Error consuming playlist file at line %d", count);
-      return false;
-    }
-    count++;
-    if (count >= amt) {
-      size_t len = strlen(buff);
-      current_value_.assign(buff, len - 1);
-      break;
+  // Restore position
+  res = f_lseek(&file_, offset);
+  if (res != FR_OK) {
+    ESP_LOGE(kTag, "Failed to restore file position after append?");
+    file_error_ = true;
+    return;
   }
+  res = f_sync(&file_);
+  if (res != FR_OK) {
+    ESP_LOGE(kTag, "Failed to sync playlist file after append");
+    file_error_ = true;
+    return;
   }
-  return true;
 }
 
 }  // namespace audio

src/tangara/audio/playlist.hpp

@@ -6,11 +6,14 @@
  */
 
 #pragma once
+
 #include <string>
 #include <variant>
+
+#include "ff.h"
+
 #include "database/database.hpp"
 #include "database/track.hpp"
-#include "ff.h"
 
 namespace audio {
 
@@ -31,38 +34,52 @@ class Playlist {
   using Item =
       std::variant<database::TrackId, database::TrackIterator, std::string>;
   auto open() -> bool;
+
+  auto filepath() const -> std::string;
   auto currentPosition() const -> size_t;
   auto size() const -> size_t;
-  auto skipTo(size_t position) -> void;
+  auto value() const -> std::string;
+  auto atEnd() const -> bool;
+
   auto next() -> void;
   auto prev() -> void;
-  auto value() const -> std::string;
-  auto atEnd() -> bool;
-  auto filepath() -> std::string;
+  auto skipTo(size_t position) -> void;
 
  protected:
-  std::string filepath_;
-  std::mutex mutex_;
+  const std::string filepath_;
+
+  mutable std::mutex mutex_;
   size_t total_size_;
-  size_t pos_;
+  ssize_t pos_;
+
   FIL file_;
+  bool file_open_;
+  bool file_error_;
+
   std::string current_value_;
 
+  /* List of offsets determined by sample size */
+  std::pmr::vector<FSIZE_t> offset_cache_;
 
-  std::pmr::vector<FSIZE_t> offset_cache_; // List of offsets determined by sample size;
   /*
-  * How many tracks per offset saved (ie, a value of 100 means every 100 tracks the file offset is saved)
-  * This speeds up searches, especially in the case of shuffling a lot of tracks.
+   * How many tracks per offset saved (ie, a value of 100 means every 100 tracks
+   * the file offset is saved) This speeds up searches, especially in the case
+   * of shuffling a lot of tracks.
    */
   const uint32_t sample_size_;
 
-  auto consumeAndCount(ssize_t upto) -> bool;
+ private:
+  auto skipToLocked(size_t position) -> void;
+  auto countItems() -> void;
   auto advanceBy(ssize_t amt) -> bool;
+  auto nextItem(std::span<TCHAR>) -> std::optional<std::string_view>;
+  auto skipToWithoutCache(size_t position) -> void;
 };
 
 class MutablePlaylist : public Playlist {
  public:
   MutablePlaylist(const std::string& playlistFilepath);
+
   auto clear() -> bool;
   auto append(Item i) -> void;
 };

src/tangara/test/audio/test_playlist.cpp

@@ -9,18 +9,16 @@
 #include <dirent.h>
 
 #include <cstdio>
-#include <fstream>
-#include <iostream>
 
 #include "catch2/catch.hpp"
 
 #include "drivers/gpios.hpp"
 #include "drivers/i2c.hpp"
-#include "drivers/storage.hpp"
 #include "drivers/spi.hpp"
+#include "drivers/storage.hpp"
+#include "ff.h"
 #include "i2c_fixture.hpp"
 #include "spi_fixture.hpp"
-#include "ff.h"
 
 namespace audio {
 
@@ -39,10 +37,18 @@ TEST_CASE("playlist file", "[integration]") {
   }
 
   {
-    std::unique_ptr<drivers::SdStorage> result(drivers::SdStorage::Create(*gpios).value());
-    Playlist plist(kTestFilePath);
+    std::unique_ptr<drivers::SdStorage> result(
+        drivers::SdStorage::Create(*gpios).value());
+    MutablePlaylist plist(kTestFilePath);
+
+    SECTION("empty file appears empty") {
       REQUIRE(plist.clear());
 
+      REQUIRE(plist.size() == 0);
+      REQUIRE(plist.currentPosition() == 0);
+      REQUIRE(plist.value().empty());
+    }
+
     SECTION("write to the playlist file") {
       plist.append("test1.mp3");
       plist.append("test2.mp3");
@@ -56,6 +62,7 @@ TEST_CASE("playlist file", "[integration]") {
 
       SECTION("read from the playlist file") {
         Playlist plist2(kTestFilePath);
+        REQUIRE(plist2.open());
         REQUIRE(plist2.size() == 8);
         REQUIRE(plist2.value() == "test1.mp3");
         plist2.next();
@@ -65,19 +72,55 @@ TEST_CASE("playlist file", "[integration]") {
       }
     }
 
-    BENCHMARK("appending item") {
-      plist.append("A/New/Item.wav");
+    REQUIRE(plist.clear());
+
+    size_t tracks = 0;
+
+    BENCHMARK("appending items") {
+      plist.append("track " + std::to_string(plist.size()));
+      return tracks++;
     };
 
-    BENCHMARK("opening playlist file") {
+    BENCHMARK("opening large playlist file") {
       Playlist plist2(kTestFilePath);
-      REQUIRE(plist2.size() > 100);
+      REQUIRE(plist2.open());
+      REQUIRE(plist2.size() == tracks);
       return plist2.size();
     };
 
-    BENCHMARK("opening playlist file and appending entry") {
+    BENCHMARK("seeking after appending a large file") {
+      REQUIRE(plist.size() == tracks);
+
+      plist.skipTo(50);
+      REQUIRE(plist.value() == "track 50");
+      plist.skipTo(99);
+      REQUIRE(plist.value() == "track 99");
+      plist.skipTo(1);
+      REQUIRE(plist.value() == "track 1");
+
+      return plist.size();
+    };
+
+    BENCHMARK("seeking after opening a large file") {
       Playlist plist2(kTestFilePath);
-      REQUIRE(plist2.size() > 100);
+      REQUIRE(plist2.open());
+      REQUIRE(plist.size() == tracks);
+      REQUIRE(tracks >= 100);
+
+      plist.skipTo(50);
+      REQUIRE(plist.value() == "track 50");
+      plist.skipTo(99);
+      REQUIRE(plist.value() == "track 99");
+      plist.skipTo(1);
+      REQUIRE(plist.value() == "track 1");
+
+      return plist.size();
+    };
+
+    BENCHMARK("opening a large file and appending") {
+      MutablePlaylist plist2(kTestFilePath);
+      REQUIRE(plist2.open());
+      REQUIRE(plist2.size() >= 100);
       plist2.append("A/Nother/New/Item.opus");
       return plist2.size();
     };

src/tangara/test/battery/test_battery.cpp

@@ -26,9 +26,10 @@ class FakeAdc : public drivers::AdcBattery {
 
 TEST_CASE("battery charge state", "[unit]") {
   I2CFixture i2c;
+  std::unique_ptr<drivers::NvsStorage> nvs{drivers::NvsStorage::OpenSync()};
 
   // FIXME: mock the SAMD21 as well.
-  std::unique_ptr<drivers::Samd> samd{drivers::Samd::Create()};
+  auto samd = std::make_unique<drivers::Samd>(*nvs);
   FakeAdc* adc = new FakeAdc{};  // Freed by Battery.
   Battery battery{*samd, std::unique_ptr<drivers::AdcBattery>{adc}};