Merge 329c897601 into 3ce11f14ce

Multiple goals are achieved with this change:
- make descriptions of settings close to their definitions
- settings validation on loading with reset to default in case of bad
  value
- more constexpr to reduce RAM usage
- less hardcoded values
- more maintainable

RAM Usage:
 From
RAM:   [===       ]  32.4% (used 106084 bytes from 327680 bytes)
Flash: [========= ]  92.8% (used 6082238 bytes from 6553600 bytes)
 To
RAM:   [===       ]  32.2% (used 105436 bytes from 327680 bytes)
Flash: [========= ]  92.8% (used 6079222 bytes from 6553600 bytes)

Boot config validation with a test where the status bar config is wrong:
[1256] [SD] SD card detected
[1256] [CPS] Loading settings from file
[1265] [CPS] Invalid value (0x3) for Status Bar, resetting to default
[1265] [CPS] Settings loaded from file

src/CrossPointSettings.cpp

@@ -20,17 +20,11 @@ bool SettingDescriptor::validate(const CrossPointSettings& settings) const {
   return validator(value);
 }
 
-uint8_t SettingDescriptor::getValue(const CrossPointSettings& settings) const {
-  return settings.*(memberPtr);
-}
+uint8_t SettingDescriptor::getValue(const CrossPointSettings& settings) const { return settings.*(memberPtr); }
 
-void SettingDescriptor::setValue(CrossPointSettings& settings, uint8_t value) const {
-  settings.*(memberPtr) = value;
-}
+void SettingDescriptor::setValue(CrossPointSettings& settings, uint8_t value) const { settings.*(memberPtr) = value; }
 
