Refactor LVGL code into kernel module #472
Conversation
📝 WalkthroughWalkthroughRemoved numerous 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Tactility/Source/service/ServiceRegistration.cpp (1)
55-61:⚠️ Potential issue | 🟡 MinorPre-existing bug: Wrong mutex used for
service_instance_mapaccess.This function accesses
service_instance_mapbut locksmanifest_mutexinstead ofinstance_mutex. Other functions accessingservice_instance_map(e.g.,startServiceat lines 75-77,stopServiceat lines 116-118) correctly useinstance_mutex.This isn't introduced by this PR but is worth fixing while touching this code.
🔒 Proposed fix
static std::shared_ptr<ServiceInstance> findServiceInstanceById(const std::string& id) { - manifest_mutex.lock(); + instance_mutex.lock(); auto iterator = service_instance_map.find(id); auto service = iterator != service_instance_map.end() ? iterator->second : nullptr; - manifest_mutex.unlock(); + instance_mutex.unlock(); return service; }Tactility/Source/service/gps/GpsService.cpp (1)
20-31:⚠️ Potential issue | 🔴 CriticalFix null pointer dereference in
removeGpsDevice.
findGpsRecord()returnsnullptrwhen a device is not found (line 31), butremoveGpsDevice()dereferences this pointer without checking (line 54:stopGpsDevice(*record)). If a non-existent device is removed—such as during a double-remove, stale reference, or API misuse—this causes a crash. Add a null check inremoveGpsDevice()before dereferencing, or enforce the precondition that only registered devices are passed to this method.Tactility/Include/Tactility/app/AppRegistration.h (1)
17-21:⚠️ Potential issue | 🟡 MinorClarify the nullable return contract.
The implementation can still returnnullptrwhen no manifest is found, but the header no longer conveys that. Either enforce a non-null contract or document thenullptrcase to prevent unchecked derefs.📝 Suggested doc update
/** Find an application manifest by its id * `@param`[in] id the manifest id - * `@return` the application manifest if it was found + * `@return` the application manifest if it was found, otherwise nullptr */ std::shared_ptr<AppManifest> findAppManifestById(const std::string& id);
🧹 Nitpick comments (14)
Tactility/Source/service/statusbar/Statusbar.cpp (1)
93-93: Consider adding a brief doc comment to document nullable return.The
_Nullableannotation was removed, but this function still returnsnullptrin two cases (lines 105, 110). The call site atupdatePowerStatusIcon()correctly handles this with a null check, so there's no functional issue.Since the AI summary mentions that documentation comments should be added where nullable annotations are removed, consider adding a brief comment to preserve this intent for future maintainers.
,
📝 Optional: Add documentation comment
+/** `@return` Icon path for current charge level, or nullptr if no power device or metric unavailable */ static const char* getPowerStatusIcon() {Tactility/Source/app/apphub/AppHubApp.cpp (1)
74-77: Remove the unusedcreateAppWidgetfunction.This static function is defined but never called within the codebase. The similarly-named functions in
AppList.cppandAppSettings.cppare separate static functions with their own implementations and usage.Devices/guition-jc3248w535c/Source/Axs15231b/Axs15231bDisplay.h (1)
121-123: Consider adding documentation for nullable returns.These public getters can return null/empty values:
getLvglDisplay()returnsnullptrbeforestartLvgl()is called or if it fails.getTouchDevice()returns an emptyshared_ptrif touch is not configured.getDisplayDriver()may return an emptyshared_ptr.With
_Nullableannotations removed, callers lose visibility into the nullable contract. The PR summary mentions documentation comments are added to preserve this information, but none are present here.📝 Suggested documentation
- lv_display_t* getLvglDisplay() const override { return lvglDisplay; } + /** `@return` LVGL display handle, or nullptr if LVGL not started */ + lv_display_t* getLvglDisplay() const override { return lvglDisplay; } - std::shared_ptr<tt::hal::touch::TouchDevice> getTouchDevice() override { return configuration->touch; } + /** `@return` Touch device, or empty shared_ptr if not configured */ + std::shared_ptr<tt::hal::touch::TouchDevice> getTouchDevice() override { return configuration->touch; } - std::shared_ptr<tt::hal::display::DisplayDriver> getDisplayDriver() override; + /** `@return` Display driver, or empty shared_ptr if not available */ + std::shared_ptr<tt::hal::display::DisplayDriver> getDisplayDriver() override;Also applies to: 135-135
Tactility/Private/Tactility/app/AppInstance.h (1)
35-75: Clarify the nullability contract forparameters.With
_Nullableremoved,parameterslooks non-nullable, but the no-arg ctor still leaves itnullptr, sogetParameters()can still return null. Either keep the “optional” contract explicit for callers or enforce non-null by default-initializing to an emptyBundle.Optional: enforce non-null by default
- std::shared_ptr<const Bundle> parameters; + std::shared_ptr<const Bundle> parameters = std::make_shared<Bundle>();Tactility/Include/Tactility/file/FileLock.h (1)
14-18: Clarify the nullable contract now that_Nullableis removed.The doc still says the function may return
nullptr, but the signature no longer encodes that. Consider expressing optionality in the type (e.g.,std::optional<std::shared_ptr<Lock>>) or, if you intend a non-null contract, return a non-null lock and update the doc accordingly.♻️ Possible refactor (explicit optional return)
+#include <optional> ... -std::shared_ptr<Lock> findLock(const std::string& path); +std::optional<std::shared_ptr<Lock>> findLock(const std::string& path);TactilityCore/Source/file/File.cpp (1)
92-96: Document nullable callbacks now that annotations are removed.
The implementation still acceptsnullptr, so please add a brief note inTactilityCore/Include/Tactility/file/File.hto preserve that contract.Tactility/Source/hal/usb/Usb.cpp (1)
23-23: Add a brief note thatgetCard()can return nullptr.
This preserves the contract after removing_Nullableand helps callers avoid unsafe assumptions.Tests/Tactility/Main.cpp (1)
71-76: Consider portability of__assert_fail.The
__assert_failfunction is a GCC/glibc internal that may not be available on all platforms (e.g., MSVC, some embedded toolchains). If the tests need to run in non-GNU environments, consider usingassert(false)or a platform-abstracted assertion macro instead.💡 Portable alternative
void vAssertCalled(unsigned long line, const char* const file) { - __assert_fail("assert failed", file, line, ""); + fprintf(stderr, "Assertion failed at %s:%lu\n", file, line); + abort(); }Drivers/EspLcdCompat/Source/EspLcdTouch.h (1)
14-14: Consider documenting nullable state forlvglDevicein this concrete class.
getLvglIndev()can returnnullptruntilstartLvgl()succeeds; adding a short note here would align with the baseTouchDevicecontract and reduce misuse risk for concrete callers.💡 Possible doc addition
- lv_indev_t* lvglDevice = nullptr; + // May remain nullptr until startLvgl() succeeds. + lv_indev_t* lvglDevice = nullptr; @@ - lv_indev_t* getLvglIndev() final { return lvglDevice; } + /** May return nullptr if not started */ + lv_indev_t* getLvglIndev() final { return lvglDevice; }Also applies to: 39-39
Devices/simulator/Source/hal/SdlTouch.h (1)
30-30: Consider adding a documentation comment for potential nullptr return.For consistency with other files in this PR (e.g.,
ButtonControl.hwhich adds/** Could return nullptr if not started */), consider adding a similar comment here sincegetLvglIndev()can returnnullptrbeforestartLvgl()is called or if it fails.📝 Suggested documentation
+ /** Could return nullptr if not started */ lv_indev_t* getLvglIndev() override { return handle; }Modules/hal-device-module/CMakeLists.txt (1)
3-4: Consider noting the GLOB_RECURSE limitation.Using
GLOB_RECURSEfor source files is convenient but won't automatically detect new source files added to the directory—CMake must be re-run. This is a common pattern and acceptable, but worth being aware of during development.Modules/lvgl-module/README.md (1)
1-8: Consider using a proper Markdown heading for the warning.The static analysis tool flagged that bold emphasis is used where a heading would be more appropriate (MD036). Using a heading improves document structure and accessibility.
📝 Suggested fix
-**WARNING: This module contains deprecated code** +# ⚠️ WARNING: This module contains deprecated codeModules/lvgl-module/Include/tactility/lvgl_module.h (1)
1-12: Consider adding a forward declaration or include forstruct Module.The
extern struct Module lvgl_module;declaration works as an implicit forward declaration in C, but for clarity and consistency, consider either:
- Adding an explicit forward declaration:
struct Module;- Or including the header that defines
ModuleThis would make the dependency on the
Moduletype explicit for readers.Modules/lvgl-module/CMakeLists.txt (1)
19-21: Consider using PRIVATE instead of PUBLIC for warning suppression.The
-Wno-unused-variableflag is applied withPUBLICvisibility, which will propagate this warning suppression to all targets that link against this component. This may unintentionally hide legitimate warnings in dependent code.Proposed fix
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU") - target_compile_options(${COMPONENT_LIB} PUBLIC -Wno-unused-variable) + target_compile_options(${COMPONENT_LIB} PRIVATE -Wno-unused-variable) endif ()
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tactility/Source/service/loader/Loader.cpp (1)
300-308:⚠️ Potential issue | 🟡 MinorAPI contract mismatch: function can still return
nullptr.The
_Nullablequalifier was removed, but the implementation at lines 303-304 still returnsnullptrwhenappStack.empty(). This creates a mismatch between the declared API contract (implying non-null) and the runtime behavior.Additionally, existing code at line 282 already dereferences this without a null check:
const auto& id = getCurrentAppContext()->getManifest().appId;Consider either:
- Adding documentation that explicitly states null can be returned, or
- Throwing an exception instead of returning null if the intent is truly non-nullable
🧹 Nitpick comments (2)
TactilityKernel/Include/tactility/time.h (1)
73-79: Inconsistent cast styles within the same function.Line 75 uses a C-style cast
(TickType_t), while line 77 still usesstatic_cast<TickType_t>. For consistency within this function (and across the file), consider using the same cast style in both branches.♻️ Suggested fix for consistency
Either use C-style casts in both branches:
`#if` configTICK_RATE_HZ == 1000 return (TickType_t)milliSeconds; `#else` - return static_cast<TickType_t>(((float)configTICK_RATE_HZ) / 1000.0f * (float)milliSeconds); + return (TickType_t)(((float)configTICK_RATE_HZ) / 1000.0f * (float)milliSeconds); `#endif`Or use C++ style casts in both:
`#if` configTICK_RATE_HZ == 1000 - return (TickType_t)milliSeconds; + return static_cast<TickType_t>(milliSeconds); `#else` return static_cast<TickType_t>(((float)configTICK_RATE_HZ) / 1000.0f * (float)milliSeconds); `#endif`Tactility/Source/Tactility.cpp (1)
374-375: Checkmodule_set_parentresult forlvgl_module.
Align with the earliercheck(...)calls to avoid silent parenting failures.✅ Proposed guard
- module_set_parent(&lvgl_module, &tactility_module_parent); + check(module_set_parent(&lvgl_module, &tactility_module_parent) == ERROR_NONE);
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
TactilityKernel/Include/tactility/time.h (1)
57-75: Consider preserving C++ cast safety (or guard with a macro).
If this header is still compiled as C++, the C-style casts reduce type-safety and can mask narrowing/const-cast issues. Please confirm intended C vs C++ usage; if C++ is intended, revert tostatic_castor wrap casts in a C/C++-aware macro.♻️ Example (C++ builds only)
- return ((int64_t)ts.tv_sec * 1000000LL) + (ts.tv_nsec / 1000); + return (static_cast<int64_t>(ts.tv_sec) * 1000000LL) + (ts.tv_nsec / 1000); - return ((int64_t)tv.tv_sec * 1000000LL) + tv.tv_usec; + return (static_cast<int64_t>(tv.tv_sec) * 1000000LL) + tv.tv_usec; - return (uint64_t)seconds * 1000U / portTICK_PERIOD_MS; + return static_cast<uint64_t>(seconds) * 1000U / portTICK_PERIOD_MS; - return (TickType_t)milliSeconds; + return static_cast<TickType_t>(milliSeconds);
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.