-
-
Notifications
You must be signed in to change notification settings - Fork 80
Implement support for module symbols #475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughAdds symbol-resolution infrastructure and related APIs: a new ModuleSymbol struct and macros, Module and ModuleParent symbol-resolution functions (module_resolve_symbol, module_parent_resolve_symbol), and a getModuleParent() accessor. The lvgl module now supplies a symbol table (lvgl_module_symbols) and its Module instance references it. Many device/platform Module initializers and tests explicitly set the new .symbols field (mostly to nullptr). The global symbol resolver delegates to module_parent_resolve_symbol as a fallback. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (5)
Tactility/Include/Tactility/Tactility.h (1)
41-42: Consider adding documentation for the new public API.Other functions in this namespace (e.g.,
getConfiguration(),getMainDispatcher()) have Doxygen comments describing their purpose, return semantics, and any warnings. For consistency and API discoverability, consider adding similar documentation forgetModuleParent().📝 Suggested documentation
+/** Provides access to the module parent for symbol resolution. + * `@return` the ModuleParent instance + */ ModuleParent& getModuleParent();Modules/lvgl-module/Source/symbols.c (1)
16-18: Minor:lv_color_hexandlv_color_makeare color functions.These functions are categorized under
// lv_objbut are actually color utility functions. Consider moving them to a separate// lv_colorsection for better organization, or renaming the comment to reflect the broader scope.TactilityKernel/Include/tactility/module.h (1)
53-58: Consider usingconstfor the symbols pointer.The
symbolsfield points to a constant array that should not be modified at runtime. Usingconst struct ModuleSymbol*would express this intent and prevent accidental modification.♻️ Suggested change
/** * A list of symbols exported by the module. * Should be terminated by MODULE_SYMBOL_TERMINATOR. * Can be a NULL value. */ - struct ModuleSymbol* symbols; + const struct ModuleSymbol* symbols;TactilityC/Source/tt_init.cpp (2)
363-365: Minor: Misplaced comment.The
// extern "C"comment appears after the closing brace rather than as a comment on the closing brace itself. Consider moving it to be inline with the brace.♻️ Suggested formatting
-} - -// extern "C" +} // extern "C"
56-57: Remove redundant forward declaration -module.his already included transitively.The forward declaration of
module_parent_resolve_symbolis redundant.<Tactility/Tactility.h>(line 52) already includes<tactility/module.h>on its line 4, which declares this function.♻️ Suggested change
`#include` <Tactility/Tactility.h> `#include` <vector> -bool module_parent_resolve_symbol(ModuleParent* pParent, const char* name, uintptr_t* pInt); extern "C" {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
TactilityKernel/Source/module.cpp (1)
45-58: Race condition fix confirmed - mutex synchronization is now in place.The mutex is correctly acquired before iterating over
data->modules(line 47) and released on both the success path (line 52) and the fallback path (line 56). This addresses the previously identified data race.However, there's no null check for
databefore dereferencing. Ifmodule_parent_constructwas never called,module_parent_privatecould be null.💡 Optional: Add defensive null check
bool module_parent_resolve_symbol(ModuleParent* parent, const char* symbol_name, uintptr_t* symbol_address) { auto* data = static_cast<ModuleParentPrivate*>(parent->module_parent_private); + if (data == nullptr) return false; mutex_lock(&data->mutex);
| DEFINE_MODULE_SYMBOL(lv_anim_start), | ||
| DEFINE_MODULE_SYMBOL(lv_anim_path_ease_in_out), | ||
| DEFINE_MODULE_SYMBOL(lv_anim_path_linear), | ||
| DEFINE_MODULE_SYMBOL(lv_obj_is_valid), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate symbol entry: lv_obj_is_valid is already defined on line 50.
This duplicate entry should be removed to avoid symbol table bloat and potential confusion during symbol resolution.
🔧 Proposed fix
DEFINE_MODULE_SYMBOL(lv_anim_path_ease_in_out),
DEFINE_MODULE_SYMBOL(lv_anim_path_linear),
- DEFINE_MODULE_SYMBOL(lv_obj_is_valid),
MODULE_SYMBOL_TERMINATOR
};📝 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.
| DEFINE_MODULE_SYMBOL(lv_obj_is_valid), | |
| DEFINE_MODULE_SYMBOL(lv_anim_path_ease_in_out), | |
| DEFINE_MODULE_SYMBOL(lv_anim_path_linear), | |
| MODULE_SYMBOL_TERMINATOR | |
| }; |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.