hi3516ev200: sensor_dvp opt-in, SC2235 DVP sensor config and IQ profile - #2446
Conversation
PR Summary by QodoAdd Imou Cue 2 IPC-C22EN board support
AI Description
Diagram
High-Level Assessment
Files changed (16)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for this — four cameras, a CH341A recovery path and a DVP sequence you clearly worked out the hard way is real work, and none of what follows says otherwise. But almost all of it is filed in the wrong repository, and a few parts do not do what the description says they do. Details are inline; the shape of it is here.
This belongs in OpenIPC/builder
Not a technicality. general/overlay/ is copied verbatim into every image this tree builds, so as written, every camera of every vendor gets majestic.yaml pointing at an SC2235, rc.local writing Hi3516EV200 PWM registers, LED scripts driving GPIO 0/4/9, and a customizer.sh that fw_setenvs them into believing they are an Imou Cue 2 — including the upgrade URL, on first boot, permanently.
Builder already has a home for every file here, at the identical paths: devices/hi3516ev200_lite_imou-cue2-c22en/general/overlay/.... 96 devices ship a customizer.sh that way, 18 a gpio.conf, 15 a muxes.sh, and 89 an excludes list.
Three things I do not think work as intended
ipctoolis not on the image.general/package/ipctool/ipctool.mkinstallsipcinfoand nothing else, soBR2_PACKAGE_IPCTOOL=ysupplies no binary of that name./usr/sbin/ipctoolis a symlink toextutils, which downloads the tool from alatestrelease into/tmpon first use. I suspect your four cameras work for a different reason, and it is worth finding out which — otherwise the fix is load-bearing on your bench and absent in the field.nrxsetis a binary with no source, which is a hard gate here (pr_compliance_checklist.yaml), and I cannot review what I cannot read. Nothing in firmware or builder reads/etc/ir/either.- The majestic restart re-opens several failures
S95majesticexists to prevent.
The SC2235 patch belongs upstream
HISILICON_OPENSDK_SITE is $(call github,openipc,openhisilicon,…), and the file being patched is checked in there. Please open it as a PR against OpenIPC/openhisilicon, which fixes it for everyone rather than only for boards built from this tree.
On the ISP not tracking gain
The behaviour the boosted 3DNR compensates for is worth its own issue on OpenIPC/majestic with your before/after, rather than being absorbed into a tuning profile here. A camera-side workaround leaves it in place for every other board.
Mechanical blockers
- The defconfig needs registering in
ALL_BOARDSin.github/scripts/ci-matrix.py(orUNBUILT_BOARDSwith a reason) or--self-testfails the merge. general/scripts/excludes/hi3516ev200_lite.listbreaks the generic board — see inline.
What can stay
The load_hisilicon sensor_dvp opt-in is genuinely good and family-wide. Split that into its own small PR and it would be welcome as-is.
Separately, and not your problem: this PR exposed several gaps in our own review rules, which I have written up in #2452.
Both come from the bot review on the pull request that added them, and both were right. The patch rule claimed no patch in the tree targets an OpenIPC-org repository. That is false: libevent-openipc carries 0001-CMakeLists-remove-epoll_pwait2-check.patch against github.com/OpenIPC/libevent. My own check missed it because the SITE grep matched LIBEVENT_OPENIPC_SITE_METHOD first and took that line. Counted properly it is 22 of 23, not 23 of 23 -- and the one exception is the legitimate shape the rule should have described from the start: one hunk, a build fix, obviously temporary, riding ahead of a bump. So the gate no longer refuses every such patch. It asks whether the patch is a bridge or a substitute -- is the upstream pull request named, is it the minimal delta, does it go at the next *_VERSION bump -- which still catches #2446, whose 114-entry table replacement is none of those. The reachability rule was worse than the bot said. It asked that an added file be "read at its installed path", but most of this tree's data is never named by anyone: hisilicon-osdrv-hi3516ev200 installs files/sensor/config/*.ini by wildcard and the one that gets used is chosen at runtime from the configured sensor, a module is reached by `modprobe <name>`, an IQ profile through isp.iqProfile. A new sensor ini would have failed the gate while working perfectly. It also contradicted a gate five entries above it in the same file, which already exempts a data file landing under a path an existing install rule globs. Both now say the same thing: trace the mechanism -- path, basename, a name built at runtime, or an install glob a convention selects from -- and raise only the residue, which is what /etc/ir/ actually is. A rule that fires on correct work costs more than the rule is worth, and best_practices.md already carries one warning about a false finding this project has seen made. These two would have been the next.
#2446 (Imou Cue 2 board support) is the shape of PR these files exist to catch, and the automated review caught eight of twelve problems in it. The four it missed, and the five it half-caught, are all gaps in what we wrote down rather than gaps in the reviewer. The binaries gate enumerated .ko/.so/.a/.bin, so an executable with no extension at all -- general/overlay/etc/ir/nrxset -- was reported back as "contains no textual changes", which is exactly what a blob looks like to anything reading a diff as text. The gate now keys on the "Binary files ... differ" marker, which is the one signal that survives, and says outright that general/overlay/ is never the place for a compiled artefact. A register table copied out of a vendor's driver gets the same treatment: it is the same provenance failure written in C. Three things the tree already owns had no rule at all. ipctool is not on a camera -- the package installs ipcinfo, and /usr/sbin/ipctool is an extutils arm that curls the tool from a "latest" release into tmpfs at first use -- so calling it from rc.local twelve seconds into boot on a WiFi-only board is four bad properties at once. An overlay file needs no Config.in, so nothing caught /etc/ir/nrxset and nrx_night_06.txt, which nothing in firmware or builder reads and which the same PR's rc.local never invokes, whatever its comment says. And S95majestic carries an -s argument, a start-stop-daemon lookup, a ten-second wait for the sensor HAL and a trap '' HUP that its own comment measured at 25 deaths out of 25 on a hi3516ev200 -- all of which a hand-rolled killall-and-relaunch throws away, on that same SoC. A patch against a package this project owns is a bridge, not a substitute: the pull request against the owning repository is named, the patch is the minimal delta, and it goes at the next *_VERSION bump. libevent-openipc is the tree's one instance and the model; #2446's 114-entry table replacement is none of the three. OpenIPC/openhisilicon and OpenIPC/sensors join the redirect table, which listed only linux, builder, ipctool and majestic. The rest is telling contributors what already exists. customizer.sh, muxes.sh, gpio.conf, late-overlays.list and the excludes lists are a complete per-device mechanism, shipped per device in builder at the same paths -- and documented nowhere, which is why somebody writes an S01leds with GPIO 0, 4 and 9 typed into it instead. "Move it to builder" is half an answer; say which seam it becomes. Two smaller notes go with it: the "additive, so it changes nothing" exemption belongs to load_<vendor> case arms and not to the overlay, where etc/wireless/usb has quietly reached 46 retail-model arms; and an excludes list added here is keyed <model>_<variant>, which is the generic board's key too, so #2446's list took libsns_imx307.so away from hi3516ev200_lite itself. Lastly, devmem in a shipped script is now a question rather than a given. It is the worst way to record what is soldered to a pad -- undone by anything that later exports it, lost at the next reflash -- and the camera can be told instead, through the /api/v1/pinmux surface the pins page in OpenIPC/majestic-webui draws. Two of these rules would themselves have generated false findings, and the bot review on this PR caught both. The patch rule claimed no patch in the tree targets an OpenIPC-org repository; libevent-openipc does, and my check had missed it because the SITE grep matched LIBEVENT_OPENIPC_SITE_METHOD first. The reachability rule asked that a file be read at its installed path, but most of this tree's data is never named by anyone -- sensor inis arrive through a wildcard and are chosen at runtime, modules through modprobe, IQ profiles through isp.iqProfile -- and it contradicted a gate five entries above it in the same file. Both now define reachability semantically and agree with each other. A rule that fires on correct work costs more than the rule is worth.
0c53e52 to
0a6f959
Compare
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.
HeytalePazguato
changed the title
|
Thanks for the detailed review. Reworked and force-pushed as one commit on current master:
Tested by building firmware + builder#160 + openhisilicon#233 and flashing one Cue 2 clean (kernel + rootfs, overlay erased). Video, both streams in Frigate, night mode and two-way audio all work with no runtime scripts. |
Family-level pieces needed by DVP-wired SC2235 boards such as the Imou Cue 2 (board profile lives in OpenIPC/builder): - load_hisilicon: `fw_setenv sensor_dvp 1` selects DVP pad routing (YUV_TYPE0=1). With the MIPI default the I2C controller is muxed to pads not wired to a DVP sensor, so detection NACKs on every address. Unset, nothing changes for existing boards. - sensor/config/sc2235_i2c_dc_1080p.ini: 1080p 10-bit parallel input. Sync polarities chosen by sweeping all 16 combinations; only this one produces VI frame interrupts. - sensor/iq/sc2235.ini: IQ profile, installed next to the existing ones. The default.ini symlink is unchanged.
0a6f959 to
f5d801c
Compare
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
Approving. This is the right shape now, and the split was done properly — thank you for taking it apart rather than arguing for it.
What is left here is family-wide and additive:
load_hisilicon—YUV_TYPE0keeps its0default and only flips when a profile setssensor_dvp, so no existing ev200 board changes behaviour. The comment explains why the MIPI default leaves the I2C controller on unconnected pads, which is the part a future reader needs.sc2235_i2c_dc_1080p.ini— lands underfiles/sensor/config/, which the existing*.iniinstall rule already globs, and is selected at runtime by sensor name. No.mkchange needed and none made.sc2235.ini— a new IQ profile for a sensor that had none on this family, with the install line it does need.smart_sc2235/libsns_sc2235is already inHISILICON_OPENSDK_SENSORS_hi3516ev200, so there is a driver behind both files.
Nothing reaches general/overlay/, there is no binary, no defconfig to register, and the sensor patch went upstream reduced to its delta. Rebased onto master and the matrix is green across all 8 affected boards and every GCC row.
Two notes, neither blocking:
- The
sensor_dvpopt-in lives only inload_hisilicon, so a Goke board with a DVP-wired sensor cannot use it yet. Additive to add later if one turns up; not this PR's job. - This is inert until OpenIPC/openhisilicon#233 lands — the sensor still drives no pixel clock without it. That is fine for merging here, since nothing in this diff changes an existing board, but it does mean OpenIPC/builder#160 should not merge until #233 does, or the published Cue 2 image would have no video.
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.
hi3516ev200 + SC2235 over DVP. The firmware side landed as OpenIPC/firmware#2446 (sensor ini, IQ profile, sensor_dvp opt-in) and #2494 (the openhisilicon pin carrying the DVP pad fix), so the profile now has everything it depends on. customizer.sh sets the board identity in the bootloader environment and presets the streamer with cli -s; muxes.sh re-applies what a power cycle drops; gpio.conf names the pins; the wireless profile and the excludes list are the device's own.
2 participants
Summary
Family-level pieces for Hi3516EV200 boards with an SC2235 on the DVP pads. The board itself (Imou Cue 2, IPC-C22EN) is OpenIPC/builder#160.
load_hisilicon:fw_setenv sensor_dvp 1selects DVP pad routing (YUV_TYPE0=1). With the MIPI default the I2C controller is muxed to pads that a DVP sensor is not wired to, so detection NACKs on every address. Unset, nothing changes for existing boards.sensor/config/sc2235_i2c_dc_1080p.ini: 1080p 10-bit parallel input. Sync polarities picked by sweeping all 16 combinations; only this one produces VI frame interrupts.sensor/iq/sc2235.ini: IQ profile, installed next to the existing three.default.inistill points atimx307.ini.The SC2235 driver change is OpenIPC/openhisilicon#233. Once that lands,
HISILICON_OPENSDK_VERSIONneeds a bump for DVP boards to get video.Testing
Built from this branch + builder#160 + openhisilicon#233, flashed (kernel + rootfs, overlay erased) to one Imou Cue 2: video from cold boot with no runtime scripts, main + sub stream into Frigate, night mode switching, two-way audio.