Tactility.py 3.0.0 and SDK updates for 0.7.0-dev #19
Conversation
|
Warning Rate limit exceeded@KenVanHoeylandt has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 20 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughManifests: app manifests updated to target sdk Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
♻️ Duplicate comments (3)
Apps/TwoEleven/tactility.py (1)
1-693: Duplicate of sharedtactility.pyscript.This file is identical to the other
tactility.pyfiles in this PR. The same issues and refactoring opportunities apply as noted inApps/GPIO/tactility.pyandApps/GraphicsDemo/tactility.py.Apps/Calculator/tactility.py (1)
1-693: Duplicate of sharedtactility.pyscript.This file is identical to the other
tactility.pyfiles in this PR. The same issues and refactoring opportunities apply as previously noted.Apps/Diceware/tactility.py (1)
1-693: Duplicate of sharedtactility.pyscript.This file is identical to the other
tactility.pyfiles in this PR. The same issues and refactoring opportunities apply as previously noted.
🧹 Nitpick comments (7)
Apps/SerialConsole/tactility.py (1)
296-298: Consider verifying zip contents before extraction.While
extractall()on trusted CDN content is low risk, a malformed or compromised archive could exploit path traversal. Consider validating member paths if security requirements increase.Apps/HelloWorld/tactility.py (1)
1-30: Consider extracting shared build logic into a common module.The
tactility.pyfiles across apps (HelloWorld, SerialConsole, Calculator, Diceware, GraphicsDemo) appear to be nearly identical copies. Extracting the shared logic into a common package would reduce maintenance burden and ensure consistent behavior across all apps.Apps/GPIO/tactility.py (2)
11-11: Unused import:dataclassThe
dataclassimport is added but never used in the file. Remove it to avoid confusion.🔎 Proposed fix
-from dataclasses import dataclass
285-285: Use idiomaticnot ininstead ofnot x in y.Per static analysis (E713), the Pythonic way is
platform not in sdk_platforms.🔎 Proposed fix
- if not platform in sdk_platforms: + if platform not in sdk_platforms:Apps/GraphicsDemo/tactility.py (1)
1-693: Major code duplication: This file is an exact copy ofApps/GPIO/tactility.py.All 5 apps in this PR (GPIO, GraphicsDemo, TwoEleven, Calculator, Diceware) contain identical copies of
tactility.py(~693 lines each). Consider extracting this into a shared module or a symlink to a single source of truth to avoid maintenance burden and drift.The same issues identified in
Apps/GPIO/tactility.pyapply here:
- Unused
dataclassimport (line 11)- File handle leaks in
read_sdk_json()(lines 110-113) andsdk_download()(lines 282-283)- Non-idiomatic
not platform inpattern (line 285)Apps/SerialConsole/main/Source/ConsoleView.h (2)
169-175: Consider usingstd::make_sharedfor consistency.The lambda-based thread creation and priority setting look correct. However, you're using
std::make_uniqueto assign to astd::shared_ptr<tt::Thread>. While this works via implicit conversion, usingstd::make_sharedis more consistent and slightly more efficient (single allocation).Suggested change
- uartThread = std::make_unique<tt::Thread>( + uartThread = std::make_shared<tt::Thread>( "SerConsUart", 4096, [this] { return uartThreadMain(); } );
215-221: Same suggestion: usestd::make_sharedfor consistency.Suggested change
- viewThread = std::make_unique<tt::Thread>( + viewThread = std::make_shared<tt::Thread>( "SerConsView", 4096, [this] { return viewThreadMain(); } );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
Apps/Calculator/manifest.propertiesApps/Calculator/tactility.pyApps/Diceware/CMakeLists.txtApps/Diceware/main/Source/Diceware.cppApps/Diceware/main/Source/Diceware.hApps/Diceware/manifest.propertiesApps/Diceware/tactility.pyApps/GPIO/main/Source/Gpio.cppApps/GPIO/main/Source/Gpio.hApps/GPIO/manifest.propertiesApps/GPIO/tactility.pyApps/GraphicsDemo/main/Include/drivers/DisplayDriver.hApps/GraphicsDemo/main/Source/Application.cppApps/GraphicsDemo/manifest.propertiesApps/GraphicsDemo/tactility.pyApps/HelloWorld/manifest.propertiesApps/HelloWorld/tactility.pyApps/SerialConsole/main/Source/ConsoleView.hApps/SerialConsole/manifest.propertiesApps/SerialConsole/tactility.pyApps/TwoEleven/manifest.propertiesApps/TwoEleven/tactility.pyLibraries/TactilityCpp/Include/TactilityCpp/Lock.hLibraries/TactilityCpp/Include/TactilityCpp/LvglLock.hLibraries/TactilityCpp/Include/TactilityCpp/Mutex.hLibraries/TactilityCpp/Include/TactilityCpp/Thread.hLibraries/TactilityCpp/Include/TactilityCpp/Uart.hLibraries/TactilityCpp/Source/Lock.cpp
💤 Files with no reviewable changes (5)
- Libraries/TactilityCpp/Source/Lock.cpp
- Apps/Diceware/CMakeLists.txt
- Libraries/TactilityCpp/Include/TactilityCpp/Thread.h
- Libraries/TactilityCpp/Include/TactilityCpp/Lock.h
- Libraries/TactilityCpp/Include/TactilityCpp/Mutex.h
🧰 Additional context used
🧬 Code graph analysis (9)
Apps/GraphicsDemo/tactility.py (1)
Apps/Calculator/tactility.py (7)
get_sdk_url(144-146)sdk_exists(148-150)get_sdk_dir(115-127)should_update_tool_json(152-162)update_tool_json(164-168)download_file(55-75)print_error(80-81)
Apps/TwoEleven/tactility.py (1)
Apps/Calculator/tactility.py (7)
get_sdk_url(144-146)sdk_exists(148-150)get_sdk_dir(115-127)should_update_tool_json(152-162)update_tool_json(164-168)download_file(55-75)print_error(80-81)
Apps/GPIO/main/Source/Gpio.h (1)
Apps/GPIO/main/Source/Gpio.cpp (4)
onTimer(50-55)onTimer(50-50)createGpioRowWrapper(40-46)createGpioRowWrapper(40-40)
Apps/Diceware/tactility.py (1)
Apps/Calculator/tactility.py (6)
get_sdk_url(144-146)sdk_exists(148-150)get_sdk_dir(115-127)should_update_tool_json(152-162)update_tool_json(164-168)download_file(55-75)
Apps/Calculator/tactility.py (1)
Apps/Diceware/tactility.py (7)
get_sdk_url(144-146)sdk_exists(148-150)get_sdk_dir(115-127)should_update_tool_json(152-162)update_tool_json(164-168)download_file(55-75)print_error(80-81)
Apps/Diceware/main/Source/Diceware.h (2)
Libraries/TactilityCpp/Include/TactilityCpp/App.h (1)
App(6-13)Apps/Diceware/main/Source/Diceware.cpp (4)
onClickGenerate(92-100)onClickGenerate(92-92)jobMain(55-67)jobMain(55-55)
Libraries/TactilityCpp/Include/TactilityCpp/LvglLock.h (1)
Apps/GraphicsDemo/main/Include/drivers/DisplayDriver.h (1)
unlock(30-36)
Apps/HelloWorld/tactility.py (1)
Apps/Calculator/tactility.py (7)
get_sdk_url(144-146)sdk_exists(148-150)get_sdk_dir(115-127)should_update_tool_json(152-162)update_tool_json(164-168)download_file(55-75)print_error(80-81)
Apps/GPIO/tactility.py (1)
Apps/Calculator/tactility.py (7)
get_sdk_url(144-146)sdk_exists(148-150)get_sdk_dir(115-127)should_update_tool_json(152-162)update_tool_json(164-168)download_file(55-75)print_error(80-81)
🪛 Ruff (0.14.10)
Apps/SerialConsole/tactility.py
285-285: Test for membership should be not in
Convert to not in
(E713)
297-297: Uses of tarfile.extractall()
(S202)
384-384: subprocess call: check for execution of untrusted input
(S603)
415-415: subprocess call: check for execution of untrusted input
(S603)
Apps/GraphicsDemo/tactility.py
285-285: Test for membership should be not in
Convert to not in
(E713)
297-297: Uses of tarfile.extractall()
(S202)
384-384: subprocess call: check for execution of untrusted input
(S603)
415-415: subprocess call: check for execution of untrusted input
(S603)
Apps/TwoEleven/tactility.py
285-285: Test for membership should be not in
Convert to not in
(E713)
297-297: Uses of tarfile.extractall()
(S202)
384-384: subprocess call: check for execution of untrusted input
(S603)
415-415: subprocess call: check for execution of untrusted input
(S603)
Apps/Diceware/tactility.py
285-285: Test for membership should be not in
Convert to not in
(E713)
297-297: Uses of tarfile.extractall()
(S202)
384-384: subprocess call: check for execution of untrusted input
(S603)
415-415: subprocess call: check for execution of untrusted input
(S603)
Apps/Calculator/tactility.py
285-285: Test for membership should be not in
Convert to not in
(E713)
297-297: Uses of tarfile.extractall()
(S202)
384-384: subprocess call: check for execution of untrusted input
(S603)
415-415: subprocess call: check for execution of untrusted input
(S603)
Apps/HelloWorld/tactility.py
285-285: Test for membership should be not in
Convert to not in
(E713)
297-297: Uses of tarfile.extractall()
(S202)
Apps/GPIO/tactility.py
285-285: Test for membership should be not in
Convert to not in
(E713)
297-297: Uses of tarfile.extractall()
(S202)
384-384: subprocess call: check for execution of untrusted input
(S603)
415-415: subprocess call: check for execution of untrusted input
(S603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build (SerialConsole)
- GitHub Check: Build (Calculator)
- GitHub Check: Build (TwoEleven)
- GitHub Check: Build (GPIO)
- GitHub Check: Build (GraphicsDemo)
- GitHub Check: Build (HelloWorld)
- GitHub Check: Build (Diceware)
🔇 Additional comments (32)
Libraries/TactilityCpp/Include/TactilityCpp/LvglLock.h (3)
3-6: LGTM! Clean migration to tt:: namespace.The inheritance from
tt::Lockand the updated include path align with the SDK 0.7.0-dev API changes. The class correctly usesfinalsince it's a concrete adapter with no subclassing intent.Note:
TickType_tandtt::kernel::MAX_TICKS(used on line 10) appear to be transitively included via<Tactility/Lock.h>. If this header is intended to be self-contained, consider explicitly including the kernel types header.
10-12: Good fix: lock result is now properly propagated.Returning the actual result of
tt_lvgl_lock(timeout)enables callers to detect lock acquisition failures. This is a correctness improvement over always returningtrue.
15-17: LGTM!The
voidreturn type is consistent with other unlock implementations in the codebase (e.g.,DisplayDriver::unlock()).Apps/GraphicsDemo/main/Source/Application.cpp (2)
69-69: LGTM! The migration fromtt_kernel_delay_ticks(1)tott::kernel::delayTicks(1)is correct and complete. No remaining old API calls detected in the codebase.
5-5: LGTM! The migration from the C-style API (<tt_kernel.h>andtt_kernel_delay_ticks()) to the modern C++ namespaced API (<Tactility/kernel/Kernel.h>andtt::kernel::delayTicks()) is correct and complete. The header is provided by the external TactilitySDK dependency and no old API references remain in the codebase.Apps/Calculator/manifest.properties (1)
4-5: Consistent manifest updates aligned with SDK 0.7.0-dev and expanded platform support.The Calculator app manifest has been properly updated with:
- SDK target bumped to 0.7.0-dev
- Platform coverage expanded to include esp32c6 and esp32p4
- App version incremented (0.2.0 → 0.3.0, versionCode 2 → 3)
These changes are consistent with the PR objectives and follow the same pattern across other app manifests.
Please verify that the newly added platforms (esp32c6, esp32p4) are fully supported by Tactility SDK 0.7.0-dev and that the Calculator app has been tested or is compatible with these targets.
Also applies to: 8-9
Apps/Diceware/manifest.properties (1)
4-5: Consistent with broader manifest updates.The Diceware app manifest follows the same coordinated update pattern as other apps in the PR (SDK 0.7.0-dev, expanded platforms, version increments).
Also applies to: 8-9
Apps/GPIO/manifest.properties (1)
4-5: Consistent coordinated update across all reviewed apps.The GPIO app manifest maintains consistency with the Calculator and Diceware manifests reviewed. All three apps uniformly target SDK 0.7.0-dev, include the expanded platform list (esp32, esp32s3, esp32c6, esp32p4), and increment their versions accordingly.
Also applies to: 8-9
Apps/GraphicsDemo/manifest.properties (2)
4-5: Verify SDK version choice: 0.7.0-dev in production manifests.The manifest targets SDK 0.7.0-dev, which is a development/prerelease version. If this PR represents a stable release, consider whether a stable SDK version (e.g., 0.7.0) should be targeted instead to avoid compatibility or support issues with a pre-release toolchain.
4-9: Platform expansion and version bump look good.The updates to target platforms (esp32, esp32s3, esp32c6, esp32p4) and version bump (0.3.0 / versionCode 3) align with the broader SDK upgrade pattern. The manifest structure is valid and consistent.
Apps/TwoEleven/manifest.properties (2)
4-5: Consistent SDK and platform updates, but verify 0.7.0-dev intent.The SDK and platform targets match the pattern applied across other app manifests. However, the same concern applies: confirm that 0.7.0-dev is the intended target for a stable release.
11-11: Description formatting uses escaped newlines—verify rendering.The description field now includes escaped newlines (\n) to format the text across multiple lines. Ensure the manifest parser and UI correctly interpret \n as line breaks rather than literal characters.
Apps/HelloWorld/manifest.properties (1)
4-5: Aligned with the SDK and platform update pattern.The manifest updates follow the same pattern as other app manifests in the PR, targeting SDK 0.7.0-dev and expanding platform support. This consistency is good for maintainability.
Apps/SerialConsole/manifest.properties (1)
4-9: LGTM!The manifest updates are consistent: SDK version bump to 0.7.0-dev, expanded platform support (esp32c6, esp32p4), and appropriate version increments align with the coordinated upgrade across the project.
Apps/Diceware/main/Source/Diceware.h (2)
18-18: LGTM!The migration from a raw thread handle to
std::unique_ptr<tt::Thread>is a solid improvement for automatic resource management and exception safety.
26-26: Clean refactor to member function pattern.Changing
jobMainfrom a static function takingvoid*to a parameterless member function allows direct access to class members and works naturally with lambda-based thread construction. This eliminates error-prone casting.Apps/GraphicsDemo/main/Include/drivers/DisplayDriver.h (1)
5-5: LGTM!The migration to
TickType_tandtt::kernel::MAX_TICKSaligns with FreeRTOS conventions and the broader namespace modernization in this PR.Also applies to: 26-26
Apps/Diceware/main/Source/Diceware.cpp (2)
69-84: LGTM!The thread lifecycle is well-managed:
cleanupJob()properly joins before reassignment, preventing dangling threads- Lambda capture of
thisis safe since the thread is always joined before theDicewareobject can be destroyed- Sequential cleanup in
startJob()prevents concurrent job execution
181-182: Good cleanup on hide.Calling
cleanupJob()inonHideensures the background thread is properly joined before the app transitions away, preventing use-after-free scenarios.Libraries/TactilityCpp/Include/TactilityCpp/Uart.h (1)
6-6: LGTM!The migration to
TickType_tis consistent with the FreeRTOS API and the broader type standardization across the codebase. Including<freertos/FreeRTOS.h>provides the necessary type definition.Also applies to: 48-57
Apps/GPIO/tactility.py (1)
16-16: LGTM: Migration fromsdk.jsontotool.jsonand index-driven SDK download.The version bump to 3.0.0 and the refactored SDK download flow using
index.jsonfor platform discovery are well-structured. The API changes (get_sdk_url(version, file),should_update_tool_json(),update_tool_json()) are consistent with the new architecture.Also applies to: 144-146, 152-168, 270-298, 639-641
Apps/SerialConsole/main/Source/ConsoleView.h (4)
13-14: LGTM!The include updates and member type changes to
tt::Threadandtt::RecursiveMutexare consistent with the SDK 0.7.0-dev migration.Also applies to: 28-32
70-73: LGTM!The explicit
lvglLock.lock()/unlock()pattern correctly guards the LVGL UI update.
78-86: Verify the delay calculation units.The API migration to
tt::kernel::getTicks()andtt::kernel::delayTicks()looks correct. However, verify that the delay calculation logic is sound:
time_diffis in ticks- The comparison
time_diff < 500Uand calculation(500U - time_diff) / portTICK_PERIOD_MSassume500Uis in ticksIf
portTICK_PERIOD_MS != 1, the math may be incorrect. For a 500ms loop period, consider:constexpr TickType_t targetDelayTicks = 500U / portTICK_PERIOD_MS; if (time_diff < targetDelayTicks) { tt::kernel::delayTicks(targetDelayTicks - time_diff); }
235-238: LGTM!The thread state checks using
tt::Thread::State::Stoppedbefore callingjoin()are correct and consistent with the new API.Also applies to: 253-256
Apps/GPIO/main/Source/Gpio.cpp (4)
3-4: LGTM: Include updated for new kernel namespace.The include change from the old header to
<Tactility/kernel/Kernel.h>aligns with the SDK 0.7.0-dev API migration.
22-22: LGTM: Constant updated to new namespace.The change from
TT_MAX_TICKStott::kernel::MAX_TICKSis consistent with the new namespaced API.
50-55: LGTM: Clean refactor from static callback to member function.The removal of the
void* contextparameter and direct use of membermutexis cleaner and more type-safe than the previous C-style callback pattern.
57-65: LGTM: Timer API simplified.The C++
tt::Timerinterface withstart()/stop()is cleaner than the previous C-style allocation pattern.Note: There's an asymmetry where
startTask()manages its own mutex locking whilestopTask()relies on the caller (inonHide) to hold the mutex. This is fine given current usage, but consider documenting this expectation or making both methods consistent.Apps/GPIO/main/Source/Gpio.h (3)
7-8: LGTM: Updated to new Tactility header includes.The migration from
<tt_timer.h>and<TactilityCpp/Mutex.h>to the new<Tactility/RecursiveMutex.h>and<Tactility/Timer.h>headers aligns with the SDK modernization.
23-23: LGTM: Method signature updated to non-static.Changing
onTimerfrom static withvoid* contextto a non-static member function is cleaner and leverages the lambda callback pattern established in the timer initialization.
17-20: Verify tt::Timer destructor's callback cancellation guarantee.Members are destroyed in reverse declaration order:
mutex(line 20) is destroyed beforetimer(line 17). TheonTimer()callback (Gpio.cpp:50-55) invokesmutex.lock(), creating a potential use-after-destroy race if the timer destructor doesn't synchronously cancel pending callbacks.This risk can be mitigated by either:
- Reordering members so
timeris declared beforemutex, or- Confirming and documenting that
tt::Timer's destructor guarantees synchronous callback cancellationThe actual severity depends on the Timer implementation from the external Tactility library, which is not available for inspection in this repository.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Apps/GPIO/tactility.py (1)
68-70: File handle leak in download_file.The file opened on line 68 is not closed using a context manager, which can lead to resource leaks.
🔎 Proposed fix
try: response = urllib.request.urlopen(request) - file = open(filepath, mode="wb") - file.write(response.read()) - file.close() + with open(filepath, mode="wb") as file: + file.write(response.read()) return TrueApps/SerialConsole/tactility.py (1)
68-70: File handle leak in download_file.The file is not closed using a context manager.
🔎 Proposed fix
try: response = urllib.request.urlopen(request) - file = open(filepath, mode="wb") - file.write(response.read()) - file.close() + with open(filepath, mode="wb") as file: + file.write(response.read()) return TrueApps/Calculator/tactility.py (1)
68-70: File handle leak in download_file.Use a context manager to ensure the file is properly closed.
🔎 Proposed fix
try: response = urllib.request.urlopen(request) - file = open(filepath, mode="wb") - file.write(response.read()) - file.close() + with open(filepath, mode="wb") as file: + file.write(response.read()) return TrueApps/GraphicsDemo/tactility.py (1)
68-70: File handle leak in download_file.Use a context manager.
🔎 Proposed fix
try: response = urllib.request.urlopen(request) - file = open(filepath, mode="wb") - file.write(response.read()) - file.close() + with open(filepath, mode="wb") as file: + file.write(response.read()) return TrueApps/HelloWorld/tactility.py (1)
68-70: File handle leak in download_file.The file should be closed using a context manager.
🔎 Proposed fix
try: response = urllib.request.urlopen(request) - file = open(filepath, mode="wb") - file.write(response.read()) - file.close() + with open(filepath, mode="wb") as file: + file.write(response.read()) return True
♻️ Duplicate comments (9)
Apps/GPIO/tactility.py (2)
110-113: LGTM! File handle properly managed.The context manager correctly ensures the file is closed after reading. This addresses the previous review concern.
282-283: LGTM! File handle properly managed.Context manager correctly ensures the index.json file is closed after reading. This addresses the previous review concern.
Apps/SerialConsole/tactility.py (2)
110-113: LGTM! Context manager properly closes the file.
282-283: LGTM! Context manager properly closes the file.Apps/Calculator/tactility.py (1)
110-113: LGTM! File handle properly managed with context manager.Apps/GraphicsDemo/tactility.py (1)
110-113: LGTM! Context manager used correctly.Apps/HelloWorld/tactility.py (2)
110-113: LGTM! File properly closed with context manager.
282-283: LGTM! Context manager ensures proper file closure.Apps/TwoEleven/tactility.py (1)
1-693: Duplicate code: This file is identical to other app'stactility.py.This is the same file duplicated across multiple apps. See the recommendation in
Apps/Diceware/tactility.pyreview for consolidation approaches.
🧹 Nitpick comments (12)
Apps/Calculator/main/Source/Calculator.cpp (1)
97-112: Consider adding defensive checks for edge cases.A few minor robustness concerns:
Line 97: Accessing
token[0]on an empty string is undefined behavior. WhileinfixToRPNshouldn't produce empty tokens, a defensive check would be safer.Line 112: When division by zero occurs, no result is pushed, which could leave the stack in an inconsistent state for subsequent operations (though the function returns early if stack underflow occurs later).
🔎 Optional defensive improvements
while (!rpnQueue.empty()) { std::string token = rpnQueue.front(); rpnQueue.pop_front(); + if (token.empty()) continue; + if (isdigit(token[0])) { double d; sscanf(token.c_str(), "%lf", &d); values.push(d); } else if (strchr("+-*/", token[0])) { if (values.size() < 2) return 0; double b = values.top(); values.pop(); double a = values.top(); values.pop(); if (token[0] == '+') values.push(a + b); else if (token[0] == '-') values.push(a - b); else if (token[0] == '*') values.push(a * b); - else if (token[0] == '/' && b != 0) values.push(a / b); + else if (token[0] == '/') values.push(b != 0 ? a / b : 0); } }Apps/GPIO/tactility.py (2)
11-11: Unused import.The
dataclassimport appears to be unused in this file.🔎 Proposed fix
-from dataclasses import dataclass
1-693: Code duplication across tactility.py files.All five tactility.py files in this PR (GPIO, SerialConsole, Calculator, GraphicsDemo, HelloWorld) contain identical code. Consider extracting this into a shared module to improve maintainability and reduce duplication.
Apps/SerialConsole/tactility.py (1)
11-11: Unused import.The
dataclassimport appears to be unused.🔎 Proposed fix
-from dataclasses import dataclassApps/Calculator/tactility.py (1)
11-11: Unused import.The
dataclassimport is not used.🔎 Proposed fix
-from dataclasses import dataclassApps/GraphicsDemo/tactility.py (1)
11-11: Unused import.The
dataclassimport is unused.🔎 Proposed fix
-from dataclasses import dataclassApps/HelloWorld/tactility.py (1)
11-11: Unused import.The
dataclassimport is not used in this file.🔎 Proposed fix
-from dataclasses import dataclassApps/Diceware/tactility.py (3)
11-11: Unused import:dataclassThe
dataclassimport is added but never used in this file. Remove it to keep the imports clean.🔎 Proposed fix
-from dataclasses import dataclass
110-113: Missing error handling for file operations.
read_sdk_json()openstool.jsonwithout handling the case where the file doesn't exist or contains invalid JSON. This could cause cryptic errors.🔎 Proposed fix with error handling
def read_sdk_json(): json_file_path = os.path.join(ttbuild_path, "tool.json") + if not os.path.exists(json_file_path): + exit_with_error(f"SDK config file not found: {json_file_path}. Run the tool again to download it.") with open(json_file_path) as json_file: - return json.load(json_file) + try: + return json.load(json_file) + except json.JSONDecodeError as e: + exit_with_error(f"Invalid JSON in {json_file_path}: {e}")
1-693: Consider consolidating duplicatetactility.pyfiles.This file is nearly identical to
Apps/TwoEleven/tactility.pyandApps/Calculator/tactility.py. Maintaining multiple copies of the same build tool script across apps creates significant maintenance burden and risks divergence.Consider one of these approaches:
- Move
tactility.pyto a shared location and symlink from each app- Publish it as a pip-installable package
- Use git submodules for the shared tooling
Would you like me to open an issue to track this consolidation effort?
Apps/TwoEleven/tactility.py (2)
11-11: Unused import:dataclassSame as in
Apps/Diceware/tactility.py— this import is unused and should be removed.
110-113: Missing error handling for file operations.Same issue as
Apps/Diceware/tactility.py—read_sdk_json()should handle missing file and JSON parse errors gracefully.🔎 Proposed fix with error handling
def read_sdk_json(): json_file_path = os.path.join(ttbuild_path, "tool.json") + if not os.path.exists(json_file_path): + exit_with_error(f"SDK config file not found: {json_file_path}. Run the tool again to download it.") with open(json_file_path) as json_file: - return json.load(json_file) + try: + return json.load(json_file) + except json.JSONDecodeError as e: + exit_with_error(f"Invalid JSON in {json_file_path}: {e}")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
Apps/Calculator/main/CMakeLists.txtApps/Calculator/main/Source/Calculator.cppApps/Calculator/main/Source/Calculator.hApps/Calculator/tactility.pyApps/Diceware/main/CMakeLists.txtApps/Diceware/tactility.pyApps/GPIO/main/CMakeLists.txtApps/GPIO/tactility.pyApps/GraphicsDemo/main/CMakeLists.txtApps/GraphicsDemo/tactility.pyApps/HelloWorld/tactility.pyApps/SerialConsole/main/CMakeLists.txtApps/SerialConsole/tactility.pyApps/TwoEleven/main/CMakeLists.txtApps/TwoEleven/tactility.pyLibraries/Str/CMakeLists.txtLibraries/Str/Include/Str.hLibraries/Str/README.mdLibraries/Str/Source/Str.cpp
💤 Files with no reviewable changes (8)
- Apps/Diceware/main/CMakeLists.txt
- Apps/SerialConsole/main/CMakeLists.txt
- Libraries/Str/README.md
- Libraries/Str/CMakeLists.txt
- Libraries/Str/Include/Str.h
- Apps/GPIO/main/CMakeLists.txt
- Apps/Calculator/main/CMakeLists.txt
- Libraries/Str/Source/Str.cpp
🧰 Additional context used
🧬 Code graph analysis (7)
Apps/Calculator/main/Source/Calculator.h (1)
Apps/Calculator/main/Source/Calculator.cpp (4)
infixToRPN(49-88)infixToRPN(49-49)evaluateRPN(90-117)evaluateRPN(90-90)
Apps/HelloWorld/tactility.py (1)
Apps/Calculator/tactility.py (7)
get_sdk_url(144-146)sdk_exists(148-150)get_sdk_dir(115-127)should_update_tool_json(152-162)update_tool_json(164-168)download_file(55-75)print_error(80-81)
Apps/Diceware/tactility.py (1)
Apps/Calculator/tactility.py (7)
get_sdk_url(144-146)sdk_exists(148-150)get_sdk_dir(115-127)should_update_tool_json(152-162)update_tool_json(164-168)download_file(55-75)print_error(80-81)
Apps/SerialConsole/tactility.py (1)
Apps/Calculator/tactility.py (7)
get_sdk_url(144-146)sdk_exists(148-150)get_sdk_dir(115-127)should_update_tool_json(152-162)update_tool_json(164-168)download_file(55-75)print_error(80-81)
Apps/GPIO/tactility.py (3)
Apps/Calculator/tactility.py (6)
get_sdk_url(144-146)sdk_exists(148-150)get_sdk_dir(115-127)should_update_tool_json(152-162)update_tool_json(164-168)download_file(55-75)Apps/HelloWorld/tactility.py (6)
get_sdk_url(144-146)sdk_exists(148-150)get_sdk_dir(115-127)should_update_tool_json(152-162)update_tool_json(164-168)download_file(55-75)Apps/SerialConsole/tactility.py (6)
get_sdk_url(144-146)sdk_exists(148-150)get_sdk_dir(115-127)should_update_tool_json(152-162)update_tool_json(164-168)download_file(55-75)
Apps/TwoEleven/tactility.py (1)
Apps/Calculator/tactility.py (7)
get_sdk_url(144-146)sdk_exists(148-150)get_sdk_dir(115-127)should_update_tool_json(152-162)update_tool_json(164-168)download_file(55-75)print_error(80-81)
Apps/GraphicsDemo/tactility.py (1)
Apps/Calculator/tactility.py (7)
get_sdk_url(144-146)sdk_exists(148-150)get_sdk_dir(115-127)should_update_tool_json(152-162)update_tool_json(164-168)download_file(55-75)print_error(80-81)
🪛 Ruff (0.14.10)
Apps/HelloWorld/tactility.py
297-297: Uses of tarfile.extractall()
(S202)
384-384: subprocess call: check for execution of untrusted input
(S603)
415-415: subprocess call: check for execution of untrusted input
(S603)
Apps/Diceware/tactility.py
297-297: Uses of tarfile.extractall()
(S202)
384-384: subprocess call: check for execution of untrusted input
(S603)
415-415: subprocess call: check for execution of untrusted input
(S603)
Apps/Calculator/tactility.py
297-297: Uses of tarfile.extractall()
(S202)
384-384: subprocess call: check for execution of untrusted input
(S603)
415-415: subprocess call: check for execution of untrusted input
(S603)
Apps/SerialConsole/tactility.py
297-297: Uses of tarfile.extractall()
(S202)
384-384: subprocess call: check for execution of untrusted input
(S603)
415-415: subprocess call: check for execution of untrusted input
(S603)
Apps/GPIO/tactility.py
297-297: Uses of tarfile.extractall()
(S202)
384-384: subprocess call: check for execution of untrusted input
(S603)
415-415: subprocess call: check for execution of untrusted input
(S603)
Apps/TwoEleven/tactility.py
297-297: Uses of tarfile.extractall()
(S202)
384-384: subprocess call: check for execution of untrusted input
(S603)
415-415: subprocess call: check for execution of untrusted input
(S603)
Apps/GraphicsDemo/tactility.py
297-297: Uses of tarfile.extractall()
(S202)
384-384: subprocess call: check for execution of untrusted input
(S603)
415-415: subprocess call: check for execution of untrusted input
(S603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build (TwoEleven)
- GitHub Check: Build (Diceware)
- GitHub Check: Build (SerialConsole)
- GitHub Check: Build (GPIO)
- GitHub Check: Build (Calculator)
- GitHub Check: Build (GraphicsDemo)
- GitHub Check: Build (HelloWorld)
🔇 Additional comments (15)
Apps/GraphicsDemo/main/CMakeLists.txt (1)
5-7: The TactilityCpp include path is correctly configured.The comment helpfully explains the elf_loader constraint, and the relative path correctly resolves to the TactilityCpp headers directory. The approach is consistent with ESP-IDF conventions.
Apps/Calculator/main/Source/Calculator.h (1)
7-7: LGTM!The migration from the custom
Strtype to standardstd::stringis a clean improvement. The function signatures are consistent with the implementation, and using standard library types improves portability and reduces external dependencies.Also applies to: 21-22
Apps/Calculator/main/Source/Calculator.cpp (3)
7-7: LGTM!The
<cstring>include is appropriately added to supportstrchr()usage in the operator parsing logic.
49-88: LGTM!The
infixToRPNfunction is correctly migrated to usestd::string. The token accumulation logic and operator-to-string conversions usingstd::string(1, ch)are appropriate.
135-137: LGTM!The
computeFormulafunction correctly creates astd::stringfromformulaBufferto pass to the updatedinfixToRPNsignature.Apps/TwoEleven/main/CMakeLists.txt (1)
9-11: Str library removal is complete and safe.Verification confirms no remaining Str library references in TwoEleven source code. The removal of Str library sources and includes from the build configuration is consistent with the broader SDK migration, and the change poses no build risk.
Apps/GPIO/tactility.py (3)
144-146: API signature change looks correct.The migration from
(version, platform)to(version, file)aligns with the new index-based SDK discovery flow. This change is consistently applied across all SDK download logic.
152-168: Function renames align with tool.json migration.The renames from
should_update_sdk_json()andupdate_sdk_json()to theirtool_jsonvariants correctly reflect the data source migration.
270-298: Index-based SDK download flow implemented correctly.The new two-step download process (index.json → platform-specific ZIP) is well-structured with proper error handling and context managers. The verbose logging additions are helpful for debugging.
Regarding the static analysis warnings:
- Line 297 (
tarfile.extractall): This is flagged as S202 but is a false positive here since the SDK comes from a trusted CDN- Lines 284, 384, 415 (
subprocesscalls): These are flagged as S603 but use hardcoded commands with no untrusted inputApps/Diceware/tactility.py (5)
16-16: Version bump and color simplification look good.The version bump to 3.1.0 and the removal of Windows-specific color branching (now using universal ANSI codes) is a clean simplification. ANSI escape codes work on modern Windows terminals (Windows 10+).
Also applies to: 24-29
144-146: Function renames and tool.json workflow look correct.The signature change for
get_sdk_url(version, file)and the renamedshould_update_tool_json()/update_tool_json()functions align with the new index-driven SDK discovery pattern. The implementation matches the relevant code snippets fromApps/Calculator/tactility.py.Also applies to: 152-168
270-298: Index-driven SDK download implementation looks good, with minor suggestions.The new flow correctly downloads
index.json, validates platform availability, and fetches the platform-specific ZIP. A few observations:
- Line 279: The TODO for 404 handling is noted — consider distinguishing HTTP errors from other network issues.
- Line 282-283: JSON parsing could fail if index.json is malformed; similar error handling as suggested for
read_sdk_json()would help.- Line 296-297: The
zipfile.extractall()is safe here since the archive comes from a controlled CDN source (cdn.tactility.one). The static analysis warning (S202) abouttarfile.extractallappears to be a false positive — this iszipfile, nottarfile.
381-384: Subprocess calls are safe; verbose logging is helpful.The static analysis warning (S603) about untrusted input is a false positive — the command array is hardcoded with
["idf.py", "-B", cmake_path, "build"]. Thecmake_pathis derived from platform names validated against the manifest, not from arbitrary user input. The added verbose logging for commands improves debuggability.Also applies to: 412-415
639-641: Updated main loop correctly uses renamed functions.The update from
should_update_sdk_json()toshould_update_tool_json()andupdate_sdk_json()toupdate_tool_json()is consistent with the function renames.Apps/TwoEleven/tactility.py (1)
16-16: Changes are consistent withApps/Diceware/tactility.py.All the SDK workflow updates (version bump, color constants, tool.json functions, index-driven downloads, verbose logging) are identical and correctly implemented. The static analysis warnings for subprocess (S603) and extractall (S202) are false positives as noted in the other file review.
Also applies to: 24-29, 144-168, 270-298, 381-384, 412-415, 639-641
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
Apps/HelloWorld/tactility.py (1)
1-693: Duplicate file - same issues as Apps/Calculator/tactility.py.This file is identical to the Calculator version. The same
download_filefile handle leak (lines 65-70) applies here. The tool.json workflow and other changes are correctly implemented.Apps/GPIO/tactility.py (1)
1-693: Duplicate file - same issues as Apps/Calculator/tactility.py.Identical to Calculator version with the same
download_filefile handle leak at lines 65-70.Apps/SerialConsole/tactility.py (1)
1-693: Duplicate file - same issues as Apps/Calculator/tactility.py.Identical to Calculator version with the same
download_filefile handle leak at lines 65-70.Apps/Diceware/tactility.py (1)
1-693: Duplicate file - same issues as Apps/Calculator/tactility.py.Identical to Calculator version with the same
download_filefile handle leak at lines 65-70.Apps/TwoEleven/tactility.py (1)
1-693: Duplicate file - same issues as Apps/Calculator/tactility.py.Identical to Calculator version with the same
download_filefile handle leak at lines 65-70.Apps/GraphicsDemo/tactility.py (1)
1-693: Duplicate file - same issues as Apps/Calculator/tactility.py.Identical to Calculator version with the same
download_filefile handle leak at lines 65-70.
🧹 Nitpick comments (2)
Apps/Calculator/tactility.py (2)
1-693: Consider extracting this build script into a shared module.This ~700-line file is duplicated verbatim across 7 app directories (Calculator, HelloWorld, GPIO, SerialConsole, Diceware, TwoEleven, GraphicsDemo). Any bug fix or feature update must be replicated manually in all locations, creating significant maintenance overhead and risk of drift.
Consider one of these approaches:
- Extract to a shared
Libraries/TactilityBuild/module that apps import- Use a single script at the repository root that apps invoke
- Template/generate the file from a single source during build setup
65-70: File handle leak if write fails.If an exception occurs between
open()andclose(), the file handle is leaked. Use a context manager for safer resource handling.🔎 Proposed fix
try: response = urllib.request.urlopen(request) - file = open(filepath, mode="wb") - file.write(response.read()) - file.close() + with open(filepath, mode="wb") as file: + file.write(response.read()) return True
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Apps/Calculator/tactility.pyApps/Diceware/tactility.pyApps/GPIO/tactility.pyApps/GraphicsDemo/tactility.pyApps/HelloWorld/tactility.pyApps/SerialConsole/tactility.pyApps/TwoEleven/tactility.py
🧰 Additional context used
🧬 Code graph analysis (3)
Apps/GPIO/tactility.py (1)
Apps/Calculator/tactility.py (6)
get_sdk_dir(114-126)should_update_tool_json(151-161)update_tool_json(163-167)download_file(54-74)print_error(79-80)build_action(486-511)
Apps/SerialConsole/tactility.py (1)
Apps/Calculator/tactility.py (5)
get_sdk_url(143-145)should_update_tool_json(151-161)update_tool_json(163-167)download_file(54-74)print_error(79-80)
Apps/TwoEleven/tactility.py (1)
Apps/Calculator/tactility.py (8)
get_sdk_url(143-145)sdk_exists(147-149)get_sdk_dir(114-126)should_update_tool_json(151-161)update_tool_json(163-167)download_file(54-74)print_error(79-80)build_action(486-511)
🪛 Ruff (0.14.10)
Apps/HelloWorld/tactility.py
296-296: Uses of tarfile.extractall()
(S202)
383-383: subprocess call: check for execution of untrusted input
(S603)
414-414: subprocess call: check for execution of untrusted input
(S603)
Apps/Diceware/tactility.py
296-296: Uses of tarfile.extractall()
(S202)
383-383: subprocess call: check for execution of untrusted input
(S603)
414-414: subprocess call: check for execution of untrusted input
(S603)
Apps/GPIO/tactility.py
296-296: Uses of tarfile.extractall()
(S202)
383-383: subprocess call: check for execution of untrusted input
(S603)
414-414: subprocess call: check for execution of untrusted input
(S603)
Apps/GraphicsDemo/tactility.py
296-296: Uses of tarfile.extractall()
(S202)
383-383: subprocess call: check for execution of untrusted input
(S603)
414-414: subprocess call: check for execution of untrusted input
(S603)
Apps/SerialConsole/tactility.py
296-296: Uses of tarfile.extractall()
(S202)
383-383: subprocess call: check for execution of untrusted input
(S603)
414-414: subprocess call: check for execution of untrusted input
(S603)
Apps/TwoEleven/tactility.py
296-296: Uses of tarfile.extractall()
(S202)
383-383: subprocess call: check for execution of untrusted input
(S603)
414-414: subprocess call: check for execution of untrusted input
(S603)
Apps/Calculator/tactility.py
296-296: Uses of tarfile.extractall()
(S202)
383-383: subprocess call: check for execution of untrusted input
(S603)
414-414: subprocess call: check for execution of untrusted input
(S603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build (GraphicsDemo)
- GitHub Check: Build (GPIO)
- GitHub Check: Build (Calculator)
- GitHub Check: Build (HelloWorld)
- GitHub Check: Build (SerialConsole)
- GitHub Check: Build (Diceware)
- GitHub Check: Build (TwoEleven)
🔇 Additional comments (5)
Apps/Calculator/tactility.py (5)
109-167: LGTM - Clean migration to tool.json workflow.The renamed functions (
should_update_tool_json,update_tool_json) and updatedget_sdk_url(version, file)signature provide a cleaner API. The context manager usage for file operations is correct.
269-297: LGTM - Well-implemented index-based SDK discovery.The two-step download flow (index.json → platform archive) with proper validation and error handling is solid. The context managers ensure files are properly closed.
23-28: LGTM - Simplified color handling.Removing the Windows-specific branching in favor of standard ANSI escape sequences is a good simplification. Modern Windows terminals support these codes.
380-397: LGTM - Build command logging.The verbose logging for build commands aids debugging. The subprocess calls to
idf.pyare safe as this is a trusted Espressif SDK tool with hardcoded arguments.
616-650: LGTM - Correct exit code handling and updated function calls.The exit code changes (returning 1 on missing args and build failures) follow CLI conventions. The call sites correctly use the renamed
should_update_tool_json()andupdate_tool_json()functions.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Apps/SerialConsole/main/Source/ConnectView.h (1)
33-42: Potential signed/unsigned mismatch and edge case with empty list.The loop uses
int ito iterate, butlist.size()returnssize_t. Whenlistis empty,list.size() - 1wraps to a large unsigned value, causing the loop to iterate unexpectedly. Additionally, line 37's comparisoni < list.size() - 1has a signed/unsigned mismatch.🔎 Proposed fix
std::string join(const std::vector<std::string>& list) { std::string output; + if (list.empty()) { + return output; + } - for (int i = list.size() - 1; i >= 0; i--) { + for (size_t i = list.size(); i-- > 0; ) { output.append(list[i].c_str()); - if (i < list.size() - 1) { + if (i < list.size() - 1) { output.append(","); } } return output; }Alternatively, consider using a simpler forward iteration:
std::string join(const std::vector<std::string>& list) { std::string output; for (size_t i = 0; i < list.size(); ++i) { if (i > 0) { output.append(","); } output.append(list[list.size() - 1 - i]); } return output; }
🧹 Nitpick comments (2)
Apps/Diceware/main/Source/Diceware.h (1)
29-29: LGTM! String type migration completed.The migration from
Strtostd::stringaligns with the PR objectives. For potential optimization, consider passing by const reference if the string can be large.💡 Optional: Pass by const reference for efficiency
-void onFinishJob(std::string result); +void onFinishJob(const std::string& result);Apps/SerialConsole/main/Source/ConsoleView.h (1)
142-156: LGTM with minor suggestion.The migration to
std::stringis correct. Minor optimization: you could simplify lines 145-146 to avoid the intermediate empty string and temporary:🔎 Optional simplification
void onSendClicked() { mutex.lock(); std::string input_text = lv_textarea_get_text(inputTextarea); - std::string to_send; - to_send.append(input_text + terminatorString); + std::string to_send = input_text + terminatorString; mutex.unlock();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Apps/Diceware/main/Source/Diceware.cppApps/Diceware/main/Source/Diceware.hApps/SerialConsole/main/Source/ConnectView.hApps/SerialConsole/main/Source/ConsoleView.hLibraries/TactilityCpp/Include/TactilityCpp/Uart.h
🧰 Additional context used
🧬 Code graph analysis (2)
Apps/SerialConsole/main/Source/ConnectView.h (1)
Libraries/TactilityCpp/Include/TactilityCpp/Uart.h (1)
vector(24-34)
Apps/Diceware/main/Source/Diceware.h (1)
Libraries/TactilityCpp/Include/TactilityCpp/App.h (1)
App(6-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build (HelloWorld)
- GitHub Check: Build (TwoEleven)
- GitHub Check: Build (GraphicsDemo)
- GitHub Check: Build (GPIO)
- GitHub Check: Build (Calculator)
- GitHub Check: Build (Diceware)
- GitHub Check: Build (SerialConsole)
🔇 Additional comments (14)
Apps/Diceware/main/Source/Diceware.h (4)
5-11: LGTM! Clean migration to modern C++ APIs.The updated includes properly support the migration from C-style threading to
tt::Threadwrapper and fromStrtostd::string. The addition of<memory>enables RAII withstd::unique_ptr.
18-18: LGTM! Proper use of RAII for thread management.Using
std::unique_ptr<tt::Thread>ensures automatic cleanup and prevents resource leaks.
26-26: LGTM! Simplified thread entry point.Removing the static modifier and
void* dataparameter eliminates unsafe casting and enables direct member access. This is more idiomatic C++.
34-35: LGTM! Proper use of override specifier.The
overridekeywords ensure compile-time verification against the base classAppinterface, preventing subtle bugs from signature mismatches.Apps/Diceware/main/Source/Diceware.cpp (4)
31-53: LGTM! Improved error handling and logging.The migration to
std::stringis clean, and the added error logging at lines 36, 48, and 50 improves debuggability. The lock acquisition and release pattern is correct.
69-74: LGTM! Proper thread cleanup with RAII.The use of
jobThread->join()followed by setting tonullptrcorrectly waits for thread completion and releases resources through thestd::unique_ptrdestructor.
86-90: LGTM! Correct LVGL integration with proper locking.The migration to
std::stringparameter and use ofresult.c_str()for LVGL is correct. The LVGL operations are properly protected with lock/unlock.
76-84: Thread API usage is correct. The constructor signatureThread(name, stackSize, callable),start()method, and lambda return typeint32_tall align with established usage patterns in the codebase (e.g., SerialConsole app). No issues detected.Libraries/TactilityCpp/Include/TactilityCpp/Uart.h (2)
24-34: LGTM!The migration from
Strtostd::stringis correctly implemented. The explicitstd::string(buffer)construction ensures clarity, though implicit conversion would also work.
48-58: LGTM!The change from
TickTypetoTickType_tcorrectly aligns with the standard FreeRTOS type definitions.Apps/SerialConsole/main/Source/ConnectView.h (1)
23-23: LGTM!The type change from
std::vector<Str>tostd::vector<std::string>aligns with the codebase refactor and correctly matches the return type ofUart::getNames().Apps/SerialConsole/main/Source/ConsoleView.h (3)
28-36: LGTM!The migration to
tt::Thread,tt::RecursiveMutex, andstd::stringfor these member variables aligns with the SDK update objectives.
70-73: LGTM!The lock/unlock pattern for LVGL access is correctly implemented with the conditional check.
235-238: LGTM!The thread state checks using
tt::Thread::State::Stoppedbefore callingjoin()are appropriate to avoid joining already-stopped threads.Also applies to: 253-256
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Apps/SerialConsole/main/Source/ConsoleView.h (1)
76-90: Fix delay calculation - incorrect unit conversion persists.The delay calculation on line 85 has incorrect units (same issue flagged in previous review):
time_diffis in ticks (fromgetTicks())500Uis compared as ticks (line 84)- But dividing
(500U - time_diff)byportTICK_PERIOD_MSis dimensionally wrongSince
portTICK_PERIOD_MSis milliseconds per tick, dividing ticks by it produces invalid units.Correct fix: If
500Urepresents ticks (as the comparison suggests), use:tt::kernel::delayTicks(500U - time_diff);Alternatively, if
500Uwas intended as milliseconds, convert it to ticks first:tt::kernel::delayTicks((500U / portTICK_PERIOD_MS) - time_diff);🔎 Proposed fix
auto end_time = tt::kernel::getTicks(); auto time_diff = end_time - start_time; if (time_diff < 500U) { - tt::kernel::delayTicks((500U - time_diff) / portTICK_PERIOD_MS); + tt::kernel::delayTicks(500U - time_diff); }
🧹 Nitpick comments (3)
Libraries/TactilityCpp/Include/TactilityCpp/Uart.h (1)
9-18: Consider explicit copy/move semantics.The class manages a resource (UartHandle) with a destructor. While the
unique_ptrAPI prevents most accidental copying, explicitly deleting copy operations would be more defensive:🔎 Optional refactor
class Uart { UartHandle handle; public: explicit Uart(UartHandle handle) : handle(handle) {} + + Uart(const Uart&) = delete; + Uart& operator=(const Uart&) = delete; + Uart(Uart&&) = default; + Uart& operator=(Uart&&) = default; ~Uart() { tt_hal_uart_free(handle); }Apps/SerialConsole/main/Source/ConsoleView.h (2)
52-74: Consider using RAII lock guards consistently.The method mixes manual lock/unlock (lines 59-67, 70-73) with scoped locks used elsewhere in the class. While the current implementation is correct, RAII lock guards reduce the risk of forgetting to unlock or missing unlocks on error paths.
Consider refactoring to use scoped locks consistently throughout the class for improved safety and maintainability.
142-156: Consider simplifying string concatenation.Line 146 creates an empty
to_send, then appends a concatenated string. This is less efficient than direct assignment:std::string to_send = input_text + terminatorString;This eliminates the unnecessary intermediate append operation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Apps/Diceware/main/Source/Diceware.cppApps/SerialConsole/main/Source/ConsoleView.hLibraries/TactilityCpp/Include/TactilityCpp/Uart.h
🧰 Additional context used
🧬 Code graph analysis (1)
Apps/SerialConsole/main/Source/ConsoleView.h (1)
Libraries/TactilityCpp/Include/TactilityCpp/Uart.h (1)
unique_ptr(20-23)
🔇 Additional comments (15)
Libraries/TactilityCpp/Include/TactilityCpp/Uart.h (3)
4-7: Includes are now complete.The missing
<memory>include flagged in the previous review has been added. All necessary headers are present forstd::unique_ptr,std::string, andTickType_t.
25-31: LGTM! Clean migration fromStrtostd::string.The refactoring from the custom
Strtype tostd::stringis correctly implemented. The logic remains unchanged with proper type substitutions.
49-59: LGTM! Correct migration toTickType_t.The timeout parameters have been properly updated to use
TickType_t, the standard FreeRTOS type. The necessary<freertos/FreeRTOS.h>header is included.Apps/Diceware/main/Source/Diceware.cpp (7)
11-12: LGTM! Include added for new kernel API.The new include provides access to
tt::kernel::MAX_TICKSused in the updated locking calls.
25-31: LGTM! Successful migration to std::string.The function correctly migrates from
Strtostd::string, and the previous critical issue with character appending has been properly addressed.
33-55: LGTM! Migration and error handling are appropriate.The function correctly migrates to
std::stringand uses the newtt::kernel::MAX_TICKSconstant. Error handling properly logs failures and returns an empty string, which is handled safely by the caller.
57-70: LGTM! Clean refactoring to member function.The conversion from static function to member function is well-executed, with direct member access replacing parameter-based access. The previous critical issue with
appendfhas been properly resolved using standard string concatenation.
72-77: LGTM! Correct migration to C++ threading API.The function properly uses the new
std::unique_ptr<tt::Thread>API with appropriate null checking and cleanup.
79-87: LGTM! Thread creation with lambda is safe and idiomatic.The lambda-based thread creation correctly captures
thisfor member access. Thread lifetime is properly managed throughcleanupJob()which joins before object destruction.
89-93: LGTM! Thread-safe UI update with correct parameter type.The function correctly migrates to
std::stringparameter and properly uses LVGL locking to ensure thread-safe label updates.Apps/SerialConsole/main/Source/ConsoleView.h (5)
169-176: LGTM! Thread initialization correctly fixed.The type mismatch from the previous review has been resolved.
std::make_unique<tt::Thread>now correctly matches thestd::unique_ptr<tt::Thread>member type (line 28). The lambda-based thread entry point is a clean, modern C++ pattern.
215-221: LGTM! Thread initialization correctly fixed.The type mismatch from the previous review has been resolved.
std::make_unique<tt::Thread>correctly matches thestd::unique_ptr<tt::Thread>member type (line 30).
235-235: LGTM! Thread state checking updated correctly.The thread state check has been correctly updated to use the new
tt::Thread::State::StoppedAPI.
253-253: LGTM! Thread state checking updated correctly.Consistent with the
stopLogic()method, the thread state check correctly uses the newtt::Thread::State::StoppedAPI.
40-50: Remove redundantlock.lock()call in scoped lock pattern.The pattern
auto lock = mutex.asScopedLock(); lock.lock();appears redundant. InConnectView.h, the sameasScopedLock()pattern is used without an explicitlock()call afterward, and elsewhere inConsoleView.hthe mutex is locked directly viaif (mutex.lock()). This suggestsasScopedLock()returns a RAII guard that locks automatically on construction, making the explicitlock.lock()call unnecessary.Update the four affected methods (lines 40-50) to use just
auto lock = mutex.asScopedLock();and rely on the scoped guard's automatic locking behavior on construction and unlocking on scope exit.
Libraries/StrTactilityCFreeRtos features withTactilityFreeRtoslibrarytactility.pyTactilityCpp