feat(firmware): add 'user data' section to model yaml for Lua scripts#7562
feat(firmware): add 'user data' section to model yaml for Lua scripts#7562philmoz 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)
✨ 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
| 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.
| 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.
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.
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
| else { | ||
| char ch = toupper(*c); | ||
| if (ch >= 'A' && ch <= 'F') { | ||
| escHexVal = (ch - 'A' + 10) << 4; | ||
| state = ps_ValEsc3; | ||
| break; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Undefined behavior when passing a raw char to toupper.
In C and C++, toupper expects an int representable as an unsigned char or EOF. Passing a negative value (which occurs if *c is a non-ASCII character from a malformed YAML sequence and char is signed) results in undefined behavior. Many standard library implementations use this value as an array index, leading to out-of-bounds memory reads and potential crashes.
radio/src/storage/yaml/yaml_parser.cpp#L245-L251: Replacetoupperwith explicit character range checks for the high nibble.radio/src/storage/yaml/yaml_parser.cpp#L263-L270: Replacetoupperwith explicit character range checks for the low nibble.
You can safely handle both cases without toupper.
🛠️ Proposed fix to remove toupper
For radio/src/storage/yaml/yaml_parser.cpp#L245-L251 (High nibble):
- else {
- char ch = toupper(*c);
- if (ch >= 'A' && ch <= 'F') {
- escHexVal = (ch - 'A' + 10) << 4;
- state = ps_ValEsc3;
- break;
- }
- }
+ else if (*c >= 'A' && *c <= 'F') {
+ escHexVal = (*c - 'A' + 10) << 4;
+ state = ps_ValEsc3;
+ break;
+ }
+ else if (*c >= 'a' && *c <= 'f') {
+ escHexVal = (*c - 'a' + 10) << 4;
+ state = ps_ValEsc3;
+ break;
+ }For radio/src/storage/yaml/yaml_parser.cpp#L263-L270 (Low nibble):
- else {
- char ch = toupper(*c);
- if (ch >= 'A' && ch <= 'F') {
- escHexVal |= (ch - 'A' + 10);
- scratch_buf += escHexVal;
- state = ps_ValQuo;
- break;
- }
- }
+ else if (*c >= 'A' && *c <= 'F') {
+ escHexVal |= (*c - 'A' + 10);
+ scratch_buf += escHexVal;
+ state = ps_ValQuo;
+ break;
+ }
+ else if (*c >= 'a' && *c <= 'f') {
+ escHexVal |= (*c - 'a' + 10);
+ scratch_buf += escHexVal;
+ state = ps_ValQuo;
+ break;
+ }📝 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.
| else { | |
| char ch = toupper(*c); | |
| if (ch >= 'A' && ch <= 'F') { | |
| escHexVal = (ch - 'A' + 10) << 4; | |
| state = ps_ValEsc3; | |
| break; | |
| } | |
| else if (*c >= 'A' && *c <= 'F') { | |
| escHexVal = (*c - 'A' + 10) << 4; | |
| state = ps_ValEsc3; | |
| break; | |
| } | |
| else if (*c >= 'a' && *c <= 'f') { | |
| escHexVal = (*c - 'a' + 10) << 4; | |
| state = ps_ValEsc3; | |
| break; | |
| } |
📍 Affects 1 file
radio/src/storage/yaml/yaml_parser.cpp#L245-L251(this comment)radio/src/storage/yaml/yaml_parser.cpp#L263-L270
🤖 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_parser.cpp` around lines 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.
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
|
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 |
|
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). |
|
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 |
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. |
|
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
| 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.
|
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. |
Fix firmware YAML parser to correctly handle Companion generated YAML files. Minor AI suggested changes.
162ac16 to
ed7d162
Compare
4 participants
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.
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