FTP Server, Mystify Demo, TwoEleven updates & Snake #21
Conversation
📝 WalkthroughWalkthroughAdds two new applications: FTPServer and MystifyDemo. FTPServer introduces build configuration, manifest, README, an app UI (FTPServer, MainView, SettingsView, View), a complete FTP core (FtpServer::Server) with a public lifecycle/config API, and a Python build/deploy tool (tactility.py). MystifyDemo adds build configuration, manifest, an app entry, a MystifyDemo renderer, PixelBuffer, driver wrappers (DisplayDriver, TouchDriver, Colors), an Application interface, and its own tactility.py helper. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (16)
Apps/MystifyDemo/main/CMakeLists.txt (1)
1-1: UnusedGLOB_RECURSEvariable.
SOURCE_FILESis populated here but never referenced. Theidf_component_registercall usesSRC_DIRSinstead, which handles source discovery differently.♻️ Proposed fix: remove unused glob
-file(GLOB_RECURSE SOURCE_FILES Source/*.c*) - idf_component_register( SRC_DIRS "Source"Apps/MystifyDemo/main/Source/Main.cpp (1)
63-77: Consider using RAII wrappers for driver lifetime management.Raw
new/deletepairs work but could leak if an unexpected error occurs between allocation and cleanup. For improved robustness, consider usingstd::unique_ptrwith custom deleters, which is available in ESP-IDF's C++ support.♻️ Optional RAII approach
- ESP_LOGI(TAG, "Creating display driver"); - auto display = new DisplayDriver(display_id); - - ESP_LOGI(TAG, "Creating touch driver"); - auto touch = new TouchDriver(touch_id); - - // Run the main logic - ESP_LOGI(TAG, "Running application"); - runApplication(display, touch); - - ESP_LOGI(TAG, "Cleanup display driver"); - delete display; - - ESP_LOGI(TAG, "Cleanup touch driver"); - delete touch; + ESP_LOGI(TAG, "Creating display driver"); + auto display = std::make_unique<DisplayDriver>(display_id); + + ESP_LOGI(TAG, "Creating touch driver"); + auto touch = std::make_unique<TouchDriver>(touch_id); + + // Run the main logic + ESP_LOGI(TAG, "Running application"); + runApplication(display.get(), touch.get()); + + ESP_LOGI(TAG, "Cleanup drivers"); + // Automatic cleanup via unique_ptr destructorsApps/MystifyDemo/main/Include/PixelBuffer.h (2)
50-53: Non-portable pointer arithmetic.Casting pointers to
uint32_tassumes 32-bit architecture. While ESP32 is 32-bit, usinguintptr_tor direct pointer arithmetic is more portable and idiomatic.♻️ Proposed fix
void* getDataAtRow(uint16_t row) const { - auto address = reinterpret_cast<uint32_t>(data) + (row * getRowDataSize()); - return reinterpret_cast<void*>(address); + return data + (row * getRowDataSize()); }
76-80: Same non-portable pointer arithmetic pattern.♻️ Proposed fix
uint8_t* getPixelAddress(uint16_t x, uint16_t y) const { uint32_t offset = ((y * getPixelWidth()) + x) * getPixelSize(); - uint32_t address = reinterpret_cast<uint32_t>(data) + offset; - return reinterpret_cast<uint8_t*>(address); + return data + offset; }Apps/MystifyDemo/main/Include/MystifyDemo.h (2)
49-51: Redundant check for unsigned types.
width_andheight_areuint16_t, so they can never be negative. The check<= 0is equivalent to== 0.♻️ Clearer intent
- if (width_ <= 0 || height_ <= 0) { + if (width_ == 0 || height_ == 0) { return false; }
56-64: Placement new pattern adds unnecessary complexity.Using
malloc+ placement new followed by explicit destructor +freeis correct but adds complexity without clear benefit. Regularnew/deletewould be simpler and functionally equivalent.♻️ Simplified approach
- // Allocate full-screen framebuffer - void* mem = malloc(sizeof(PixelBuffer)); - if (!mem) { - return false; - } - framebuffer_ = new(mem) PixelBuffer(width_, height_, display->getColorFormat()); + // Allocate full-screen framebuffer + framebuffer_ = new(std::nothrow) PixelBuffer(width_, height_, display->getColorFormat()); + if (!framebuffer_) { + return false; + }And in
deinit():void deinit() { if (framebuffer_) { - framebuffer_->~PixelBuffer(); - free(framebuffer_); + delete framebuffer_; framebuffer_ = nullptr; } display_ = nullptr; }Apps/MystifyDemo/tactility.py (6)
195-199: Use idiomatic boolean checks.Per Python style guidelines, avoid
== Falseand== Truecomparisons.♻️ 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.")
201-216: Usenot infor membership tests.The pattern
not "key" in dictshould be written as"key" not in dictfor clarity.♻️ Example fix
- if not "toolVersion" in sdk_json: + if "toolVersion" not in sdk_json: exit_with_error("Server returned invalid SDK data format (toolVersion not found)") - if not "toolCompatibility" in sdk_json: + if "toolCompatibility" not in sdk_json: exit_with_error("Server returned invalid SDK data format (toolCompatibility not found)") - if not "toolDownloadUrl" in sdk_json: + if "toolDownloadUrl" not in sdk_json: exit_with_error("Server returned invalid SDK data format (toolDownloadUrl not found)")Apply similar changes throughout
validate_manifest(lines 227-247).
211-212: Remove unnecessary f-string prefix.This f-string has no placeholders.
♻️ Proposed fix
- print_warning(f"Run 'tactility.py updateself' to update.") + print_warning("Run 'tactility.py updateself' to update.")
469-478: Consider catching specific exceptions.Catching bare
Exceptioncan mask unexpected errors. Consider catchingOSErrorandtarfile.TarErrorspecifically.♻️ Proposed fix
try: tar_path = package_name(platforms) tar = tarfile.open(tar_path, mode="w", format=tarfile.USTAR_FORMAT) tar.add(os.path.join("build", "package-intermediate"), arcname="") tar.close() print_status_success(status) return True - except Exception as e: + except (OSError, tarfile.TarError) as e: print_status_error(f"Building package failed: {e}") return False
65-74: File handle not properly closed on download error.If an exception occurs after opening the file but before
file.close(), the file handle leaks. Use a context manager instead.♻️ 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 except OSError as error: if verbose: print_error(f"Failed to fetch URL {url}\n{error}") return False
295-296: Validate archive contents before extraction to prevent path traversal attacks.While the SDK is downloaded from a controlled CDN,
zipfile.extractall()is vulnerable to "Zip Slip" attacks where malicious ZIP entries can escape the target directory via paths containing..or absolute paths. Python'szipfiledocumentation explicitly warns against extracting untrusted archives without validation. Implement path checks to reject entries with absolute paths,..segments, and symlinks before extraction, or stream-extract files manually with destination validation.Apps/FTPServer/main/Source/FtpServerCore.h (1)
13-44: Consider usingconstexprinstead of#definefor constants andstd::min/std::maxfrom<algorithm>.The current approach works, but has some style inconsistencies:
#definemacros inside a namespace don't respect namespace scoping—they leak globally.- Custom
MIN/MAXtemplates duplicate standard library functionality.♻️ Suggested improvement (optional)
namespace FtpServer { -// Constants -#define FTP_CMD_PORT 21 -#define FTP_PASSIVE_DATA_PORT 2024 -... +// Constants +static constexpr uint16_t FTP_CMD_PORT = 21; +static constexpr uint16_t FTP_PASSIVE_DATA_PORT = 2024; +// ... (convert other `#defines` similarly) -#ifndef MIN -template<typename T> -inline constexpr T MIN(const T& x, const T& y) { return (x < y) ? x : y; } -#endif -#ifndef MAX -template<typename T> -inline constexpr T MAX(const T& x, const T& y) { return (x > y) ? x : y; } -#endif +// Use std::min/std::max from <algorithm> insteadApps/FTPServer/main/Source/FtpServerCore.cpp (2)
96-134: Static variables cause shared state across Server instances.The file-scope static variables (
last_screen_log_ms,screen_log_count) and the static localprogress_counterinsidelog_to_screenare shared across allServerinstances. If multiple servers are ever instantiated, they will interfere with each other's throttling.For an embedded system with a single FTP server, this is acceptable. If multi-instance support is ever needed, these should become member variables.
141-198: Path sanitization logic is functional but has fragile prefix detection.The traversal prevention at lines 166-192 relies on detecting the storage prefix by checking if the character after
/is'd'or's'(line 172). This works for/dataand/sdcardbut is fragile—any path starting withdorswould be treated as a prefix.The defense-in-depth (rejecting
depth < 0) should still catch actual traversal attacks, so this is not a security vulnerability, but clearer prefix matching would improve maintainability.♻️ Consider explicit prefix matching
- if (!after_prefix && (*scan == 'd' || *scan == 's')) { - // Skip first path component (data or sdcard) - while (*scan && *scan != '/') scan++; - after_prefix = true; - continue; - } + if (!after_prefix) { + // Check for known storage prefixes + if (strncmp(scan, FTP_STORAGE_NAME_INTERNAL, strlen(FTP_STORAGE_NAME_INTERNAL)) == 0 || + strncmp(scan, FTP_STORAGE_NAME_SDCARD, strlen(FTP_STORAGE_NAME_SDCARD)) == 0) { + while (*scan && *scan != '/') scan++; + after_prefix = true; + continue; + } + }Apps/FTPServer/main/Source/MainView.cpp (1)
87-128: Consider potential log message truncation.The 512-byte buffer may truncate content when combining existing log history with a new message. With 50 lines of potentially varying length, this could silently lose log data.
For robustness, consider either:
- Dynamically sizing the buffer based on content length
- Adding bounds checking to detect truncation
This is a minor concern since typical FTP log messages are short.
| class TouchDriver { | ||
|
|
||
| TouchDriverHandle handle = nullptr; | ||
|
|
||
| public: | ||
|
|
||
| explicit TouchDriver(DeviceId id) { | ||
| assert(tt_hal_touch_driver_supported(id)); | ||
| handle = tt_hal_touch_driver_alloc(id); | ||
| assert(handle != nullptr); | ||
| } | ||
|
|
||
| ~TouchDriver() { | ||
| tt_hal_touch_driver_free(handle); | ||
| } | ||
|
|
||
| bool getTouchedPoints(uint16_t* x, uint16_t* y, uint16_t* strength, uint8_t* count, uint8_t maxCount) const { | ||
| return tt_hal_touch_driver_get_touched_points(handle, x, y, strength, count, maxCount); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Missing copy/move operations (Rule of Five violation).
TouchDriver owns a resource (handle) via RAII but doesn't delete or define copy/move operations. If an instance is copied, both copies will hold the same handle, causing double-free in the destructors.
🐛 Proposed fix: Delete copy/move operations
class TouchDriver {
TouchDriverHandle handle = nullptr;
public:
explicit TouchDriver(DeviceId id) {
assert(tt_hal_touch_driver_supported(id));
handle = tt_hal_touch_driver_alloc(id);
assert(handle != nullptr);
}
~TouchDriver() {
tt_hal_touch_driver_free(handle);
}
+ // Non-copyable, non-movable
+ TouchDriver(const TouchDriver&) = delete;
+ TouchDriver& operator=(const TouchDriver&) = delete;
+ TouchDriver(TouchDriver&&) = delete;
+ TouchDriver& operator=(TouchDriver&&) = delete;
+
bool getTouchedPoints(uint16_t* x, uint16_t* y, uint16_t* strength, uint8_t* count, uint8_t maxCount) const {
return tt_hal_touch_driver_get_touched_points(handle, x, y, strength, count, maxCount);
}
};| class PixelBuffer { | ||
| uint16_t pixelWidth; | ||
| uint16_t pixelHeight; | ||
| ColorFormat colorFormat; | ||
| uint8_t* data; | ||
|
|
||
| public: | ||
|
|
||
| PixelBuffer(uint16_t pixelWidth, uint16_t pixelHeight, ColorFormat colorFormat) : | ||
| pixelWidth(pixelWidth), | ||
| pixelHeight(pixelHeight), | ||
| colorFormat(colorFormat) | ||
| { | ||
| data = static_cast<uint8_t*>(malloc(pixelWidth * pixelHeight * getPixelSize())); | ||
| assert(data != nullptr); | ||
| } | ||
|
|
||
| ~PixelBuffer() { | ||
| free(data); | ||
| } |
There was a problem hiding this comment.
Missing copy/move semantics deletion.
PixelBuffer owns raw memory via malloc/free but doesn't delete copy/move constructors and assignment operators. Copying a PixelBuffer would lead to double-free bugs.
🐛 Proposed fix - add after line 24
// Non-copyable, non-movable (owns raw memory)
PixelBuffer(const PixelBuffer&) = delete;
PixelBuffer& operator=(const PixelBuffer&) = delete;
PixelBuffer(PixelBuffer&&) = delete;
PixelBuffer& operator=(PixelBuffer&&) = delete;| default: | ||
| // TODO: Crash with error | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Returning 0 for unknown color format is dangerous.
If an unsupported ColorFormat is passed, getPixelSize() returns 0, which would cause:
- Zero-size allocation in the constructor
- Potential division-by-zero in size calculations
Consider asserting or using a safe fallback.
🐛 Proposed fix
default:
- // TODO: Crash with error
- return 0;
+ assert(false && "Unsupported color format");
+ return 1; // Fallback to prevent zero-size issues if assertions are disabled📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| default: | |
| // TODO: Crash with error | |
| return 0; | |
| } | |
| default: | |
| assert(false && "Unsupported color format"); | |
| return 1; // Fallback to prevent zero-size issues if assertions are disabled | |
| } |
| case COLOR_FORMAT_MONOCHROME: | ||
| *address = (uint8_t)((uint16_t)r + (uint16_t)g + (uint16_t)b / 3); | ||
| break; |
There was a problem hiding this comment.
Operator precedence bug in monochrome calculation.
Division has higher precedence than addition, so b / 3 is evaluated first instead of (r + g + b) / 3. This produces incorrect grayscale values.
🐛 Proposed fix
case COLOR_FORMAT_MONOCHROME:
- *address = (uint8_t)((uint16_t)r + (uint16_t)g + (uint16_t)b / 3);
+ *address = (uint8_t)(((uint16_t)r + (uint16_t)g + (uint16_t)b) / 3);
break;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case COLOR_FORMAT_MONOCHROME: | |
| *address = (uint8_t)((uint16_t)r + (uint16_t)g + (uint16_t)b / 3); | |
| break; | |
| case COLOR_FORMAT_MONOCHROME: | |
| *address = (uint8_t)(((uint16_t)r + (uint16_t)g + (uint16_t)b) / 3); | |
| break; |
| response = requests.get(url) | ||
| if response.status_code != 200: | ||
| print_error("Run failed") | ||
| else: | ||
| print_status_success(f"Received device info:") | ||
| print(response.json()) | ||
| except requests.RequestException as e: | ||
| print_status_error(f"Device info request failed: {e}") |
There was a problem hiding this comment.
HTTP requests missing timeout parameter.
All requests.get/requests.post/requests.put calls lack a timeout parameter, which could cause the script to hang indefinitely if the device is unreachable or unresponsive.
🐛 Proposed fix for get_device_info
try:
- response = requests.get(url)
+ response = requests.get(url, timeout=10)
if response.status_code != 200:Apply similar fixes to other requests calls at lines 556, 580, and 600.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| response = requests.get(url) | |
| if response.status_code != 200: | |
| print_error("Run failed") | |
| else: | |
| print_status_success(f"Received device info:") | |
| print(response.json()) | |
| except requests.RequestException as e: | |
| print_status_error(f"Device info request failed: {e}") | |
| response = requests.get(url, timeout=10) | |
| if response.status_code != 200: | |
| print_error("Run failed") | |
| else: | |
| print_status_success(f"Received device info:") | |
| print(response.json()) | |
| except requests.RequestException as e: | |
| print_status_error(f"Device info request failed: {e}") |
🧰 Tools
🪛 Ruff (0.14.14)
541-541: Probable use of requests call without timeout
(S113)
545-545: f-string without any placeholders
Remove extraneous f prefix
(F541)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Apps/FTPServer/main/Source/FtpServerCore.cpp (2)
1474-1474: Use atomic operations forftp_stopflag.The
ftp_stopmember is declared asstd::atomic<uint8_t>in the header, but the code uses plain assignment and comparison operators throughout. While this may work due to implicit conversions, it's better practice to use explicit.load()and.store()methods with appropriate memory ordering for clarity and to ensure correct semantics.Affected locations:
- Line 1420:
ftp_stop = 0→ftp_stop.store(0, std::memory_order_release)- Line 1474:
if (ftp_stop)→if (ftp_stop.load(std::memory_order_acquire))- Line 1661:
ftp_stop = 1→ftp_stop.store(1, std::memory_order_release)- Line 1668:
ftp_stop == 1→ftp_stop.load(std::memory_order_acquire) == 1- Line 1698:
if (ftp_stop || ...)→if (ftp_stop.load(std::memory_order_acquire) || ...)- Line 1799:
ftp_stop = 1→ftp_stop.store(1, std::memory_order_release)- Line 1852:
ftp_stop = 0→ftp_stop.store(0, std::memory_order_release)
1861-1887: Consider makingftp_mutexmutable for const-correct mutex usage.The
isEnabled()andgetState()methods are declaredconstbut callxSemaphoreTake/xSemaphoreGiveonftp_mutex. While this works in practice, it's technically const-incorrect since the mutex state is being modified.Suggested fix in header
- SemaphoreHandle_t ftp_mutex; + mutable SemaphoreHandle_t ftp_mutex;
Shadowtrance
changed the title
1 participant
Title says it all.
Finally getting around adding the FTP Server if you'll have it (just finished making some minor adjustments and fixes)
And the Mystify screensaver demo adapted to the graphics demo app because why not? 😂
TwoEleven (changes) and Snake...
Adds keyboard control and selectiondialog and other misc changes
Add Snake game :)
Both requires fixes to be applied to tt_app_alertdialog and tt_app_selectiondialog to work.
And a single symbol export, lv_group_set_editing
So builds here WILL fail until then.
EDIT: The above fixes have been included in my latest Tactility firmware PR.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.