Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis pull request adds support for the CYD 3248S035C board. Changes: CI build matrix includes board id 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9569e53-cba1-4405-8ee9-bdeea7271f5d
📒 Files selected for processing (11)
.github/workflows/build.ymlDevices/cyd-3248s035c/CMakeLists.txtDevices/cyd-3248s035c/Source/Configuration.cppDevices/cyd-3248s035c/Source/devices/Display.cppDevices/cyd-3248s035c/Source/devices/Display.hDevices/cyd-3248s035c/Source/devices/SdCard.cppDevices/cyd-3248s035c/Source/devices/SdCard.hDevices/cyd-3248s035c/Source/module.cppDevices/cyd-3248s035c/cyd,3248s035c.dtsDevices/cyd-3248s035c/device.propertiesDevices/cyd-3248s035c/devicetree.yaml
| dependencies: | ||
| - Platforms/platform-esp32 | ||
| dts: cyd,3248s035c.dts |
There was a problem hiding this comment.
Normalize this YAML file to LF line endings.
yamllint reports invalid newline characters on this file. Please resave with \n (LF) to avoid CI lint failures.
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 1-1: wrong new line character: expected \n
(new-lines)
| auto display = std::make_shared<St7796Display>(std::move(configuration)); | ||
| return std::reinterpret_pointer_cast<tt::hal::display::DisplayDevice>(display); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify inheritance and cast sites for this display type.
rg -nP --type=cpp --type=h 'class\s+St7796Display\s*:\s*public\s+tt::hal::display::DisplayDevice'
rg -nP --type=cpp --type=h 'reinterpret_pointer_cast<tt::hal::display::DisplayDevice>|static_pointer_cast<tt::hal::display::DisplayDevice>'Repository: TactilityProject/Tactility
Length of output: 1887
🏁 Script executed:
# Find St7796Display class definition
rg -nP --type=cpp --type=h 'class\s+St7796Display'Repository: TactilityProject/Tactility
Length of output: 160
🏁 Script executed:
# Find DisplayDevice class definition to understand the hierarchy
rg -nP --type=cpp --type=h 'class\s+DisplayDevice' | head -20Repository: TactilityProject/Tactility
Length of output: 166
🏁 Script executed:
# Look for the actual file containing St7796Display and examine its definition
fd -type f -name '*.h' -o -name '*.cpp' | xargs grep -l 'St7796Display' | head -10Repository: TactilityProject/Tactility
Length of output: 240
🏁 Script executed:
# Find EspLcdDisplay class definition and its base class
rg -nP --type=cpp --type=h 'class\s+EspLcdDisplay'Repository: TactilityProject/Tactility
Length of output: 414
🏁 Script executed:
# Check the Display.cpp file for the complete context around line 35
cat -n Devices/cyd-3248s035c/Source/devices/Display.cpp | sed -n '1,45p'Repository: TactilityProject/Tactility
Length of output: 1389
Use static_pointer_cast instead of reinterpret_pointer_cast for upcasting.
Line 35 performs an unsafe cast for what should be a normal polymorphic upcast. St7796Display inherits from EspLcdDisplay, which inherits from tt::hal::display::DisplayDevice. The reinterpret_pointer_cast treats the pointer bits directly without adjustment, which is unsafe and can cause undefined behavior. Use static_pointer_cast for compile-time known polymorphic upcasts.
🛠️ Safe cast fix
- return std::reinterpret_pointer_cast<tt::hal::display::DisplayDevice>(display);
+ return std::static_pointer_cast<tt::hal::display::DisplayDevice>(display);| #include "Display.h" | ||
|
|
||
| #include <Tactility/hal/display/DisplayDevice.h> | ||
| #include <memory> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where DisplayDevice is declared and whether this include path exists.
fd -i 'DisplayDevice.h'
rg -n -C2 --iglob '*.{h,cpp}' 'Tactility/hal/display/DisplayDevice.h|class DisplayDevice|namespace tt::hal::display' Devices TactilityKernel Firmware DriversRepository: TactilityProject/Tactility
Length of output: 22049
Remove the self-include of Display.h.
Line 3 includes itself (#include "Display.h"), which is redundant and can mask include hygiene issues. The include path <Tactility/hal/display/DisplayDevice.h> is valid and widely used across the codebase (50+ device files use it successfully). No forward declaration is necessary; keep the existing include as-is and simply remove the self-include.
🧰 Tools
🪛 Clang (14.0.6)
[error] 5-5: 'Tactility/hal/display/DisplayDevice.h' file not found
(clang-diagnostic-error)
1 participant
This pull request adds support for a new device, the CYD 3248S035C, to Tactility.
Summary by CodeRabbit
New Features
Chores