Two Eleven App #16
Two Eleven App #16
Conversation
Windows friendly tactility.py
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughMultiple apps' build scripts were updated (Calculator, Diceware, GPIO, GraphicsDemo, HelloWorld, SerialConsole) to standardize ANSI color initialization on Windows, provide platform-specific IDF_PATH guidance, refine process I/O handling, conditionally set subprocess shell use on Windows, replace shell Pre-merge checks❌ Failed checks (1 warning, 2 inconclusive)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 (4)
Apps/SerialConsole/tactility.py (1)
23-409: Changes are consistent with other tactility modules.The cross-platform improvements (Windows ANSI colors, platform-aware error messages, non-blocking I/O, shutil.copy, and Windows shell handling) are correctly applied here, matching the implementation in other apps.
Apps/Diceware/tactility.py (1)
23-409: Changes are consistent with other tactility modules.The cross-platform improvements are correctly applied, identical to the other apps in this PR.
Apps/Calculator/tactility.py (1)
23-409: Changes are consistent with other tactility modules.The cross-platform improvements are correctly applied, identical to the other apps in this PR.
Apps/GPIO/tactility.py (1)
23-409: Changes are consistent with other tactility modules.The cross-platform improvements are correctly applied, identical to the other apps in this PR.
🧹 Nitpick comments (13)
Apps/GraphicsDemo/tactility.py (1)
23-36: Redundant platform branch — color codes are now identical.Both the Windows and non-Windows branches now assign the same ANSI escape codes. This duplication could be simplified by removing the conditional entirely. However, this is a minor cleanup that can be deferred.
🔎 Optional simplification
-if sys.platform == "win32": - shell_color_red = "\033[91m" - shell_color_orange = "\033[93m" - shell_color_green = "\033[32m" - shell_color_purple = "\033[35m" - shell_color_cyan = "\033[36m" - shell_color_reset = "\033[m" -else: - shell_color_red = "\033[91m" - shell_color_orange = "\033[93m" - shell_color_green = "\033[32m" - shell_color_purple = "\033[35m" - shell_color_cyan = "\033[36m" - shell_color_reset = "\033[m" +shell_color_red = "\033[91m" +shell_color_orange = "\033[93m" +shell_color_green = "\033[32m" +shell_color_purple = "\033[35m" +shell_color_cyan = "\033[36m" +shell_color_reset = "\033[m"Apps/HelloWorld/tactility.py (1)
1-36: Significant code duplication across all tactility.py files.This file is nearly identical to
Apps/GraphicsDemo/tactility.py,Apps/SerialConsole/tactility.py,Apps/Diceware/tactility.py,Apps/Calculator/tactility.py, andApps/GPIO/tactility.py. The cross-platform changes applied here are correct, but maintaining six copies of the same ~690-line script is a maintenance burden.Consider extracting the common build logic into a shared module (e.g.,
common/tactility_core.py) that each app can import, leaving only app-specific configuration in individual files.Apps/TwoEleven/main/Source/TwoElevenHelpers.c (1)
10-14: Consider consolidating duplicate cases.Cases 0 and 1 return the same color. In 2048, tile values are powers of 2 (2, 4, 8, ...), so a value of 1 (which would represent 2¹ = 2 in the internal exponent representation) should likely use
ELEVENTWO_NUMBER_2_COLOR. If internal values are exponents, case 1 should map to the 2-tile color.Apps/TwoEleven/main/Source/TwoEleven.cpp (3)
9-16: Global static UI pointers may cause issues with multiple app instances.Using file-scoped static pointers for UI elements assumes only one instance of the app runs at a time. If the framework ever supports multiple instances, this would cause state conflicts. Consider storing these in a class member or context structure.
27-33: Potential buffer overflow with high scores.The buffer is 64 bytes, and with
uint16_tscores (max 65535), the formatted strings fit. However, scores in 2048 can exceed 65535 easily. If the score type is ever changed touint32_t, the buffer may overflow with very high scores.Consider using
snprintffor safety:🔎 Proposed fix
if (twoeleven_get_best_tile(obj_2048) >= 2048) { char message[64]; - sprintf(message, "YOU WIN!\n\nSCORE: %d", twoeleven_get_score(obj_2048)); + snprintf(message, sizeof(message), "YOU WIN!\n\nSCORE: %d", twoeleven_get_score(obj_2048)); tt_app_alertdialog_start("YOU WIN!", message, alertDialogLabels, 1); } else if (twoeleven_get_status(obj_2048)) { char message[64]; - sprintf(message, "GAME OVER!\n\nSCORE: %d", twoeleven_get_score(obj_2048)); + snprintf(message, sizeof(message), "GAME OVER!\n\nSCORE: %d", twoeleven_get_score(obj_2048)); tt_app_alertdialog_start("GAME OVER!", message, alertDialogLabels, 1);
154-154: Use explicit cast touintptr_tfor consistency.Line 159 correctly uses
(uintptr_t)when retrieving the user data. For symmetry and clarity, use(void*)(uintptr_t)swhen setting it.🔎 Proposed fix
- lv_obj_add_event_cb(btn, size_select_cb, LV_EVENT_CLICKED, (void*)s); + lv_obj_add_event_cb(btn, size_select_cb, LV_EVENT_CLICKED, (void*)(uintptr_t)s);Apps/TwoEleven/main/Source/TwoElevenLogic.c (2)
27-31: Static initialization of RNG is not thread-safe.The static
initializedflag andsrand()call are not thread-safe. Ifadd_randomis called concurrently from multiple threads (unlikely in this single-player game), this could cause issues. For embedded single-threaded use, this is fine.
32-32: Variable-length array may not be portable.The VLA
empty[matrix_size * matrix_size][2]requires C99 or later and may not be supported on all compilers (e.g., MSVC). Sincematrix_sizeis bounded to 3-6, consider using a fixed-size array[36][2]for maximum portability.🔎 Proposed fix
- uint16_t empty[matrix_size * matrix_size][2]; + uint16_t empty[36][2]; // Max 6x6 = 36 cellsApps/TwoEleven/tactility.py (5)
23-36: Remove redundant conditional - both branches are identical.The ANSI color codes are the same for both Windows and non-Windows platforms. This conditional provides no value.
🔎 Proposed fix
-if sys.platform == "win32": - shell_color_red = "\033[91m" - shell_color_orange = "\033[93m" - shell_color_green = "\033[32m" - shell_color_purple = "\033[35m" - shell_color_cyan = "\033[36m" - shell_color_reset = "\033[m" -else: - shell_color_red = "\033[91m" - shell_color_orange = "\033[93m" - shell_color_green = "\033[32m" - shell_color_purple = "\033[35m" - shell_color_cyan = "\033[36m" - shell_color_reset = "\033[m" +shell_color_red = "\033[91m" +shell_color_orange = "\033[93m" +shell_color_green = "\033[32m" +shell_color_purple = "\033[35m" +shell_color_cyan = "\033[36m" +shell_color_reset = "\033[m"
73-77: Use context manager for file operations.Opening files without a context manager can lead to resource leaks if an exception occurs before
close()is called.🔎 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
117-121: Use context manager for JSON file reading.The file handle is not closed after reading, which can cause resource leaks.
🔎 Proposed fix
def read_sdk_json(): json_file_path = os.path.join(ttbuild_path, "sdk.json") - json_file = open(json_file_path) - return json.load(json_file) + with open(json_file_path) as json_file: + return json.load(json_file)
203-206: Use Pythonic boolean comparisons.Per static analysis hints and Python style guidelines, use
not use_local_sdkinstead ofuse_local_sdk == False, anduse_local_sdkinstead ofuse_local_sdk == True.🔎 Proposed fix
- if use_local_sdk == False and os.environ.get("TACTILITY_SDK_PATH") is not None: + if not use_local_sdk and os.environ.get("TACTILITY_SDK_PATH") is not None: print_warning("TACTILITY_SDK_PATH is set, but will be ignored by this command.") print_warning("If you want to use it, use the '--local-sdk' parameter") - elif use_local_sdk == True and os.environ.get("TACTILITY_SDK_PATH") is None: + elif use_local_sdk and os.environ.get("TACTILITY_SDK_PATH") is None: exit_with_error("local build was requested, but TACTILITY_SDK_PATH environment variable is not set.")
471-472: Avoid catching broadException; also fix attribute access.Catching
Exceptionmasks unexpected errors. Additionally,e.messageis not a standard attribute. Consider catching specific exceptions or usingstr(e).🔎 Proposed fix
- except Exception as e: - print_status_error(f"Building package failed: {e.message}") + except (OSError, tarfile.TarError) as e: + print_status_error(f"Building package failed: {e}") return False
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
Apps/TwoEleven/images/3x3.pngis excluded by!**/*.pngApps/TwoEleven/images/4x4.pngis excluded by!**/*.pngApps/TwoEleven/images/5x5.pngis excluded by!**/*.pngApps/TwoEleven/images/6x6.pngis excluded by!**/*.pngApps/TwoEleven/images/selection.pngis excluded by!**/*.png
📒 Files selected for processing (20)
Apps/Calculator/tactility.pyApps/Diceware/tactility.pyApps/GPIO/tactility.pyApps/GraphicsDemo/tactility.pyApps/HelloWorld/tactility.pyApps/SerialConsole/tactility.pyApps/TwoEleven/CMakeLists.txtApps/TwoEleven/README.mdApps/TwoEleven/main/CMakeLists.txtApps/TwoEleven/main/Source/TwoEleven.cppApps/TwoEleven/main/Source/TwoEleven.hApps/TwoEleven/main/Source/TwoElevenHelpers.cApps/TwoEleven/main/Source/TwoElevenHelpers.hApps/TwoEleven/main/Source/TwoElevenLogic.cApps/TwoEleven/main/Source/TwoElevenLogic.hApps/TwoEleven/main/Source/TwoElevenUi.cApps/TwoEleven/main/Source/TwoElevenUi.hApps/TwoEleven/main/Source/main.cppApps/TwoEleven/manifest.propertiesApps/TwoEleven/tactility.py
🧰 Additional context used
🧬 Code graph analysis (8)
Apps/TwoEleven/main/Source/TwoEleven.h (2)
Libraries/TactilityCpp/Include/TactilityCpp/App.h (1)
App(6-13)Apps/TwoEleven/main/Source/TwoEleven.cpp (14)
twoElevenEventCb(18-38)twoElevenEventCb(18-18)newGameBtnEvent(40-43)newGameBtnEvent(40-40)create_game(45-94)create_game(45-45)create_selection(96-156)create_selection(96-96)size_select_cb(158-168)size_select_cb(158-158)onShow(170-188)onShow(170-170)onResult(190-202)onResult(190-190)
Apps/SerialConsole/tactility.py (6)
Apps/Calculator/tactility.py (3)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)Apps/Diceware/tactility.py (3)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)Apps/GPIO/tactility.py (3)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)Apps/GraphicsDemo/tactility.py (3)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)Apps/HelloWorld/tactility.py (3)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)Apps/TwoEleven/tactility.py (3)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)
Apps/TwoEleven/main/Source/TwoEleven.cpp (2)
Apps/TwoEleven/main/Source/TwoElevenLogic.c (3)
twoeleven_get_best_tile(149-162)twoeleven_get_score(129-134)twoeleven_get_status(139-144)Apps/TwoEleven/main/Source/TwoElevenUi.c (2)
twoeleven_set_new_game(110-122)twoeleven_create(38-105)
Apps/HelloWorld/tactility.py (1)
Apps/Calculator/tactility.py (3)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)
Apps/TwoEleven/main/Source/TwoElevenHelpers.h (1)
Apps/TwoEleven/main/Source/TwoElevenHelpers.c (6)
get_num_color(8-40)count_empty(45-53)find_pair_down(58-65)rotate_matrix(71-82)slide_array(87-109)update_btnm_map(114-130)
Apps/Calculator/tactility.py (1)
Apps/Diceware/tactility.py (3)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)
Apps/GraphicsDemo/tactility.py (6)
Apps/Calculator/tactility.py (3)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)Apps/Diceware/tactility.py (3)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)Apps/GPIO/tactility.py (3)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)Apps/HelloWorld/tactility.py (3)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)Apps/SerialConsole/tactility.py (3)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)Apps/TwoEleven/tactility.py (3)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)
Apps/GPIO/tactility.py (1)
Apps/Calculator/tactility.py (3)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)
🪛 LanguageTool
Apps/TwoEleven/README.md
[style] ~57-~57: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... ## TODO - Maybe trackball one day? - Maybe other keys rather being limited to arro...
(REP_MAYBE)
🪛 Ruff (0.14.10)
Apps/SerialConsole/tactility.py
381-381: subprocess call: check for execution of untrusted input
(S603)
381-381: Starting a process with a partial executable path
(S607)
409-409: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
Apps/HelloWorld/tactility.py
381-381: subprocess call: check for execution of untrusted input
(S603)
381-381: Starting a process with a partial executable path
(S607)
409-409: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
Apps/Calculator/tactility.py
381-381: subprocess call: check for execution of untrusted input
(S603)
381-381: Starting a process with a partial executable path
(S607)
409-409: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
Apps/GraphicsDemo/tactility.py
381-381: subprocess call: check for execution of untrusted input
(S603)
381-381: Starting a process with a partial executable path
(S607)
409-409: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
Apps/Diceware/tactility.py
381-381: subprocess call: check for execution of untrusted input
(S603)
381-381: Starting a process with a partial executable path
(S607)
409-409: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
Apps/TwoEleven/tactility.py
66-72: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
74-74: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
78-78: Consider moving this statement to an else block
(TRY300)
203-203: Avoid equality comparisons to False; use not use_local_sdk: for false checks
Replace with not use_local_sdk
(E712)
206-206: Avoid equality comparisons to True; use use_local_sdk: for truth checks
Replace with use_local_sdk
(E712)
211-211: Test for membership should be not in
Convert to not in
(E713)
216-216: Test for membership should be not in
Convert to not in
(E713)
220-220: Test for membership should be not in
Convert to not in
(E713)
222-222: Test for membership should be not in
Convert to not in
(E713)
224-224: Test for membership should be not in
Convert to not in
(E713)
230-230: f-string without any placeholders
Remove extraneous f prefix
(F541)
245-245: Test for membership should be not in
Convert to not in
(E713)
247-247: Test for membership should be not in
Convert to not in
(E713)
250-250: Test for membership should be not in
Convert to not in
(E713)
252-252: Test for membership should be not in
Convert to not in
(E713)
254-254: Test for membership should be not in
Convert to not in
(E713)
257-257: Test for membership should be not in
Convert to not in
(E713)
259-259: Test for membership should be not in
Convert to not in
(E713)
261-261: Test for membership should be not in
Convert to not in
(E713)
263-263: Test for membership should be not in
Convert to not in
(E713)
265-265: Test for membership should be not in
Convert to not in
(E713)
295-295: Uses of tarfile.extractall()
(S202)
381-381: subprocess call: check for execution of untrusted input
(S603)
381-381: Starting a process with a partial executable path
(S607)
409-409: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
470-470: Consider moving this statement to an else block
(TRY300)
471-471: Do not catch blind exception: Exception
(BLE001)
497-497: Test for membership should be not in
Convert to not in
(E713)
536-536: f-string without any placeholders
Remove extraneous f prefix
(F541)
539-539: Probable use of requests call without timeout
(S113)
543-543: f-string without any placeholders
Remove extraneous f prefix
(F541)
554-554: Probable use of requests call without timeout
(S113)
578-578: Probable use of requests call without timeout
(S113)
598-598: Probable use of requests call without timeout
(S113)
Apps/GPIO/tactility.py
381-381: subprocess call: check for execution of untrusted input
(S603)
381-381: Starting a process with a partial executable path
(S607)
409-409: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
🔇 Additional comments (28)
Apps/GraphicsDemo/tactility.py (4)
196-200: Good improvement: Platform-aware IDF SDK guidance.Providing Windows-specific (
%IDF_PATH%\export.ps1) vs Unix-style (export.sh) instructions helps users on each platform resolve the missing SDK issue more easily.
338-358: Non-blocking I/O handling looks correct.The
os.set_blocking()call is appropriately gated for non-Windows platforms, and the final drain loop (lines 353-357) ensures remaining output is captured after process termination.
369-381: Cross-platform build improvements look good.Using
shutil.copyinstead of shell commands and addingshell=Trueon Windows forsubprocess.Popenare appropriate changes for Windows compatibility. Theidf.pycommand requires shell interpretation on Windows to resolve via PATH.Regarding static analysis hints (S603/S607): these are false positives in this context — the command is hardcoded, arguments come from internal configuration, and this is a local build tool, not a web service.
403-409: Consistent cross-platform handling in consecutive builds.The same cross-platform patterns applied in
build_firstare correctly mirrored here.Apps/HelloWorld/tactility.py (2)
338-358: Cross-platform process handling is correct.The changes to
wait_for_process(non-blocking I/O on non-Windows, drain loop for remaining output) are correctly implemented, matching the pattern in other tactility modules.
369-409: Build function changes are consistent and correct.The
shutil.copyusage and Windows shell handling for subprocess are properly implemented.Apps/TwoEleven/CMakeLists.txt (1)
1-16: LGTM! Clean CMake configuration.The CMake setup follows standard ESP-IDF and TactilitySDK patterns with proper environment variable handling and a sensible fallback path.
Apps/TwoEleven/README.md (1)
1-57: Excellent documentation!The README is comprehensive, well-structured, and user-friendly. It covers all essential aspects of the game clearly.
The static analysis tool flagged a minor stylistic repetition of "Maybe" in the TODO section (lines 56-57), but this is purely cosmetic and doesn't impact clarity.
Apps/TwoEleven/manifest.properties (1)
1-11: LGTM! Manifest configuration looks good.The manifest properly identifies the app with appropriate versioning, target platforms, and a clear description. Past feedback regarding naming conventions has been addressed.
Apps/TwoEleven/main/Source/main.cpp (1)
1-11: LGTM! Clean entry point.The main entry point correctly registers the TwoEleven app using the template-based registration system, consistent with most other apps in the codebase.
Apps/TwoEleven/main/CMakeLists.txt (1)
1-15: LGTM! Component registration is correctly configured.The ESP-IDF component setup properly includes sources and dependencies. The glob pattern
Source/*.c*is intentionally broad to capture both C and C++ files, which is acceptable for a controlled source directory. The comments clearly explain why library sources and headers must be included directly due to elf_loader behavior.Apps/TwoEleven/main/Source/TwoElevenUi.h (1)
1-35: LGTM! Clean and well-documented UI API.The header provides a minimal, well-designed public API with proper C linkage, clear documentation of parameter constraints, and appropriate include guards.
Apps/TwoEleven/main/Source/TwoEleven.h (1)
1-24: LGTM! Well-structured app class.The TwoEleven class properly extends the App base class with appropriate lifecycle method overrides. The use of
finalprevents unintended inheritance, and static methods for LVGL callbacks is the standard pattern for event handling.Apps/TwoEleven/main/Source/TwoElevenLogic.h (1)
1-71: LGTM! Well-designed game logic API.The header provides a clean, focused API with proper C linkage and clear documentation. The separation between matrix manipulation functions and state accessors is logical, and the documentation helpfully notes that
game_overis non-mutating.Apps/TwoEleven/main/Source/TwoElevenHelpers.c (5)
45-53: LGTM!The empty cell counting logic is correct. Using
uint8_tfor the count is appropriate since the maximum matrix size is 6×6 = 36 cells, well within uint8_t range.
58-65: LGTM!The adjacent pair detection correctly checks for vertical pairs in columns. The boundary check
y < matrix_size - 1properly prevents out-of-bounds access.
71-82: LGTM!Standard in-place 90° clockwise matrix rotation using the four-corner swap technique. The algorithm is correct for square matrices.
87-109: LGTM!The slide and merge logic correctly implements 2048 mechanics: slides non-zero elements toward the start, merges adjacent equal values (incrementing the exponent), and uses a
stopmarker to prevent double-merging in a single move.
114-130: LGTM!The button map update logic correctly handles the newline separators in the LVGL button matrix format and converts internal exponent values to display values using
1 << matrix[x][y].Apps/TwoEleven/main/Source/TwoElevenUi.c (3)
139-154: Verify gesture/key direction mapping is intentional.The direction mapping appears inverted:
LV_DIR_TOP/LV_KEY_UP→move_leftLV_DIR_LEFT/LV_KEY_LEFT→move_upThis means swiping up moves tiles left. If this is intentional for the UI layout, please add a comment explaining the mapping. Otherwise, consider aligning directions logically.
Also applies to: 163-177
13-33: LGTM!The cleanup logic properly frees all dynamically allocated resources in the correct order: button map entries, button map array, matrix rows, matrix array, and finally the game struct. Setting user data to NULL prevents dangling pointer access.
196-226: LGTM!The button matrix drawing callback correctly applies tile colors based on values and sets appropriate text colors (dark for low values, white for values ≥ 8). The null checks are properly in place.
Apps/TwoEleven/main/Source/TwoEleven.cpp (2)
170-188: LGTM!The
onShowmethod properly initializes the layout hierarchy, creates the toolbar, and displays the selection UI. The styling and flex layout configuration is appropriate.
190-201: LGTM!The
onResulthandler correctly acquires the LVGL lock before manipulating UI objects and releases it afterward. Resetting the global pointers to nullptr before recreating the selection UI prevents stale pointer access.Apps/TwoEleven/main/Source/TwoElevenHelpers.h (1)
1-89: LGTM!Well-structured header with proper include guards, C++ linkage compatibility, clear section organization, and comprehensive documentation. The color constants follow a consistent naming convention and the struct/function declarations align with their implementations.
Apps/TwoEleven/main/Source/TwoElevenLogic.c (3)
94-124: LGTM with a note on efficiency.The
game_overfunction correctly allocates a temporary matrix to check for possible merges without mutating the original. All code paths properly free the temporary memory.The function performs up to 3 rotations and merge checks, which is O(n²) per check. For small matrices (max 6×6), this is acceptable.
55-88: LGTM!The movement functions correctly use rotation to reduce the slide logic to a single direction. The rotation counts are correct:
move_up: direct slidemove_down: 180° rotate, slide, 180° restoremove_left: 90° rotate, slide, 270° restoremove_right: 270° rotate, slide, 90° restore
129-162: LGTM!The accessor functions properly handle null checks and correctly convert internal exponent representation to display values (e.g.,
1 << best_tile).
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Apps/GraphicsDemo/tactility.py (1)
592-605: Use error status for uninstall request failures (and mirror across tactility tools)In the
uninstall_actionexception handler you callprint_status_successwhen the request fails:except requests.RequestException as e: print_status_success(f"Uninstall request failed: {e}")This reports a failure as success and will confuse users and any tooling relying on the visual status. This should be
print_status_error, and the same fix should be applied to the other tactility.py variants that use the same pattern.Proposed fix
- except requests.RequestException as e: - print_status_success(f"Uninstall request failed: {e}") + except requests.RequestException as e: + print_status_error(f"Uninstall request failed: {e}")
♻️ Duplicate comments (5)
Apps/HelloWorld/tactility.py (1)
338-358: Samewait_for_processand uninstall error-path concerns as GraphicsDemoThis file mirrors
Apps/GraphicsDemo/tactility.py:
wait_for_processuses non-blocking stdout withos.set_blocking(..., False)and a tightreadline()loop, which has the same potentialBlockingIOError/ busy-spin issues.- The
uninstall_actionexception handler still usesprint_status_successon failure, which should beprint_status_error.Please apply the same adjustments here as suggested for GraphicsDemo.
Also applies to: 381-381, 592-605
Apps/GPIO/tactility.py (1)
338-358: Replicate fixes from other tactility scripts (wait_for_process & uninstall)
wait_for_processanduninstall_actionhere have the same patterns as in Apps/GraphicsDemo/HelloWorld:
- Consider removing or hardening the non-blocking
os.set_blocking(..., False)+readline()loop.- Switch the uninstall exception path to
print_status_errorinstead ofprint_status_success.Keeping these helpers consistent across apps will avoid subtle, app-specific behavior differences.
Also applies to: 381-381, 592-605
Apps/SerialConsole/tactility.py (1)
338-358: Align SerialConsole tactility helpers with other appsSerialConsole’s
wait_for_processanduninstall_actionmirror the other tactility.py files:
- The non-blocking stdout + tight
readline()loop has the same potential robustness issues.- The uninstall exception path still logs via
print_status_successinstead ofprint_status_error.Recommend applying the same fixes here to keep behavior consistent.
Also applies to: 381-381, 592-605
Apps/Calculator/tactility.py (1)
338-358: Calculator tactility: carry over wait_for_process and uninstall fixesThis copy of tactility.py has the same:
wait_for_processnon-blocking pattern, anduninstall_actionexception handler usingprint_status_successon failure,as the other apps. Please update them consistently (drop or harden the non-blocking read, and use
print_status_errorin the uninstall exception path).Also applies to: 381-381, 592-605
Apps/Diceware/tactility.py (1)
338-358: Diceware tactility: same shared helper issues
wait_for_processanduninstall_actionhere are identical to the other tactility.py variants:
- Non-blocking stdout + tight
readline()loop – same robustness concerns.- Uninstall
requests.RequestExceptionpath still callsprint_status_successinstead ofprint_status_error.Once you settle on a fix in one file, I'd mirror it across all tactility tools.
Also applies to: 381-381, 592-605
🧹 Nitpick comments (2)
Apps/TwoEleven/tactility.py (2)
203-208: Minor style nits (optional)Purely stylistic / Ruff-driven cleanups you may want to batch later:
- Replace
use_local_sdk == False/use_local_sdk == Truewithnot use_local_sdk/use_local_sdk.- Prefer
key not in mappingovernot key in mappingin the various manifest/SDK validation helpers.These don’t change behavior; they just align with PEP 8 and silence the E712/E713 hints.
Also applies to: 211-218, 220-235, 243-267
535-547: Add timeouts to HTTP requests in the tactility CLIAll
requests.get/post/putcalls lack atimeoutargument (lines 539, 554, 578, 598). Without explicit timeouts, the requests library waits indefinitely if the target device is slow or unresponsive, which will hang the CLI.A modest default (e.g. 10–30 seconds) is appropriate for a CLI tool. Add a module-level constant:
DEFAULT_REQUEST_TIMEOUT = 30Then apply it to all request calls:
- response = requests.get(url) + response = requests.get(url, timeout=DEFAULT_REQUEST_TIMEOUT) - response = requests.post(url, params=params) + response = requests.post(url, params=params, timeout=DEFAULT_REQUEST_TIMEOUT) - response = requests.put(url, files=files) + response = requests.put(url, files=files, timeout=DEFAULT_REQUEST_TIMEOUT) - response = requests.put(url, params=params) + response = requests.put(url, params=params, timeout=DEFAULT_REQUEST_TIMEOUT)Also applies to: lines 548–561, 562–585, 592–605
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Apps/Calculator/tactility.pyApps/Diceware/tactility.pyApps/GPIO/tactility.pyApps/GraphicsDemo/tactility.pyApps/HelloWorld/tactility.pyApps/SerialConsole/tactility.pyApps/TwoEleven/main/Source/TwoElevenUi.cApps/TwoEleven/tactility.py
🧰 Additional context used
🧬 Code graph analysis (5)
Apps/Diceware/tactility.py (6)
Apps/Calculator/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)Apps/GPIO/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)Apps/GraphicsDemo/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)Apps/HelloWorld/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)Apps/SerialConsole/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)Apps/TwoEleven/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)
Apps/HelloWorld/tactility.py (1)
Apps/Calculator/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)
Apps/SerialConsole/tactility.py (1)
Apps/Calculator/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)
Apps/GPIO/tactility.py (5)
Apps/Calculator/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)Apps/Diceware/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)Apps/GraphicsDemo/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)Apps/HelloWorld/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)Apps/SerialConsole/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)
Apps/GraphicsDemo/tactility.py (6)
Apps/Calculator/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)Apps/Diceware/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)Apps/GPIO/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)Apps/HelloWorld/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)Apps/SerialConsole/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)Apps/TwoEleven/tactility.py (5)
exit_with_error(101-103)get_cmake_path(314-315)print_status_busy(90-91)print_status_error(97-99)print_status_success(93-95)
🪛 Ruff (0.14.10)
Apps/Diceware/tactility.py
381-381: subprocess call: check for execution of untrusted input
(S603)
381-381: Starting a process with a partial executable path
(S607)
409-409: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
Apps/HelloWorld/tactility.py
381-381: subprocess call: check for execution of untrusted input
(S603)
381-381: Starting a process with a partial executable path
(S607)
409-409: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
Apps/SerialConsole/tactility.py
381-381: subprocess call: check for execution of untrusted input
(S603)
381-381: Starting a process with a partial executable path
(S607)
409-409: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
Apps/Calculator/tactility.py
381-381: subprocess call: check for execution of untrusted input
(S603)
381-381: Starting a process with a partial executable path
(S607)
409-409: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
Apps/GPIO/tactility.py
381-381: subprocess call: check for execution of untrusted input
(S603)
381-381: Starting a process with a partial executable path
(S607)
409-409: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
Apps/TwoEleven/tactility.py
66-72: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
74-74: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
78-78: Consider moving this statement to an else block
(TRY300)
203-203: Avoid equality comparisons to False; use not use_local_sdk: for false checks
Replace with not use_local_sdk
(E712)
206-206: Avoid equality comparisons to True; use use_local_sdk: for truth checks
Replace with use_local_sdk
(E712)
211-211: Test for membership should be not in
Convert to not in
(E713)
216-216: Test for membership should be not in
Convert to not in
(E713)
220-220: Test for membership should be not in
Convert to not in
(E713)
222-222: Test for membership should be not in
Convert to not in
(E713)
224-224: Test for membership should be not in
Convert to not in
(E713)
230-230: f-string without any placeholders
Remove extraneous f prefix
(F541)
245-245: Test for membership should be not in
Convert to not in
(E713)
247-247: Test for membership should be not in
Convert to not in
(E713)
250-250: Test for membership should be not in
Convert to not in
(E713)
252-252: Test for membership should be not in
Convert to not in
(E713)
254-254: Test for membership should be not in
Convert to not in
(E713)
257-257: Test for membership should be not in
Convert to not in
(E713)
259-259: Test for membership should be not in
Convert to not in
(E713)
261-261: Test for membership should be not in
Convert to not in
(E713)
263-263: Test for membership should be not in
Convert to not in
(E713)
265-265: Test for membership should be not in
Convert to not in
(E713)
295-295: Uses of tarfile.extractall()
(S202)
381-381: subprocess call: check for execution of untrusted input
(S603)
381-381: Starting a process with a partial executable path
(S607)
409-409: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
470-470: Consider moving this statement to an else block
(TRY300)
471-471: Do not catch blind exception: Exception
(BLE001)
497-497: Test for membership should be not in
Convert to not in
(E713)
536-536: f-string without any placeholders
Remove extraneous f prefix
(F541)
539-539: Probable use of requests call without timeout
(S113)
543-543: f-string without any placeholders
Remove extraneous f prefix
(F541)
554-554: Probable use of requests call without timeout
(S113)
578-578: Probable use of requests call without timeout
(S113)
598-598: Probable use of requests call without timeout
(S113)
Apps/GraphicsDemo/tactility.py
381-381: subprocess call: check for execution of untrusted input
(S603)
381-381: Starting a process with a partial executable path
(S607)
409-409: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
🔇 Additional comments (1)
Apps/GraphicsDemo/tactility.py (1)
23-36: Windows color handling and IDF_PATH guidance look goodThe Windows-specific ANSI color initialization and split IDF SDK guidance are consistent with the other tactility tools and improve usability on Windows without changing behavior elsewhere.
Also applies to: 195-208
|
Thanks a lot! You've made the very first official game for Tactility! 🎉 |
2 participants
Windows friendly tactility.py also.