Build setup: minimal fs_data + override example#343
Build setup: minimal fs_data + override example#343GraspGG wants to merge 4 commits intoMoonModules:mdevfrom
Conversation
📝 WalkthroughWalkthroughAdded a minimal local filesystem payload: three empty JSON placeholders in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@GraspGG what is fs_data used for? I've never seen the folder, maybe you can say a few words about the purpose? |
fs_data is a local/minimal PlatformIO data_directory used to build/upload a small filesystem image (via buildfs/uploadfs). Thank you for pointing me on that documentation gap! |
|
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fs_data/README.md`:
- Around line 12-14: The README currently says cfg.json/presets.json/wsec.json
"can be {}" but cfg deserialization in wled00/cfg.cpp dereferences null/empty
objects causing crashes; update the README to either (A) specify the minimal
valid JSON skeleton required for each file (list required keys and example
minimal structure for cfg.json, presets.json, and wsec.json) so users won't
create `{}` that triggers null access in the deserializer, or (B) add a clear
safety warning that empty `{}` is unsafe and requires fixing wled00::cfg.cpp
deserialization; also update the repo example JSON files to match the minimal
valid structures you document so examples are safe.
In `@platformio_override.ini.olaf.example`:
- Around line 6-8: The example config is missing the data_dir directive so users
won't enable the fs_data payload; update the [env] block to add a data_dir entry
(e.g., data_dir = ${PROJECT_DIR}/fs_data) alongside the existing build_flags (-D
WLED_RELEASE_NAME="WLEDMM-OLAF") so the template matches the fs_data/README.md
usage; ensure the new line uses the same indentation style as build_flags.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61f2cacb-da53-454c-b8e0-7f5a33a145d1
📒 Files selected for processing (5)
fs_data/README.mdfs_data/cfg.jsonfs_data/presets.jsonfs_data/wsec.jsonplatformio_override.ini.olaf.example
| - `cfg.json` (optional, can be `{}`) | ||
| - `presets.json` (optional, can be `{}`) | ||
| - `wsec.json` (optional) — **do not commit secrets** (WiFi credentials). Keep it empty (`{}`) or manage locally. |
There was a problem hiding this comment.
Documentation incorrectly states files "can be {}" without safety warnings.
Lines 12-14 state that cfg.json and presets.json "can be {}" and that wsec.json should be kept "empty ({})". However, the deserialization code in wled00/cfg.cpp has unsafe null-object access patterns that will cause undefined behavior/crashes when these files contain only {}.
The documentation should either:
- Provide the minimal valid JSON structure needed to avoid crashes (not empty
{}), OR - Explicitly warn that empty
{}requires upstream fixes to cfg.cpp deserialization code
📝 Proposed documentation fix
## What to put here
-- `cfg.json` (optional, can be `{}`)
-- `presets.json` (optional, can be `{}`)
-- `wsec.json` (optional) — **do not commit secrets** (WiFi credentials). Keep it empty (`{}`) or manage locally.
+- `cfg.json` — minimal valid structure required (see file for example)
+- `presets.json` — minimal valid structure required (see file for example)
+- `wsec.json` — minimal valid structure without secrets (see file for example)
+
+**Note:** These files cannot be completely empty `{}` because the deserialization code in `wled00/cfg.cpp` accesses nested keys without null checks. Use the provided minimal structures to avoid crashes.Then update the actual JSON files to use the minimal valid structures suggested in the earlier comments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fs_data/README.md` around lines 12 - 14, The README currently says
cfg.json/presets.json/wsec.json "can be {}" but cfg deserialization in
wled00/cfg.cpp dereferences null/empty objects causing crashes; update the
README to either (A) specify the minimal valid JSON skeleton required for each
file (list required keys and example minimal structure for cfg.json,
presets.json, and wsec.json) so users won't create `{}` that triggers null
access in the deserializer, or (B) add a clear safety warning that empty `{}` is
unsafe and requires fixing wled00::cfg.cpp deserialization; also update the repo
example JSON files to match the minimal valid structures you document so
examples are safe.
| [env] | ||
| build_flags = | ||
| -D WLED_RELEASE_NAME=\"WLEDMM-OLAF\" No newline at end of file |
There was a problem hiding this comment.
Missing data_dir directive documented in README.
The fs_data/README.md (lines 17-18) documents that fs_data is "Typically enabled via platformio_override.ini, e.g.: data_dir = ${PROJECT_DIR}/fs_data", but this example template doesn't include that directive. Users copying this example won't actually use the minimal fs_data payload for buildfs/uploadfs, defeating the purpose of this PR.
📝 Proposed fix: Add data_dir to demonstrate documented usage
[env]
+data_dir = ${PROJECT_DIR}/fs_data
build_flags =
-D WLED_RELEASE_NAME=\"WLEDMM-OLAF\"This ensures the example demonstrates the primary use case documented in the README.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [env] | |
| build_flags = | |
| -D WLED_RELEASE_NAME=\"WLEDMM-OLAF\" | |
| [env] | |
| data_dir = ${PROJECT_DIR}/fs_data | |
| build_flags = | |
| -D WLED_RELEASE_NAME=\"WLEDMM-OLAF\" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platformio_override.ini.olaf.example` around lines 6 - 8, The example config
is missing the data_dir directive so users won't enable the fs_data payload;
update the [env] block to add a data_dir entry (e.g., data_dir =
${PROJECT_DIR}/fs_data) alongside the existing build_flags (-D
WLED_RELEASE_NAME="WLEDMM-OLAF") so the template matches the fs_data/README.md
usage; ensure the new line uses the same indentation style as build_flags.
2 participants
Adds minimal fs_data JSON placeholders (no secrets) and an override example for local builds.
Summary by CodeRabbit
Chores
Documentation