feat(firmware): add 'user data' section to model yaml for Lua scripts - #7562
Open
philmoz wants to merge 7 commits into
Open
feat(firmware): add 'user data' section to model yaml for Lua scripts#7562philmoz wants to merge 7 commits into
philmoz wants to merge 7 commits into
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds bounded typed model user-data storage with Lua APIs, Companion model copying and YAML conversion, YAML serialization across supported targets, parser escape handling, and a shared string replacement utility. ChangesModel User Data
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LuaScript
participant modellib
participant ModelData
participant ModelYAML
LuaScript->>modellib: setUserData(app, key, value)
modellib->>ModelData: set typed user data
ModelData-->>modellib: success boolean
ModelData->>ModelYAML: persist dirty model data
LuaScript->>modellib: getUserData(app, key)
modellib->>ModelData: retrieve UserData
ModelData-->>LuaScript: integer, number, or string
Suggested labels: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Description checkExplanation The description identifies issue ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/datastructs_model.cpp`:
- Around line 266-272: Replace the file-scoped userData storage with
per-ModelData ownership, such as a mapping keyed by ModelData instances, without
changing the ModelData binary layout or memset behavior. Update
ModelData::hasUserData and all YAML/user-data helpers, including setUD, to read
and write the invoking instance’s storage rather than g_model. Ensure
per-instance storage is initialized and cleaned up with the corresponding
ModelData lifecycle.
- Around line 299-303: Update the existing user-data branch around ud->str and
ud->type so it also applies changes when typ differs, even if the string value
is unchanged. Preserve the assignments and storageDirty(EE_MODEL) behavior for
either a value or type change, including conversions such as UD_INT to
UD_STRING.
In `@radio/src/lua/api_model.cpp`:
- Around line 2038-2049: Update the user-data type dispatch around
g_model.setUserData so LUA_TSTRING values are detected explicitly before the
lua_isnumber check. Preserve numeric handling for actual integer and number
values, while ensuring numeric-looking Lua strings remain stored through the
string overload.
- Around line 1979-1985: Replace the throwing std::stoi and std::stof
conversions in luaGetUserData at radio/src/lua/api_model.cpp:1979-1985 and
luaGetAllUserData at radio/src/lua/api_model.cpp:2009-2015 with the non-throwing
strtol and strtof alternatives, preserving the existing Lua integer and number
results for valid values.
In `@radio/src/storage/yaml/yaml_datastructs_funcs.cpp`:
- Around line 2892-2909: Update r_userdata_key to construct the key from the
bounded buffer using val_len, including when initializing user data, instead of
treating val as null-terminated. Update w_userdata_key to emit ud->key through
yaml_output_string so special characters are YAML-escaped before writing.
- Around line 2918-2949: The userdata handlers must tolerate missing entries and
preserve the parser’s explicit value length. In r_userdata_type,
r_userdata_value, w_userdata_type, and w_userdata_value, check the result of
g_model.getUserData(tw->getElmts(1)) before dereferencing; return safely or emit
the appropriate empty/default output when it is null, consistent with
surrounding handlers. Update r_userdata_value to assign exactly val_len bytes
from val rather than treating it as NUL-terminated.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a9c97902-e811-4564-ae66-16a1ed05bf96
📒 Files selected for processing (31)
radio/src/dataconstants.hradio/src/datastructs_model.cppradio/src/datastructs_private.hradio/src/lua/api_model.cppradio/src/model_init.cppradio/src/storage/sdcard_yaml.cppradio/src/storage/yaml/yaml_datastructs_128x64.cppradio/src/storage/yaml/yaml_datastructs_f16.cppradio/src/storage/yaml/yaml_datastructs_funcs.cppradio/src/storage/yaml/yaml_datastructs_gx12.cppradio/src/storage/yaml/yaml_datastructs_nb4p.cppradio/src/storage/yaml/yaml_datastructs_nv14.cppradio/src/storage/yaml/yaml_datastructs_pa01.cppradio/src/storage/yaml/yaml_datastructs_pl18.cppradio/src/storage/yaml/yaml_datastructs_pl18u.cppradio/src/storage/yaml/yaml_datastructs_st16.cppradio/src/storage/yaml/yaml_datastructs_t15.cppradio/src/storage/yaml/yaml_datastructs_t15pro.cppradio/src/storage/yaml/yaml_datastructs_t20.cppradio/src/storage/yaml/yaml_datastructs_t22.cppradio/src/storage/yaml/yaml_datastructs_tpro.cppradio/src/storage/yaml/yaml_datastructs_tx15.cppradio/src/storage/yaml/yaml_datastructs_tx16smk3.cppradio/src/storage/yaml/yaml_datastructs_x10.cppradio/src/storage/yaml/yaml_datastructs_x9d.cppradio/src/storage/yaml/yaml_datastructs_x9dp2019.cppradio/src/storage/yaml/yaml_datastructs_x9e.cppradio/src/storage/yaml/yaml_datastructs_xlite.cppradio/src/storage/yaml/yaml_datastructs_xlites.cppradio/src/storage/yaml/yaml_tree_walker.cppradio/src/storage/yaml/yaml_tree_walker.h
Comment on lines
+2892
to
+2909
| void r_userdata_key(void* user, uint8_t* data, uint32_t bitoffs, | ||
| const char* val, uint8_t val_len) | ||
| { | ||
| auto tw = reinterpret_cast<YamlTreeWalker*>(user); | ||
| auto ud = g_model.getUserData(tw->getElmts(1)); | ||
| if (ud == nullptr) | ||
| g_model.setUserData(val, ""); | ||
| else | ||
| ud->key = val; | ||
| } | ||
|
|
||
| bool w_userdata_key(void* user, uint8_t* data, uint32_t bitoffs, | ||
| yaml_writer_func wf, void* opaque) | ||
| { | ||
| auto tw = reinterpret_cast<YamlTreeWalker*>(user); | ||
| auto ud = g_model.getUserData(tw->getElmts(1)); | ||
| return wf(opaque, ud->key.c_str(), ud->key.size()); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Prevent buffer over-read and fix YAML escaping for keys.
- The buffer
valmay not be null-terminated; construct the string usingstd::string(val, val_len)to prevent reading out of bounds. w_userdata_keywrites the raw string directly viawf. If a user-data key contains special characters like a colon or a newline, it will break the YAML structure. Useyaml_output_stringto properly escape the key.
🛠️ Proposed fix
void r_userdata_key(void* user, uint8_t* data, uint32_t bitoffs,
const char* val, uint8_t val_len)
{
auto tw = reinterpret_cast<YamlTreeWalker*>(user);
auto ud = g_model.getUserData(tw->getElmts(1));
if (ud == nullptr)
- g_model.setUserData(val, "");
+ g_model.setUserData(std::string(val, val_len), "");
else
- ud->key = val;
+ ud->key = std::string(val, val_len);
}
bool w_userdata_key(void* user, uint8_t* data, uint32_t bitoffs,
yaml_writer_func wf, void* opaque)
{
auto tw = reinterpret_cast<YamlTreeWalker*>(user);
auto ud = g_model.getUserData(tw->getElmts(1));
+ if (!ud) return false;
- return wf(opaque, ud->key.c_str(), ud->key.size());
+ return yaml_output_string(ud->key.c_str(), ud->key.size(), wf, opaque);
}📝 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.
Suggested change
| void r_userdata_key(void* user, uint8_t* data, uint32_t bitoffs, | |
| const char* val, uint8_t val_len) | |
| { | |
| auto tw = reinterpret_cast<YamlTreeWalker*>(user); | |
| auto ud = g_model.getUserData(tw->getElmts(1)); | |
| if (ud == nullptr) | |
| g_model.setUserData(val, ""); | |
| else | |
| ud->key = val; | |
| } | |
| bool w_userdata_key(void* user, uint8_t* data, uint32_t bitoffs, | |
| yaml_writer_func wf, void* opaque) | |
| { | |
| auto tw = reinterpret_cast<YamlTreeWalker*>(user); | |
| auto ud = g_model.getUserData(tw->getElmts(1)); | |
| return wf(opaque, ud->key.c_str(), ud->key.size()); | |
| } | |
| void r_userdata_key(void* user, uint8_t* data, uint32_t bitoffs, | |
| const char* val, uint8_t val_len) | |
| { | |
| auto tw = reinterpret_cast<YamlTreeWalker*>(user); | |
| auto ud = g_model.getUserData(tw->getElmts(1)); | |
| if (ud == nullptr) | |
| g_model.setUserData(std::string(val, val_len), ""); | |
| else | |
| ud->key = std::string(val, val_len); | |
| } | |
| bool w_userdata_key(void* user, uint8_t* data, uint32_t bitoffs, | |
| yaml_writer_func wf, void* opaque) | |
| { | |
| auto tw = reinterpret_cast<YamlTreeWalker*>(user); | |
| auto ud = g_model.getUserData(tw->getElmts(1)); | |
| if (!ud) return false; | |
| return yaml_output_string(ud->key.c_str(), ud->key.size(), wf, opaque); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@radio/src/storage/yaml/yaml_datastructs_funcs.cpp` around lines 2892 - 2909,
Update r_userdata_key to construct the key from the bounded buffer using
val_len, including when initializing user data, instead of treating val as
null-terminated. Update w_userdata_key to emit ud->key through
yaml_output_string so special characters are YAML-escaped before writing.
Collaborator
Author
There was a problem hiding this comment.
The val buffer is always 0 terminated.
There was a problem hiding this comment.
@philmoz Good point, thanks for the clarification — that resolves the buffer over-read concern, since val follows the same null-terminated convention used elsewhere in this file (e.g. r_screen_id, r_wov_string).
The escaping concern for w_userdata_key still stands though: it writes ud->key raw via wf instead of yaml_output_string, so a key containing YAML special characters (:, #, newline, etc.) would produce malformed YAML on save. Worth using yaml_output_string(ud->key.c_str(), ud->key.size(), wf, opaque) there, consistent with how w_userdata_value already does it.
philmoz
force-pushed
the
philmoz/model-user-data
branch
from
July 16, 2026 02:13
5872b42 to
7765bc9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/storage/yaml/yaml_datastructs_st16.cpp`:
- Around line 941-947: Guard the userData lookup in r_userdata_type and
r_userdata_value before dereferencing the result, ensuring reads are skipped or
handled safely when the key entry has not yet been processed. Apply the same fix
at the corresponding struct_UserData definitions in
radio/src/storage/yaml/yaml_datastructs_st16.cpp lines 941-947,
radio/src/storage/yaml/yaml_datastructs_t20.cpp lines 828-834, and
radio/src/storage/yaml/yaml_datastructs_x9e.cpp lines 810-816.
In `@radio/src/storage/yaml/yaml_parser.cpp`:
- Around line 245-251: In the YAML escape parsing logic, replace the raw-char
toupper calls at radio/src/storage/yaml/yaml_parser.cpp lines 245-251 (high
nibble) and 263-270 (low nibble) with explicit checks for uppercase and
lowercase hexadecimal ranges. Preserve the existing nibble conversion and parser
state transitions while avoiding undefined behavior for negative char values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7cd1b04e-ead0-4f44-a37f-a191d3a8bf6d
📒 Files selected for processing (36)
companion/src/constants.hcompanion/src/firmwares/edgetx/yaml_modeldata.cppcompanion/src/firmwares/modeldata.cppcompanion/src/firmwares/modeldata.hradio/src/dataconstants.hradio/src/datastructs_model.cppradio/src/datastructs_private.hradio/src/lua/api_model.cppradio/src/model_init.cppradio/src/storage/sdcard_yaml.cppradio/src/storage/yaml/yaml_datastructs_128x64.cppradio/src/storage/yaml/yaml_datastructs_f16.cppradio/src/storage/yaml/yaml_datastructs_funcs.cppradio/src/storage/yaml/yaml_datastructs_gx12.cppradio/src/storage/yaml/yaml_datastructs_nb4p.cppradio/src/storage/yaml/yaml_datastructs_nv14.cppradio/src/storage/yaml/yaml_datastructs_pa01.cppradio/src/storage/yaml/yaml_datastructs_pl18.cppradio/src/storage/yaml/yaml_datastructs_pl18u.cppradio/src/storage/yaml/yaml_datastructs_st16.cppradio/src/storage/yaml/yaml_datastructs_t15.cppradio/src/storage/yaml/yaml_datastructs_t15pro.cppradio/src/storage/yaml/yaml_datastructs_t20.cppradio/src/storage/yaml/yaml_datastructs_t22.cppradio/src/storage/yaml/yaml_datastructs_tpro.cppradio/src/storage/yaml/yaml_datastructs_tx15.cppradio/src/storage/yaml/yaml_datastructs_tx16smk3.cppradio/src/storage/yaml/yaml_datastructs_x10.cppradio/src/storage/yaml/yaml_datastructs_x9d.cppradio/src/storage/yaml/yaml_datastructs_x9dp2019.cppradio/src/storage/yaml/yaml_datastructs_x9e.cppradio/src/storage/yaml/yaml_datastructs_xlite.cppradio/src/storage/yaml/yaml_datastructs_xlites.cppradio/src/storage/yaml/yaml_parser.cppradio/src/storage/yaml/yaml_tree_walker.cppradio/src/storage/yaml/yaml_tree_walker.h
🚧 Files skipped from review as they are similar to previous changes (24)
- radio/src/model_init.cpp
- radio/src/storage/sdcard_yaml.cpp
- radio/src/storage/yaml/yaml_datastructs_f16.cpp
- radio/src/storage/yaml/yaml_datastructs_x9dp2019.cpp
- radio/src/storage/yaml/yaml_datastructs_t15pro.cpp
- radio/src/storage/yaml/yaml_datastructs_x9d.cpp
- radio/src/storage/yaml/yaml_datastructs_t15.cpp
- radio/src/storage/yaml/yaml_datastructs_xlite.cpp
- radio/src/storage/yaml/yaml_datastructs_tx15.cpp
- radio/src/storage/yaml/yaml_datastructs_pa01.cpp
- radio/src/storage/yaml/yaml_datastructs_128x64.cpp
- radio/src/storage/yaml/yaml_datastructs_gx12.cpp
- radio/src/storage/yaml/yaml_datastructs_tpro.cpp
- radio/src/datastructs_private.h
- radio/src/storage/yaml/yaml_datastructs_x10.cpp
- radio/src/storage/yaml/yaml_datastructs_tx16smk3.cpp
- radio/src/dataconstants.h
- radio/src/storage/yaml/yaml_datastructs_xlites.cpp
- radio/src/storage/yaml/yaml_datastructs_funcs.cpp
- radio/src/storage/yaml/yaml_datastructs_t22.cpp
- radio/src/storage/yaml/yaml_datastructs_pl18u.cpp
- radio/src/lua/api_model.cpp
- radio/src/datastructs_model.cpp
- radio/src/storage/yaml/yaml_datastructs_nv14.cpp
philmoz
force-pushed
the
philmoz/model-user-data
branch
from
July 16, 2026 03:11
7765bc9 to
921e7da
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/model_init.cpp`:
- Line 152: Replace the raw memset-based ModelData reset in setModelDefaults()
with a lifetime-safe reset or reconstruction that preserves the
std::vector<UserData> object before calling clearUserData(); ensure full-model
initialization clears user data on every supported target. Apply the same
lifetime-safe reset after the model buffer is cleared in
radio/src/storage/sdcard_yaml.cpp at lines 352-352, with no separate direct
change required there if the shared reset logic fixes it.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c4f67f9-de3d-4d75-87fd-dfd4be41c3e6
📒 Files selected for processing (36)
companion/src/constants.hcompanion/src/firmwares/edgetx/yaml_modeldata.cppcompanion/src/firmwares/modeldata.cppcompanion/src/firmwares/modeldata.hradio/src/dataconstants.hradio/src/datastructs_model.cppradio/src/datastructs_private.hradio/src/lua/api_model.cppradio/src/model_init.cppradio/src/storage/sdcard_yaml.cppradio/src/storage/yaml/yaml_datastructs_128x64.cppradio/src/storage/yaml/yaml_datastructs_f16.cppradio/src/storage/yaml/yaml_datastructs_funcs.cppradio/src/storage/yaml/yaml_datastructs_gx12.cppradio/src/storage/yaml/yaml_datastructs_nb4p.cppradio/src/storage/yaml/yaml_datastructs_nv14.cppradio/src/storage/yaml/yaml_datastructs_pa01.cppradio/src/storage/yaml/yaml_datastructs_pl18.cppradio/src/storage/yaml/yaml_datastructs_pl18u.cppradio/src/storage/yaml/yaml_datastructs_st16.cppradio/src/storage/yaml/yaml_datastructs_t15.cppradio/src/storage/yaml/yaml_datastructs_t15pro.cppradio/src/storage/yaml/yaml_datastructs_t20.cppradio/src/storage/yaml/yaml_datastructs_t22.cppradio/src/storage/yaml/yaml_datastructs_tpro.cppradio/src/storage/yaml/yaml_datastructs_tx15.cppradio/src/storage/yaml/yaml_datastructs_tx16smk3.cppradio/src/storage/yaml/yaml_datastructs_x10.cppradio/src/storage/yaml/yaml_datastructs_x9d.cppradio/src/storage/yaml/yaml_datastructs_x9dp2019.cppradio/src/storage/yaml/yaml_datastructs_x9e.cppradio/src/storage/yaml/yaml_datastructs_xlite.cppradio/src/storage/yaml/yaml_datastructs_xlites.cppradio/src/storage/yaml/yaml_parser.cppradio/src/storage/yaml/yaml_tree_walker.cppradio/src/storage/yaml/yaml_tree_walker.h
🚧 Files skipped from review as they are similar to previous changes (31)
- radio/src/dataconstants.h
- radio/src/storage/yaml/yaml_datastructs_t15.cpp
- radio/src/storage/yaml/yaml_datastructs_x10.cpp
- radio/src/storage/yaml/yaml_datastructs_tpro.cpp
- radio/src/storage/yaml/yaml_datastructs_nb4p.cpp
- radio/src/storage/yaml/yaml_datastructs_128x64.cpp
- radio/src/storage/yaml/yaml_datastructs_t22.cpp
- companion/src/firmwares/modeldata.h
- radio/src/storage/yaml/yaml_datastructs_xlite.cpp
- radio/src/storage/yaml/yaml_datastructs_tx15.cpp
- radio/src/storage/yaml/yaml_datastructs_pl18.cpp
- radio/src/storage/yaml/yaml_datastructs_xlites.cpp
- radio/src/storage/yaml/yaml_datastructs_f16.cpp
- radio/src/storage/yaml/yaml_datastructs_st16.cpp
- radio/src/storage/yaml/yaml_parser.cpp
- companion/src/firmwares/modeldata.cpp
- radio/src/storage/yaml/yaml_datastructs_x9dp2019.cpp
- radio/src/storage/yaml/yaml_datastructs_tx16smk3.cpp
- radio/src/storage/yaml/yaml_datastructs_x9d.cpp
- radio/src/storage/yaml/yaml_datastructs_pa01.cpp
- radio/src/storage/yaml/yaml_tree_walker.h
- radio/src/storage/yaml/yaml_datastructs_nv14.cpp
- radio/src/storage/yaml/yaml_datastructs_funcs.cpp
- radio/src/lua/api_model.cpp
- radio/src/storage/yaml/yaml_datastructs_t20.cpp
- radio/src/datastructs_model.cpp
- radio/src/storage/yaml/yaml_datastructs_x9e.cpp
- companion/src/firmwares/edgetx/yaml_modeldata.cpp
- radio/src/storage/yaml/yaml_tree_walker.cpp
- radio/src/datastructs_private.h
- radio/src/storage/yaml/yaml_datastructs_gx12.cpp
Collaborator
|
I always have the concern with lua that users didn't write can fill those table, and he has no way to do housekeeping on this, since deleting the lua won't delete corresponding keys |
Member
|
True. However, with this, a lua tool should be able to see all the data (i.e. model.getAllUserData()) and then allow you to delete all the entires or just the entries you don't want (via model.deleteUserData(key)). But, given that is 50 entries per model, sure, you could have some entries left over over time, but you are unlikely to run out of "space". Not sure we can really do anything about that really, as sure, companion could cleanup if say a widget/tool was no longer on the SD card, but that could also result in loss of settings you want later. i.e. I think manual cleanup is the only valid option here, since we don't have a packaging system in place (yet). |
Collaborator
|
Why is it that your guys always think users are developers, when only a ridiculously tiny proportion is. The average user struggle with copying data on their SD. They follow YT video, and guess what, they will have quickly tons of useless data after trying lua and seing it doesn't work for them. And no tool provided for them to be able to manage. And even if someone builds an lua to do so, no way to identify 'lua widget' owner to find which data belongs to whom |
Member
Oi, stop looking at my 101 OpenTX and EdgeTX data folders... there is a reason I copy and ignore! I'm sure of it!
This is the real problem... do you have any suggestions on how to implement this differently to mitigate this? Do we require a UUID for each script/tool or something (and how do we enforce it?) One way or another we really do need to have an API for widgets/lua to be able to have a proper way to store settings/freeform data. We can't just put our heads in the sand and ignore it. |
Collaborator
|
Maybe set and get should ask for both a "script name" and "key name". That will also help 2 scripts setting values for some generic key name like "settings" |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
radio/src/strhelpers.cpp (1)
1318-1336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for the shared replacement helper.
Cover empty
from, emptyto, no matches, overlapping matches, and replacements wheretocontainsfrom; this utility is now used by CSV/YAML serialization paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/strhelpers.cpp` around lines 1318 - 1336, Add focused tests for strReplaceAll covering empty from, empty to, no matches, overlapping matches, and cases where to contains from. Assert the resulting string for each scenario, including the helper’s existing behavior of leaving the input unchanged when from is empty.radio/src/lua/api_model.cpp (3)
1969-1969: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix typographical error.
Both of these comments misspell "mandatory".
radio/src/lua/api_model.cpp#L1969-L1969: Changemandataorytomandatory.radio/src/lua/api_model.cpp#L2066-L2066: Changemandataorytomandatory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/lua/api_model.cpp` at line 1969, Correct the spelling of “mandataory” to “mandatory” in the comments at radio/src/lua/api_model.cpp lines 1969-1969 and 2066-2066; no code behavior changes are needed.
2100-2104: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePrevent accidental deletion logic with empty keys.
If
getUDKeyreturns an empty string, callingdeleteUserData("")will perform a full scan of the user data vector and might inadvertently mutate state if empty slots exist. Guarding against an empty string prevents this edge case.♻️ Proposed refactor
static int luaDeleteUserData(lua_State *L) { std::string s = getUDKey(L); - g_model.deleteUserData(s.c_str()); + if (!s.empty()) { + g_model.deleteUserData(s.c_str()); + } return 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/lua/api_model.cpp` around lines 2100 - 2104, Update luaDeleteUserData to check the string returned by getUDKey before calling g_model.deleteUserData; only invoke deletion for a non-empty key, otherwise return without mutating user data.
1999-2001: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueShort-circuit on empty keys to avoid unnecessary lookups.
getUDKeyreturns an empty string if theapporkeyarguments are empty. It's safer and slightly more efficient to check for this and returnnilimmediately, rather than queryingg_model.getUserDatawith an empty string.♻️ Proposed refactor
std::string s = getUDKey(L); + + if (s.empty()) { + lua_pushnil(L); + return 1; + } auto ud = g_model.getUserData(s.c_str());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/lua/api_model.cpp` around lines 1999 - 2001, Update the code after getUDKey in the surrounding API function to check whether the returned key string is empty and immediately return Lua nil before calling g_model.getUserData. Preserve the existing lookup and result handling for non-empty keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/lua/api_model.cpp`:
- Around line 2062-2087: In luaSetUserData, validate argument 3’s supported Lua
type before calling getUDKey and allocating std::string s, while preserving the
existing string, integer, number, and fallback type-error behavior. Keep the
subsequent empty-key check and g_model.setUserData dispatch unchanged, and avoid
any luaL_checkstring call after the local std::string has been created.
---
Nitpick comments:
In `@radio/src/lua/api_model.cpp`:
- Line 1969: Correct the spelling of “mandataory” to “mandatory” in the comments
at radio/src/lua/api_model.cpp lines 1969-1969 and 2066-2066; no code behavior
changes are needed.
- Around line 2100-2104: Update luaDeleteUserData to check the string returned
by getUDKey before calling g_model.deleteUserData; only invoke deletion for a
non-empty key, otherwise return without mutating user data.
- Around line 1999-2001: Update the code after getUDKey in the surrounding API
function to check whether the returned key string is empty and immediately
return Lua nil before calling g_model.getUserData. Preserve the existing lookup
and result handling for non-empty keys.
In `@radio/src/strhelpers.cpp`:
- Around line 1318-1336: Add focused tests for strReplaceAll covering empty
from, empty to, no matches, overlapping matches, and cases where to contains
from. Assert the resulting string for each scenario, including the helper’s
existing behavior of leaving the input unchanged when from is empty.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0487aa61-ddd9-4d19-9bd3-252fbf430bd3
📒 Files selected for processing (6)
radio/src/gui/colorlcd/setup_menus/quick_menu.cppradio/src/lua/api_model.cppradio/src/storage/modelslist.cppradio/src/storage/modelslist.hradio/src/strhelpers.cppradio/src/strhelpers.h
💤 Files with no reviewable changes (1)
- radio/src/storage/modelslist.h
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/lua/api_model.cpp`:
- Around line 2034-2041: Update the argument handling block to treat any call
with at least one argument as an app filter, changing the lua_gettop check from
an exact count to a lower-bound check. Validate argument 1 with luaL_checkstring
before constructing or assigning to std::string s, so Lua type errors occur
before a C++ string allocation can be bypassed by longjmp.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fd157797-7201-4191-89d8-0aec34eeed3b
📒 Files selected for processing (2)
radio/src/datastructs_model.cppradio/src/lua/api_model.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- radio/src/datastructs_model.cpp
Comment on lines
+2034
to
+2041
| bool matchApp = false; | ||
| std::string s; | ||
| if (lua_gettop(L) == 2) { | ||
| s = luaL_checkstring(L, 1); | ||
| strReplaceAll(s, "|", "_"); | ||
| s += "|"; | ||
| matchApp = true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix argument count check and prevent std::string memory leak on Lua type errors.
There are two distinct issues in this block:
- Incorrect argument check (Functional correctness):
lua_gettop(L) == 2will only filter byappif exactly two arguments are passed. Sinceappis the first and only expected argument, the check should be>= 1. - Memory leak on
longjmp(Stability & availability):luaL_checkstring(L, 1)is called afterstd::string sis allocated on the stack. If the argument provided is not a string, Lua will throw a type error vialongjmp, bypassings's destructor and permanently leaking the string allocation.
Validate the argument type before allocating the std::string to safely trigger standard Lua type errors.
🔒️ Proposed fix
- bool matchApp = false;
- std::string s;
- if (lua_gettop(L) == 2) {
- s = luaL_checkstring(L, 1);
- strReplaceAll(s, "|", "_");
- s += "|";
- matchApp = true;
- }
+ const char* appName = nullptr;
+ if (lua_gettop(L) >= 1) {
+ appName = luaL_checkstring(L, 1);
+ }
+
+ bool matchApp = false;
+ std::string s;
+ if (appName) {
+ s = appName;
+ strReplaceAll(s, "|", "_");
+ s += "|";
+ matchApp = true;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@radio/src/lua/api_model.cpp` around lines 2034 - 2041, Update the argument
handling block to treat any call with at least one argument as an app filter,
changing the lua_gettop check from an exact count to a lower-bound check.
Validate argument 1 with luaL_checkstring before constructing or assigning to
std::string s, so Lua type errors occur before a C++ string allocation can be
bypassed by longjmp.
Contributor
|
Great feature, thanks 🙏 The api looks good (visually) For the over populated data, One thing to wonder is how do validate the correctness of "app"? Maybe app should be the path on disk? so future validation will be possible. only the set operation should validate the data. |
philmoz
force-pushed
the
philmoz/model-user-data
branch
from
July 21, 2026 07:24
162ac16 to
ed7d162
Compare
|
Great work! Nice feature! Excited to check it out. Currently I just have lua make a text file to save my last lua settings with io.open then io.read(cant read multiple lines tho) and io.write. Works great for remembering and automatically turning on my last background music song that was playing or automatically turning on my RGB lights before a model switch . I figured it out and I had no prior programing experience. |
philmoz
force-pushed
the
philmoz/model-user-data
branch
from
July 25, 2026 11:07
ed7d162 to
68a8fb0
Compare
philmoz
force-pushed
the
philmoz/model-user-data
branch
from
August 3, 2026 22:18
68a8fb0 to
21031c0
Compare
philmoz
force-pushed
the
philmoz/model-user-data
branch
2 times, most recently
from
August 16, 2026 06:29
34a6d9f to
575d0a7
Compare
added 7 commits
August 20, 2026 08:42
Fix firmware YAML parser to correctly handle Companion generated YAML files. Minor AI suggested changes.
philmoz
force-pushed
the
philmoz/model-user-data
branch
from
August 19, 2026 22:42
575d0a7 to
9cbdd7a
Compare
There was a problem hiding this comment.
Pull request overview
Adds persistent per-model userData storage for Lua scripts, with firmware and Companion YAML support.
Changes:
- Adds typed, 50-entry user-data storage and Lua CRUD APIs.
- Adds YAML serialization, parsing, escaping, and generated schemas.
- Adds shared string replacement support and selected LTO build options.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Reviewed changes and final comments |
|---|---|
tools/build-common.sh |
Adds LTO options for selected targets. |
radio/src/strhelpers.h |
Declares the shared replacement helper. |
radio/src/strhelpers.cpp |
Implements the helper. Nit (4 votes): Correct occurances to occurrences. |
radio/src/storage/yaml/yaml_tree_walker.h |
Exposes YAML string serialization. |
radio/src/storage/yaml/yaml_tree_walker.cpp |
Updates YAML escaping. Moderate (2 votes): NUL bytes are still truncated despite max_len; use length-aware output for user-data strings. |
radio/src/storage/yaml/yaml_parser.cpp |
Adds escaped-string parsing. Critical (4 votes): Add a direct ctype include for toupper. |
radio/src/storage/yaml/yaml_datastructs_xlites.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_xlite.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_x9e.cpp |
Critical (2 votes): Generated C14, GX15, and V12 tables lack userData; regenerate/include them. |
radio/src/storage/yaml/yaml_datastructs_x9dp2019.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_x9d.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_x10.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_tx16smk3.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_tx15.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_tpro.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_t22.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_t20.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_t15pro.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_t15.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_st16.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_pl18u.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_pl18.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_pa01.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_nv14.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_nb4p.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_gx12.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_funcs.cpp |
Critical (2 votes): Callbacks use global storage instead of the supplied YAML model context, causing cross-model data corruption. Moderate (4 votes): Sparse YAML indexes are appended at index zero instead of their parsed index. |
radio/src/storage/yaml/yaml_datastructs_f16.cpp |
Adds generated user-data schema. |
radio/src/storage/yaml/yaml_datastructs_128x64.cpp |
Adds generated user-data schema. |
radio/src/storage/sdcard_yaml.cpp |
Critical (4 votes): Clear user data on every model load, including B&W targets, to prevent stale entries. |
radio/src/storage/modelslist.h |
Supports model-list changes. |
radio/src/storage/modelslist.cpp |
Updates model-label handling and string replacement. |
radio/src/model_init.cpp |
Critical (4 votes): Move clearUserData() outside the COLORLCD guard so resets clear data on B&W radios. |
radio/src/lua/api_model.cpp |
Critical (3 votes): Reject the pipe character in app names rather than replacing it, including app filtering. Moderate (2 votes): Preserve embedded NUL bytes using Lua string lengths. Critical (1 vote): Add or gate APIs for targets whose schemas lack userData. |
radio/src/gui/colorlcd/setup_menus/quick_menu.cpp |
Critical (1 vote): Removing replaceAll leaves existing declarations and callers unresolved; retain or migrate them together. |
radio/src/datastructs_private.h |
Critical (1 vote): Add a direct <vector> include. Critical (1 vote): Regenerate missing C14, GX15, and V12 YAML outputs. |
radio/src/datastructs_model.cpp |
Moderate (4 votes): Use round-trip-safe float formatting. Critical (2 votes): Associate user data with the active YAML/model buffer instead of global storage. Critical (1 vote): Add the direct <vector> include. |
radio/src/dataconstants.h |
Defines the user-data capacity. |
companion/src/firmwares/modeldata.h |
Adds Companion user-data storage. |
companion/src/firmwares/modeldata.cpp |
Copies and clears Companion user data. |
companion/src/firmwares/edgetx/yaml_modeldata.cpp |
Adds Companion YAML encode/decode support. |
companion/src/constants.h |
Defines the Companion user-data capacity. |
Suppressed comments (6)
radio/src/datastructs_model.cpp:302
r_userdata_key()calls this setter while YAML is being read, so every loaded entry invokesstorageDirty(EE_MODEL).readModelYaml()is also used by read-only model-list scans and temporary model edits; merely inspecting another model can therefore mark the current model dirty and trigger an unnecessary model save. YAML deserialization should use a no-dirty insertion path (or suppress dirty tracking while loading).
storageDirty(EE_MODEL);
radio/src/datastructs_private.h:778
- The new string storage uses C-string construction, so a Lua/YAML string containing
\x00is truncated at the first NUL. The YAML callbacks andyaml_output_string()use the same C-string semantics, losing the byte and everything after it despite the stated support for any ASCII character. Carry explicit string lengths through storage, parsing, and writing.
UserData(const char* k, const char* v, UDType t) { key = k; value = v; type = t; }
radio/src/lua/api_model.cpp:2040
- This second normalization path repeats the app-name collision:
getAllUserData("A|B")is treated asgetAllUserData("A_B"). Even ifgetUDKeyrejects the character, this API would still expose the valid app's entries; use the same rejection/validation here rather than rewriting the name.
if (lua_gettop(L) >= 2) {
s = luaL_checkstring(L, 1);
strReplaceAll(s, "|", "_");
s += "|";
radio/src/lua/api_model.cpp:2037
model.getAllUserData(app)receives one argument, but both the type check and the filtering branch requirelua_gettop(L) >= 2. The documented one-argument form therefore returns every entry, with compositeapp|keynames, instead of filtering by app. Use>= 1for this optional argument in both checks.
if (lua_gettop(L) >= 2 && lua_type(L, 1) != LUA_TSTRING) return 1;
bool matchApp = false;
std::string s;
if (lua_gettop(L) >= 2) {
radio/src/storage/yaml/yaml_datastructs_funcs.cpp:2945
- The parser passes a length, but assigning
valas a C string discards it at the first embedded NUL. Thus a YAML value such as"A\x00B"cannot be loaded intact; preserve the decoded length for user-data values and make the corresponding Lua/write path length-aware.
auto tw = reinterpret_cast<YamlTreeWalker*>(user);
auto ud = g_model.getUserData(tw->getElmts(1));
if (ud) {
ud->value = val;
radio/src/storage/yaml/yaml_datastructs_funcs.cpp:2925
- YAML mapping member order is not semantic, but these callbacks only create the entry in
r_userdata_key. Iftypeorvalueappears beforekey, the lookup is null and that field is silently ignored, leaving the default type/value. Resolve or allocate the entry by array index independently of which child field is encountered first.
auto ud = g_model.getUserData(tw->getElmts(1));
if (ud) {
ud->type = (UDType)yaml_parse_enum(enum_UDType, val, val_len);
} else {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+324
to
+326
| bool ModelData::setUserData(const char* key, float num) | ||
| { | ||
| return setUD(key, std::to_string(num).c_str(), UD_FLOAT); |
|
|
||
| // User Data store. Each entry is a key/value pair. | ||
| // Can be used by Lua scripts to save configuration data. | ||
| std::vector<UserData> userData; |
|
|
||
| // User Data store. Each entry is a key/value pair. | ||
| // Can be used by Lua scripts to save configuration data. | ||
| std::vector<UserData> userData; |
| #if defined(YAML_GENERATOR) | ||
| CUST_ATTR(key, r_userdata_key, w_userdata_key); | ||
| CUST_ATTR(type, r_userdata_type, w_userdata_type); | ||
| CUST_ATTR(value, r_userdata_value, w_userdata_value); |
Comment on lines
+2896
to
+2898
| auto ud = g_model.getUserData(tw->getElmts(1)); | ||
| if (ud == nullptr) | ||
| g_model.setUserData(val, ""); |
Comment on lines
121
to
+122
| while(max_len > 0 && *str) { | ||
| if (*str >= 0x20 && *str <= 0x7E && *str != '"') { | ||
| if (*str >= 0x20 && *str <= 0x7E && *str != '"' && *str != '\\') { |
| } | ||
|
|
||
| /** | ||
| * @brief Replace all occurances of <from> to <to> in <str> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5760
Add a 'userData' section to the model YAML file to store key/value data entries.
Can be used by any Lua script to save miscellaneous data, or share data between scripts.
LTO has been enabled on more targets
Notes:
Lua API:
Scripts must supply both an app name and key for each entry. These are combined internally to build the user data key. The app name cannot contain the '|' character.
Sample script (stand alone tool):
TODO:
Summary by CodeRabbit
Summary of changes
New Features
Bug Fixes
Enhancements