install: add Hikvision U-Boot migration for HiWatch DS-I203 - #137
Conversation
PR Summary by QodoAdd Hikvision U-Boot migration for HiWatch DS-I203
AI Description
Diagram
High-Level Assessment
Files changed (25)
|
Code Review by Qodo
1. Protocol tools miss Hikvision recovery
|
![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 DS-I203 path itself is careful work, the hardware evidence in the description is exactly what this repo asks for, and the docs (docs/boards/hiwatch-ds-i203.md, README) are genuinely good. CI is fully green on the branch here too: 842 passed / 3 skipped, fuzz 16 passed, ruff check clean, mypy clean across 77 files.
Requesting changes because the PR bundles three separable things — the Hikvision bootstrap, the extraction of the installer out of cli/app.py, and a semantics change to Transport.flush_output() — and the defects below all live in the shared install/transport layers used by every chip, not in the new Hikvision code. The DS-I203 hardware run cannot have exercised any of them, and none is caught by CI.
Blocking:
_wait_for_openipc_shell_after_resetis out of scope on thedownload_modebranch —UnboundLocalErrorfires immediately after the env partition is erased.SerialTransport.flush_output()now calls unboundedtcdrain(), directly below the comment explaining whywrite()needs a ceiling.- The installer dropped the explicit
tftpbootaddress and now hard-requires a verifiedloadaddrround-trip. - The post-
saveenv"verified" check reads the in-RAM env and cannot detect a failed SPI write. TransportTimeoutis not a builtinTimeoutError, so echo-verification failures escape as a bare traceback.loady/gobypass the echo verification added for exactly that corrupting UART.
Also worth fixing before merge: the RFC2217 flush_output() no-op, --nor-size behaving as an assertion rather than the override the README promises, the missing hi3518ev100 RAM_BASE entry, the --output json regression, and the substring scan in uboot_flash_command_error().
Two smaller repo-convention asks:
- All three commit bodies are empty.
CLAUDE.mdis explicit that bodies carry the real documentation — the why, the exact command, and the hardware verification evidence. All of that already exists in the PR description; it just needs to land in the commits. CLAUDE.mdneeds updating:src/defib/vendors/is a new registration mechanism alongside profiles and the V500/CV6xx frozensets, and "CLI — one largeapp.py" is no longer accurate.
One thing I checked and am not asking you to change: the switch from run mtdpartsnor{8,16}m to a literal setenv mtdparts hi_sfc:.... I traced it to 9c9ea03, which introduced the identical bare-literal form for the 32 MiB case and verified it end to end on a hi3516av300, so the format is proven. Worth one glance that the exact 8/16 MiB strings match OpenIPC's default env, but I don't consider it a risk.
If you're open to it, splitting the transport change and the cli/app.py extraction into their own PRs would make the DS-I203 work much easier to land — the Hikvision bootstrap, the vendor registry and the board docs are close to ready as-is.
| ) | ||
|
|
||
| await _cmd("reset", timeout=1.0) | ||
| await _wait_for_openipc_shell_after_reset() |
There was a problem hiding this comment.
Blocking — UnboundLocalError at the most dangerous point in the flow.
_wait_for_openipc_shell_after_reset is defined at line 483, inside the shell-mode else: branch of the if download_mode: split (col_offset 8). This call site is on the has_stock_uboot and not nand path and is not guarded by that condition.
If step 3.5's detection sets download_mode = True, only the if branch's _cmd is bound. The install then proceeds normally, flashes U-Boot/kernel/rootfs, erases rootfs_data, erases the env partition at line 1013, runs reset at 1048 — and raises UnboundLocalError here. The board is left with a wiped environment, no mtdparts, and the compiled-in default MAC.
Reachability is narrow (a chainloaded U-Boot answering in download-command mode), but this is the single worst place in the file to abort. Either hoist the helper out of the branch or assert not download_mode before the stock-U-Boot env migration begins.
There was a problem hiding this comment.
Fixed. _wait_for_openipc_shell_after_reset is no longer branch-scoped, and a stock migration is also rejected before destructive work if it unexpectedly lands in download-command mode. Added regression coverage for the download-mode case.
| # pyserial reset_output_buffer() does the opposite: it discards queued | ||
| # bytes, which can truncate bootloader/YMODEM traffic on USB-UART links. | ||
| # Serial.flush() blocks until the OS/driver TX queue has drained. | ||
| await asyncio.get_event_loop().run_in_executor(None, self._port.flush) |
There was a problem hiding this comment.
Blocking — unbounded tcdrain() with no cancellation path.
The semantics change itself is right: Transport.flush_output() is documented "Ensure all buffered output has been sent", and reset_output_buffer() did the opposite. Good catch.
But Serial.flush() is termios.tcdrain(self.fd) on POSIX — no timeout, not interruptible. Compare write() twenty lines above, which sets write_timeout = 5.0 with a comment spelling out exactly this hazard: "asyncio.wait_for can't help here — cancelling a run_in_executor future leaves the underlying thread still blocked."
A hung USB-serial adapter, an unplugged cable or a CTS stall mid-install blocks tcdrain forever, permanently consuming the executor thread; the event loop never gets the result and Ctrl-C does not recover it. Please give this the same bounded treatment — e.g. poll out_waiting against a deadline — rather than an unguarded drain.
Separately: this is the primary recovery path for all 112 UART chips, and hisilicon_standard.py:198 calls flush_input(); flush_output() before every frame retransmission. Whether that call site was relying on the old purge is worth a hardware check on any HiSilicon board before merge.
There was a problem hiding this comment.
Fixed. SerialTransport.flush_output() no longer calls Serial.flush() / tcdrain() and never purges the TX buffer. It polls out_waiting against a bounded deadline instead. tests/test_transport_serial.py covers successful drain and timeout behavior.
| # from the live shell before issuing tftpboot. | ||
| await set_uboot_env_verified(_cmd, "ipaddr", device_ip) | ||
| await set_uboot_env_verified(_cmd, "serverip", serverip) | ||
| await set_uboot_env_verified(_cmd, "loadaddr", f"0x{ram_addr:x}") |
There was a problem hiding this comment.
Blocking — the explicit tftpboot address was load-bearing.
On master this was tftpboot 0x{ram_addr:x} {filename} with fire-and-forget setenv ipaddr / setenv serverip (cli/app.py:2717-2725). The address never depended on env state at all.
Now all three values go through set_uboot_env_verified(), which raises RuntimeError unless parse_printenv_value() returns an exact match, and _tftp_to_ram relies on loadaddr. In download_process mode _cmd goes through DownloadCommandClient.send_command, which warns and returns partial output on ok=False; if that dialect's printenv reply isn't in name=value form the install now aborts with "U-Boot runtime environment verify failed for ipaddr" on chips where it previously completed.
The stated motivation — "Keep TFTP command lines short on old UART consoles" — applies to the Hikvision link specifically. Could the short-filename/loadaddr form be gated on has_stock_uboot, leaving the explicit-address form (which is strictly more robust) for everyone else?
There was a problem hiding this comment.
Fixed. Generic/download-mode installs again use tftpboot 0x<ram_addr> and do not depend on a printenv loadaddr round-trip. The short loadaddr form is limited to the stock-U-Boot path where command length matters.
| if has_stock_uboot: | ||
| from defib.uboot_env import parse_printenv_value | ||
|
|
||
| verify_resp = await _cmd("printenv ethaddr", timeout=5.0) |
There was a problem hiding this comment.
Blocking — this verification cannot fail for the reason it exists.
printenv reports U-Boot's in-RAM environment hashtable, not what landed in SPI. These two read-backs return the values set at lines 1115 and 1078, so both comparisons pass by construction.
Concretely: saveenv emits OK-looking output (so uboot_flash_command_error at 1123 passes) but the SPI write is short, or the env sector wasn't fully erased. Both checks here still pass, the CLI prints "Environment saved and verified", and the first cold boot loads a bad-CRC env, falls back to compiled defaults, and comes up with ethaddr=00:00:23:34:45:66 and the wrong mtdparts.
Every other partition in this file is verified with sf read + crc32 read-back (837-864, 953-972, 1023-1046). The environment — the one thing this migration path exists to get right — is the only one that isn't. A sf read of the env partition plus a CRC comparison would close it.
There was a problem hiding this comment.
Fixed. Persistent environment verification now checks the actual SPI contents after saveenv, rather than relying only on printenv.
Defib re-runs sf probe 0, reads the environment partition from SPI, computes the CRC over the environment data, stores that result in RAM, and compares it with the CRC stored in the flash environment header using cmp.l.
The extra sf probe 0 is intentional: hardware validation showed that the SPI selection is lost across the mandatory internal reset, so attempting the read without re-probing fails with No SPI flash selected.
This path was revalidated on the physical DS-I203. The final hardware run completed the persistent check successfully with SPI CRC 7A58A0B5 before reporting the environment as verified.
| except Exception: | ||
| pass | ||
|
|
||
| raise TransportTimeout( |
There was a problem hiding this comment.
Blocking — this exception is not caught anywhere upstream.
TransportTimeout derives from TransportError(Exception) (transport/base.py:9-14), not the builtin TimeoutError. The orchestrator guards bootstrap() with except TimeoutError (orchestrator.py:375), and the install body's only handler is the no-op except Exception: raise at 1159.
So after 8 failed echo attempts on a noisy UART this propagates out of run_install as a bare Python traceback: the user sees a stack dump instead of a CLI error, and transport.close() / power_controller.close() at 1164-1166 never run — serial port and PoE session both left open.
Same gap applies to transport.write() raising TransportTimeout inside hikvision._run_stock_command. Either raise a builtin TimeoutError here or widen the handlers to except (TimeoutError, TransportError).
There was a problem hiding this comment.
Fixed. TransportTimeout is handled through the broader TransportError path, so UART command/echo failures no longer escape as an unhandled traceback.
The installer now converts those failures into the normal controlled error path and releases UART/power/TFTP resources. The destructive phase is protected by cleanup handling, and the preserved/transient environment verification path before TFTP is also covered so a normal RuntimeError there still closes the transport.
Regression coverage includes a forced stock-migration transport timeout and verifies that the installer exits cleanly and closes the UART.
| _STOCK_UBOOT_TARGETS: dict[str, StockUBootTarget] = { | ||
| "hi3518ev100:hiwatch-ds-i203": StockUBootTarget( | ||
| selector="hi3518ev100:hiwatch-ds-i203", | ||
| handler="hikvision", |
There was a problem hiding this comment.
hi3518ev100 needs an explicit RAM_BASE entry.
get_ram_staging_addr("hi3518ev100:hiwatch-ds-i203") returns 0x82000000, which is correct — but only by accident. The chip isn't in RAM_BASE, load_profile() raises (there's no hi3518ev100.json), so it falls through to the last-resort scan:
for prefix, base in RAM_BASE.items():
if chip_lower.startswith(prefix[:8]):
return base + RAM_STAGING_OFFSETThat matches on the bare "hi3518" key (0x80000000) purely because it appears before "hi3518ev300" (0x40000000) in an unordered dict. Alphabetise or reorder RAM_BASE and the answer flips to 0x42000000 — the installer would TFTP into unmapped memory and sf write whatever is there into flash.
The host-side CRC compare would catch it (expected CRC is computed from the file, not re-read from the device), so it fails safe rather than bricking — but a one-line "hi3518ev100": 0x80000000 entry in flashdump.RAM_BASE removes the dependency on dict ordering entirely.
There was a problem hiding this comment.
Fixed. RAM_BASE now includes an explicit hi3518ev100: 0x80000000 mapping.
get_ram_staging_addr() also normalizes a chip:variant selector to its base chip before exact lookup, so hi3518ev100:hiwatch-ds-i203 deterministically stages at 0x82000000 without relying on prefix matching or dictionary ordering.
Regression coverage verifies the DS-I203 staging address.
| try: | ||
| mode = recovery_mode(chip) | ||
| except ValueError as exc: | ||
| console.print(f"[red]{exc}[/red]") |
There was a problem hiding this comment.
--output json regression.
Master routed both of these early exits through _usb_fail() (cli/app.py:2292), which still exists at app.py:3107 and carries the comment "Routed through _usb_fail so --output json still gets an event." It emits {"event": "error", "message": ...} for JSON consumers and rich.markup.escapes the message otherwise.
This prints raw Rich markup instead, so automation parsing stdout as JSON gets an unparseable line — and the unescaped {exc} means a ValueError mentioning e.g. hi3516cv608:[variant] has that fragment swallowed as markup.
Same applies to the USB-recovery exit just below at 92-95.
There was a problem hiding this comment.
Fixed. Installer preflight failures now go through the common CLI-safe failure helper.
In JSON mode it emits a structured error event, while human mode uses escaped Rich output. This covers the early selector/profile and unsupported recovery-mode paths that previously printed non-JSON text.
Regression coverage includes early CLI failure in --output json mode.
| return crc & 0xFFFFFFFF | ||
|
|
||
|
|
||
| def uboot_flash_command_error(response: str) -> str | None: |
There was a problem hiding this comment.
Substring scan over the whole response, and it fires after the erase.
Two things:
" failed"can never match without"failed"also matching — the first entry is dead.- More substantively, this runs against the full
_cmdbuffer. Withverify_echooff (every generic install) that includes the echoed command plus anything the device printed earlier in the same read window. A U-Boot that emits e.g."...failed to read"during an unrelated banner aborts the install atorchestrator.py:805— after the erase has already happened.
Anchoring on the command's own result line rather than scanning the whole buffer would make this safe to rely on for destructive operations.
There was a problem hiding this comment.
Fixed. U-Boot command failure detection is now line/result-oriented rather than searching arbitrary substrings across the complete response buffer.
Unrelated text such as warning: failed to read optional otp calibration no longer turns into a false destructive-command failure, while actual erase/write/probe failures are still detected.
No SPI flash selected is also recognized explicitly; that case was encountered during physical environment-verification testing after the internal reset.
| self.timing.read_timeout, max(0.0, deadline - loop.time()) | ||
| ), | ||
| ) | ||
| except TransportTimeout: |
There was a problem hiding this comment.
Hot spin: continue with no sleep.
Both this loop and YModemSender._read_control (ymodem.py:103-104) do except TransportTimeout: continue inside a while loop.time() < deadline. Transports that raise without blocking — MockTransport.read on an empty buffer, the new ScriptedTransport, and a SocketTransport/rack bridge after the peer closes — turn the timeout window into a full-core busy-wait: 30 s for _detect_console's boot_timeout, 15 s for _read_control's start_timeout.
_chainload's loop at line 308 already does the right thing with await asyncio.sleep(self.timing.openipc_poll_interval). Same treatment here and in _read_control.
There was a problem hiding this comment.
The Hikvision console-detection timeout path now yields with asyncio.sleep() instead of immediately continuing a tight loop when a transport raises TransportTimeout.
For YMODEM, _read_control() no longer retries a tight continue loop on TransportTimeout; that timeout ends the current control-read attempt instead.
This avoids the full-core busy-wait behavior described here for transports that can raise immediately rather than blocking for the requested timeout.
| ) | ||
|
|
||
|
|
||
| def nor_bootargs() -> str: |
There was a problem hiding this comment.
Dead in production — five new helpers have no non-test callers.
nor_bootargs() here, plus parse_printenv(), expand_env_references() and env_values_equivalent() in uboot_env.py, and list_stock_uboot_variants() in registry.py.
Two of those look like they were meant to be wired up and weren't:
env_values_equivalent()'s docstring says "Verification needs semantic rather than byte-for-byte comparison because legacy U-Boot may expand variables while executing setenv" — butorchestrator.py:1140verifiesmtdpartswith a plainsaved_mtdparts != expected_mtdparts, exactly the byte-for-byte compare it was written to replace.list_stock_uboot_variants()would solve a real discoverability gap:hi3518ev100:hiwatch-ds-i203doesn't appear indefib list-chipsat all (no profile JSON), so the README is currently the only way to find it.
Also in this area: create_uboot_bootstrap is imported twice (orchestrator.py:55 and :320); parse_printenv_value/select_install_ethaddr are re-imported three times (:987, :1087, :1127); re_mod.search(r"==>\s*([0-9a-fA-F]{8})", ...) is open-coded at :963 and :1033 despite parse_uboot_crc32 being imported at :24; and there's a no-op except Exception: raise at :1159.
Separately, two gaps worth closing while you're here: the chainload sends the 0xFF-padded 256 KiB image rather than raw U-Boot (uboot_data feeds both bootstrap() and the flash write — several wasted seconds of YMODEM per run), and preserved_stock_env is applied with a bare setenv at :513 while the far less critical transient_env gets set_uboot_env_verified() at :523. The factory ethaddr is the one value that cannot be re-derived.
Finally: YModemSender.send(), its NAK-retry loop and the EOT/final-header handshake are never executed by the suite — test_ds_i203_final_contract.py monkeypatches the sender out, and test_ymodem.py covers only CRC16 and packet framing. That's the new wire protocol carrying U-Boot onto a live camera; a scripted-receiver test (happy path, one NAK retry, EOT sequence) would be cheap. While you're in there, _chainload starts YMODEM without a flush_input(), so _wait_crc_request skips strays hunting for 0x43 — any uppercase C in a modified loady banner would be misread as a CRC request.
There was a problem hiding this comment.
Addressed the cleanup and protocol gaps from this thread.
Unused environment/layout helpers and duplicate imports were removed. Stock-U-Boot selectors are now exposed through list-chips.
The chainload path now sends the raw U-Boot binary over YMODEM and only pads the image for the fixed-size flash partition. Preserved factory environment values use the verified setter path.
YMODEM now has scripted protocol tests covering the successful transfer path, NAK retry, two-EOT negotiation, and the final empty header. The Hikvision chainload path also flushes stale input before waiting for the receiver's CRC request.
The final DS-I203 hardware run used the raw U-Boot YMODEM path successfully.
Move the install implementation out of the CLI into reusable defib.install modules and add a separate stock-U-Boot bootstrap abstraction for migrations that start from an already-running vendor bootloader rather than the SoC boot ROM. Add the vendor bootstrap registry, the reusable Hikvision U-Boot console/YMODEM implementation, firmware artifact override support, standard NOR layout helpers, environment helpers, and installer plumbing needed for a board-specific stock migration. Keep this mechanism separate from BootProtocol because BootProtocol describes the SoC boot-ROM recovery dialect, while vendors.* starts from a running vendor U-Boot. Verification: python -m pytest tests/test_install_flash_helpers.py tests/test_uart_command_integrity.py tests/test_uboot_env.py tests/test_ymodem.py -q Hardware verification is not standalone for this commit because it intentionally does not register a concrete board selector. The following HiWatch DS-I203 commit binds this reusable layer to physical hardware and carries the end-to-end stock-U-Boot migration evidence.
Bind the reusable stock-U-Boot migration path to the physical HiWatch DS-I203. Register hi3518ev100:hiwatch-ds-i203 as a Hikvision stock-U-Boot target, map it to the DDR3/256 MiB OpenIPC U-Boot variant, use the safe 0x81000000 chainload address, and apply transient phyaddru=3 only for installer TFTP. Add the board documentation and migration contract tests covering the stock Hikvision console, existing-OpenIPC detection, factory MAC preservation, NOR layout ownership, and release artifact selection. Verification: python -m pytest tests/test_ds_i203_final_contract.py tests/test_firmware.py -q Hardware verification used a physical HiWatch DS-I203 with Hi3518EV100, IMX122, 256 MiB DDR3 and 16 MiB GD25Q128 SPI NOR. Defib entered Hikvision U-Boot 2010.06 through Ctrl+U / HKVS, chainloaded the DDR3/256 MiB OpenIPC U-Boot over YMODEM, detected the 16 MiB NOR layout, flashed OpenIPC firmware, preserved the factory MAC, and booted the matching DS-I203 firmware profile successfully. The validated path was: Hikvision U-Boot -> Ctrl+U / HKVS -> YMODEM OpenIPC U-Boot -> TFTP flash -> environment migration -> OpenIPC boot.
Harden the stock-U-Boot migration path so destructive install steps fail closed instead of continuing on ambiguous or invalid state. Require explicit --wipe-env for registered stock-U-Boot NOR migrations, reject oversized U-Boot overrides through the normal CLI error path, broaden NOR-size parsing for valid U-Boot probe formats, and require parseable CRC values for both TFTP RAM verification and flash readback instead of silently skipping verification when output is incomplete. Keep vendor migrations from erasing the environment as part of the U-Boot partition write, preserve the captured factory ethaddr across the explicit environment migration, and add focused regression coverage for the destructive preflight and CRC failure cases. Verification: python -m pytest tests/test_ds_i203_final_contract.py tests/test_install_flash_helpers.py -q Hardware verification used the HiWatch DS-I203 stock-U-Boot migration path with explicit --wipe-env. The board completed the migration on 16 MiB NOR, preserved the factory MAC and reached OpenIPC successfully; later review commits further strengthen transport and persistent-environment verification without changing this commit's fail-closed contract.
Fix the shared installer and transport issues found during review of the HiWatch DS-I203 stock-U-Boot migration. Restore explicit TFTP RAM addressing for generic installs, bound serial TX draining, keep RFC2217 flush semantics explicit, verify persistent SPI environment contents after saveenv, handle TransportError cleanly, and protect Hikvision loady/go with UART echo verification. Also make --nor-size a true override, add the hi3518ev100 RAM base, restore JSON error output, harden U-Boot error parsing, remove dead helpers, use raw U-Boot for YMODEM and pad only for flash, and expand YMODEM/transport/install regression coverage. Hardware verification: python -m defib install -c hi3518ev100:hiwatch-ds-i203 --firmware $HOME\Downloads\hiwatch-ds-i203-202609151816.tgz --uboot $HOME\Downloads\u-boot-hi3518ev100-ddr3-256m-universal.bin --wipe-env -p COM15 --tftp-via host --nic "Беспроводная сеть" --host-ip 192.168.1.11 --device-ip 192.168.1.64 --no-final-reset -d The DS-I203 completed stock Hikvision U-Boot -> OpenIPC U-Boot migration on hardware, detected 16 MiB NOR, flashed and CRC-verified U-Boot/kernel/rootfs/rootfs_data, preserved the factory MAC, saved the environment, re-probed SPI, and physically verified the environment CRC before leaving the board at the OpenIPC prompt.
Add explicit install-stage selection for development, recovery, and targeted validation without replaying the complete production install. --stage selects an exact set from uboot, kernel, rootfs, rootfs-data, env, and reset. --skip-stage subtracts stages from the normal production sequence, and the two forms cannot be combined. Explicit stage selection performs reset only when reset is selected. Keep the default install path unchanged. Start TFTP only when selected stages need image transfer, keep environment erase coupled to the env stage, and reject unsafe partial persistent installs when a genuine stock U-Boot was only chainloaded temporarily. Regression coverage verifies default and explicit stage resolution, skip semantics, env-only execution without TFTP or partition writes, reset selection, invalid combinations, and stock-migration safety. Verification: python -m pytest tests/test_install_stages.py -q The stage controls were also exercised on the HiWatch DS-I203 with an env-only run against an already-running OpenIPC U-Boot. That run completed the environment migration and its physical SPI CRC verification and was used while validating the required post-reset sf probe handling.
aa18898 to
ab20fbe
Compare
|
Addressed the requested review changes and force-pushed the rewritten five-commit series. The original three commits now have full bodies with rationale, verification commands, and hardware evidence. Review remediation is isolated in The final tree was revalidated on the physical DS-I203 after the review fixes, including UART echo verification for Local final gate: 873 passed / 10 skipped / 4 known Windows-baseline tests deselected, fuzz 16 passed, locked Ruff 0.15.8 clean, strict mypy clean, and |
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
All 13 findings from the previous review are fixed. I verified each one in the code rather than going by the thread replies:
Blockers
_wait_for_openipc_shell_after_resetis no longer branch-scoped (col_offset4, was 8), and a stock migration that lands in download-command mode is now rejected before any destructive work.SerialTransport.flush_output()pollsout_waitingagainst a 5.0 s deadline with a 10 ms interval — matching the ceilingwrite()already had — and raisesTransportTimeoutinstead of blocking intcdrainforever.- Generic and download-mode installs use explicit
tftpboot 0x<ram_addr> <file>again; the shortloadaddrform is gated tohas_stock_uboot. verify_spi_environment_crc()re-probes SPI, reads the env partition back, CRCs the data region into a scratch word andcmp.ls it against the on-flash header CRC. The re-probe after the internal reset is a good catch, and usingcmp.lto avoid host-endianness assumptions is the right call.except (TimeoutError, TransportError)now covers the bootstrap call site, with cleanup around the destructive phase.loady,goand the stock command path all route throughwrite_uboot_line_with_echo_verify.
Remaining seven — --nor-size warns and honours the override; explicit hi3518ev100 RAM_BASE entry plus :variant stripping; RFC2217 flush_output() an explicit documented no-op; fail() emitting JSON events and escaping markup; line-oriented uboot_flash_command_error(); both hot-spin loops yielding; dead helpers gone, list_stock_uboot_selectors() wired into list-chips, raw U-Boot over YMODEM with padding only for flash, preserved ethaddr through the verified setter, and YMODEM sender tests covering the happy path and NAK retry.
Both convention asks are done: all five commit bodies now carry rationale, verification commands and hardware evidence — including 72424c9 stating plainly why it has no standalone evidence — and CLAUDE.md gained the vendor-bootstrap section with corrected CLI and safety-invariant text.
I also read install: add selectable install stages, which arrived outside the review scope. It holds up: mutually exclusive modes, unknown stages rejected, --stage reset / --no-final-reset conflict caught, empty selection rejected, default flow unchanged, and a real guard against a partial persistent install that omits uboot while the board still boots vendor U-Boot.
Local gate on ab20fbea: 868 passed / 3 skipped, fuzz 16 passed, ruff check clean, mypy clean across 77 source files.
One non-blocking follow-up, noted so it isn't lost: protocol/hisilicon_standard.py:198 is unchanged and still calls flush_input(); flush_output() before every frame retransmission, so that path now drains where it previously purged — for all 112 UART chips. The divergence only shows up when a retry fires while the previous frame is still partly queued, and sending a complete frame there is probably better than a truncated one, so I don't think it's a risk. But the DS-I203 evidence can't speak to it, and a single defib burn on any HiSilicon board would retire the question.
) 52582f0 (#137) changed `Transport.flush_output()` from "discard queued TX" to "wait for queued TX to drain". That matches the documented contract, but it also turned a call that effectively never raised into one that raises `TransportTimeout` after 5 s — and two retry loops called it from outside their own try block. `HiSiliconStandard._send_frame_with_retry` runs the flushes before its try, and `RecoverySession.run()` invokes the protocol inside try/finally with no handler, so the raise propagated out of `burn`. Measured against a port whose `out_waiting` never reaches zero, the old purge behaviour completed four attempts and returned False while the drain behaviour raised with zero frames written: a stalled PL2303 or FT232R aborted the burn instead of retrying, on the primary recovery path for all 112 UART chips. `YModemSender._send_packet` had the same shape with a loop that only caught `YModemError`, aborting a stock-U-Boot chainload mid-transfer. This was a re-introduction, not a new class of defect. `tests/test_protocol_standard.py::TestWriteTimeoutRetry` already exists because `transport.write()` used to sit outside that same try, and its docstring names the same trigger — a hung write with the PL2303 TX buffer not draining. It pinned the invariant for `write()` only, so moving the flushes out went unnoticed. Move the flushes inside the try in both loops. A flush that waits on hardware is an I/O operation, not bookkeeping, so it belongs where transient failures are already absorbed. The bounded drain from #137 is kept exactly as merged. Adding that retry then created a second problem, caught in review. Retrying a stalled drain retransmits a packet whose first copy is still queued, so when the link recovers the receiver sees it twice and answers twice. `_send_packet` consumed one response and left the spare behind, where the next packet's read window picked it up — a NAK the receiver really sent was read as an ACK, and the data it asked to have resent was never retransmitted. Silent truncation of the chainloaded image rather than a failed transfer. Flush the receive buffer before every attempt, which is what `_send_frame_with_retry` already does and why the same duplicate-transmission window never bit the HiSilicon path. `_finish()` gets the same flush, because the last data packet can leave a spare response that would otherwise be read as the EOT answer. Purge-then-resend is self-synchronising; drain-then-resend is not. Verification: uv run pytest tests/ -q --ignore=tests/fuzz uv run pytest tests/fuzz/ -q --hypothesis-seed=0 871 passed / 3 skipped, fuzz 16 passed, ruff and mypy clean. Three regression tests, each beside the existing one it mirrors: the two stall tests fail against 52582f0 with a raw TransportTimeout, and the duplicate-response test NAKs a packet after an earlier one stalls and duplicates, then asserts it is actually retransmitted. Not hardware verified. Reproduced against a port whose `out_waiting` never reaches zero; on real hardware the divergence needs a link where the frame is still draining when the per-attempt ACK timeout fires. The change only restores the previous behaviour on the stalled path, so a `defib burn` on any HiSilicon board should look exactly as it does today. `YModemSender._finish()` still lets a transport error escape. It has no retry semantics to restore, and the installer reports it cleanly through the orchestrator's TransportError handler.
2 participants
While porting OpenIPC to a HiWatch DS-I203 (HI3518EV100, IMX122, 256 MiB DDR3, 16 MiB SPI NOR), I found that Defib's normal HiSilicon boot-ROM recovery path is not usable through the camera's exposed UART.
The recovery path available on the stock camera is Hikvision U-Boot 2010.06:
Ctrl+Uenters theHKVS #console, andloadycan receive a replacement U-Boot over YMODEM.The board also needs the DDR3/256 MiB OpenIPC U-Boot added here: OpenIPC/u-boot-hi3516cv100#6
The matching OpenIPC device profile is here: OpenIPC/builder#159
This PR connects that stock Hikvision recovery path to Defib's normal install flow and hardens the shared install/transport code found during review.
What changed
BootProtocol: boot protocols describe SoC boot-ROM recovery, whilevendors.*starts from an already-running vendor U-Boot.Ctrl+U,HKVS #, factory-MAC capture,loady/YMODEM,go, and detection of an already-running OpenIPC U-Boot.loadyandgo, now use UART echo verification before the terminating CR is sent.hi3518ev100:hiwatch-ds-i203resolves tou-boot-hi3518ev100-ddr3-256m-universal.binwithout changing the generichi3518ev100artifact.--ubootremains available for a local override.cli/app.pyintodefib.install.--nor-sizeis a genuine explicit override; the CLI default is now0, meaning auto-detect.tftpboot; only the stock-U-Boot path uses the shortloadaddrform.hi3518ev100RAM base instead of relying on prefix/fallback ordering.rootfs_dataerase verification.ethaddracross environment replacement. Installer-only values such as DS-I203phyaddru=3remain transient; persistent runtime board policy stays in the Builder profile.saveenv: Defib re-probes SPI, reads the env partition, computes its data CRC, and compares it to the stored environment CRC.TransportTimeout, now use the controlled installer failure path and release UART/power/TFTP resources. Pre-TFTP environment verification failures are covered as well.SerialTransport.flush_output()uses a boundedout_waitingdrain instead of unboundedtcdrain()or output-buffer purging.flush_output()is intentionally an explicit no-op and never uses PURGE_DATA.--output jsonerror behavior for installer preflight failures.list-chips, and added protocol-level YMODEM tests including retry and final-handshake behavior.--stage/--skip-stageinstall controls in a separate commit for targeted development/recovery validation. The normal no-flag production flow is unchanged; explicit stage selection only performs the requested persistent operations and starts TFTP only when required.Hardware verification
Final end-to-end acceptance was performed on a physical HiWatch DS-I203:
The final migration run used:
The run completed:
HKVS #entry;loadyandgo;rootfs_dataerase;mtdpartspersistence;saveenv;SPI CRC 7A58A0B5);The environment-only stage path was also exercised against an already-running OpenIPC U-Boot while validating the post-reset SPI re-probe and persistent CRC check.
Earlier complete device-profile acceptance also confirmed 256 MiB physical RAM, the intended 128 MiB Linux / 128 MiB MMZ split, PHY address 3 / MDIO0, IMX122, and Majestic startup.
Testing
Final local gate on the published tree:
873 passed, 10 skipped, 4 deselectedin the full Python suite.netshdecoding and Windows symlink privilege.16 passedintests/fuzz.0.15.8from the repository lockfile: clean onsrc/andtests/.git diff --check: clean.The first three commits were also rewritten to include the rationale, verification commands, and hardware evidence required by
CLAUDE.md.