Skip to content

LilyGo T-Embed CC1101 Plus & Led Strip Driver - #670

Merged
KenVanHoeylandt merged 2 commits into
TactilityProject:mainfrom
Shadowtrance:tembed-led-strip
Oct 3, 2026
Merged

KenVanHoeylandt merged 2 commits into
TactilityProject:mainfrom
Shadowtrance:tembed-led-strip

Conversation

@Shadowtrance

@Shadowtrance Shadowtrance commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Adds LilyGo T-Embed CC1101 Plus device
Adds Led Strip driver and kernel API + Settings app
Fix toolbar close button double sending close when using nav custom callback
Symbol exports

Summary by CodeRabbit

  • New Features
    • Added support for configuring LED strips with solid colors, alternating colors, or gradients, including brightness and custom color controls.
    • Added LED strip settings to the app launcher on devices that support LED strips.
    • Added LED strip support for the LilyGO T-Embed CC1101 Plus, including startup settings for its built-in LEDs.
    • Added a light-strip icon for the new settings screen.

Adds LilyGo T-Embed CC1101 Plus device
Adds Led Strip driver and kernel API + Settings app
Fix toolbar close double send when using nav custom callback
Symbol exports
@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: dd1cc430-0886-449a-9826-405a340cad68
📥 Commits

Reviewing files that changed from the base of the PR and between 5bac1ad and bdbb45d.

📒 Files selected for processing (38)
  • .gitignore
  • Devices/lilygo-tembed-cc1101-plus/CMakeLists.txt
  • Devices/lilygo-tembed-cc1101-plus/LICENSE-Apache-2.0.md
  • Devices/lilygo-tembed-cc1101-plus/device.properties
  • Devices/lilygo-tembed-cc1101-plus/lilygo,tembed-cc1101-plus.dts
  • Devices/lilygo-tembed-cc1101-plus/module.yaml
  • Devices/lilygo-tembed-cc1101-plus/source/module.cpp
  • Drivers/esp32-led-strip-module/CMakeLists.txt
  • Drivers/esp32-led-strip-module/LICENSE-Apache-2.0.md
  • Drivers/esp32-led-strip-module/README.md
  • Drivers/esp32-led-strip-module/bindings/espressif,esp32-led-strip.yaml
  • Drivers/esp32-led-strip-module/include/bindings/esp32_led_strip.h
  • Drivers/esp32-led-strip-module/include/drivers/esp32_led_strip.h
  • Drivers/esp32-led-strip-module/include/esp32_led_strip_module.h
  • Drivers/esp32-led-strip-module/include/esp32_led_strip_rmt.h
  • Drivers/esp32-led-strip-module/include/esp32_led_strip_rmt_encoder.h
  • Drivers/esp32-led-strip-module/include/esp32_led_strip_types.h
  • Drivers/esp32-led-strip-module/module.yaml
  • Drivers/esp32-led-strip-module/source/esp32_led_strip.cpp
  • Drivers/esp32-led-strip-module/source/esp32_led_strip_rmt_dev.c
  • Drivers/esp32-led-strip-module/source/esp32_led_strip_rmt_encoder.c
  • Drivers/esp32-led-strip-module/source/module.cpp
  • Modules/lvgl-module/assets/generate-all.py
  • Modules/lvgl-module/include/lvgl/icons/shared.h
  • Modules/lvgl-module/source-fonts/material_symbols_shared_12.c
  • Modules/lvgl-module/source-fonts/material_symbols_shared_16.c
  • Modules/lvgl-module/source-fonts/material_symbols_shared_20.c
  • Modules/lvgl-module/source-fonts/material_symbols_shared_24.c
  • Modules/lvgl-module/source-fonts/material_symbols_shared_32.c
  • Modules/lvgl-module/source/widgets/toolbar.cpp
  • Tactility/Include/Tactility/settings/LedStripSettings.h
  • Tactility/Source/Tactility.cpp
  • Tactility/Source/app/ledstripsettings/LedStripSettings.cpp
  • Tactility/Source/app/settings/Settings.cpp
  • Tactility/Source/settings/LedStripSettings.cpp
  • TactilityKernel/include/tactility/drivers/led_strip.h
  • TactilityKernel/source/drivers/led_strip.cpp
  • TactilityKernel/source/symbols.c

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The pull request adds a kernel LED-strip API and an ESP32 RMT driver. It adds persistent LED-strip settings and a settings app with controls for output, pattern, colors, and brightness. The settings app is registered when an LED-strip device is present. The LilyGO T-Embed CC1101 Plus configuration includes an LED strip and restores its saved settings at boot. The LVGL shared icon fonts gain a light-strip glyph. Toolbar navigation callback updates now replace the existing close-button callback.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to bdbb4

