Merged
Conversation
Refactor USB descriptors to include UVC and CDC support with updated endpoint configurations.
- Add Python script for patching Signed-off-by: Daniel Kampert <DanielKampert@kampis-elektroecke.de>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (56)
📝 WalkthroughWalkthroughComprehensive refactor introducing composite USB device architecture (MSC, UVC, CDC), implementing soft-mount/remount storage APIs, standardizing SPIRAM-aware memory allocation across modules, updating UI terminology from Flash to Memory, expanding USB and network configurations, and adding supporting documentation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Pull request overview
This PR reworks the firmware’s USB subsystem toward a composite TinyUSB device (MSC + UVC + CDC), adds an automated ESP-IDF patching step to enable SPI LCD PSRAM mode support, and updates memory/storage handling to safely support MSC by soft-unmounting/remounting the VFS.
Changes:
- Enable/configure TinyUSB CDC and add a composite USB manager with command-queue based MSC/UVC enable/disable.
- Add a PlatformIO pre-build patch system (
scripts/patch.py) and a framework patch to addpsram_modesupport toesp_lcdSPI panel IO. - Introduce MemoryManager soft-unmount/remount APIs and update GUI/Lepton/network code to allocate buffers conditionally based on PSRAM availability; update related documentation (USB/Mem).
Reviewed changes
Copilot reviewed 55 out of 56 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| sdkconfig.release | Enables TinyUSB CDC in release config. |
| sdkconfig.debug | Updates ESP-IDF config (5.5.3) and enables/configures CDC + other options. |
| platformio.ini | Pins platform-espressif32 to a commit and adds pre-build patch script. |
| scripts/patch.py | Applies patches/*.patch idempotently to the framework package at build time. |
| patches/0001-*.patch | Adds psram_mode flag handling to ESP-IDF esp_lcd SPI panel IO. |
| main/main.cpp | Initializes USB manager during boot. |
| main/idf_component.yml | Adds espressif/esp-dsp dependency. |
| main/Application/Tasks/* | Logging level tweaks, task creation refactors, PSRAM-cap-aware allocations, GUI memory page/USB UI updates. |
| main/Application/Manager/USB/* | Composite USB manager, new CDC module, moved/rewritten descriptors, MSC/VFS handling updates. |
| main/Application/Manager/Memory/* | Adds soft-unmount/remount APIs; updates usage calculation and SD card handle naming. |
| main/Application/Manager/Network/* | Small refactors + allocation caps changes; SNTP sync interval fix. |
| docs/USBManager.adoc | Updates USB manager documentation (partially) for new behavior. |
| docs/MemoryMap.adoc | New comprehensive memory map documentation. |
| docs/MemoryManager.adoc | Documents new soft-unmount/remount APIs and revised MSC workflow. |
| .github/copilot-instructions.md | Adds rule to keep MemoryMap.adoc in sync with allocations/stacks. |
Comments suppressed due to low confidence (3)
main/Application/Tasks/GUI/UI/ui_settings.cpp:832
- Style: missing space after
if(should beif (...)to match the project’s formatting rules).
main/Application/Manager/USB/usbManager.cpp:401 - usbManager.h documents that enabling MSC/UVC is ignored when the USB cable is not connected, but USBManager_EnableMSC()/EnableUVC() currently enqueue the command unconditionally. This can lead to MSC soft-unmounting/locking the filesystem even when no host is present. Add an isCableConnected check (or update the API contract/docs) to prevent enabling classes unless the device is enumerated.
docs/USBManager.adoc:218 - This section still documents/examples
USB_Manager_Config_t(which was removed from the codebase) and describes runtime descriptor customization, but the implementation now uses fixed composite descriptors and a parameterless USBManager_Init(). Update these examples and the surrounding text so they match the new USB Manager API and descriptor approach.
**Example (MSC Mode):**
[source,cpp]
----
const USB_Manager_Config_t USB_MSC_Config = {
.Mode = USB_MODE_MSC,
.MountPoint = MemoryManager_GetStoragePath(),
.Manufacturer = "Kampi's Elektroecke",
.Product = "PyroVision",
.SerialNumber = "PV-12345678",
.FrameWidth = 0, // Not used in MSC mode
.FrameHeight = 0, // Not used in MSC mode
.FrameInterval = 0, // Not used in MSC mode
};
----
**Example (UVC Mode):**
[source,cpp]
----
const USB_Manager_Config_t USB_UVC_Config = {
.Mode = USB_MODE_UVC,
.MountPoint = NULL, // Not used in UVC mode
.Manufacturer = "Kampi's Elektroecke",
.Product = "PyroVision Thermal Webcam",
.SerialNumber = "PV-12345678",
.FrameWidth = 160, // Lepton thermal camera resolution
.FrameHeight = 120,
.FrameInterval = 1111111, // 9 FPS (10000000 / 9)
};
Comment on lines
+73
to
+74
| _DNS_Server_State.isRunning = true; | ||
|
|
There was a problem hiding this comment.
DNS_Server_Start() no longer sets _DNS_Server_State.isRunning before creating the task; instead the task sets it at startup. This introduces a race where DNS_Server_Stop() called right after Start() will early-return (isRunning is still false), leaving the socket open and the task running. Set isRunning=true in DNS_Server_Start() before xTaskCreatePinnedToCore (and remove the assignment in the task), so Stop() is always effective immediately after Start().
Suggested change
| _DNS_Server_State.isRunning = true; |
Comment on lines
+306
to
+309
| if (_USB_Manager_State.MonitoringTask != NULL) { | ||
| vTaskDelete(_USB_Manager_State.MonitoringTask); | ||
| _USB_Manager_State.MonitoringTask = NULL; | ||
| } |
There was a problem hiding this comment.
The USB monitoring task registers itself with the task watchdog via esp_task_wdt_add(NULL), but USBManager_Deinit() deletes the task via vTaskDelete(_USB_Manager_State.MonitoringTask) without first removing it from the watchdog. This can leave a stale WDT subscription and may trigger watchdog faults later. Before deleting the task, call esp_task_wdt_delete(_USB_Manager_State.MonitoringTask) (or implement a clean exit path where the task calls esp_task_wdt_delete(NULL) before self-deleting).
Comment on lines
+227
to
242
| _USB_Manager_State.StringDescriptors[0] = USB_LangID; | ||
| _USB_Manager_State.StringDescriptors[1] = CONFIG_DEVICE_MANUFACTURER; | ||
| _USB_Manager_State.StringDescriptors[2] = CONFIG_DEVICE_NAME; | ||
| _USB_Manager_State.StringDescriptors[3] = NULL; | ||
|
|
||
| ESP_LOGD(TAG, "USB Descriptors: VID:PID = 0x%04X:0x%04X", | ||
| get_Desc_Device()->idVendor, | ||
| get_Desc_Device()->idProduct); | ||
| ESP_LOGD(TAG, " Manufacturer: %s, Product: %s", | ||
| _USB_Manager_State.StringDescriptors[1], _USB_Manager_State.StringDescriptors[2]); | ||
|
|
||
| USB_Config.descriptor.device = get_Desc_Device(); | ||
| USB_Config.descriptor.full_speed_config = get_Desc_Config(); | ||
| USB_Config.descriptor.string = _USB_Manager_State.StringDescriptors; | ||
| USB_Config.descriptor.string_count = 3; | ||
|
|
There was a problem hiding this comment.
The device descriptor uses STRID_SERIAL (see descriptors.cpp), but USBManager_Init() sets StringDescriptors[3] = NULL and sets USB_Config.descriptor.string_count = 3. This means the host may request a serial string index that TinyUSB can't provide, causing enumeration issues. Provide a valid serial string entry and set string_count to include it (or set iSerialNumber=0 in the device descriptor).
Comment on lines
+45
to
+46
| #define SETTINGS_DEFAULT_USB_UVC_ENABLE false | ||
| #define SETTINGS_DEFAULT_USB_CDC_ENABLE false |
There was a problem hiding this comment.
SETTINGS_DEFAULT_USB_UVC_ENABLE and SETTINGS_DEFAULT_USB_CDC_ENABLE are defined twice here, which will trigger macro redefinition warnings/errors (depending on compiler flags) and is likely unintended. Remove the duplicate defines so each default is declared only once.
Suggested change
| #define SETTINGS_DEFAULT_USB_UVC_ENABLE false | |
| #define SETTINGS_DEFAULT_USB_CDC_ENABLE false |
Comment on lines
+1
to
+2
| From c87755f74b4379079ae46dbcd736c5f03d3091f4 Mon Sep 17 00:00:00 2001 | ||
| From: Daniel Kampert <DanielKampert@kampis-elektroecke.de> |
There was a problem hiding this comment.
This patch file begins with a UTF-8 BOM (the first line starts with an invisible character before "From"). git apply can fail to parse patches with a BOM on the first line. Save the patch without BOM (UTF-8 without signature) so the pre-build patch script can apply it reliably.
Comment on lines
+1
to
+5
| #include <class/msc/msc.h> | ||
| #include <class/video/video.h> | ||
|
|
||
| #include "descriptors.h" | ||
|
|
There was a problem hiding this comment.
This new source file is missing the project’s required GPL/license header block at the top (every .cpp/.h/.py file should include it). Add the standard header so licensing stays consistent across the codebase.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Improvements