load_*: stop a failed sensor detection latching the camera off for good - #2441
Conversation
PR Summary by QodoRetry sensor detection after an unsuccessful boot
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
f3f5730 to
0dc3351
Compare
A camera whose sensor is not detected on first boot never detects it again, however the underlying problem is fixed. Reported from a GK7202V500 in #2428, whose log shows the moment it happens: goke: Get data from ipcinfo and set SENSOR as unknown goke: Writing unknown to U-Boot ENV goke: SENSOR is not detected, aborting... The detect path stores whatever it found, and `unknown` is a value like any other. Every later boot takes the branch above it -- if fw_printenv -n sensor >/dev/null; then export SENSOR=$(fw_printenv -n sensor) -- reads `unknown` back, and aborts without running detection at all. Reseating the ribbon cable, fixing the bus, adding the missing driver: none of it helps, because nothing looks again. Nothing in the log says why, either; the second boot's output is identical to a camera that genuinely has no sensor. Guarding the write is the whole fix. This path is only reached when the variable was unset, so not writing leaves it unset and the next boot probes again. load_ingenic has always done this correctly -- on a miss it runs `unset SENSOR; fw_setenv sensor` and clears the variable rather than storing the miss -- so the six ingenic scripts are already right and are left alone. The other thirteen get the same behaviour by not writing at all.
The previous attempt at this put the recovery inside the branch reached when
SENSOR is empty, which on a real camera is the one branch that never runs.
/etc/init.d/rcS line 2 is
export SENSOR=$(fw_printenv -n sensor)
so by the time S70vendor calls load_<vendor> -i, an already-latched camera has
SENSOR=unknown in its environment. The loader takes
if [ -n "$SENSOR" ]; then
logger ... "SENSOR: ${SENSOR}"
logs the sentinel as a manually-set sensor, and the recovery below it is
unreachable. The fix was inert on exactly the cameras it was written for.
Clearing before that test, and blanking the shell variable as well as the
stored one, puts the camera back on the detection path.
Verified with the shipped block run under busybox ash, with fw_printenv /
fw_setenv backed by a file and -- this time -- SENSOR exported from that file
first, the way rcS does. The earlier harness set SENSOR= by hand, which is
precisely why it reported a fix that could not work:
A. latched camera, sensor now detectable
before: boot1 SENSOR=unknown detection_runs=0 env=[sensor=unknown]
after: boot1 SENSOR=sc2336 detection_runs=1 env=[sensor=sc2336]
B. fresh camera, undetectable then detectable
before: boot2 SENSOR=unknown detection_runs=1 env=[sensor=unknown]
after: boot2 SENSOR=sc2336 detection_runs=2 env=[sensor=sc2336]
C. working camera, sensor already stored
before: boot1 SENSOR=sc2336 detection_runs=0 env=[sensor=sc2336]
after: boot1 SENSOR=sc2336 detection_runs=0 env=[sensor=sc2336]
C is the no-regression case and is byte-identical either side.
Found by the Qodo review on #2442, which reviewed this branch's code against
that PR.
0dc3351 to
fdd6d00
Compare
1 participant
Problem
A camera whose sensor is not detected on the first boot never detects it
again, no matter what is fixed afterwards.
The reporter of #2428 hit this on a GK7202V500. Their log catches the exact
moment:
The detect path stores whatever it found, and
unknownis a value like anyother. Every subsequent boot takes the branch above it:
sensorexists, so it is read back asunknownand the script aborts —detection never runs again. Reseating a ribbon cable, fixing a bus,
shipping the missing driver: none of it helps, because nothing looks a second
time. And the second boot's log is indistinguishable from a camera that
genuinely has no sensor, so there is nothing to tell the owner that a stale
value is the reason.
The recovery exists (
fw_setenv sensor <name>, or deleting the variable) but isundiscoverable from the symptom.
What changes
Two things, and the second is the one that matters on a real camera.
Do not persist a failed detection. The detect path is only reached when the
variable was unset, so declining to write leaves it unset and the next boot
probes again.
Clear a stale
unknownbefore anything reads it. This is the half thatrecovers a camera already latched by older firmware, and it has to happen early:
/etc/init.d/rcSline 2 isso by the time
S70vendorrunsload_<vendor> -i, a latched camera already hasSENSOR=unknownin its environment. It takes the[ -n "$SENSOR" ]branch, logsthe sentinel as a manually-set sensor, and never reaches anything below. A guard
placed in the
elseis unreachable on exactly the cameras it is for — which iswhat the first version of this PR did, and what the Qodo review on #2442 caught.
The clear now runs ahead of that test and blanks the shell variable as well as
the stored one.
load_ingenichas always done this correctly — on a miss it runsunset SENSOR; fw_setenv sensor. Those six scripts are untouched; the otherthirteen get the same behaviour.
Not in scope, deliberately: the four
load_sigmastarscripts. They cannotlatch, because on a failed probe
ipcinfo -sprints nothing to stdout(
src/main.cwritesNo sensor detectedto stderr and returnsEXIT_FAILURE),so
SENSORis empty and their[ -z "$SENSOR" ]gate re-detects. The literalstring
unknownonly exists where a loader supplies it itself via${SENSOR_DETECT:=unknown}, which sigmastar has no equivalent of.Hardware tested on
No physical camera. The half that is real hardware is the failure: the
GK7202V500 log in #2428 shows
Writing unknown to U-Boot ENV, and that cameranever probing again afterwards.
For the fix itself this is shell logic with no device interaction — it decides
whether to call
fw_setenv— so it is verified by executing the shipped blockwith the surrounding calls stubbed.
Evidence
The block is lifted verbatim from the shipped
load_goke(master vs thisbranch), run under busybox
ashwithfw_printenv/fw_setenvbacked by a fileand
ipcinfostubbed.SENSORis exported from that file first, the wayrcSdoes — the first version of this harness set it by hand instead, which is
precisely why it certified a fix that could not work.
detection_runscountsipcinfoinvocations.A is the case the first version got wrong:
detection_runs=0on every boot,before — the old code does not record a wrong answer, it stops asking the
question. C is byte-identical either side.
Shell gates:
Scope
Thirteen
load_<vendor>scripts across hisilicon, goke, grainmedia and novatek.That is a wide reach for one change, so what it can do is worth being precise
about: it removes a write. A camera that detects its sensor is byte-identical in
behaviour; a camera that does not is today latched off permanently and
afterwards retries. There is no path by which this makes a working camera stop
working.
general/package/all-patches/linux/general/overlay/or in a sharedload_<vendor>script hardcodes a value specific to my board — this removes a value being written, and adds noneLD_PRELOAD, and no binaries that cannot be rebuilt from source