The change adds LED strip support and fixes toolbar callback replacement. No actionable merge-blocking issue was identified in the review.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to bdbb4

The new hardware API has material lifecycle and failure-recovery risks. Shutdown can invalidate active LED-strip operations, and failed cleanup can discard ownership of hardware resources. The demonstrated scope is one device; remote exploitation or privilege escalation has not been established.

Retained concerns

  • Medium · security · inferred: The new LED-strip API does not coordinate active operations with device shutdown. Driver operations access shared heap-backed state without lifecycle serialization, while stop frees that state. The settings application holds a device reference, but the unchanged framework does not enforce the documented guarantee that references prevent stop. Concurrent shutdown can therefore invalidate an active operation and cause a stale-memory access or device-wide failure. This is a new consumer of an inherited lifecycle weakness; external attacker reachability is not established.
  • Medium · reliability · observed: The new RMT recovery protocol loses resource ownership on cleanup failure. Refresh returns on transmit or wait errors without reaching channel disable. Deletion returns on its first error, but driver stop ignores that result, frees its owning state, clears driver data, and reports success. A failed deletion consequently leaves no retained handle for retry or recovery, undermining failure containment and clean restart.
Security review details

Security Blast Radius

  • inferred — The demonstrated output authority is the configured strip on one device. Lifecycle memory hazards and unrecovered RMT allocations operate within the shared native runtime, so their containment may extend beyond incorrect LED output to device availability. Tenant, cross-service, and remote reachability are not established.

Security Findings and Attack Paths

  • inferred — A native caller able to overlap LED operations with device stop can cause operations to access state freed by stop. This is a conditional local-runtime path, not a verified remote attack or privilege escalation; admission of less-trusted code to these exports remains unresolved.

Trust Boundaries and Controls

  • observed — Module symbol resolution returns native addresses by name from started modules. Existing exports already include device lookup, driver lifecycle, and hardware-control operations, and existing driver wrappers also accept raw pointers. The new LED exports expand functionality within that convention; these facts alone do not establish a newly bypassed authorization boundary.

Resilience and Maintainability Implications

  • observed — The refresh and deletion error paths do not preserve a complete recovery transition: disable may be skipped, deletion may stop early, and driver stop still clears ownership and reports success. Successful startup and normal refresh ordering do not resolve these failure-state gaps.

Hardening Proposals

  • proposed — Define and enforce a per-device operation and lifetime protocol that prevents shutdown from freeing state while operations are active. Preserve ownership when cleanup fails, propagate teardown errors, and provide an explicit disable or recovery path after transmission failure.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 1.28% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 78 functions across 27 files. (11 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly names the new LilyGo T-Embed CC1101 Plus device and LED strip driver, which are the main changes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 1.28% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 78 functions across 27 files. (11 skipped: 11 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@KenVanHoeylandt
KenVanHoeylandt merged commit ffab897 into TactilityProject:main Oct 3, 2026
66 checks passed
@Shadowtrance
Shadowtrance deleted the tembed-led-strip branch October 3, 2026 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants