Add Imou Cue 2 (IPC-C22EN) device profile - #160
Conversation
PR Summary by QodoAdd Imou Cue 2 Hi3516EV200 device profile
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
Thanks for this — the profile is well shaped and four tested cameras is a good basis. The problem is that it is only half of the change, and two of the three dependencies on the firmware side do not need to exist. Requesting changes; details inline, plus the cross-cutting parts here.
Blocking: nothing here works without firmware#2446, which is still open
That PR carries the SC2235 DVP register sequence, sensor_dvp support in load_hisilicon, the sc2235 sensor ini and IQ profile, and the rtl8188fu-hi3516ev200-imou-cue2 stanza in /etc/wireless/usb. I ran this exclude list against what the build actually installs on firmware master. The complete set of sensor-related files that survive into the image is:
/usr/lib/sensors/libsns_sc2235.so
/etc/sensors/high-fps/imx335_1280x720_120fps.ini
/etc/sensors/high-fps/imx335_1296x972_64fps.ini
/etc/sensors/high-fps/imx335_1920x1080_55fps.ini
/etc/sensors/high-fps/imx335_2592x1944_45fps.ini
/etc/sensors/high-fps/imx335_800x480_240fps.ini
No sensor config, no IQ profile at all, and no WiFi. So this cannot merge before its firmware half — but two of those dependencies are avoidable today, see the comments on customizer.sh and wpa_passphrase.
README row missing
Step 7 of CLAUDE.md: the device table in README.md needs a row. Worth clarifying the title's "IPC-C22EN / IPC-C22EP" too — Imou IPC-C22EP-S2 is already in the table as an SSC325DE board, so if the C22EP really is this board it needs a clones-table row rather than a second meaning for the same model number.
Worth reconsidering in firmware#2446
etc/init.d/S01leds, etc/init.d/S99leds, etc/majestic.yaml, etc/rc.local, etc/ir/* and general/scripts/excludes/hi3516ev200_lite.list all sit in firmware's shared overlay, so they would land on every OpenIPC board, not just this one — that is exactly the split builder exists to avoid, and all of them belong in this PR instead. What genuinely belongs in firmware is the opensdk SC2235 patch, the load_hisilicon change and the osdrv sensor config + IQ ini. Note also that replacing the SC2235 init table changes that driver for every board using the sensor, across hi3516cv200 and hi3516cv300 as well as this family, so it deserves its own testing note. etc/ir/nrxset is committed empty there. Finally, the two PRs disagree on the board name: hi3516ev200_lite_imou-cue2-c22en here versus hi3516ev200_lite_imou-cue2 there.
What already checks out
ci-matrix.py --self-test passes on the branch (116 devices, 15 smoke, 39 cases), so registration is automatic and nothing needs adding to the workflows. File modes and the device directory naming are right, and the exclude list correctly spares libsns_sc2235.so. I have not run a local build, so the 8 MB NOR fit is still unverified on my side.
Hi3516EV200 + SC2235 on the DVP pads, RTL8188FTV USB WiFi, 8 MB NOR. Everything board-specific lives here; the family-level pieces (sensor_dvp in load_hisilicon, SC2235 sensor ini and IQ profile) are in OpenIPC/firmware#2446, and the SC2235 DVP pad enables go to OpenIPC/openhisilicon. - customizer.sh: soc/sensor/sensor_dvp, upgrade URL, wlandev, and the majestic settings (sensor ini, IQ profile, mirror/flip, streams, night mode pins, two-way audio) as cli -s writes. - etc/wireless/usb: own copy with the rtl8188fu arm (GPIO 52 power). - gpio.conf: pin map (button 56, IR-cut 55, LEDs 0/9, IR 39, speaker 53, WiFi power 52). - S01leds/S99leds: red while booting, green when up, led_disabled=1 turns them off. - rc.local: PWM drive for the IR illuminator. - defconfig: WPA supplicant CLI + passphrase from Buildroot instead of a shell shim; libevent listed like every other defconfig. - excludes: generated from what the hi3516ev200 osdrv installs, keeping the SC2235 files plus iq/default.ini and its imx307.ini target.
c08bb35 to
487ed46
Compare
|
Thanks — reworked and force-pushed as one commit on current master:
Tested: built with firmware#2446 + openhisilicon#233 and flashed one camera clean (kernel + rootfs, overlay erased). Video, Frigate streams, night mode and two-way audio all work. |
…ile (#2446) An SC2235 wired to the DVP pads is not detected on this family: open_sys_config routes the pads from g_cmos_yuv_flag, and with the MIPI default the I2C controller is muxed to pads the sensor is not on, so every address NACKs and nothing is ever found. YUV_TYPE0 keeps its 0 default and only flips when a device profile sets sensor_dvp, so no existing board changes behaviour. The sensor config lands under files/sensor/config/, which the existing *.ini install rule already globs and which is selected at runtime by sensor name; the IQ profile is the first one this family has had for the SC2235 and gets the install line it needs. libsns_sc2235 is already in the hi3516ev200 sensor list, so there is a driver behind both files. This is inert until OpenIPC/openhisilicon#233 lands -- without it the sensor drives no pixel clock -- but nothing here touches an existing board, so it costs nothing to have the family support in place first. Originally submitted with a device profile, a shared-overlay rc.local, LED scripts, a customizer, an excludes list and a downstream patch against openhisilicon. Those went to OpenIPC/builder#160 and OpenIPC/openhisilicon#233 respectively, the latter reduced to the register delta rather than a replacement of the whole init table.
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
Approving on content — all ten findings from my earlier review are addressed, and the profile is now the shape a device profile should be.
What changed since:
- all three
wpa_supplicantsymbols are set, and thewpa_passphraseshim is deleted rather than carried - the profile ships its own
general/overlay/etc/wireless/usb, with an arm name that matchesfw_setenv wlandevexactly — so the adapter actually comes up instead of falling through to the bareexit 1 - 28
cli -slines carry what used to be a globalmajestic.yaml:.isp.sensorConfig,.isp.iqProfile, the nightMode pins, codec and fps, the second stream gpio.confnames the pins rather than leaving the numbers scattered through the scriptssensor_dvpis no longer a no-op: OpenIPC/firmware#2446 merged asce578244, soload_hisiliconreads it
One thing before this is merged, and it is not a change request. This should land after OpenIPC/openhisilicon#233. That PR's own comment says the stock sequence "leaves the SoC with no pixel clock" on this camera, so until it is in, a published hi3516ev200_lite_imou-cue2-c22en image would flash cleanly and show no video — which is a worse outcome for anyone who tries it than the profile not existing yet. Approving now so nothing is waiting on review; happy for it to merge the moment #233 is in.
Also worth noting that #233 has an open question of its own about whether the fix reaches gk7205v200, which has its own copy of the driver. That does not affect this profile — the Cue 2 is hi3516ev200 — but it may change what #233 ends up looking like.
openhisilicon c1f9eb5c is one commit on from the current pin: the SC2235 driver repeats its DVP pad setup once the sensor is streaming, with stronger drive on 0x3641, and starts streaming again. The stock sequence sets those enables before stream start, which on a DVP-wired board leaves the SoC with no pixel clock -- the sensor answers on the bus and no VI interrupts arrive. Measured rather than assumed. Building gk7205v200_lite at both pins in the same tree, with the SDK build directory cleared each time, changes four of 378 rootfs files: libsns_sc2235.so, the os-release build stamp, and open_mipi_rx.ko and open_wdt.ko, which embed __DATE__/__TIME__ and so differ between any two builds minutes apart -- the commit touches nothing under kernel/. So one functional file changes, and it is the sensor library. No board regresses, because nothing selects that sensor yet: no defconfig here and no builder device sets it, and the two builder devices that mention it prune it in exclusion lists. On a running gk7205v200 the library is on disk and mapped by no process. The Imou Cue 2 will be the first device to select it, and its contributor verified this exact commit on one. This is what #2446's sensor ini and IQ profile were waiting for, and what OpenIPC/builder#160 needs before a Cue 2 image is worth publishing.
2 participants
Summary
Device profile for the Imou Cue 2 (IPC-C22EN): Hi3516EV200, SC2235 on the DVP pads, RTL8188FTV USB WiFi, 8 MB NOR.
customizer.sh:soc/sensor/sensor_dvp, upgrade URL,wlandev, and the majestic settings ascli -swrites (sensor ini, IQ profile, mirror/flip, main + 640x360 sub stream, night mode pins and software light monitor, two-way audio).etc/wireless/usb: own copy with the rtl8188fu arm (GPIO 52 power).gpio.conf: button 56, IR-cut 55, LEDs 0/9, IR 39, speaker 53, WiFi power 52.S01leds/S99leds: red while booting, green when up;led_disabled=1turns them off.rc.local: PWM drive for the IR illuminator.iq/default.iniandimx307.ini. Also dropsmac80211(8188fu only needscfg80211) and the USB serial/net/gadget modules, which is what gets the rootfs to 5048/5120 KB.Dependencies
sensor_dvp, the SC2235 sensor ini and IQ profile.HISILICON_OPENSDK_VERSIONbump in firmware).Hardware notes
The stock firmware runs Dahua's signed U-Boot with RSA secure boot, and the UART console is locked. The first flash needs a CH341A on the NOR chip.
Testing
Built locally against firmware#2446 + openhisilicon#233 and flashed to one camera clean (kernel + rootfs, overlay erased). Video from cold boot with no runtime scripts, both streams in Frigate, night mode, two-way audio.