feat: Migrate binary settings to json by jpirnay · Pull Request #920 · crosspoint-reader/crosspoint-reader

jpirnay · 2026-02-16T12:28:18Z

Summary

This PR introduces a migration from binary file storage to JSON-based storage for application settings, state, and various credential stores. This improves readability, maintainability, and allows for easier manual configuration editing.
Benefits:
- Settings files are now JSON and can be easily read/edited manually
- Easier to inspect application state and settings during development
- JSON structure is more flexible for future changes
Drawback: around 15k of additional flash usage
Compatibility: Seamless migration preserves existing user data

Additional Context

New JSON I/O Infrastructure files:
- JsonSettingsIO: Core JSON serialization/deserialization logic using ArduinoJson library
- ObfuscationUtils: XOR-based password obfuscation for sensitive data
Migrated Components (now use JSON storage with automatic binary migration):
- CrossPointSettings (settings.json): Main application settings
- CrossPointState (state.json): Application state (open book, sleep mode, etc.)
- WifiCredentialStore (wifi.json): WiFi network credentials (Password Obfuscation: Sensitive data like WiFi passwords, uses XOR encryption with fixed keys. Note: This is obfuscation, not cryptographic security - passwords can be recovered with the key)
- KOReaderCredentialStore (koreader.json): KOReader sync credentials
- RecentBooksStore (recent.json): Recently opened books list
Migration Logic
- Forward Compatibility: New installations use JSON format
- Backward Compatibility: Existing binary files are automatically migrated to JSON on first load
- Backup Safety: Original binary files are renamed with .bak extension after successful migration
- Fallback Handling: If JSON parsing fails, system falls back to binary loading
Infrastructure Updates
- HalStorage: Added rename() method for backup operations

AI Usage

While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it
helps set the right context for reviewers.

Did you use AI tools to help write this code? ** YES**

…oint-reader into feat-jsonsettings

aplanas · 2026-02-16T13:59:47Z

src/JsonSettingsIO.cpp

