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. |
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
| [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.
There was a problem hiding this comment.
this example should be moved into platformio_override.ini.sample, but without modifying [env] directly.
@coderabbitai please remember that we don't want users to add their own platformio.ini buildenv files. New examples should be added in platformio_override.ini.sample, or stored locally in case of a usermod (i.e. in usermods folder or documented in the usermod readme.md).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
@GraspGG on second thought - please remove this example from your PR entirely.
It has too many bugs, like missing WLED-MM specific build flags, overwriting a standard target instead of using "extends = ", and more (probably you did not know these... 😅 n00b guides in WLED-MM are not the best).
The important step is adding data_dir = ${PROJECT_DIR}/fs_data into a custom buildenv => just describe this in your README.md.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Adds minimal fs_data JSON placeholders (no secrets) and an override example for local builds.
Summary by CodeRabbit
Chores
Documentation