Tab5 camera + other stuff#558
Conversation
Tab5 camera driver - SC2356 Custom SliderBox widget - slider with plus and minus buttons + value label and snapping Rtc Time service + rtc api Sdk release - only include drivers built for that specific target, eg: sc2356 driver is mipi / p4 only. No more hardcoded manual sdk cmakelists New function to find device by compatible string match. no more static cast bool wildness when trying to match a single device (like M5Stack PaperS3 for example)
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds SC2356 MIPI CSI camera support with kernel camera APIs, module packaging, Tab5 hardware configuration, and camera clock initialization. It introduces a shared RTC interface, migrates existing RTC drivers, and adds RTC-based system time synchronization. SDK release generation now detects available driver archives and produces driver-specific CMake files. The PR also adds an LVGL SliderBox widget, exports additional LVGL symbols, synchronizes GUI application hiding, changes internal data paths, and updates simulator documentation. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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
🧹 Nitpick comments (4)
Buildscripts/release-sdk.py (1)
124-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a post-replacement assertion to catch silent placeholder failures.
str.replace()silently does nothing if the placeholder text or indentation in the template drifts from the search string. The generated CMake file would then contain a literal# DRIVER_*_PLACEHOLDERcomment instead of actual driver entries, causing confusing downstream build failures that are hard to trace back to the generator.♻️ Proposed fix: assert placeholder was replaced
def generate_tactility_sdk_cmake(target_path, available_drivers): src = os.path.join('Buildscripts', 'TactilitySDK', 'TactilitySDK.cmake') with open(src) as f: content = f.read() components = "\n".join(f" {d}" for d in available_drivers) content = content.replace(" # DRIVER_COMPONENTS_PLACEHOLDER", components) + assert "# DRIVER_COMPONENTS_PLACEHOLDER" not in content, "Placeholder not replaced in TactilitySDK.cmake template" with open(os.path.join(target_path, 'TactilitySDK.cmake'), 'w') as f: f.write(content) def generate_tactility_sdk_top_cmakelists(target_path, available_drivers): src = os.path.join('Buildscripts', 'TactilitySDK', 'CMakeLists.txt') with open(src) as f: content = f.read() include_dirs = "\n".join(f' "Drivers/{d}/include"' for d in available_drivers) content = content.replace(" # DRIVER_INCLUDE_DIRS_PLACEHOLDER", include_dirs) + assert "# DRIVER_INCLUDE_DIRS_PLACEHOLDER" not in content, "Placeholder not replaced in CMakeLists.txt template" with open(os.path.join(target_path, 'CMakeLists.txt'), 'w') as f: f.write(content)Drivers/sc2356-module/source/sc2356.cpp (1)
312-322: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd VIDIOC_STREAMOFF before unmapping buffers in error cleanup path.
When
jpeg_new_encoder_enginefails (line 305),VIDIOC_STREAMONhas already succeeded (line 293), buterr_unmapmunmaps buffers without first stopping the stream. Whileclose(state->fd)in theerr_closepath will eventually tear down streaming, callingSTREAMOFFbefore munmap is the correct V4L2 cleanup sequence and avoids potential DMA-into-unmapped-buffer races on implementations that don't defer cleanup to fd close.🔧 Proposed fix: stop stream before unmapping
err_unmap: + { + int type = V4L2_BUF_TYPE_VIDEO_CAPTURE; + ioctl(state->fd, VIDIOC_STREAMOFF, &type); + } for (int i = 0; i < VIDEO_BUFFER_COUNT; i++) { if (state->buffers[i]) munmap(state->buffers[i], state->buf_lengths[i]); }Devices/m5stack-tab5/Source/Configuration.cpp (1)
286-286: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider logging when camera oscillator init fails.
initCameraOsc()logs internally on failure, butinitBoot()continues silently. This is likely intentional (board functions without camera), but consider whether a warning at theinitBoot()level would help diagnose camera issues at boot.Tactility/Source/service/rtctime/RtcTimeService.cpp (1)
75-98: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
localtime_rhere for thread safety.localtimeshares static storage, so concurrent callers can race and write the wrong time to the RTC.localtime_ris the safe drop-in replacement available in this build.Source: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ef68af0-790f-46b3-bdfe-c5b8b32cef01
📒 Files selected for processing (43)
Buildscripts/TactilitySDK/CMakeLists.txtBuildscripts/TactilitySDK/TactilitySDK.cmakeBuildscripts/release-sdk.pyDevices/generic-esp32p4/devicetree.yamlDevices/m5stack-tab5/CMakeLists.txtDevices/m5stack-tab5/Source/Configuration.cppDevices/m5stack-tab5/device.propertiesDevices/m5stack-tab5/devicetree.yamlDevices/m5stack-tab5/m5stack,tab5.dtsDocumentation/README.mdDrivers/bm8563-module/include/drivers/bm8563.hDrivers/bm8563-module/source/bm8563.cppDrivers/bm8563-module/source/module.cppDrivers/bm8563-module/source/symbols.cDrivers/rx8130ce-module/include/drivers/rx8130ce.hDrivers/rx8130ce-module/source/module.cppDrivers/rx8130ce-module/source/rx8130ce.cppDrivers/rx8130ce-module/source/symbols.cDrivers/sc2356-module/CMakeLists.txtDrivers/sc2356-module/LICENSE-Apache-2.0.mdDrivers/sc2356-module/README.mdDrivers/sc2356-module/bindings/smartsens,sc2356.yamlDrivers/sc2356-module/devicetree.yamlDrivers/sc2356-module/include/bindings/sc2356.hDrivers/sc2356-module/include/drivers/sc2356.hDrivers/sc2356-module/include/sc2356_module.hDrivers/sc2356-module/source/module.cppDrivers/sc2356-module/source/sc2356.cppDrivers/sc2356-module/source/symbols.cFirmware/idf_component.ymlModules/lvgl-module/source/symbols.cTactility/Include/Tactility/lvgl/SliderBox.hTactility/Include/Tactility/service/rtctime/RtcTime.hTactility/Include/Tactility/service/rtctime/RtcTimeService.hTactility/Source/Tactility.cppTactility/Source/lvgl/SliderBox.cppTactility/Source/service/rtctime/RtcTime.cppTactility/Source/service/rtctime/RtcTimeService.cppTactilityKernel/include/tactility/device.hTactilityKernel/include/tactility/drivers/rtc.hTactilityKernel/source/device.cppTactilityKernel/source/drivers/rtc.cppTactilityKernel/source/kernel_symbols.c
💤 Files with no reviewable changes (2)
- Drivers/bm8563-module/source/symbols.c
- Drivers/rx8130ce-module/source/symbols.c
KenVanHoeylandt
left a comment
KenVanHoeylandt
left a comment
There was a problem hiding this comment.
Thanks, that looks solid! I had very little feedback.
Fixed external app user data path. fix(gui): block app teardown until onHide() completes, preventing ELF unload racing a still-running app added camera device type and api
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ddd5114-efe6-42cb-a796-05248888befc
📒 Files selected for processing (11)
Buildscripts/release-sdk.pyDrivers/sc2356-module/include/drivers/sc2356.hDrivers/sc2356-module/source/module.cppDrivers/sc2356-module/source/sc2356.cppTactility/Private/Tactility/service/gui/GuiService.hTactility/Source/app/AppPaths.cppTactility/Source/service/gui/GuiService.cppTactility/Source/service/rtctime/RtcTimeService.cppTactilityKernel/include/tactility/drivers/camera.hTactilityKernel/source/drivers/camera.cppTactilityKernel/source/kernel_symbols.c
🚧 Files skipped from review as they are similar to previous changes (2)
- Tactility/Source/service/rtctime/RtcTimeService.cpp
- Drivers/sc2356-module/source/sc2356.cpp
|
Thanks for the new features! I like that you added the camera API. The build is red cause I cancelled it after resolving the merge conflict. |
2 participants
Tab5 camera driver - SC2356
Custom SliderBox widget - slider with plus and minus buttons + value label and snapping
Rtc Time service + rtc api
Sdk release - only include drivers built for that specific target, eg: sc2356 driver is mipi / p4 only.
No more hardcoded manual sdk cmakelists
New function to find device by compatible string match.
Summary by CodeRabbit