-void SettingDescriptor::resetToDefault(CrossPointSettings& settings) const {
-  settings.*(memberPtr) = defaultValue;
-}
+void SettingDescriptor::resetToDefault(CrossPointSettings& settings) const { settings.*(memberPtr) = defaultValue; }
 
 void SettingDescriptor::save(FsFile& file, const CrossPointSettings& settings) const {
   serialization::writePod(file, settings.*(memberPtr));
@@ -42,10 +36,10 @@ void SettingDescriptor::load(FsFile& file, CrossPointSettings& settings) const {
   settings.*(memberPtr) = value;
 }
 
-
 namespace {
 constexpr uint8_t SETTINGS_FILE_VERSION = 1;
 // Increment this when adding new persisted settings fields
+constexpr uint8_t SETTINGS_COUNT = CrossPointSettings::DESCRIPTOR_COUNT + 1;  // descriptors + opdsServerUrl string
 constexpr char SETTINGS_FILE[] = "/.crosspoint/settings.bin";
 }  // namespace
 
@@ -55,7 +49,8 @@ constexpr const char* sleepScreenValues[] = {"Dark", "Light", "Custom", "Cover",
 constexpr const char* shortPwrBtnValues[] = {"Ignore", "Sleep", "Page Turn"};
 constexpr const char* statusBarValues[] = {"None", "No Progress", "Full"};
 constexpr const char* orientationValues[] = {"Portrait", "Landscape CW", "Inverted", "Landscape CCW"};
-constexpr const char* frontButtonLayoutValues[] = {"Back/Confirm/Left/Right", "Left/Right/Back/Confirm", "Left/Back/Confirm/Right"};
+constexpr const char* frontButtonLayoutValues[] = {"Back/Confirm/Left/Right", "Left/Right/Back/Confirm",
+                                                   "Left/Back/Confirm/Right"};
 constexpr const char* sideButtonLayoutValues[] = {"Prev/Next", "Next/Prev"};
 constexpr const char* fontFamilyValues[] = {"Bookerly", "Noto Sans", "Open Dyslexic"};
 constexpr const char* fontSizeValues[] = {"Small", "Medium", "Large", "X Large"};
@@ -67,45 +62,53 @@ constexpr const char* sleepScreenCoverModeValues[] = {"Fit", "Crop"};
 constexpr const char* hideBatteryPercentageValues[] = {"Never", "In Reader", "Always"};
 
 // Helper function template to deduce array size automatically
-template<size_t N>
-constexpr SettingDescriptor makeEnumDescriptor(
-    const char* name,
-    uint8_t CrossPointSettings::* ptr,
-    uint8_t defaultValue,
-    const char* const (&enumValues)[N]
-) {
-    return SettingDescriptor(name, SettingType::ENUM, ptr, defaultValue,
-                            validateEnum<N>, enumValues, N);
-}
+template <size_t N>
+constexpr SettingDescriptor makeEnumDescriptor(const char* name, uint8_t CrossPointSettings::* ptr,
+                                               uint8_t defaultValue, const char* const (&enumValues)[N]) {
+  return SettingDescriptor(name, SettingType::ENUM, ptr, defaultValue, validateEnum<N>, enumValues, N);
 }
+}  // namespace
 
 // Define static constexpr members (required in C++14 and earlier)
 constexpr size_t CrossPointSettings::DESCRIPTOR_COUNT;
 
 // Define the static constexpr array of all setting descriptors
 // Order must match current serialization order for file format compatibility!
-const std::array<SettingDescriptor, CrossPointSettings::DESCRIPTOR_COUNT> CrossPointSettings::descriptors = {{
-    makeEnumDescriptor("Sleep Screen", &CrossPointSettings::sleepScreen, CrossPointSettings::DARK, sleepScreenValues),
-    {"Extra Paragraph Spacing", SettingType::TOGGLE, &CrossPointSettings::extraParagraphSpacing, 1, validateToggle, nullptr, 0},
-    makeEnumDescriptor("Short Power Button Click", &CrossPointSettings::shortPwrBtn, CrossPointSettings::IGNORE, shortPwrBtnValues),
-    makeEnumDescriptor("Status Bar", &CrossPointSettings::statusBar, CrossPointSettings::FULL, statusBarValues),
-    makeEnumDescriptor("Reading Orientation", &CrossPointSettings::orientation, CrossPointSettings::PORTRAIT, orientationValues),
-    makeEnumDescriptor("Front Button Layout", &CrossPointSettings::frontButtonLayout, CrossPointSettings::BACK_CONFIRM_LEFT_RIGHT, frontButtonLayoutValues),
-    makeEnumDescriptor("Side Button Layout", &CrossPointSettings::sideButtonLayout, CrossPointSettings::PREV_NEXT, sideButtonLayoutValues),
-    makeEnumDescriptor("Reader Font Family", &CrossPointSettings::fontFamily, CrossPointSettings::BOOKERLY, fontFamilyValues),
-    makeEnumDescriptor("Reader Font Size", &CrossPointSettings::fontSize, CrossPointSettings::MEDIUM, fontSizeValues),
-    makeEnumDescriptor("Reader Line Spacing", &CrossPointSettings::lineSpacing, CrossPointSettings::NORMAL, lineSpacingValues),
-    makeEnumDescriptor("Reader Paragraph Alignment", &CrossPointSettings::paragraphAlignment, CrossPointSettings::JUSTIFIED, paragraphAlignmentValues),
-    makeEnumDescriptor("Time to Sleep", &CrossPointSettings::sleepTimeout, CrossPointSettings::SLEEP_10_MIN, sleepTimeoutValues),
-    makeEnumDescriptor("Refresh Frequency", &CrossPointSettings::refreshFrequency, CrossPointSettings::REFRESH_15, refreshFrequencyValues),
-    {"Reader Screen Margin", SettingType::VALUE, &CrossPointSettings::screenMargin, 5, validateRange<5, 40>, ValueRange{5, 40, 5}},
-    makeEnumDescriptor("Sleep Screen Cover Mode", &CrossPointSettings::sleepScreenCoverMode, CrossPointSettings::FIT, sleepScreenCoverModeValues),
-    // Note: opdsServerUrl (string) at position 15 is handled separately in serialization
-    {"Text Anti-Aliasing", SettingType::TOGGLE, &CrossPointSettings::textAntiAliasing, 1, validateToggle, nullptr, 0},
-    makeEnumDescriptor("Hide Battery %", &CrossPointSettings::hideBatteryPercentage, CrossPointSettings::HIDE_NEVER, hideBatteryPercentageValues),
-    {"Long-press Chapter Skip", SettingType::TOGGLE, &CrossPointSettings::longPressChapterSkip, 1, validateToggle, nullptr, 0},
-    {"Hyphenation", SettingType::TOGGLE, &CrossPointSettings::hyphenationEnabled, 0, validateToggle, nullptr, 0}
-}};
+const std::array<SettingDescriptor, CrossPointSettings::DESCRIPTOR_COUNT> CrossPointSettings::descriptors = {
+    {makeEnumDescriptor("Sleep Screen", &CrossPointSettings::sleepScreen, CrossPointSettings::DARK, sleepScreenValues),
+     {"Extra Paragraph Spacing", SettingType::TOGGLE, &CrossPointSettings::extraParagraphSpacing, 1, validateToggle,
+      nullptr, 0},
+     makeEnumDescriptor("Short Power Button Click", &CrossPointSettings::shortPwrBtn, CrossPointSettings::IGNORE,
+                        shortPwrBtnValues),
+     makeEnumDescriptor("Status Bar", &CrossPointSettings::statusBar, CrossPointSettings::FULL, statusBarValues),
+     makeEnumDescriptor("Reading Orientation", &CrossPointSettings::orientation, CrossPointSettings::PORTRAIT,
+                        orientationValues),
+     makeEnumDescriptor("Front Button Layout", &CrossPointSettings::frontButtonLayout,
+                        CrossPointSettings::BACK_CONFIRM_LEFT_RIGHT, frontButtonLayoutValues),
+     makeEnumDescriptor("Side Button Layout", &CrossPointSettings::sideButtonLayout, CrossPointSettings::PREV_NEXT,
+                        sideButtonLayoutValues),
+     makeEnumDescriptor("Reader Font Family", &CrossPointSettings::fontFamily, CrossPointSettings::BOOKERLY,
+                        fontFamilyValues),
+     makeEnumDescriptor("Reader Font Size", &CrossPointSettings::fontSize, CrossPointSettings::MEDIUM, fontSizeValues),
+     makeEnumDescriptor("Reader Line Spacing", &CrossPointSettings::lineSpacing, CrossPointSettings::NORMAL,
+                        lineSpacingValues),
+     makeEnumDescriptor("Reader Paragraph Alignment", &CrossPointSettings::paragraphAlignment,
+                        CrossPointSettings::JUSTIFIED, paragraphAlignmentValues),
+     makeEnumDescriptor("Time to Sleep", &CrossPointSettings::sleepTimeout, CrossPointSettings::SLEEP_10_MIN,
+                        sleepTimeoutValues),
+     makeEnumDescriptor("Refresh Frequency", &CrossPointSettings::refreshFrequency, CrossPointSettings::REFRESH_15,
+                        refreshFrequencyValues),
+     {"Reader Screen Margin", SettingType::VALUE, &CrossPointSettings::screenMargin, 5, validateRange<5, 40>,
+      ValueRange{5, 40, 5}},
+     makeEnumDescriptor("Sleep Screen Cover Mode", &CrossPointSettings::sleepScreenCoverMode, CrossPointSettings::FIT,
+                        sleepScreenCoverModeValues),
+     // Note: opdsServerUrl (string) at position 15 is handled separately in serialization
+     {"Text Anti-Aliasing", SettingType::TOGGLE, &CrossPointSettings::textAntiAliasing, 1, validateToggle, nullptr, 0},
+     makeEnumDescriptor("Hide Battery %", &CrossPointSettings::hideBatteryPercentage, CrossPointSettings::HIDE_NEVER,
+                        hideBatteryPercentageValues),
+     {"Long-press Chapter Skip", SettingType::TOGGLE, &CrossPointSettings::longPressChapterSkip, 1, validateToggle,
+      nullptr, 0},
+     {"Hyphenation", SettingType::TOGGLE, &CrossPointSettings::hyphenationEnabled, 0, validateToggle, nullptr, 0}}};
 
 bool CrossPointSettings::saveToFile() const {
   // Make sure the directory exists
@@ -117,20 +120,19 @@ bool CrossPointSettings::saveToFile() const {
   }
 
   serialization::writePod(outputFile, SETTINGS_FILE_VERSION);
-  serialization::writePod(outputFile, CrossPointSettings::DESCRIPTOR_COUNT);
+  serialization::writePod(outputFile, SETTINGS_COUNT);
 
   // Use descriptors to automatically serialize all uint8_t settings
-  uint8_t settingsWritten = 0;
-
+  // opdsServerUrl string is written at position 15 (between descriptors 14 and 15)
+  uint8_t descriptorIndex = 0;
   for (const auto& desc : descriptors) {
-    // Special handling for opdsServerUrl (string field at position 15)
-    if (settingsWritten == 15) {
+    // Write opdsServerUrl string before descriptor 15 (at position 15)
+    if (descriptorIndex == 15) {
       serialization::writeString(outputFile, std::string(opdsServerUrl));
-      settingsWritten++;
     }
 
     desc.save(outputFile, *this);
-    settingsWritten++;
+    descriptorIndex++;
   }
 
   outputFile.close();
@@ -140,8 +142,10 @@ bool CrossPointSettings::saveToFile() const {
 }
 
 bool CrossPointSettings::loadFromFile() {
+  Serial.printf("[%lu] [CPS] Loading settings from file\n", millis());
   FsFile inputFile;
   if (!SdMan.openFileForRead("CPS", SETTINGS_FILE, inputFile)) {
+    Serial.printf("[%lu] [CPS] Deserialization failed: Could not open settings file\n", millis());
     return false;
   }
 
@@ -157,29 +161,31 @@ bool CrossPointSettings::loadFromFile() {
   serialization::readPod(inputFile, fileSettingsCount);
 
   // Use descriptors to automatically deserialize all uint8_t settings
-  uint8_t settingsRead = 0;
+  // opdsServerUrl string is at position 15 (between descriptors 14 and 15)
+  uint8_t descriptorIndex = 0;
+  uint8_t filePosition = 0;
 
   for (const auto& desc : descriptors) {
-    if (settingsRead >= fileSettingsCount) {
+    if (filePosition >= fileSettingsCount) {
       break;  // File has fewer settings than current version
     }
 
-    // Special handling for opdsServerUrl (string field at position 15)
-    if (settingsRead == 15) {
-      if (settingsRead < fileSettingsCount) {
-        std::string urlStr;
-        serialization::readString(inputFile, urlStr);
-        strncpy(opdsServerUrl, urlStr.c_str(), sizeof(opdsServerUrl) - 1);
-        opdsServerUrl[sizeof(opdsServerUrl) - 1] = '\0';
-        settingsRead++;
-      }
-      if (settingsRead >= fileSettingsCount) {
+    // Read opdsServerUrl string at position 15 (before descriptor 15)
+    if (descriptorIndex == 15) {
+      std::string urlStr;
+      serialization::readString(inputFile, urlStr);
+      strncpy(opdsServerUrl, urlStr.c_str(), sizeof(opdsServerUrl) - 1);
+      opdsServerUrl[sizeof(opdsServerUrl) - 1] = '\0';
+      filePosition++;
+
+      if (filePosition >= fileSettingsCount) {
         break;
       }
     }
 
     desc.load(inputFile, *this);
-    settingsRead++;
+    descriptorIndex++;
+    filePosition++;
   }
 
   inputFile.close();
@@ -187,7 +193,8 @@ bool CrossPointSettings::loadFromFile() {
   // Validate each setting and reset invalid values to defaults
   for (const auto& desc : descriptors) {
     if (!desc.validate(*this)) {
-      Serial.printf("[%lu] [CPS] Invalid value for %s, resetting to default\n", millis(), desc.name);
+      Serial.printf("[%lu] [CPS] Invalid value (0x%X) for %s, resetting to default\n", millis(), desc.getValue(*this),
+                    desc.name);
       desc.resetToDefault(*this);
     }
   }

src/CrossPointSettings.h

@@ -1,15 +1,15 @@
 #pragma once
+#include <array>
 #include <cstdint>
 #include <iosfwd>
-#include <array>
-#include <vector>
 #include <string>
+#include <vector>
 
 // Setting descriptor infrastructure
 enum class SettingType { TOGGLE, ENUM, VALUE };
 
 // Validator function pointer (not std::function to save memory)
-using SettingValidator = bool(*)(uint8_t);
+using SettingValidator = bool (*)(uint8_t);
 
 // Forward declare for descriptors
 class CrossPointSettings;
@@ -19,72 +19,77 @@ class FsFile;
 
 // Base descriptor for all settings (non-virtual for constexpr)
 struct SettingDescriptorBase {
-    const char* name;              // Display name
-    SettingType type;
+  const char* name;  // Display name
+  SettingType type;
 };
 
 // Value range for VALUE type settings
 struct ValueRange {
-    uint8_t min, max, step;
+  uint8_t min, max, step;
 };
 
 // Concrete descriptor for uint8_t settings (constexpr-compatible)
 struct SettingDescriptor : public SettingDescriptorBase {
-    uint8_t CrossPointSettings::* memberPtr;  // Member pointer
-    uint8_t defaultValue;
-    SettingValidator validator;  // Optional validator function
+  uint8_t CrossPointSettings::* memberPtr;  // Member pointer
+  uint8_t defaultValue;
+  SettingValidator validator;  // Optional validator function
 
-    union {
-        // For ENUM types
-        struct {
-            const char* const* values;
-            uint8_t count;
-        } enumData;
+  union {
+    // For ENUM types
+    struct {
+      const char* const* values;
+      uint8_t count;
+    } enumData;
 
-        // For VALUE types
-        ValueRange valueRange;
-    };
+    // For VALUE types
+    ValueRange valueRange;
+  };
 
-    // Constexpr constructors for different setting types
-    // TOGGLE/ENUM constructor
-    constexpr SettingDescriptor(const char* name_, SettingType type_,
-                                 uint8_t CrossPointSettings::* ptr, uint8_t defVal, SettingValidator val,
-                                 const char* const* enumVals, uint8_t enumCnt)
-        : SettingDescriptorBase{name_, type_},
-          memberPtr(ptr), defaultValue(defVal), validator(val),
-          enumData{enumVals, enumCnt} {}
+  // Constexpr constructors for different setting types
+  // TOGGLE/ENUM constructor
+  constexpr SettingDescriptor(const char* name_, SettingType type_, uint8_t CrossPointSettings::* ptr, uint8_t defVal,
+                              SettingValidator val, const char* const* enumVals, uint8_t enumCnt)
+      : SettingDescriptorBase{name_, type_},
+        memberPtr(ptr),
+        defaultValue(defVal),
+        validator(val),
+        enumData{enumVals, enumCnt} {}
 
-    // VALUE constructor
-    constexpr SettingDescriptor(const char* name_, SettingType type_,
-                                 uint8_t CrossPointSettings::* ptr, uint8_t defVal, SettingValidator val,
-                                 ValueRange valRange)
-        : SettingDescriptorBase{name_, type_},
-          memberPtr(ptr), defaultValue(defVal), validator(val),
-          valueRange(valRange) {}
+  // VALUE constructor
+  constexpr SettingDescriptor(const char* name_, SettingType type_, uint8_t CrossPointSettings::* ptr, uint8_t defVal,
+                              SettingValidator val, ValueRange valRange)
+      : SettingDescriptorBase{name_, type_},
+        memberPtr(ptr),
+        defaultValue(defVal),
+        validator(val),
+        valueRange(valRange) {}
 
-    bool validate(const CrossPointSettings& settings) const;
-    uint8_t getValue(const CrossPointSettings& settings) const;
-    void setValue(CrossPointSettings& settings, uint8_t value) const;
-    void resetToDefault(CrossPointSettings& settings) const;
-    void save(FsFile& file, const CrossPointSettings& settings) const;
-    void load(FsFile& file, CrossPointSettings& settings) const;
+  bool validate(const CrossPointSettings& settings) const;
+  uint8_t getValue(const CrossPointSettings& settings) const;
+  void setValue(CrossPointSettings& settings, uint8_t value) const;
+  void resetToDefault(CrossPointSettings& settings) const;
+  void save(FsFile& file, const CrossPointSettings& settings) const;
+  void load(FsFile& file, CrossPointSettings& settings) const;
 
-    // Helper to get enum value as string
-    const char* getEnumValueString(uint8_t index) const {
-        if (index < enumData.count && enumData.values) {
-            return enumData.values[index];
-        }
-        return "";
+  // Helper to get enum value as string
+  const char* getEnumValueString(uint8_t index) const {
+    if (index < enumData.count && enumData.values) {
+      return enumData.values[index];
     }
+    return "";
+  }
 };
 
-
 // Validator functions (constexpr for compile-time optimization)
 constexpr bool validateToggle(uint8_t v) { return v <= 1; }
-template<uint8_t MAX>
-constexpr bool validateEnum(uint8_t v) { return v < MAX; }
-template<uint8_t MIN, uint8_t MAX>
-constexpr bool validateRange(uint8_t v) { return v >= MIN && v <= MAX; }
+template <uint8_t MAX>
+constexpr bool validateEnum(uint8_t v) {
+  return v < MAX;
+}
+template <uint8_t MIN, uint8_t MAX>
+constexpr bool validateRange(uint8_t v) {
+  return v >= MIN && v <= MAX;
+}
 
 class CrossPointSettings {
  private:

src/activities/settings/CategorySettingsActivity.cpp

@@ -203,6 +203,10 @@ void CategorySettingsActivity::render() const {
     renderer.drawText(UI_10_FONT_ID, 20, settingY, action.name, !isSelected);
   }
 
+  // Draw version text above button hints
+  renderer.drawText(SMALL_FONT_ID, pageWidth - 20 - renderer.getTextWidth(SMALL_FONT_ID, CROSSPOINT_VERSION),
+                    pageHeight - 60, CROSSPOINT_VERSION);
+
   // Draw help text
   const auto labels = mappedInput.mapLabels("« Back", "Toggle", "", "");
   renderer.drawButtonHints(UI_10_FONT_ID, labels.btn1, labels.btn2, labels.btn3, labels.btn4);

src/activities/settings/SettingsActivity.cpp

@@ -3,6 +3,8 @@
 #include <GfxRenderer.h>
 #include <HardwareSerial.h>
 
+#include <algorithm>
+
 #include "CategorySettingsActivity.h"
 #include "CrossPointSettings.h"
 #include "MappedInputManager.h"
@@ -12,12 +14,9 @@ const char* SettingsActivity::categoryNames[categoryCount] = {"Display", "Reader
 
 // Helper function to find descriptor by member pointer
 static const SettingDescriptor* findDescriptor(uint8_t CrossPointSettings::* memberPtr) {
-  for (const auto& desc : CrossPointSettings::descriptors) {
-    if (desc.memberPtr == memberPtr) {
-      return &desc;
-    }
-  }
-  return nullptr;
+  auto it = std::find_if(CrossPointSettings::descriptors.begin(), CrossPointSettings::descriptors.end(),
+                         [memberPtr](const SettingDescriptor& desc) { return desc.memberPtr == memberPtr; });
+  return (it != CrossPointSettings::descriptors.end()) ? &(*it) : nullptr;
 }
 
 void SettingsActivity::taskTrampoline(void* param) {
@@ -132,10 +131,10 @@ void SettingsActivity::enterCategory(int categoryIndex) {
   xSemaphoreTake(renderingMutex, portMAX_DELAY);
   exitActivity();
   enterNewActivity(new CategorySettingsActivity(renderer, mappedInput, categoryNames[categoryIndex], descriptors,
-                                                 actionItems, [this] {
-                                                   exitActivity();
-                                                   updateRequired = true;
-                                                 }));
+                                                actionItems, [this] {
+                                                  exitActivity();
+                                                  updateRequired = true;
+                                                }));
   xSemaphoreGive(renderingMutex);
 }
 
@@ -169,7 +168,6 @@ void SettingsActivity::render() const {
 
     // Draw category name
     renderer.drawText(UI_10_FONT_ID, 20, categoryY, categoryNames[i], i != selectedCategoryIndex);
-
   }
 
   // Draw version text above button hints