diff --git a/.pr_agent.toml b/.pr_agent.toml index cf27cd623e..27c8b61bf6 100644 --- a/.pr_agent.toml +++ b/.pr_agent.toml @@ -49,6 +49,16 @@ Prioritise, in order: family. A sensor, I2C address, GPIO number, or resolution that is true only of the contributor's bench must not become the default for everyone else. This breaks already deployed cameras at upgrade time, which is the worst failure mode this project has. + "Additive, so it changes no existing board" is the right exemption for a load_ + sensor case arm and is not a licence for the shared overlay: etc/wireless/usb has + accumulated 46 arms keyed on retail model names while 18 builder devices override the + whole file, and those 46 are grandfathered, not precedent. + Question a devmem write in a shipped script. It used to be the only way to tell a + camera something is soldered to a pad; majestic now owns the pads through its pins + configuration and /api/v1/pinmux, which put the choice back at startup and after a + reload, and drive a PWM lamp without anyone computing a duty cycle. A devmem line + under general/overlay/ has to say why configuration cannot express it and - since + rc.local is shared - why every other camera should execute it. 2. Toolchain-wide flags added without measurement. BR2_TARGET_OPTIMIZATION is appended to TOOLCHAIN_WRAPPER_OPTS in Buildroot's toolchain/toolchain-wrapper.mk, so it is baked @@ -65,14 +75,40 @@ Prioritise, in order: URL the .mk actually fetches. Binaries lifted out of a camera's factory firmware are not a source of supply: there is no source, no vendor SDK, and no way to rebuild them for the next kernel. + Detect a binary by the DIFF MARKER, never the extension. A hunk rendered as + "Binary files a/... and b/... differ" is a blob whatever it is named - an executable + with no extension at all reads to a text-diff summariser as "contains no textual + changes", and that is how one shipped past review into general/overlay/, which is + copied verbatim into every image. Report it, and say you cannot see its contents. + A register initialisation table copied out of a camera vendor's own driver is the same + provenance failure written in C: unverifiable against a datasheet, and replacing a + whole table rather than adding the deltas a board needs changes every other board + using that sensor. + A new *.patch is fine against a third-party upstream (every patch in the tree today is + - ffmpeg, mbedtls, vtund, libre, siproxd, zerotier, libwebsockets, f2fs-tools, the + Realtek drivers) and is a finding when the package's *_SITE resolves to an openipc + repository, because there the fix has somewhere better to go and a *_VERSION bump + silently drops it. 4. Changes that belong to a different repository. Kernel code and kernel patches belong to OpenIPC/linux. Support for one retail camera model belongs to OpenIPC/builder, under devices/common/br-ext-chip-/ - general/overlay/usr/sbin/sysupgrade already encodes this split, routing lite|ultimate|neo to firmware and everything else to - builder. Hardware probing and bring-up tools belong to OpenIPC/ipctool. Bugs in the - majestic streamer belong to majestic's maintainers. Redirecting a contributor is a + builder. Hardware probing and bring-up tools belong to OpenIPC/ipctool. HiSilicon + sensor drivers and open SDK code belong to OpenIPC/openhisilicon, which is what + hisilicon-opensdk fetches; Sigmastar sensor drivers belong to OpenIPC/sensors. Bugs in + the majestic streamer belong to majestic's maintainers. Redirecting a contributor is a normal, useful review outcome; say which repo and why. + Say which SEAM too, not only which repo. The tree already has one for every + board-specific thing, and a contributor who has not been shown it writes a new init + script with the pin numbers typed in: /usr/share/openipc/customizer.sh and + /usr/share/openipc/muxes.sh (both run by S30customizer), the declarative pin map + /usr/share/openipc/gpio.conf, late-overlays.list, and + general/scripts/excludes/_.list. Builder ships all of them per device + at those same paths under devices//. An excludes list added to THIS repo is not + per-board - the key is _, which the generic board answers to as well. + A comment explaining why a workaround is needed has usually already named the + repository that owns the fix. Read it as a redirect and quote it back. 5. Monkey-patching in place of a fix. Dynamic-linker injection shims - a library forced ahead of the real one through the linker's LD_* environment knob, spelled out in @@ -109,6 +145,28 @@ Prioritise, in order: builds is not compiled by CI, so nothing proves it even builds. And a diff must do what its title says: a PR adding one sensor has no business repointing a SITE that every board of that vendor consumes. + The overlay's mirror image of this needs saying separately, because general/overlay/ + needs no Config.in and no .mk - anything dropped there ships, so a file nothing on the + camera ever opens passes every dead-code check while costing every board of every + vendor, several of which sit within 32 KB of their squashfs cap. Grep the installed + path of an added overlay file; if nothing reads it, that is a finding. + +8. Commands a shipped script calls must exist on the image. A camera has no package + manager and often no network, and a missing command does not fail loudly - sh prints + "not found" to a console nobody reads and carries on, so the script reports success + having done nothing. Busybox applets and binaries installed by a package the board's + defconfig selects are the whole set. ipctool is the trap that looks like an exception: + general/package/ipctool/ipctool.mk installs ipcinfo and nothing else, so + BR2_PACKAGE_IPCTOOL=y puts no "ipctool" on the image; /usr/sbin/ipctool is a symlink to + extutils, whose ipctool arm downloads the tool from the ipctool repository's "latest" + release into /tmp on first use. That is a convenience for a person at a shell, and in + a boot script it is a network fetch during boot, from a floating tag, repeated every + boot because /tmp is tmpfs, and a silent no-op until the network is up. + Relatedly, a shipped script bounces the streamer with /etc/init.d/S95majestic restart. + A hand-rolled killall-and-relaunch loses the -s argument, the start-stop-daemon -x + lookup, the ten-second wait for the old process to release the sensor HAL, and the + trap '' HUP that S95majestic carries because the disposition survives exec() where a + handler does not. Do not report: speculation about whether a change was written with AI assistance - judge the diff on blast radius, provenance, evidence, and repo ownership, never on writing diff --git a/CLAUDE.md b/CLAUDE.md index a44e267342..d516dc0210 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -35,6 +35,8 @@ else. Check this first — it costs a minute and can save an evening. | Kernel source and kernel patches | [OpenIPC/linux][linux] | | Support for one specific retail camera model | [OpenIPC/builder][builder] | | Probing, identification, bring-up tooling | [OpenIPC/ipctool][ipctool] | +| HiSilicon sensor drivers and open SDK code | [OpenIPC/openhisilicon][openhisilicon] | +| Sigmastar sensor drivers | [OpenIPC/sensors][sensors] | | A bug in the video stream itself | majestic's maintainers, not a shim here | | Documentation, device notes, how-tos | [OpenIPC/wiki][wiki], [docs][docs] | @@ -148,15 +150,31 @@ Summarised from `pr_compliance_checklist.yaml`; the reasoning is in `best_practi every caller, is invisible to anyone debugging the process, and freezes the real bug in place. - **No binaries without buildable source.** Nothing lifted out of a factory image, nothing a script in the same pull request generated. A `PROVENANCE.md` documents the problem; it does not - solve it. + solve it. The test is the diff marker — anything rendered `Binary files ... differ` — not the + extension; an executable with no extension is still a blob, and `general/overlay/` is never the + place for one. A register table copied out of a camera vendor's driver is the same problem + written in C. +- **No patches against OpenIPC's own packages.** Every `*.patch` in the tree targets a third-party + upstream. If a package's `*_SITE` is an `openipc` repository, the fix is a pull request there + and a `*_VERSION` bump here. +- **Nothing ships that nothing runs.** An overlay file needs no `Config.in`, so nothing catches a + file the camera never opens. Grep the installed path before adding one. +- **Shipped scripts call only what the image contains.** Busybox applets, or binaries a package in + the board's defconfig installs. `ipctool` looks installed and is not: the package ships + `ipcinfo`, and `/usr/sbin/ipctool` is an `extutils` arm that downloads the tool at first use. - **No runtime patching of vendor blob memory**, and no `kallsyms` address hooking. - **`*_SITE` points at an OpenIPC-org repository or a documented upstream**, never a personal fork, and a `*_VERSION` bump never becomes less specific than the pin it replaces — a full 40-character SHA for anything new. `Config.in` help text must name the URL the `.mk` fetches. - **No board-specific value in shared files.** A sensor name, I2C address, GPIO number, resolution, MAC prefix or IP literal does not belong in `general/overlay/` or in a shared - `load_` default. Extending a case arm, or adding a sensor to a package's list, is - additive and fine. + `load_` default. Extending a `load_` case arm, or adding a sensor to a + package's list, is additive and fine; that exemption does not extend to the overlay. + Per-board values have their own seams — `/usr/share/openipc/customizer.sh` and + `/usr/share/openipc/muxes.sh`, both run by `S30customizer`; the declarative pin map + `/usr/share/openipc/gpio.conf`; `late-overlays.list`; and + `general/scripts/excludes/_.list`. All of them live per-device in + OpenIPC/builder under `devices//`, at the same paths. ## Verifying a change @@ -341,4 +359,6 @@ Maintainers append the pull request number at merge time; you do not need to. [docs]: https://docs.openipc.org [ipctool]: https://github.com/OpenIPC/ipctool [linux]: https://github.com/OpenIPC/linux +[openhisilicon]: https://github.com/OpenIPC/openhisilicon +[sensors]: https://github.com/OpenIPC/sensors [wiki]: https://github.com/OpenIPC/wiki diff --git a/best_practices.md b/best_practices.md index 287b37e372..ff46c93415 100644 --- a/best_practices.md +++ b/best_practices.md @@ -6,8 +6,9 @@ are shared, so one file there reaches every camera of a family. Those cameras si places nobody can physically reach, there is no staged rollout, and a bad change is found only after `sysupgrade` has already written it to flash. -Each rule below was written from a pull request that was actually closed. The -referenced PR is the one that motivated it. +Each rule below was written from a pull request this project actually received. The +referenced PR is the one that motivated it — usually one that was closed, occasionally +one still open where the pattern is clear enough to write down now. --- @@ -100,6 +101,67 @@ Flag any added or changed `BR2_TARGET_OPTIMIZATION`, `BR2_TARGET_LDFLAGS`, or `BR2_GLOBAL_PATCH_DIR` without a named symptom and a before/after image-size and boot check on the affected board. If the flag fixes one package, fix that package. +### 1.4 The per-device seam already exists — name it, do not just say "move it" + +"This is board-specific, take it to OpenIPC/builder" is the correct verdict and half an +answer. The tree already carries a mechanism for every board-specific thing a retail +camera needs, and a contributor who has not been shown it invents a new init script +with the pin numbers typed into it. + +`general/overlay/etc/init.d/S30customizer` is the entry point. On first boot it runs +`/usr/share/openipc/customizer.sh` once, guarded by `/etc/custom.ok`; on every boot it +runs `/usr/share/openipc/muxes.sh`, which is where a board's pinmux and GPIO presets +go. `/usr/share/openipc/gpio.conf` is the declarative pin map beside them — `button`, +`ircut1`, `ircut2`, `led1`, `led2`, `light_ir`, `light_wl`, `light_sensor`, `speaker`, +`usb`, `-1` for a pin the board does not have — read by consumers such as +`general/package/quirc-openipc/files/qrscan.sh`. Per-image pruning has a seam too: +`general/scripts/rootfs_script.sh` applies `general/scripts/excludes/_.list`, +and `late-overlays.list` ships a file only when a config symbol is set. + +All of it is per-device *in OpenIPC/builder*, at the same paths under +`devices//`: 96 devices ship a `customizer.sh`, 18 a `gpio.conf`, 15 a +`muxes.sh`, and the excludes lists live there too — this repository deliberately ships +no `general/scripts/excludes/` directory at all. The exclusion key is +`_`, which is the *generic* board's key as well, so a list added here +prunes the family board and not only the contributor's camera. + +`#2446` added `S01leds` and `S99leds` driving GPIO 0, 4 and 9 on every camera the tree +builds, a `customizer.sh` in the shared overlay branding every image an Imou Cue 2, and +a `general/scripts/excludes/hi3516ev200_lite.list` that took `libsns_imx307.so` and +`default.ini` away from the generic `hi3516ev200_lite` board. + +When redirecting, say which of these the work becomes. Note also that none of +`gpio.conf`, `muxes.sh` or `customizer.sh` is documented in OpenIPC/wiki — +`en/gpio-settings.md` is a human-readable pin table only — so the contributor has had +no way to find them. + +### 1.5 `devmem` in a shipped script is now a question, not a given + +Writing an SoC register from `rc.local` used to be the only way to tell a camera that +something is soldered to a pad. It no longer is, and majestic's own commit message for +the feature that replaced it states the case better than a rule can: + +> Somebody who solders an IR lamp, a button or an I²C sensor to their board writes +> devmem into rc.local, because nothing in the camera knows the pad exists. That line is +> undone by anything that exports the pad and lost on the next reflash. + +majestic owns the pads now: `pins` configuration and `GET`/`POST /api/v1/pinmux` name +what a pad is wired to and put it back at startup and after a reload, and the PWM lamp +and `nightMode` actuators drive an IR illuminator without anyone computing a duty +cycle. `#2446` added a `devmem` block to the shared `rc.local` programming PWM1 for its +IR LED four days after that landed. + +So ask what owns the pad before accepting a register write. A `devmem` line under +`general/overlay/` needs to say why configuration cannot express it, and — because +`rc.local` is shared — why every other camera should execute it. + +A related trap: "additive, so it changes no existing board" is the right exemption for a +`load_` sensor case arm, and it is not a licence for the shared overlay. +`general/overlay/etc/wireless/usb` has accumulated 46 arms keyed on retail model names +in 327 lines, while 18 builder devices override the whole file. Treat those 46 as +grandfathered, not as precedent: a new arm is a board-specific value in +`general/overlay/` and belongs in the device's own copy of the file. + --- ## 2. Provenance of sources and binaries @@ -169,6 +231,37 @@ Flag any added `.ko`, `.so`, `.bin`, or firmware image that cannot be traced to vendor SDK release or a buildable source tree. A `PROVENANCE.md` documents the problem; it does not solve it. +Do not read that list of extensions as the definition. `#2446` added +`general/overlay/etc/ir/nrxset`, an executable with no extension at all, and it went +unremarked through an automated review that reported it as "the supplied patch contains +no textual changes" — which is exactly what a binary looks like to anything reading the +diff as text. The signal that always survives is the diff marker itself: + +``` +Binary files /dev/null and b/general/overlay/etc/ir/nrxset differ +``` + +Any hunk rendered that way is a binary, whatever it is called and wherever it sits, and +`general/overlay/` is never the right place for a compiled artefact — the overlay is +copied verbatim into every image, so a blob there ships to every camera of every vendor. + +### 2.5 A register table lifted from a vendor's driver is the same problem as a binary + +§2.4 is about what a file *is*; this is about what it *contains*. A sensor init sequence +copied out of a vendor's shipped driver has no more provenance than the driver did — it +cannot be corrected against a datasheet nobody has, and the reasoning behind any one +register is gone. Written as C it passes every check aimed at blobs. + +`#2446` replaced the SC2235 init table with a "Dahua DVP register sequence (114 +entries)" whose only stated origin was Dahua's own firmware, dropping about forty +registers the OpenSDK driver sets and adding others, for every Hi3516EV200 board using +that sensor. + +Flag an added or replaced register table that names a camera vendor rather than a +datasheet as its source. Ask for the deltas the board actually needs — here the PCLK +output-enable and pad-drive registers the PR itself identifies — rather than a wholesale +swap, and ask which other boards were retested. + --- ## 3. Repo boundaries @@ -227,6 +320,25 @@ Flag changes to `general/package/majestic/files/*` that alter how majestic runs order to compensate for how majestic behaves. Redirect the contributor to file the underlying issue with the majestic maintainers. +### 3.5 SoC driver behaviour belongs to the SDK repository + +`hisilicon-opensdk` fetches **OpenIPC/openhisilicon**, and the Sigmastar sensor drivers +come from **OpenIPC/sensors**. Both are OpenIPC-org repositories that take pull +requests. A sensor that mis-detects, an ISP that does not track gain, a driver that +leaves a pad unconfigured — those are changes to the driver, made once, for every tree +that consumes it. + +`#2446` is the shape to recognise. Its `rc.local` waited twelve seconds, wrote five +SC2235 registers over I²C, then killed and relaunched majestic, and the comment above it +said what it was for: the sensor's DVP pad enables, and majestic not re-reading +orientation. Both halves name an owner. The pad enables are a two-line change to +`libraries/sensor/hi3516ev200/smart_sc2235/sc2235_sensor_ctl.c` in +**OpenIPC/openhisilicon**; the orientation re-read is an issue for **OpenIPC/majestic**. +Neither is a shell script that runs on every camera the tree builds. + +A comment that explains *why* a workaround is needed has usually named the repository +the work belongs to. Read it as a redirect and quote it back. + --- ## 4. No monkey-patching @@ -271,6 +383,27 @@ reviewable — nobody rebuilds it, and the committed `.so` is what actually ship Flag any committed binary produced by a script in the same PR. If it is generated, the build system generates it; if the build system cannot, the change needs the real SDK. +### 4.4 A patch against an OpenIPC package is a pull request to that repository + +Patches in a package directory are normal here, and every one of them targets code +this project cannot commit to: ffmpeg, mbedTLS, vtund, baresip's libre, siproxd, +ZeroTier, libwebsockets, f2fs-tools, and the Realtek WiFi drivers. Not one patches a +repository under the OpenIPC organisation, because for those the fix has somewhere +better to go. + +A downstream patch against our own code is monkey-patching with extra steps. It is +invisible to anyone reading the SDK, it is silently dropped the moment someone bumps +`*_VERSION`, and every other consumer of that repository keeps the bug. + +`#2446` added `general/package/hisilicon-opensdk/0001-sc2235-replace-init-table-with-dahua-dvp-sequence.patch`. +`HISILICON_OPENSDK_SITE` is `$(call github,openipc,openhisilicon,...)`, and the file it +patches is checked in there. + +Flag a new `*.patch` in a package whose `*_SITE` resolves to an `openipc` repository. +Redirect to that repository; once it lands, bump `*_VERSION` here and the patch is not +needed. Where the change must ride ahead of the bump, say so explicitly and keep the +patch to the delta, not a wholesale replacement (§2.5). + --- ## 5. Evidence @@ -379,6 +512,28 @@ which no reviewer reading the title would look for. Flag files in the diff that the stated purpose does not explain, especially shared `.mk`, defconfig, and overlay files. Ask for them to be split into their own PR. +### 6.3 Nothing ships that nothing runs + +§6.1 catches source nobody compiles. Its mirror image is a file that reaches the rootfs +perfectly well and that nothing on the camera ever opens. `general/overlay/` needs no +`Config.in` and no `.mk` — anything dropped in it ships — so the usual dead-code check +never fires, and the cost lands on every board of every vendor, several of which sit +within 32 KB of their squashfs cap. + +`#2446` added `general/overlay/etc/ir/nrxset` and +`general/overlay/etc/ir/nrx_night_06.txt`. No script, package, init file or +configuration in firmware, builder or majestic reads either path. The PR's own two +comments disagree about whether they are used at all: `rc.local` says "nrxset applies +VPSS 3DNR params directly via `HI_MPI_VPSS_SetGrpNRXParam` (IQ profile 3DNR is not +used)", while the IQ profile it adds says "Vendor (Dahua) used nrxset runtime patching; +we bake it into the profile". The profile is the one that is right — majestic parses the +`[3dnr]` section of the IQ `.ini` and calls `HI_MPI_VPSS_SetGrpNRXParam` itself, +interpolating per ISO — so the files were redundant on arrival and `rc.local` never +invoked them either. + +Grep the tree for the installed path of any added overlay file. If nothing reads it, +ask what does; "the vendor's firmware had it" is not an answer. + --- ## 7. Shipped shell scripts @@ -448,6 +603,50 @@ whether a script has a good reason to signal by hand. Lifecycle signalling is ne sysupgrade's SIGQUIT, and SIGTERM to stop the daemon, are not reload attempts and are not in scope for either. +### 7.3 A shipped script may only call commands the image contains + +A script under `general/overlay/` runs on a camera with no package manager, no +`$PATH` beyond what the rootfs holds, and frequently no network. A command that is not +there does not fail loudly: `sh` prints "not found" to a console nobody is reading and +carries straight on to the next line, so the script reports success having done nothing. + +`ipctool` is the trap worth knowing by name, because a defconfig line looks like it +supplies it and does not. `general/package/ipctool/ipctool.mk` installs `ipcinfo` and +nothing else, so `BR2_PACKAGE_IPCTOOL=y` puts no binary called `ipctool` on the image. +What answers to that name is `/usr/sbin/ipctool`, a symlink to +`general/overlay/usr/sbin/extutils`, whose `ipctool)` arm curls the tool from +`https://github.com/OpenIPC/ipctool/releases/download/latest/` into `/tmp` the first +time somebody asks for it — "installed as remote GitHub plugin", as it says. That is a +debugging convenience for a person at a shell, and it is four bad properties in a boot +script: a network fetch during boot, from a floating `latest` tag, repeated every boot +because `/tmp` is tmpfs, and a silent no-op until the network is up. + +`#2446` used `ipctool i2cset --bus 0 0x60 ...` from `rc.local`, twelve seconds into +boot, on a WiFi-only camera, to apply the sensor registers the whole change depends on. + +Flag a call in `general/overlay/` or `general/package/*/files/` to anything that is not +a busybox applet and not installed by a package the board's defconfig selects. Reach for +`ipcinfo` where the information is what is wanted, and for the vendor `load_` +script or the SDK driver where hardware must actually be programmed (§3.5). + +### 7.4 Restart majestic through its init script, not by hand + +`general/package/majestic/files/S95majestic` is not a thin wrapper. It re-reads `/etc/TZ` +so a restart picks up a zone change; it passes `-s`; it finds the daemon with +`start-stop-daemon -x` rather than a pidfile, because a stale pidfile makes `-S` start a +*second* majestic that dies on the busy sensor HAL; it waits up to ten seconds for the +old process to actually exit; and it starts the new one under `trap '' HUP`, because the +disposition survives `exec()` and a handler does not — the script's own comment records +25 out of 25 deaths without it, measured on a hi3516ev200. + +A hand-rolled `killall majestic; sleep 3; majestic &` re-opens every one of those, which +is what `#2446` shipped, on a hi3516ev200. + +Flag a shipped script that stops or starts majestic directly. `/etc/init.d/S95majestic +restart` does it correctly; `reload` is the SIGHUP path in §7.2. Lifecycle signalling by +other consumers — sysupgrade's SIGQUIT to make majestic release the SDK while staying +alive, or SIGTERM to stop it — is a different thing and is not in scope here. + --- ## 8. Things that must not reach `master` @@ -456,15 +655,23 @@ These are hard gates rather than judgement calls; `pr_compliance_checklist.yaml` enforces them. Summarised here because they are the most common review findings: - `LD_PRELOAD` in any shipped script, package file, or overlay. -- Binaries extracted from a camera's factory firmware, or any `.ko`/`.so`/`.bin` with - no vendor SDK or buildable source behind it. +- Binaries extracted from a camera's factory firmware, or anything the diff renders as + `Binary files ... differ`, with no vendor SDK or buildable source behind it — + extension and path do not matter, and `general/overlay/` is never the place for one. - New kernel patches under `general/package/all-patches/linux/` — those go to OpenIPC/linux. +- A new `*.patch` against a package whose `*_SITE` is an `openipc` repository — that + goes to the repository itself. - A `*_SITE` pointing at a personal fork, or a `*_VERSION` that is an abbreviated SHA. - A sensor, GPIO, I2C address, or other board-specific value written into `general/overlay/` or into a shared `load_` default. -- Single-board scripts and packages in the shared tree — those go to OpenIPC/builder. -- New sources that no `Config.in` selects and no defconfig builds. -- `insmod` where the tree uses `modprobe`, or an OpenSDK module not named `open_*`. +- Single-board scripts and packages in the shared tree — those go to OpenIPC/builder, + through the per-device seams in §1.4. +- New sources that no `Config.in` selects and no defconfig builds, and overlay files + that nothing on the camera reads. +- A shipped script calling a command the image does not contain — `ipctool` is the one + that looks installed and is not. +- `insmod` where the tree uses `modprobe`, an OpenSDK module not named `open_*`, or a + hand-rolled majestic restart in place of `/etc/init.d/S95majestic restart`. - A test plan whose boxes are unchecked, or a description stating the change was not tested on hardware. diff --git a/pr_compliance_checklist.yaml b/pr_compliance_checklist.yaml index 52b8b9e44e..9a7605238a 100644 --- a/pr_compliance_checklist.yaml +++ b/pr_compliance_checklist.yaml @@ -27,13 +27,25 @@ pr_compliances: cannot be fixed, and is not known to work on any board other than the one it came off. Firmware built that way cannot be supported. success_criteria: > - Any added .ko, .so, .a, .bin, or firmware image traces to a vendor SDK release or - to a source tree the build system compiles. Existing vendor blobs may be moved or - bumped. + Any added binary traces to a vendor SDK release or to a source tree the build + system compiles. Existing vendor blobs may be moved or bumped. failure_criteria: > The diff adds a binary extracted from factory firmware, a binary generated by a - script in the same PR, or any .ko/.so/.bin whose origin the PR cannot name. A + script in the same PR, or any binary whose origin the PR cannot name. A PROVENANCE-style file documenting the absence of source is itself a failure. + Judge this on the diff marker, never on the extension: a hunk rendered as + "Binary files a/... and b/... differ" IS a binary, whatever the file is called and + wherever it sits. .ko/.so/.a/.bin are the common spellings, not the definition — + #2446 added general/overlay/etc/ir/nrxset, an executable with no extension, and an + automated review reported it as "contains no textual changes", which is precisely + what a blob looks like to anything reading the diff as text. A binary anywhere + under general/overlay/ fails outright: the overlay is copied verbatim into every + image, so it ships to every camera of every vendor. + A register table copied out of a camera vendor's own driver is the same failure + wearing C: it cannot be checked against a datasheet, and the reason for any one + register is gone. #2446 replaced the SC2235 init sequence with a 114-entry "Dahua + DVP register table" for every Hi3516EV200 board using that sensor. The deltas a + board needs are reviewable; a wholesale swap sourced from a vendor image is not. - title: "Package SITE is an OpenIPC or upstream repository" compliance_label: true @@ -80,10 +92,24 @@ pr_compliances: script is expected and passes as long as it is additive — extending an existing case arm, adding a new .ini, or adding an entry to a package's sensor list, none of which change an existing board's behaviour. + Per-board values have their own seams, and naming the right one is part of the + finding: /usr/share/openipc/customizer.sh and /usr/share/openipc/muxes.sh (both run + by S30customizer), the declarative pin map /usr/share/openipc/gpio.conf, the + late-overlays.list symbol gate, and general/scripts/excludes/_.list. + All of them live per-device in OpenIPC/builder under devices//, at the same + paths. failure_criteria: > The diff writes a sensor name, I2C address, GPIO number, resolution, MAC prefix, or IP literal into general/overlay/, or changes an existing SNS_TYPE*, default resolution, or default profile in a shared *-osdrv-*/files/script/load_. + The additive exemption above is for load_ sensor case arms; it is not a + licence for the shared overlay. general/overlay/etc/wireless/usb already carries 46 + arms keyed on retail model names while 18 builder devices override the whole file — + those are grandfathered, and a new one fails like any other board-specific value. + An excludes list added HERE is also not per-board: the key is _, + which is the generic board's key too, so #2446's hi3516ev200_lite.list took + libsns_imx307.so and default.ini away from the generic hi3516ev200_lite image. This + repository ships no general/scripts/excludes/ directory; the lists belong in builder. - title: "Toolchain-wide flags are justified and measured" compliance_label: true @@ -143,6 +169,64 @@ pr_compliances: states the package must not be enabled on generic SoC defconfigs, or it collides with an existing first-class package at the same installed path. + - title: "No patches against OpenIPC's own packages" + compliance_label: true + objective: > + A patch in a package directory is the sanctioned way to carry a fix for code this + project cannot commit to. Against code it CAN commit to it is monkey-patching with + extra steps: invisible to anyone reading the SDK, silently dropped at the next + *_VERSION bump, and leaving the bug in place for every other consumer. + success_criteria: > + A new *.patch sits in a package whose *_SITE names a third-party upstream. Every + patch in the tree today does — ffmpeg, mbedtls, vtund, libre, siproxd, zerotier, + libwebsockets, f2fs-tools, webrtc-audio-processing and the Realtek drivers. + A fix to OpenIPC-org code lands in that repository and arrives here as a + *_VERSION bump. + failure_criteria: > + The diff adds a *.patch to a package whose *_SITE resolves to a repository in the + openipc organisation. hisilicon-opensdk fetches openipc/openhisilicon, and #2446 + added 0001-sc2235-replace-init-table-with-dahua-dvp-sequence.patch against a file + checked in there. Redirect to that repository. Sensor and SDK work for HiSilicon + goes to OpenIPC/openhisilicon; Sigmastar sensor drivers go to OpenIPC/sensors. + + - title: "Shipped files are reachable from something that runs" + compliance_label: true + objective: > + general/overlay/ needs no Config.in and no .mk — anything dropped there ships — so + the dead-code gate above never fires on it. A file nothing opens still costs every + board of every vendor, and several sit within 32 KB of their squashfs cap. + success_criteria: > + Every added file under general/overlay/ or a package's files/ directory is read at + its installed path by a script, init file, package, or configuration somewhere in + firmware, builder or majestic. Documentation and license text shipped on purpose + are fine. + failure_criteria: > + The diff adds an overlay file whose installed path appears nowhere else in the + tree. #2446 added /etc/ir/nrxset and /etc/ir/nrx_night_06.txt, which nothing reads + — and its own two comments contradict each other about whether they are used, one + claiming nrxset applies the VPSS 3DNR parameters and the other saying they were + baked into the IQ profile instead. "The vendor's firmware had it" is not a reason. + + - title: "A shipped script calls only commands the image contains" + compliance_label: true + objective: > + A camera has no package manager and frequently no network. A missing command does + not fail loudly — sh prints "not found" to a console nobody reads and continues to + the next line, so the script reports success having done nothing. + success_criteria: > + Commands invoked from general/overlay/ or general/package/*/files/ are busybox + applets, or binaries installed by a package the board's defconfig selects. + failure_criteria: > + The diff calls, from a shipped script, a command no package installs. ipctool is + the specific trap: general/package/ipctool/ipctool.mk installs ipcinfo and nothing + else, so BR2_PACKAGE_IPCTOOL=y puts no "ipctool" on the image. /usr/sbin/ipctool + is a symlink to extutils, which curls the tool from the ipctool repository's + "latest" release into /tmp on first use — an interactive convenience, and in a + boot script a network fetch during boot, from a floating tag, repeated every boot + because /tmp is tmpfs, and a silent no-op before the network is up. #2446 ran + `ipctool i2cset` from rc.local twelve seconds into boot on a WiFi-only camera. + Do not raise this for .github/ or contrib/, which run off-device. + - title: "No runtime patching of vendor code" compliance_label: true objective: > @@ -164,14 +248,22 @@ pr_compliances: naming for OpenSDK modules that replace vendor-prefixed blobs. Both are mechanical and checkable from the diff. success_criteria: > - Shell scripts that ship in the image load kernel modules with modprobe, and OpenSDK - modules are named open_* rather than with a vendor-specific prefix. This gate - applies only to files installed onto a camera — general/overlay/ and + Shell scripts that ship in the image load kernel modules with modprobe, OpenSDK + modules are named open_* rather than with a vendor-specific prefix, and a script + that needs to bounce the streamer calls `/etc/init.d/S95majestic restart`. This + gate applies only to files installed onto a camera — general/overlay/ and general/package/*/files/. Scripts under .github/ and contrib/ run on CI runners or a developer's machine under bash and are out of scope. failure_criteria: > The diff calls insmod where the surrounding tree uses modprobe, or introduces an OpenSDK module under a vendor-prefixed name such as gk7205v200_* or hi3516ev200_*. + Or it stops and starts majestic by hand instead of through S95majestic: #2446 + shipped `killall majestic; sleep 3; majestic &`, which drops the -s argument, + replaces start-stop-daemon -x with a pidfile-free killall, replaces a ten-second + wait for exit with a fixed sleep into the window where a second majestic dies on + the busy sensor HAL, and loses the `trap '' HUP` that S95majestic carries because + the disposition survives exec() where a handler does not — 25 deaths out of 25 + without it, measured on a hi3516ev200, which is the SoC that PR targets. Do not raise this for anything under .github/ or contrib/. Do not raise shell portability here: syntax is already enforced by .github/workflows/shell-tests.yml against busybox ash, and "avoid bashisms" is not binary on this target — the shipped