+// ---- WifiCredentialStore ----
+
+namespace {
+constexpr uint8_t WIFI_OBFUSCATION_KEY[] = {0x43, 0x72, 0x6F, 0x73, 0x73, 0x50, 0x6F, 0x69, 0x6E, 0x74};


Why no generate a random one per user, and persist it in the SD card?

In the end that would not make a lot difference, would it? If you have the SD-card, you have stuff...

Indeed ... I was thinking on when some settings are shared in a forum, for example. In that case, then maybe the obfuscation can be dropped?

I am just checking whether the unique MAC-address of the chip would not work... Then you would need device + sd card, sd card alone (or publishing) would not provide an (immediate) breach

Yupp, that works 😄

Not my call to make - removal of obfuscation is easy, but someone needs to say what's more important - security or convenience.
(Although I could check whether a "password_clear" exists, read it, obfuscate and write back, delete it)

password_clear

that is a nice idea...

A more simple solution is to create a file wifi.txt that has first line as wifi name and second line as wifi password:

MyHomeWifi789 this_1s_the_passw0rd

The objective is not to modernize wifi or koreader settings save / restore, just for sake of easier external readibility. I want to step away from that binary fixed format setting file to ease the addition of new settings for future extensions / personalisation. I faced the challenge myself a couple of times during the few days I started contributing to this repo. Any new feature I wanted to be user-selectable faced the same challenge - when would the PR be merged aka in which position do I need to store / will I find my setting. Now this is no longer relavant: regardless where I define, when I define a new setting - it will just end up in the json file.

And btw: I created my wifi.json file from scratch:

{"lastConnectedSsid":"PYSY","credentials":[{"ssid":"PYSY","password":"my_very_secret_password"}]

and after the next connection, ie read of the setting it got converted to:

{"lastConnectedSsid":"PYSY","credentials":[{"ssid":"PYSY","password_obf":"qSb/nO9XvDmR1tko"}]

coderabbitai · 2026-02-16T15:00:12Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds JsonSettingsIO JSON (de)serialization for multiple stores, implements hardware-key XOR obfuscation utilities, and introduces per-store binary→JSON migration paths (read legacy .bin, write .json, rename .bin→.bak). Also adds HalStorage::rename and per-store loadFromBinaryFile helpers for migration.

Changes

Cohort / File(s)	Summary
JSON Settings I/O Framework `src/JsonSettingsIO.h`, `src/JsonSettingsIO.cpp`	New JsonSettingsIO namespace with save/load functions for CrossPointState, CrossPointSettings, KOReaderCredentialStore, WifiCredentialStore, RecentBooksStore; implements JSON (de)serialization, defaults, clamping, deobfuscation handling, needsResave signaling, and logging.
Obfuscation Utilities `lib/Serialization/ObfuscationUtils.h`, `lib/Serialization/ObfuscationUtils.cpp`	New obfuscation API using a hardware-derived XOR key, two xorTransform overloads, base64 obfuscate/deobfuscate helpers, and a selfTest routine; used for JSON persistence and legacy binary migration.
Storage API `lib/hal/HalStorage.h`, `lib/hal/HalStorage.cpp`	Added `bool rename(const char* oldPath, const char* newPath)` delegating to SDCard.rename for use when backing up migrated .bin files.
KOReader Credential Store `lib/KOReaderSync/KOReaderCredentialStore.h`, `lib/KOReaderSync/KOReaderCredentialStore.cpp`	Switched to JSON path and JsonSettingsIO integration; removed private obfuscate, added friend access for JsonSettingsIO, added `loadFromBinaryFile()` to migrate legacy koreader.bin → koreader.json and create .bak; adjusted logging and path constants.
CrossPoint Settings `src/CrossPointSettings.h`, `src/CrossPointSettings.cpp`	JSON-first persistence with new SETTINGS_FILE_* (BIN/JSON/BAK), added `loadFromBinaryFile()` for migration, added `validateFrontButtonMapping`, and updated load/save to migrate legacy .bin to .json.
CrossPoint State `src/CrossPointState.h`, `src/CrossPointState.cpp`	JSON-first persistence, new STATE_FILE_* constants, added `loadFromBinaryFile()` implementation and migration flow (save JSON + rename .bin→.bak), updated load/save logic and includes.
WiFi Credential Store `src/WifiCredentialStore.h`, `src/WifiCredentialStore.cpp`	JSON-first persistence with wifi.json, added JsonSettingsIO friendship, removed private obfuscate, added `loadFromBinaryFile()` using legacy deobfuscation for migration, and updated logging/path constants.
Recent Books Store `src/RecentBooksStore.h`, `src/RecentBooksStore.cpp`	Switch to JSON persistence, added `loadFromBinaryFile()` to read versioned legacy formats (v1/v2), migrate entries (cap to 10), save JSON and rename binary to .bak; adjusted file constants and logs.
JsonSettingsIO Friendships & Access `src/`, `lib/KOReaderSync/`, `src/RecentBooksStore.h`, `src/WifiCredentialStore.h`	Multiple stores add friend declarations for JsonSettingsIO loaders/savers to access private members; removed several private obfuscate helpers in favor of ObfuscationUtils.
Misc — Logging & Constants various `src/.cpp`, `lib/`	Updated log messages to indicate JSON vs binary sources; added per-store file-path constants (_FILE_BIN, _FILE_JSON, *_FILE_BAK) and adjusted save/load flows to prefer JSON with binary fallback and backup rename.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Store as Store (e.g., Wifi/CrossPoint/KOReader/Recent)
participant JsonIO as JsonSettingsIO
participant Storage as HalStorage
participant Obf as ObfuscationUtils
Store->>Storage: check for *.json
alt json exists
Storage->>Store: read json
Store->>JsonIO: JsonSettingsIO::loadX(json)
JsonIO->>Store: populate fields
else json missing
Storage->>Storage: check for *.bin
alt bin exists
Storage->>Store: read bin bytes
Store->>Obf: deobfuscate fields (if needed)
Store->>Store: parse legacy binary (loadFromBinaryFile)
Store->>JsonIO: JsonSettingsIO::saveX(path, json)
JsonIO->>Storage: write *.json
Store->>Storage: rename *.bin -> *.bin.bak (HalStorage::rename)
else none
Store->>Store: initialize defaults
end
end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'feat: Migrate binary settings to json' clearly and concisely describes the primary change: migration from binary to JSON-based storage for application settings and credentials.
Description check	✅ Passed	The description comprehensively explains the migration objectives, benefits, components affected, migration logic, and infrastructure changes, with transparency about AI usage and resource tradeoffs.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

lib/KOReaderSync/KOReaderCredentialStore.h (1)
17-21: ⚠️ Potential issue | 🟡 Minor

Outdated documentation comment.

The comment mentions koreader.bin but the implementation now uses koreader.json as the primary storage format. Consider updating the comment to reflect the JSON-based storage.
📝 Proposed fix
 /**
  * Singleton class for storing KOReader sync credentials on the SD card.
- * Credentials are stored in /sd/.crosspoint/koreader.bin with basic
- * XOR obfuscation to prevent casual reading (not cryptographically secure).
+ * Credentials are stored in /sd/.crosspoint/koreader.json with password
+ * obfuscation to prevent casual reading (not cryptographically secure).
  */
src/WifiCredentialStore.h (1)
16-20: ⚠️ Potential issue | 🟡 Minor

Outdated documentation comment.

Similar to KOReaderCredentialStore.h, the comment mentions wifi.bin but the implementation now uses wifi.json as the primary storage format.
📝 Proposed fix
 /**
  * Singleton class for storing WiFi credentials on the SD card.
- * Credentials are stored in /sd/.crosspoint/wifi.bin with basic
- * XOR obfuscation to prevent casual reading (not cryptographically secure).
+ * Credentials are stored in /sd/.crosspoint/wifi.json with password
+ * obfuscation to prevent casual reading (not cryptographically secure).
  */

🤖 Fix all issues with AI agents

In `@lib/KOReaderSync/KOReaderCredentialStore.cpp`:
- Around line 51-59: The migration currently renames KOREADER_FILE_BIN to
KOREADER_FILE_BAK even if saveToFile() fails, risking data loss; change the flow
in the block that checks Storage.exists(KOREADER_FILE_BIN) so that after
loadFromBinaryFile() returns true you call saveToFile() and only if saveToFile()
succeeds then call Storage.rename(KOREADER_FILE_BIN, KOREADER_FILE_BAK) and
return true; if saveToFile() fails, log an error (use LOG_DBG/LOG_ERR consistent
with file) and do not rename the binary or return success so the original binary
remains intact.

In `@lib/Serialization/ObfuscationUtils.h`:
- Around line 52-63: In deobfuscateFromBase64, check the return values of both
mbedtls_base64_decode calls (the size-query call and the actual decode call) and
handle non-zero errors instead of proceeding to use decodedLen; on error, abort
decoding (e.g., log/return empty string or propagate an error) and avoid
resizing/using result or calling xorTransform with invalid data. Specifically,
in deobfuscateFromBase64 capture the int ret from mbedtls_base64_decode for the
initial call that populates decodedLen and for the second call that writes into
result, validate decodedLen before allocating result, and only call
xorTransform(result) when the decode succeeded.
- Around line 17-25: getHwKey() performs non-thread-safe lazy initialization of
the static key[] and initialized flag which can race if called concurrently;
replace the manual bool guard with a thread-safe one-time init (e.g. add a
static std::once_flag and use std::call_once to invoke
esp_efuse_mac_get_default(key)) or convert key into a function-local static
initialized via a single-expression initializer so C++11 guarantees thread-safe
init; update references to getHwKey, key, and the initialization call
esp_efuse_mac_get_default accordingly.

In `@src/CrossPointState.cpp`:
- Around line 31-39: The migration code currently renames STATE_FILE_BIN to
STATE_FILE_BAK regardless of saveToFile() outcome, risking data loss; change the
logic in the block that checks Storage.exists(STATE_FILE_BIN) so that after
loadFromBinaryFile() returns true you call saveToFile() and verify its return
value (or success) before calling Storage.rename(STATE_FILE_BIN, STATE_FILE_BAK)
and logging the migration; if saveToFile() fails, log an error (using
LOG_ERR/LOG_DBG as appropriate), do not rename the original binary, and return
false (or propagate failure) so the original data remains intact.

In `@src/JsonSettingsIO.cpp`:
- Around line 70-72: The OPDS password is being written as plaintext
(doc["opdsPassword"] = s.opdsPassword) while other passwords use obfuscation;
update saveSettings to store the OPDS password using
obfuscation::obfuscateToBase64(s.opdsPassword) and update loadSettings to
reverse this using the corresponding deobfuscation helper (e.g.,
obfuscation::deobfuscateFromBase64 or equivalent) when reading
doc["opdsPassword"]; also apply the same change for the other occurrence noted
(lines ~142-144) and handle empty/missing values safely so you don't attempt to
deobfuscate a null/empty string.

🧹 Nitpick comments (4)

lib/KOReaderSync/KOReaderCredentialStore.cpp (1)

9-9: Consider using a non-relative include path.

The relative path ../../src/JsonSettingsIO.h is fragile and could break if the directory structure changes. Consider adding the src directory to the include path in the build configuration, allowing #include <JsonSettingsIO.h> or #include "JsonSettingsIO.h".
src/RecentBooksStore.cpp (1)
90-110: Migration may silently fail to persist if saveToFile returns false.

After successfully loading from the binary file, if saveToFile() fails on line 102, the migration isn't logged and the binary file isn't renamed. The function still returns true with data in memory, but the next startup will retry migration. This is actually reasonable resilience, but consider logging a warning when the JSON save fails during migration.
💡 Optional: Add warning log on migration save failure
   // Fall back to binary migration
   if (Storage.exists(RECENT_BOOKS_FILE_BIN)) {
     if (loadFromBinaryFile()) {
-      saveToFile();
-      Storage.rename(RECENT_BOOKS_FILE_BIN, RECENT_BOOKS_FILE_BAK);
-      LOG_DBG("RBS", "Migrated recent.bin to recent.json");
+      if (saveToFile()) {
+        Storage.rename(RECENT_BOOKS_FILE_BIN, RECENT_BOOKS_FILE_BAK);
+        LOG_DBG("RBS", "Migrated recent.bin to recent.json");
+      } else {
+        LOG_WARN("RBS", "Loaded from binary but failed to save JSON");
+      }
       return true;
     }
   }
src/JsonSettingsIO.cpp (2)
252-253: Hardcoded limit of 10 instead of using a constant.

The limit check uses a hardcoded 10 while RecentBooksStore.cpp defines MAX_RECENT_BOOKS = 10. Consider using the constant for maintainability, or expose it via the store's interface.

💡 Optional: Use constant instead of magic number

The simplest fix would be to expose MAX_RECENT_BOOKS from RecentBooksStore or document why 10 is used here. If getCount() already returns a value capped at the maximum, this check is redundant anyway.

175-176: Consider validating matchMethod range to handle potential invalid JSON values.

The matchMethod value is loaded from JSON and cast directly to DocumentMatchMethod without range validation. The enum only defines two valid values: FILENAME (0) and BINARY (1). If the JSON contains an invalid value, this could result in an out-of-range enum value causing unexpected behavior. The default fallback (| (uint8_t)0) only handles missing fields, not invalid ones.
💡 Optional: Add range check for matchMethod
   uint8_t method = doc["matchMethod"] | (uint8_t)0;
+  if (method > static_cast<uint8_t>(DocumentMatchMethod::BINARY)) {
+    method = static_cast<uint8_t>(DocumentMatchMethod::FILENAME);
+  }
   store.matchMethod = static_cast<DocumentMatchMethod>(method);

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6702060 and 26d8c71.

📒 Files selected for processing (15)

lib/KOReaderSync/KOReaderCredentialStore.cpp
lib/KOReaderSync/KOReaderCredentialStore.h
lib/Serialization/ObfuscationUtils.h
lib/hal/HalStorage.cpp
lib/hal/HalStorage.h
src/CrossPointSettings.cpp
src/CrossPointSettings.h
src/CrossPointState.cpp
src/CrossPointState.h
src/JsonSettingsIO.cpp
src/JsonSettingsIO.h
src/RecentBooksStore.cpp
src/RecentBooksStore.h
src/WifiCredentialStore.cpp
src/WifiCredentialStore.h

🧰 Additional context used

🧬 Code graph analysis (13)

lib/hal/HalStorage.h (1)

lib/hal/HalStorage.cpp (2)

rename (39-39)

rename (39-39)

src/CrossPointState.h (1)

src/CrossPointState.cpp (2)

loadFromBinaryFile (44-77)

loadFromBinaryFile (44-44)

src/WifiCredentialStore.h (2)

src/JsonSettingsIO.h (1)

JsonSettingsIO (9-31)

src/JsonSettingsIO.cpp (4)

saveWifi (184-198)

saveWifi (184-184)

loadWifi (200-222)

loadWifi (200-200)

src/RecentBooksStore.cpp (1)

src/JsonSettingsIO.cpp (4)

saveRecentBooks (226-240)

saveRecentBooks (226-226)

loadRecentBooks (242-264)

loadRecentBooks (242-242)

lib/KOReaderSync/KOReaderCredentialStore.cpp (1)

src/JsonSettingsIO.cpp (4)

saveKOReader (152-162)

saveKOReader (152-152)

loadKOReader (164-180)

loadKOReader (164-164)

src/RecentBooksStore.h (3)

src/JsonSettingsIO.h (1)

JsonSettingsIO (9-31)

src/JsonSettingsIO.cpp (2)

loadRecentBooks (242-264)

loadRecentBooks (242-242)

src/RecentBooksStore.cpp (2)

loadFromBinaryFile (112-165)

loadFromBinaryFile (112-112)

lib/KOReaderSync/KOReaderCredentialStore.h (3)

src/JsonSettingsIO.h (1)

JsonSettingsIO (9-31)

src/JsonSettingsIO.cpp (4)

saveKOReader (152-162)

saveKOReader (152-152)

loadKOReader (164-180)

loadKOReader (164-164)

lib/KOReaderSync/KOReaderCredentialStore.cpp (2)

loadFromBinaryFile (65-109)

loadFromBinaryFile (65-65)

src/CrossPointSettings.cpp (4)

src/WifiCredentialStore.cpp (6)

saveToFile (32-35)

saveToFile (32-32)

loadFromFile (37-57)

loadFromFile (37-37)

loadFromBinaryFile (59-94)

loadFromBinaryFile (59-59)

src/CrossPointState.cpp (6)

saveToFile (17-20)

saveToFile (17-17)

loadFromFile (22-42)

loadFromFile (22-22)

loadFromBinaryFile (44-77)

loadFromBinaryFile (44-44)

src/RecentBooksStore.cpp (6)

saveToFile (57-60)

saveToFile (57-57)

loadFromFile (90-110)

loadFromFile (90-90)

loadFromBinaryFile (112-165)

loadFromBinaryFile (112-112)

src/JsonSettingsIO.cpp (4)

saveSettings (47-83)

saveSettings (47-47)

loadSettings (85-148)

loadSettings (85-85)

src/JsonSettingsIO.h (3)

lib/KOReaderSync/KOReaderCredentialStore.h (2)

KOReaderCredentialStore (22-74)

JsonSettingsIO (12-15)

src/WifiCredentialStore.h (1)

JsonSettingsIO (11-14)

src/RecentBooksStore.h (1)

JsonSettingsIO (15-17)

src/JsonSettingsIO.cpp (2)

lib/Serialization/ObfuscationUtils.h (2)

string (52-63)

obfuscateToBase64 (44-49)

src/CrossPointSettings.cpp (2)

validateFrontButtonMapping (62-76)

validateFrontButtonMapping (62-62)

src/CrossPointState.cpp (1)

src/JsonSettingsIO.cpp (4)

saveState (18-28)

saveState (18-18)

loadState (30-43)

loadState (30-30)

src/WifiCredentialStore.cpp (2)

lib/KOReaderSync/KOReaderCredentialStore.cpp (4)

loadFromBinaryFile (65-109)

loadFromBinaryFile (65-65)

legacyDeobfuscate (30-34)

legacyDeobfuscate (30-30)

src/JsonSettingsIO.cpp (4)

saveWifi (184-198)

saveWifi (184-184)

loadWifi (200-222)

loadWifi (200-200)

src/CrossPointSettings.h (1)

src/CrossPointSettings.cpp (4)

validateFrontButtonMapping (62-76)

validateFrontButtonMapping (62-62)

loadFromBinaryFile (105-212)

loadFromBinaryFile (105-105)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: build
GitHub Check: cppcheck

🔇 Additional comments (27)

lib/hal/HalStorage.cpp (1)

39-39: LGTM — clean delegation for rename.
Matches the existing storage wrapper pattern and keeps behavior centralized.

lib/hal/HalStorage.h (1)

31-31: LGTM — header update matches implementation.
The new API is consistent with the storage wrapper surface.

lib/Serialization/ObfuscationUtils.h (1)

1-88: LGTM overall - good device-specific obfuscation approach.

The use of the ESP32 MAC address as a device-specific key improves upon a fixed key, making data non-portable (as intended for credential protection). The self-test function and clear documentation about XOR not being cryptographically secure are appreciated.

src/CrossPointState.h (1)

22-25: LGTM!

Clean addition of the private loadFromBinaryFile() helper method to support the binary-to-JSON migration path.

src/CrossPointSettings.h (1)

194-199: LGTM - consider consolidating access specifier sections.

The functionality is correct. The alternating public/private/public sections (lines 194-199) work but could be consolidated for readability by grouping all private members together. This is a minor organizational suggestion.

src/RecentBooksStore.h (2)

14-17: LGTM!

The forward declarations and friend access pattern is consistent with other credential stores in this PR, enabling JsonSettingsIO::loadRecentBooks to directly modify the private recentBooks vector.

Also applies to: 25-25

50-53: LGTM!

The private loadFromBinaryFile() declaration follows the established migration pattern.

src/CrossPointState.cpp (1)

44-77: LGTM!

The loadFromBinaryFile() implementation correctly handles versioned binary deserialization with proper file closure in all code paths.

lib/KOReaderSync/KOReaderCredentialStore.h (1)

11-15: LGTM!

The forward declarations, friend access pattern, and private loadFromBinaryFile() helper are consistent with other credential stores in this PR.

Also applies to: 33-36

src/WifiCredentialStore.h (1)

10-14: LGTM!

The forward declarations and friend access pattern are consistent with other stores in this PR.

Also applies to: 32-35

lib/KOReaderSync/KOReaderCredentialStore.cpp (2)

26-34: LGTM - legacy deobfuscation preserved correctly.

The legacy "KOReader" XOR key is correctly preserved for migrating existing binary credentials to the new JSON format with hardware-key-based obfuscation.

65-109: LGTM!

The loadFromBinaryFile() implementation correctly handles the legacy binary format with proper field-by-field reading and legacy deobfuscation of the password.

src/RecentBooksStore.cpp (3)

16-18: LGTM!

The file path constants follow a consistent naming pattern and support the JSON-first migration strategy with backup retention.

57-60: LGTM!

The save implementation correctly ensures the directory exists before delegating to the JSON serializer.

112-164: LGTM!

The binary loading logic correctly handles multiple file versions with proper error handling and file cleanup. The v1/v2 migration path that enriches metadata from the actual book files is a nice touch for data recovery.

src/WifiCredentialStore.cpp (3)

21-29: LGTM!

The legacy obfuscation handling is correctly scoped to the anonymous namespace and only used during binary migration. Keeping this separate from the new device-specific obfuscation in ObfuscationUtils.h is the right approach for backward compatibility.

37-57: LGTM!

The JSON-first loading with binary migration fallback follows the established pattern consistently.

59-94: LGTM!

The binary loading correctly handles version compatibility, applies legacy deobfuscation during migration, and has proper bounds checking on credential count.

src/JsonSettingsIO.cpp (4)

1-14: LGTM!

The includes are well-organized and bring in the necessary dependencies for JSON handling, storage, logging, and obfuscation utilities.

18-43: LGTM!

State save/load functions correctly serialize all fields with appropriate defaults on load.

93-94: LGTM!

The clamp lambda provides a clean validation pattern for bounded enum values with sensible fallback to defaults.

184-222: LGTM!

WiFi credential save/load correctly handles the array of credentials with proper obfuscation and respects the MAX_NETWORKS limit.

src/CrossPointSettings.cpp (4)

26-28: LGTM!

File path constants follow the established naming pattern for the JSON migration.

62-76: LGTM!

The validation logic correctly detects duplicate button mappings and resets to safe defaults. The nested loop approach is appropriate for the small fixed-size array.

78-103: LGTM!

The save and load functions follow the established JSON-first migration pattern consistently with other stores.

105-212: LGTM!

The binary loading logic correctly handles the extensible settings format with partial reads, properly tracks which fields were read, and applies appropriate validation or legacy conversion for button mappings.

src/JsonSettingsIO.h (1)

1-31: LGTM!

The header provides a clean, minimal interface with forward declarations to avoid circular dependencies. The namespace organization and consistent function naming (save/load pairs) make the API intuitive.

lib/KOReaderSync/KOReaderCredentialStore.cpp

lib/Serialization/ObfuscationUtils.h

src/CrossPointState.cpp

src/JsonSettingsIO.cpp

ngxson · 2026-02-16T16:45:40Z

Personally I think it's not a good idea to use JSON. I'm pretty sure that the JSON parsing code will add quite a lot of overhead for both flash and RAM usage, especially when used with templated types. This PR seems to take >10KB flash already:

PR:
RAM:   [===       ]  31.0% (used 101692 bytes from 327680 bytes)
Flash: [==========]  97.3% (used 6376920 bytes from 6553600 bytes)

master:
RAM:   [===       ]  31.0% (used 101708 bytes from 327680 bytes)
Flash: [==========]  97.5% (used 6389766 bytes from 6553600 bytes)

Instead, I think we can implement a more future-proof binary format (can be shared for caching too).

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@lib/Serialization/ObfuscationUtils.cpp`:
- Around line 51-55: The xorTransform function can divide by zero when keyLen is
0; add a defensive guard at the start of xorTransform (the function taking
std::string& data, const uint8_t* key, size_t keyLen) to handle keyLen == 0 —
for example return early or throw std::invalid_argument if keyLen is zero and/or
log an error — so the subsequent data[i] ^= key[i % keyLen] never performs a
modulo by zero.

🧹 Nitpick comments (3)

lib/KOReaderSync/KOReaderCredentialStore.cpp (2)
9-9: Prefer angle brackets for project includes.

The relative path "../../src/JsonSettingsIO.h" is fragile and depends on the directory structure. Consider adding the src/ directory to the include paths and using angle brackets instead.
-#include "../../src/JsonSettingsIO.h"
+#include <JsonSettingsIO.h>
102-108: Consider validating the matchMethod value before casting.

The method value read from the file is cast directly to DocumentMatchMethod without validation. A corrupted or malicious file could contain an invalid enum value.
   if (file.available()) {
     uint8_t method;
     serialization::readPod(file, method);
-    matchMethod = static_cast<DocumentMatchMethod>(method);
+    matchMethod = (method <= static_cast<uint8_t>(DocumentMatchMethod::BINARY))
+                    ? static_cast<DocumentMatchMethod>(method)
+                    : DocumentMatchMethod::FILENAME;
   } else {
     matchMethod = DocumentMatchMethod::FILENAME;
   }
lib/Serialization/ObfuscationUtils.cpp (1)
89-109: Consider adding error-path coverage to selfTest.

The self-test covers round-trip correctness but doesn't verify that deobfuscateFromBase64 correctly handles invalid input and sets the ok flag to false.
🧪 Optional: Add error handling test
   if (allPassed) {
     JSON_LOG_DBG("OBF", "Obfuscation self-test PASSED");
   }
+
+  // Verify error handling for invalid base64
+  bool ok = true;
+  std::string result = deobfuscateFromBase64("!!!invalid!!!", &ok);
+  if (ok || !result.empty()) {
+    JSON_LOG_ERR("OBF", "FAIL: invalid base64 should set ok=false");
+  }
 }

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 26d8c71 and a187384.

📒 Files selected for processing (6)

lib/KOReaderSync/KOReaderCredentialStore.cpp
lib/Serialization/ObfuscationUtils.cpp
lib/Serialization/ObfuscationUtils.h
src/CrossPointState.cpp
src/JsonSettingsIO.cpp
src/WifiCredentialStore.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

src/JsonSettingsIO.cpp

🧰 Additional context used

🧬 Code graph analysis (3)

src/CrossPointState.cpp (4)

src/WifiCredentialStore.cpp (6)

saveToFile (32-35)

saveToFile (32-32)

loadFromFile (37-61)

loadFromFile (37-37)

loadFromBinaryFile (63-98)

loadFromBinaryFile (63-63)

src/RecentBooksStore.cpp (6)

saveToFile (57-60)

saveToFile (57-57)

loadFromFile (90-110)

loadFromFile (90-90)

loadFromBinaryFile (112-165)

loadFromBinaryFile (112-112)

src/CrossPointSettings.cpp (6)

saveToFile (78-81)

saveToFile (78-78)

loadFromFile (83-103)

loadFromFile (83-83)

loadFromBinaryFile (105-212)

loadFromBinaryFile (105-105)

src/JsonSettingsIO.cpp (4)

saveState (35-45)

saveState (35-35)

loadState (47-60)

loadState (47-47)

lib/Serialization/ObfuscationUtils.h (1)

lib/Serialization/ObfuscationUtils.cpp (10)

xorTransform (44-49)

xorTransform (44-44)

xorTransform (51-55)

xorTransform (51-51)

obfuscateToBase64 (57-62)

obfuscateToBase64 (57-57)

deobfuscateFromBase64 (64-87)

deobfuscateFromBase64 (64-64)

selfTest (89-109)

selfTest (89-89)

lib/KOReaderSync/KOReaderCredentialStore.cpp (2)

lib/KOReaderSync/KOReaderCredentialStore.h (1)

saveToFile (47-53)

src/JsonSettingsIO.cpp (4)

saveKOReader (172-182)

saveKOReader (172-172)

loadKOReader (184-204)

loadKOReader (184-184)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: cppcheck
GitHub Check: build

🔇 Additional comments (9)

src/CrossPointState.cpp (4)

4-4: LGTM!

The new include and file path constants are well-organized and consistent with the migration pattern used across other stores in this PR.

Also applies to: 10-12

17-20: LGTM!

Clean implementation delegating to JsonSettingsIO::saveState, consistent with the pattern used in other stores.

31-43: Migration safety concern addressed.

The code now properly checks the saveToFile() return value before renaming the binary file to .bak. If the JSON save fails, the original binary is preserved, matching the safer pattern used in WifiCredentialStore.

48-81: LGTM!

The legacy binary deserialization logic is correctly preserved with proper version handling and file cleanup. This ensures backward compatibility during migration.

lib/Serialization/ObfuscationUtils.h (1)

1-27: LGTM!

The header is clean and well-organized. The API surface is minimal and appropriate:

Two xorTransform overloads (hardware key vs. custom key) for flexibility

Base64-based obfuscation/deobfuscation with optional error reporting via bool* ok

Self-test function for validation

lib/KOReaderSync/KOReaderCredentialStore.cpp (1)

51-63: Migration logic correctly handles save failures.

The code now properly checks if saveToFile() succeeds before renaming the binary to .bak. This prevents data loss if the JSON save fails during migration.

src/WifiCredentialStore.cpp (2)

46-58: Migration logic correctly guards against save failures.

The pattern correctly prevents data loss by only renaming the binary file to .bak after confirming that saveToFile() succeeded.

69-81: LGTM - Good backward compatibility handling.

The version check correctly rejects future versions while gracefully handling older (version 1) files by clearing lastConnectedSsid when not present.

lib/Serialization/ObfuscationUtils.cpp (1)

64-87: Good error handling for base64 decode failures.

The implementation correctly checks both mbedtls_base64_decode calls and properly handles failures by setting *ok = false and returning an empty string. This addresses the past review concern about missing error handling.

lib/Serialization/ObfuscationUtils.cpp

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/CrossPointSettings.cpp`:
- Around line 98-106: The migration path that moves SETTINGS_FILE_BIN to
SETTINGS_FILE_BAK must verify that saveToFile() succeeded before renaming and
returning; update the block that calls Storage.exists(SETTINGS_FILE_BIN) ->
loadFromBinaryFile() -> saveToFile() to check the boolean result of
saveToFile(), only call Storage.rename(SETTINGS_FILE_BIN, SETTINGS_FILE_BAK) and
log the "Migrated settings.bin to settings.json" message when saveToFile()
returns true, and on save failure log an error (using LOG_*), do not rename the
binary, and return false (or propagate failure) so the original binary is
retained.

🧹 Nitpick comments (4)

lib/Serialization/ObfuscationUtils.cpp (1)

72-91: Self-test logs passwords to debug output.

The selfTest() function logs test inputs including the string "WiFi P@ssw0rd!" via LOG_ERR. While these are test values, be mindful that in production builds, logging obfuscation test data (even failures) could inadvertently expose patterns. Consider reducing verbosity or using placeholder representations in logs.
lib/KOReaderSync/KOReaderCredentialStore.cpp (1)
42-55: Consider logging when JSON file exists but is empty.

When the JSON file exists but json.isEmpty(), the code silently falls through to binary migration. Adding a debug log here would help diagnose issues where a JSON file was created but is empty/corrupted.
💡 Suggested improvement
   if (Storage.exists(KOREADER_FILE_JSON)) {
     String json = Storage.readFile(KOREADER_FILE_JSON);
     if (!json.isEmpty()) {
       bool resave = false;
       bool result = JsonSettingsIO::loadKOReader(*this, json.c_str(), &resave);
       if (result && resave) {
         saveToFile();
         LOG_DBG("KRS", "Resaved KOReader credentials to update format");
       }
       return result;
+    } else {
+      LOG_DBG("KRS", "JSON file exists but is empty, trying binary migration");
     }
   }
src/JsonSettingsIO.cpp (2)
186-187: Consider validating matchMethod before casting to enum.

Unlike the clamping pattern used consistently in loadSettings for all enum values, matchMethod is cast directly from uint8_t to DocumentMatchMethod without validation. If a corrupted or out-of-range value is stored, this could produce undefined behavior.
♻️ Proposed fix to add validation
   uint8_t method = doc["matchMethod"] | (uint8_t)0;
+  // Validate matchMethod is within valid enum range
+  constexpr uint8_t MAX_MATCH_METHOD = 2;  // Adjust based on DocumentMatchMethod enum size
+  if (method >= MAX_MATCH_METHOD) method = 0;
   store.matchMethod = static_cast<DocumentMatchMethod>(method);
Alternatively, if DocumentMatchMethod has a _COUNT enum value like other enums in this codebase, use that for the validation bound.
270-270: Consider using a named constant instead of magic number.

The hard-coded 10 is inconsistent with the pattern used in loadWifi which references store.MAX_NETWORKS. Using a named constant (e.g., RecentBooksStore::MAX_BOOKS) would improve maintainability and make the limit easily discoverable.
♻️ Proposed fix
-    if (store.getCount() >= 10) break;
+    if (store.getCount() >= RecentBooksStore::MAX_BOOKS) break;
This assumes a constant is defined in RecentBooksStore. If not, consider adding one there.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a187384 and 44bac4b.

📒 Files selected for processing (9)

lib/KOReaderSync/KOReaderCredentialStore.cpp
lib/KOReaderSync/KOReaderCredentialStore.h
lib/Serialization/ObfuscationUtils.cpp
lib/Serialization/ObfuscationUtils.h
src/CrossPointSettings.cpp
src/JsonSettingsIO.cpp
src/JsonSettingsIO.h
src/WifiCredentialStore.cpp
src/WifiCredentialStore.h

🧰 Additional context used

🧬 Code graph analysis (7)

lib/KOReaderSync/KOReaderCredentialStore.h (2)

src/JsonSettingsIO.h (1)

JsonSettingsIO (9-31)

lib/KOReaderSync/KOReaderCredentialStore.cpp (2)

loadFromBinaryFile (75-119)

loadFromBinaryFile (75-75)

lib/Serialization/ObfuscationUtils.h (1)

lib/Serialization/ObfuscationUtils.cpp (10)

xorTransform (27-32)

xorTransform (27-27)

xorTransform (34-38)

xorTransform (34-34)

obfuscateToBase64 (40-45)

obfuscateToBase64 (40-40)

deobfuscateFromBase64 (47-70)

deobfuscateFromBase64 (47-47)

selfTest (72-92)

selfTest (72-72)

src/WifiCredentialStore.h (3)

src/JsonSettingsIO.h (1)

JsonSettingsIO (9-31)

src/JsonSettingsIO.cpp (4)

saveWifi (195-209)

saveWifi (195-195)

loadWifi (211-239)

loadWifi (211-211)

src/WifiCredentialStore.cpp (2)

loadFromBinaryFile (69-104)

loadFromBinaryFile (69-69)

src/JsonSettingsIO.h (4)

lib/KOReaderSync/KOReaderCredentialStore.h (2)

KOReaderCredentialStore (23-75)

JsonSettingsIO (12-15)

src/WifiCredentialStore.h (1)

JsonSettingsIO (11-14)

src/RecentBooksStore.h (1)

JsonSettingsIO (15-17)

src/JsonSettingsIO.cpp (20)

saveSettings (47-83)

saveSettings (47-47)

loadSettings (85-153)

loadSettings (85-85)

saveState (18-28)

saveState (18-18)

loadState (30-43)

loadState (30-30)

saveWifi (195-209)

saveWifi (195-195)

loadWifi (211-239)

loadWifi (211-211)

saveKOReader (157-167)

saveKOReader (157-157)

loadKOReader (169-191)

loadKOReader (169-169)

saveRecentBooks (243-257)

saveRecentBooks (243-243)

loadRecentBooks (259-281)

loadRecentBooks (259-259)

src/JsonSettingsIO.cpp (2)

lib/Serialization/ObfuscationUtils.cpp (4)

obfuscateToBase64 (40-45)

obfuscateToBase64 (40-40)

deobfuscateFromBase64 (47-70)

deobfuscateFromBase64 (47-47)

src/CrossPointSettings.cpp (2)

validateFrontButtonMapping (62-76)

validateFrontButtonMapping (62-62)

src/WifiCredentialStore.cpp (1)

src/JsonSettingsIO.cpp (4)

saveWifi (195-209)

saveWifi (195-195)

loadWifi (211-239)

loadWifi (211-211)

src/CrossPointSettings.cpp (1)

src/JsonSettingsIO.cpp (4)

saveSettings (47-83)

saveSettings (47-47)

loadSettings (85-153)

loadSettings (85-85)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: build
GitHub Check: cppcheck

🔇 Additional comments (11)

src/WifiCredentialStore.h (1)

1-68: LGTM!

The header changes properly introduce the JSON I/O integration with appropriate forward declarations, friend access for the JsonSettingsIO functions, and the new loadFromBinaryFile() private method for binary migration support. The updated documentation accurately describes the XOR obfuscation approach.

lib/KOReaderSync/KOReaderCredentialStore.h (1)

1-78: LGTM!

The header changes mirror the pattern established in WifiCredentialStore.h, properly introducing JSON I/O integration with forward declarations, friend access, and the loadFromBinaryFile() method for binary migration.

lib/KOReaderSync/KOReaderCredentialStore.cpp (1)

57-69: LGTM - Migration data loss issue addressed.

The migration flow now correctly checks the result of saveToFile() before renaming the binary file to .bak, preventing data loss if JSON save fails. This addresses the previous review concern.

src/WifiCredentialStore.cpp (2)

37-67: LGTM!

The migration flow correctly implements JSON-first loading with binary fallback. The save result is properly checked before renaming the binary file, preventing data loss during migration.

69-104: LGTM!

The loadFromBinaryFile() implementation properly handles version checking, reads credentials with legacy deobfuscation, and respects the MAX_NETWORKS limit.

lib/Serialization/ObfuscationUtils.h (1)

1-35: LGTM!

The header provides a clean API for the obfuscation utilities with appropriate documentation clarifying that XOR obfuscation is not cryptographically secure but serves to prevent casual reading and tie credentials to the specific device.

src/CrossPointSettings.cpp (1)

62-76: LGTM!

The validateFrontButtonMapping method correctly detects duplicate hardware button mappings and resets to defaults when conflicts are found.

src/JsonSettingsIO.h (1)

1-31: LGTM!

The header establishes a clean and consistent API for JSON-based persistence across all stores. The needsResave parameter is appropriately included only for stores containing obfuscated passwords that may need format updates.

src/JsonSettingsIO.cpp (3)

18-43: LGTM!

The state serialization/deserialization is straightforward with proper error handling and sensible defaults.

47-153: LGTM! Previous OPDS password concern addressed.

The settings serialization now properly obfuscates the OPDS password (line 72), with backward-compatible fallback to plaintext on load (lines 143-148) and the needsResave flag to trigger re-saving in obfuscated format. The clamping pattern for enum values and validateFrontButtonMapping call ensure data integrity.

195-239: LGTM!

WiFi credential handling properly uses MAX_NETWORKS for array bounds, obfuscates passwords on save, and handles legacy plaintext fallback with the needsResave pattern.

src/CrossPointSettings.cpp

jpirnay · 2026-02-16T19:09:22Z

Personally I think it's not a good idea to use JSON. I'm pretty sure that the JSON parsing code will add quite a lot of overhead for both flash and RAM usage, especially when used with templated types. This PR seems to take >10KB flash already:
PR:
RAM:   [===       ]  31.0% (used 101692 bytes from 327680 bytes)
Flash: [==========]  97.3% (used 6376920 bytes from 6553600 bytes)

master:
RAM:   [===       ]  31.0% (used 101708 bytes from 327680 bytes)
Flash: [==========]  97.5% (used 6389766 bytes from 6553600 bytes)
Instead, I think we can implement a more future-proof binary format (can be shared for caching too).

This wouldn't address my grievance I outlined above. Yes we are losing 10-12k (and you know how much I fight for flash size reduction) but we would gain a lot of flexibility.

ngxson

On second though, it can be a good idea to completely replace the settings REST API with this system, see CrossPointWebServer, should be done in a follow-up PR I think.

For now, the only thing I think should be improved (can be a follow-up PR) is the way settings are being defined. Imagine a new developer comes to the project, need to add a new setting, and now there are multiple places to be modified.

Instead, a better pattern is to have a centralized definition, much like SettingsList.h, something like:

class SettingEntryUInt8 {
  const char* name;
  uint8_t valueDefault;
  uint8_t valueMin;
  uint8_t valueMax;
  bool read(uint8_t& output);
  void write();
};
class SettingEntryString {
  const char* name;
  bool read(std::string& output);
  void write();
};

This will also technically reduce some flash space, because less templated function instances are being compiled.

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 44bac4b and ae88163.

📒 Files selected for processing (2)

lib/Serialization/ObfuscationUtils.cpp
src/CrossPointSettings.cpp

🧰 Additional context used

🧬 Code graph analysis (1)

src/CrossPointSettings.cpp (2)

src/CrossPointState.cpp (6)

saveToFile (17-20)

saveToFile (17-17)

loadFromFile (22-46)

loadFromFile (22-22)

loadFromBinaryFile (48-81)

loadFromBinaryFile (48-48)

src/JsonSettingsIO.cpp (4)

saveSettings (47-83)

saveSettings (47-47)

loadSettings (85-153)

loadSettings (85-85)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: build
GitHub Check: cppcheck

🔇 Additional comments (11)

src/CrossPointSettings.cpp (6)

4-4: LGTM!

Include addition is appropriate for the new JSON-based settings IO.

26-28: LGTM!

File path constants follow the established naming convention used in other migrated stores.

62-76: LGTM!

The validation logic correctly detects duplicate mappings and resets to safe defaults. The pairwise comparison is appropriate for 4 elements.

78-81: LGTM!

Save implementation follows the established pattern from CrossPointState.

98-113: LGTM! Previous review concern has been addressed.

The migration now properly checks saveToFile() result before renaming the binary file to .bak, matching the pattern in CrossPointState.cpp. This prevents data loss if JSON save fails.

115-222: LGTM!

The binary loading logic correctly handles versioned deserialization with forward compatibility. The distinction between legacy layout mapping and explicit button mapping is properly handled through the frontButtonMappingRead flag.

lib/Serialization/ObfuscationUtils.cpp (5)

12-24: Lazy init of hardware key looks good.
Single-use static initialization is clear and lightweight for this target.

27-32: xorTransform (hardware key) is straightforward and correct.

34-38: Guarded overload is solid.
The early return prevents modulo-by-zero and null key access.

41-46: obfuscateToBase64 implementation is clean.

73-93: Self-test coverage is helpful and unobtrusive.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/Serialization/ObfuscationUtils.cpp`:
- Around line 48-51: In deobfuscateFromBase64, when the input encoded is null or
empty you must set *ok = false (if ok is non-null) before returning the empty
string; currently ok is set to true unconditionally which incorrectly signals
success. Update the function deobfuscateFromBase64 to only set *ok = true after
validating encoded (and set *ok = false immediately when encoded == nullptr ||
encoded[0] == '\0'), preserving behavior when ok itself is nullptr.

In `@src/CrossPointSettings.cpp`:
- Around line 90-93: The code unconditionally logs success after calling
saveToFile() even though its return value is ignored; modify the block that
checks result && resave so that you capture the boolean/string result of
saveToFile() (call saveToFile() and store its return), then only call
LOG_DBG("CPS", "Resaved settings to update format") when that return indicates
success; if saveToFile() fails, log an error (e.g., LOG_ERR) with context so
failure is visible and consider propagating the failure or setting an error flag
— update the code paths referencing result, resave, saveToFile(), and LOG_DBG
accordingly.

lib/Serialization/ObfuscationUtils.cpp

src/CrossPointSettings.cpp

jpirnay · 2026-02-17T09:00:30Z

For now, the only thing I think should be improved (can be a follow-up PR) is the way settings are being defined. Imagine a new developer comes to the project, need to add a new setting, and now there are multiple places to be modified.

You are right: to add a single new uint8_t setting today, I need to touch 3 files, 4 locations:

CrossPointSettings.h: - add data member + enum constants
SettingsList.h - add UI/API registry entry
JsonSettingsIO.cpp - add to saveSettings() (line ~49-78) AND loadSettings() (line ~97-133)

The irony is that SettingsList.h already has almost everything needed for serialization (JSON key, member pointer, enum count via enumValues.size()), but JsonSettingsIO.cpp duplicates all of it manually.

What about JsonSettingsIO iterate over getSettingsList() instead of hardcoding each field. The existing SettingInfo struct already has:

key — JSON key name
valuePtr — member pointer to the uint8_t
enumValues.size() — max valid value for clamping
type — toggle/enum/value/string
stringPtr/stringMaxLen — for string fields

This would reduce "add a new setting" to just 2 files (CrossPointSettings.h + SettingsList.h) with zero changes to serialization code.

It would need some additional logic for default values and obfuscating, though.

Can we move this to a new PR?

CaptainFrito · 2026-02-20T08:52:14Z

How is the performance of JSON parsing on the device? This is the main advantage of using binary, the performance impact of reading files is minimal.

ngxson · 2026-02-20T16:48:01Z

Given the complexity of the current setting, I think the difference will be in the order of < 10ms, making it too small to be noticeable.

But FWIW, in my experience working at llama.cpp, we try not to use json whenever possible as it is quite a double sword. Main problems are: templated functions can take huge amount of time to compile, takes up program space, developers tend to abuse JSON in many circumstances, making weak typing and difficult to do static code analysis, etc. If you look closer into the project, you will see that we have a format called GGUF which is an implementation of key-value but in binary format.

However, we still need a high-level system to make it accessible for everyone. That's why we recently add support for INI format: ggml-org/llama.cpp#17850 . The main point of INI is that the parser is dead simple.

But honestly I have no preference if we stay on binary format or move to JSON or something else, the reason I explained above: we already allow changing settings from web API via a JSON request. That make it less likely to get rid of JSON. Otherwise, if we choose to use something other than JSON, we can update the API accordingly, but still the same amount of works I think.

jpirnay · 2026-02-20T17:07:01Z

I chose json, as that library was alread used. Apart from that: I couldnt care less whether its yaml, json, ini or whatever. my usecase was the need to step away from a fixed order, fixed format setting file.

ngxson · 2026-02-20T17:58:11Z

my usecase was the need to step away from a fixed order, fixed format setting file.

Just my 2c, no intention to push back this PR, but I just want to clarify one or two things:

If "human-readable" is not one of your goals, the using json here is pretty much overkill. A simple example is: when you want to read a number, json parser need to parse text to number, then because it can be either a float or int, depending on the templated getter, it must return you the correct data.

If you need a simple out-of-order serialization protocol, just a simple mapping key ID --> value should be enough. Number types can be stored as-is without converting to text. Dynamic-length data like a string can be handled by appending an int to indicate the number of bytes to be read. IIRC gRPC work more or less this way.

daveallie · 2026-02-22T06:05:44Z

src/JsonSettingsIO.cpp

+  s.frontButtonConfirm = clamp(doc["frontButtonConfirm"] | (uint8_t)S::FRONT_HW_CONFIRM, S::FRONT_BUTTON_HARDWARE_COUNT,
+                               S::FRONT_HW_CONFIRM);
+  s.frontButtonLeft =
+      clamp(doc["frontButtonLeft"] | (uint8_t)S::FRONT_HW_LEFT, S::FRONT_BUTTON_HARDWARE_COUNT, S::FRONT_HW_LEFT);


For all these lines, what's the purpose of the logical or? Feels like it's going to change the value in a lot of cases. E.g. this case here, if you save frontButtonLeft as "Confirm", next time this is loaded, it's going to load frontButtonLeft as "Right" (as confirm is mapped to 1 and FRONT_HW_LEFT is 2, so it's loaded as 1 | 2).

Afaict, there's no need to OR each of the input values?

@coderabbitai is that right?

@daveallie Good catch questioning this syntax! However, the | operator here is not a bitwise OR—it's actually ArduinoJson's API for providing default values.

In ArduinoJson, when you write:

doc["frontButtonLeft"] | (uint8_t)S::FRONT_HW_LEFT

The | operator is overloaded by JsonVariant to mean:

If the key exists in the JSON and has a value → return that value

If the key is missing or null → return the default value (the right operand)

So in your example:

If "frontButtonLeft" is saved as 1 (Confirm) in the JSON, it will load as 1

The | (uint8_t)S::FRONT_HW_LEFT part only provides a fallback default of 2 if the key is missing

This is standard ArduinoJson idiom for safe deserialization with defaults. The saved value is returned unchanged; no bitwise operations are performed on it. The default value on the right side of | is only used when the JSON key doesn't exist or is null.

TIL, thanks @coderabbitai

@daveallie You're welcome! Happy to help clarify that ArduinoJson syntax—it can be confusing at first since | looks like bitwise OR at a glance. 😊

daveallie · 2026-02-22T06:07:02Z

Given the reliance on JSON for the API, and ubiquity of it, I'm fine with moving ahead here with JSON here over other formats.

* This PR introduces a migration from binary file storage to JSON-based storage for application settings, state, and various credential stores. This improves readability, maintainability, and allows for easier manual configuration editing. * Benefits: - Settings files are now JSON and can be easily read/edited manually - Easier to inspect application state and settings during development - JSON structure is more flexible for future changes * Drawback: around 15k of additional flash usage * Compatibility: Seamless migration preserves existing user data 1. New JSON I/O Infrastructure files: - JsonSettingsIO: Core JSON serialization/deserialization logic using ArduinoJson library - ObfuscationUtils: XOR-based password obfuscation for sensitive data 2. Migrated Components (now use JSON storage with automatic binary migration): - CrossPointSettings (settings.json): Main application settings - CrossPointState (state.json): Application state (open book, sleep mode, etc.) - WifiCredentialStore (wifi.json): WiFi network credentials (Password Obfuscation: Sensitive data like WiFi passwords, uses XOR encryption with fixed keys. Note: This is obfuscation, not cryptographic security - passwords can be recovered with the key) - KOReaderCredentialStore (koreader.json): KOReader sync credentials - RecentBooksStore (recent.json): Recently opened books list 3. Migration Logic - Forward Compatibility: New installations use JSON format - Backward Compatibility: Existing binary files are automatically migrated to JSON on first load - Backup Safety: Original binary files are renamed with .bak extension after successful migration - Fallback Handling: If JSON parsing fails, system falls back to binary loading 4. Infrastructure Updates - HalStorage: Added rename() method for backup operations --- While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _** YES**_ --------- Co-authored-by: Dave Allie <[email protected]> (cherry picked from commit 6e4d0e5)

## Summary * This PR introduces a migration from binary file storage to JSON-based storage for application settings, state, and various credential stores. This improves readability, maintainability, and allows for easier manual configuration editing. * Benefits: - Settings files are now JSON and can be easily read/edited manually - Easier to inspect application state and settings during development - JSON structure is more flexible for future changes * Drawback: around 15k of additional flash usage * Compatibility: Seamless migration preserves existing user data ## Additional Context 1. New JSON I/O Infrastructure files: - JsonSettingsIO: Core JSON serialization/deserialization logic using ArduinoJson library - ObfuscationUtils: XOR-based password obfuscation for sensitive data 2. Migrated Components (now use JSON storage with automatic binary migration): - CrossPointSettings (settings.json): Main application settings - CrossPointState (state.json): Application state (open book, sleep mode, etc.) - WifiCredentialStore (wifi.json): WiFi network credentials (Password Obfuscation: Sensitive data like WiFi passwords, uses XOR encryption with fixed keys. Note: This is obfuscation, not cryptographic security - passwords can be recovered with the key) - KOReaderCredentialStore (koreader.json): KOReader sync credentials - RecentBooksStore (recent.json): Recently opened books list 3. Migration Logic - Forward Compatibility: New installations use JSON format - Backward Compatibility: Existing binary files are automatically migrated to JSON on first load - Backup Safety: Original binary files are renamed with .bak extension after successful migration - Fallback Handling: If JSON parsing fails, system falls back to binary loading 4. Infrastructure Updates - HalStorage: Added rename() method for backup operations --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _** YES**_ --------- Co-authored-by: Dave Allie <[email protected]>

jpirnay added 8 commits February 14, 2026 11:31

Web pages

f750853

Merge branch 'master' of https://github.com/jpirnay/crosspoint-reader

153e593

New json based settings

122a220

Merge branch 'master' into feat-jsonsettings

d02ebe4

Remove unrelated files

799385c

Merge branch 'feat-jsonsettings' of https://github.com/jpirnay/crossp…

6523eb8

…oint-reader into feat-jsonsettings

clang

cf963be

Another clang

5c9af78

aplanas reviewed Feb 16, 2026

View reviewed changes

Use device specific obfuscation key

26d8c71

coderabbitai bot requested changes Feb 16, 2026

View reviewed changes

coderabbitai bot previously approved these changes Feb 16, 2026

View reviewed changes

Acknowledge review comments

a187384

jpirnay dismissed coderabbitai[bot]’s stale review via a187384 February 16, 2026 17:34

coderabbitai bot requested changes Feb 16, 2026

View reviewed changes

lib/Serialization/ObfuscationUtils.cpp Show resolved Hide resolved

Allow unobfsuscated passwords

44bac4b

coderabbitai bot requested changes Feb 16, 2026

View reviewed changes

src/CrossPointSettings.cpp Show resolved Hide resolved

coderabbitai bot previously approved these changes Feb 16, 2026

View reviewed changes

ngxson reviewed Feb 16, 2026

View reviewed changes

two more review comments

ae88163

jpirnay dismissed coderabbitai[bot]’s stale review via ae88163 February 17, 2026 08:55

coderabbitai bot reviewed Feb 17, 2026

View reviewed changes

lib/Serialization/ObfuscationUtils.cpp Show resolved Hide resolved

src/CrossPointSettings.cpp Show resolved Hide resolved

Coderabbit findings

436f151

jpirnay mentioned this pull request Feb 17, 2026

[Feature Request] Easy way to (pre)load Wi-Fi network credentials #941

Closed

daveallie requested changes Feb 22, 2026

View reviewed changes

Merge branch 'master' into fork/jpirnay/feat-jsonsettings

863dcd7

daveallie approved these changes Feb 22, 2026

View reviewed changes

daveallie merged commit 6e4d0e5 into crosspoint-reader:master Feb 22, 2026
6 checks passed

This was referenced Feb 22, 2026

feat: rework CrossPointSettings #510

Open

refactor: Unify sleep screen settings and simplify UI #606

Closed

Uh oh!

Conversation

jpirnay commented Feb 16, 2026

Summary

Additional Context

AI Usage

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpirnay Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot commented Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ngxson commented Feb 16, 2026

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jpirnay commented Feb 16, 2026

Uh oh!

ngxson left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jpirnay commented Feb 17, 2026

Uh oh!

CaptainFrito commented Feb 20, 2026

Uh oh!

ngxson commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpirnay commented Feb 20, 2026

Uh oh!

ngxson commented Feb 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Feb 22, 2026

Choose a reason for hiding this comment

jpirnay Feb 16, 2026 •

edited

Loading

coderabbitai bot commented Feb 16, 2026 •

edited

Loading

ngxson commented Feb 20, 2026 •

edited

Loading