From 119502a878030c6850482db62722b3e29ca13a76 Mon Sep 17 00:00:00 2001 From: Jan Szumiec Date: Thu, 30 Jul 2026 20:35:34 +0100 Subject: [PATCH] Stop the driver sweep at the first driver that claims the image MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Loading an image with no metadata trailer used to import all 191 CHIRP driver modules before detection could run — in the browser that is 191 sequential CDN fetches, the slowest thing the app does. Detection only ever needs the modules up to the winning driver, so import in the same order and stop at the first match instead. Measured over the 108 metadata-less images in chirp/tests/images, the median image is identified after 72.5 of 191 modules and the mean after 78, so roughly 60% of the fetches disappear. Worst case is unchanged. This cannot change which driver is chosen: get_radio_by_image returns the first match in DRV_TO_RADIO insertion order, and insertion order is decided by import order alone. Reordering the list for speed would not be safe, since the default match_model is a bare memory-size comparison several drivers can satisfy at once. Rebased onto the ImageDetectionError backstop from #46: the sweep is now what loadImageWithDriverFallback() injects, so both the unresolved path and the retry after a wrong fast-path resolve stop early. The injection point is renamed importDriversForDetection, since it no longer imports every driver. _image_class_matches() also returns after the metadata-less match_model branch instead of falling into the alias comparison the way upstream does. Upstream compares against meta_vendor/meta_model of None, which no registered class declares, so the two are equivalent — but only the early return makes the docstring's "two branches" true of the code. Co-Authored-By: Claude Opus 5 (1M context) --- FINDINGS.md | 7 +- scripts/test-image-metadata.mjs | 14 +- scripts/test-metadataless-image-load.mjs | 135 ++++++++++++++++- web/js/image-metadata.mjs | 35 +++-- web/js/runtime-rpc.js | 139 +++++++++-------- web/python/runtime_bridge.py | 185 ++++++++++++++++++++--- 6 files changed, 406 insertions(+), 109 deletions(-) diff --git a/FINDINGS.md b/FINDINGS.md index 6e89805..602c33f 100644 --- a/FINDINGS.md +++ b/FINDINGS.md @@ -35,15 +35,16 @@ that produced them is the durable part. ## Radio images (.img) & metadata -- **image-driver-resolution** (2026-07-23, consolidated and fixed 2026-07-30): `directory.get_radio_by_image()` only searches drivers already in `DRV_TO_RADIO`, and the browser imports drivers lazily — so `handleLoadImage` (`web/js/runtime-rpc.js`) parses the metadata trailer first (`read_image_metadata_base64`, driver-free), resolves the module via the catalog (`findCatalogRadioForImageMetadata`, `web/js/image-metadata.mjs`), imports it, then runs detection. When nothing resolves, `ensureAllDriverModules()` imports every driver so `match_model()` byte-sniffing can run — the fallback desktop CHIRP gets for free from `import_drivers()`. **The governing invariant: a resolved-but-wrong match is worse than no match, because it suppresses the sweep.** The matcher must therefore be at least as precise as the `get_radio_by_image()` it front-runs, which compares VENDOR/MODEL/VARIANT across `rclass.ALIASES + [rclass]` (`chirp/chirp/directory.py:202-204`). Four things it has to get right: +- **image-driver-resolution** (2026-07-23, consolidated and fixed 2026-07-30): `directory.get_radio_by_image()` only searches drivers already in `DRV_TO_RADIO`, and the browser imports drivers lazily — so `handleLoadImage` (`web/js/runtime-rpc.js`) parses the metadata trailer first (`read_image_metadata_base64`, driver-free), resolves the module via the catalog (`findCatalogRadioForImageMetadata`, `web/js/image-metadata.mjs`), imports it, then runs detection. When nothing resolves, `importDriverModulesUntilImageMatches()` imports drivers until one claims the image so `match_model()` byte-sniffing can run — the fallback desktop CHIRP gets for free from `import_drivers()` (see **incremental-detection-is-order-safe** for why stopping early is sound). **The governing invariant: a resolved-but-wrong match is worse than no match, because it suppresses the sweep.** The matcher must therefore be at least as precise as the `get_radio_by_image()` it front-runs, which compares VENDOR/MODEL/VARIANT across `rclass.ALIASES + [rclass]` (`chirp/chirp/directory.py:202-204`). Four things it has to get right: - **rclass may be unregistered or synthetic, so it cannot be the primary key.** `export_image_base64` from `uv5r.BaofengUV5R` stamps a class name whose registered entry is `BaofengUV5RGeneric`; more importantly, CHIRP stamps `DynamicRadioAlias` — the synthetic subclass it creates in `get_radio_by_image` (directory.py:206) — whenever detection went through an alias. 69 of the 250 trailer-bearing corpus images carry an rclass absent from the catalog (2026-08-01 pin), so identity, not class name, is the load-bearing path. A class-name hit now only wins if the recorded identity agrees with it. Matching also applies `directory.MODEL_COMPAT` remaps (e.g. Retevis RT-5R → RT5R). - **A class name is not unique across modules.** `Kenwood_TS-480_CloneMode.img` stamps `rclass=TS480Radio`, which exists as both `kenwood_live:TS480Radio` (model `TS-480_LiveMode`) and `ts480:TS480_CRadio` (model `TS-480_CloneMode`). Requiring the identity to agree resolves it to the clone driver directly. The `isLiveRadio` guard in `handleLoadImage` remains as a second line — a live radio can never own a clone image. - **`variant` is what separates same-vendor/model drivers.** `Quansheng_UV-K5_egzumer.img` stamps `variant=egzumer`; without it, vendor/model alone matched four catalog entries and resolved `uvk5.OSFWUVK5Radio`, which then failed with `Unsupported model Quansheng UV-K5`. The catalog now records `variant` and the full alias identity list, and the matcher compares them exactly as CHIRP does. Note `None` (no variant recorded) and `""` (an explicitly empty variant) are **different**: CHIRP skips the comparison for the former and demands `VARIANT == ""` for the latter, so `read_image_metadata_base64` must not collapse them. - **Ambiguity must decline, not guess.** 12 vendor/model pairs map to multiple catalog entries. When more than one candidate survives, the matcher returns null and takes the sweep rather than picking whichever sorts first. - Even so the matcher cannot be trusted absolutely, so `loadImageWithDriverFallback()` retries through the sweep when detection *fails* after a fast-path resolve — that backstop, not the matching precision, is what makes a wrong match merely slow instead of fatal. The retry is gated on `ImageDetectionError`, the one failure importing more drivers can fix; every other image failure (not clone-mode, bad payload, a driver raising while reading memories) fails identically after the sweep, so retrying it would only delay the real error by the sweep's ~20 s. That gate reads a Python class name out of the traceback Pyodide hands JS, which nothing else pins — hence the end-to-end assertion in `scripts/test-metadataless-image-load.mjs` that a real detection failure trips it and a bad payload does not. + Even so the matcher cannot be trusted absolutely, so `loadImageWithDriverFallback()` retries through the sweep when detection *fails* after a fast-path resolve — that backstop, not the matching precision, is what makes a wrong match merely slow instead of fatal. The retry is gated on `ImageDetectionError`, the one failure importing more drivers can fix; every other image failure (not clone-mode, bad payload, a driver raising while reading memories) fails identically after the sweep, so retrying it would only delay the real error by the sweep's seconds of CDN fetches. That gate reads a Python class name out of the traceback Pyodide hands JS, which nothing else pins — hence the end-to-end assertion in `scripts/test-metadataless-image-load.mjs` that a real detection failure trips it and a bad payload does not. - **image-corpus-measurements-need-fresh-runtimes** (2026-07-30): driver imports accumulate in a Pyodide session, so walking `chirp/tests/images/` in one runtime measures "a session where earlier files already imported things", not each image's own path — and the corpus is walked alphabetically, where `Alinco_DJ175.img` (no metadata trailer) triggers the full sweep on the very first file. That artifact produced both a 356/357 and a 357/357 figure for work that actually leaves one image failing. Measure per-path instead: import only what that image's branch would import (its catalog-resolved module, or the sweep), which needs no per-image runtime restart because a fast-path-only pass can only ever *hide* failures, never invent them. A cheaper oracle avoids the question entirely for matching: import every driver once, let CHIRP detect each image, and assert the matcher resolves either the same driver or nothing — a null resolution is correct by construction, because it takes the sweep. Split at the 2026-08-01 pin (CHIRP `b7ae1b6`): 358 `.img` files, 108 with no trailer that sweep, 250 resolved on the fast path, all 250 agreeing with detection — and re-measuring after that submodule bump is what caught the earlier 357/249/68 figures going stale, so treat every count here as pinned to a revision rather than durable. - **synchronous-python-can-drive-progress** (2026-07-30): a plain `for` loop inside one `runPythonAsync` *can* animate browser UI, which looks impossible and is why the driver sweep shipped silent at first. Every CHIRP import suspends the interpreter on a CDN fetch through `ChirpCdnFinder`'s JSPI `run_sync`, so the JS event loop runs between iterations and anything the loop wrote to the DOM paints — the same mechanism that lets CHIRP's synchronous clone loops drive `#clone-progress` via `serial_progress`. So reach for a callback argument (`import_all_driver_modules(names, progress_cb)`) before restructuring a loop into per-item RPCs; the latter would pay a full call-queue round trip per module for no benefit. Two rules for such callbacks: guard every call in `try/except` so a reporting failure can never abort the work being reported on, and clear the `pyodide.globals` slot afterwards. The reverse also holds — a Python loop that never suspends will not paint, so this is a property of the I/O, not of Pyodide. -- **all-driver-import-mechanics** (2026-07-30): `import_all_driver_modules()` (`runtime_bridge.py`) tolerates per-module failures — each failure is recorded and surfaced as `DRIVERS SKIP : ` in the debug log, never aborting the sweep. With the pyserial shim in place (see **pyserial-shim-and-idrp-never-registers**) all 191 driver modules import and 551 radio classes register, so the skip list is empty at this pin; keep reporting it anyway, because importability cannot be judged by grepping for `import serial` (`hf90`/`tmv71_ll` have function-scope imports that are harmless) and the next submodule bump can reintroduce a failure silently. `scripts/build-catalog.mjs` calls the same function so the catalog's "not importable, absent from catalog" warnings and the runtime's skip lines can never diverge. The sweep is one synchronous Python loop in a single `runPythonAsync` (~1.8 s in Node off the local checkout; ~20 s in-browser, where each module is fetched individually from jsDelivr), cached per session as a promise that resets on failure so a later load retries instead of caching the wreck. +- **all-driver-import-mechanics** (2026-07-30): `import_all_driver_modules()` (`runtime_bridge.py`) tolerates per-module failures — each failure is recorded and surfaced as `DRIVERS SKIP : ` in the debug log, never aborting the sweep. With the pyserial shim in place (see **pyserial-shim-and-idrp-never-registers**) all 191 driver modules import and 551 radio classes register, so the skip list is empty at this pin; keep reporting it anyway, because importability cannot be judged by grepping for `import serial` (`hf90`/`tmv71_ll` have function-scope imports that are harmless) and the next submodule bump can reintroduce a failure silently. `scripts/build-catalog.mjs` calls the same function so the catalog's "not importable, absent from catalog" warnings and the runtime's skip lines can never diverge. The sweep is one synchronous Python loop in a single `runPythonAsync` (~1.8 s in Node off the local checkout; ~20 s in-browser for the full list, where each module is fetched individually from jsDelivr). The runtime no longer runs the full sweep: `detect_image_driver_incremental()` shares the same loop and failure handling but stops at the first driver that claims the image, and `import_all_driver_modules()` now exists only for the catalog build and the tests. Only an *exhausted* run may be memoised for the session — a boolean, not a promise, because a run that stopped early leaves nothing reusable to cache and a failed one must not be cached at all. +- **incremental-detection-is-order-safe** (2026-07-30): stopping the driver sweep at the first driver that claims the image cannot change *which* driver is chosen, as long as the module list is imported in the same order as before. `directory.get_radio_by_image()` returns the first match in `DRV_TO_RADIO` insertion order, and insertion order is a pure function of import order — so the first hit found incrementally is the hit the full sweep would have returned. Measured over the 108 metadata-less images in `chirp/tests/images/` (191 modules, one CDN round trip each in the browser): all 108 match, min 1 module, p25 30, **median 72.5**, p75 116, max 188 (`Yaesu_VX-8*`) — roughly 60% of the fetches skipped at the median, worst case unchanged. What is **not** safe is reordering the list to try likely drivers first: 111 registered classes inherit the default `match_model`, which is the bare comparison `len(filedata) == cls._memsize`, and several classes share a memory size, so among those the winner is decided by order alone. Two things the implementation has to keep: classes registered *before* the sweep starts (whatever the user's radio selection imported) are checked first, because the full sweep would have seen them first; and detection mirrors *both* branches of `get_radio_by_image`, `match_model` for metadata-less images and the vendor/model/variant alias comparison for images whose trailer the catalog could not match (`Kenwood_TS-480_CloneMode.img`, which matches at module 168). The one deliberate divergence from upstream is that the mirror returns early when there is no metadata instead of falling into the alias comparison with `meta_vendor`/`meta_model` both `None`; no registered class declares a `None` VENDOR or MODEL, so the comparison upstream still runs can never match. ## Repeater directories diff --git a/scripts/test-image-metadata.mjs b/scripts/test-image-metadata.mjs index 23c2210..f10d2f2 100644 --- a/scripts/test-image-metadata.mjs +++ b/scripts/test-image-metadata.mjs @@ -418,7 +418,7 @@ test("detection failure after a resolved match retries against all drivers", asy } return { module: "uvk5_egzumer" }; }, - importAllDrivers: () => { + importDriversForDetection: () => { calls.push("sweep"); return Promise.resolve(); }, @@ -429,9 +429,9 @@ test("detection failure after a resolved match retries against all drivers", asy assert.deepEqual(result, { module: "uvk5_egzumer" }); }); -// The sweep is the slowest thing the app does (~20 s in the browser, every -// driver fetched individually from a CDN). Spending it on a failure it cannot -// possibly fix just delays the real error by 20 s. +// The sweep is the slowest thing the app does (every driver fetched +// individually from a CDN, seconds even when it stops early). Spending it on a +// failure it cannot possibly fix just delays the real error. test("a failure the sweep cannot fix is surfaced without sweeping", async () => { const calls = []; await assert.rejects( @@ -445,7 +445,7 @@ test("a failure the sweep cannot fix is surfaced without sweeping", async () => "Loaded image is not a clone-mode CHIRP image", ); }, - importAllDrivers: () => { + importDriversForDetection: () => { calls.push("sweep"); return Promise.resolve(); }, @@ -463,7 +463,7 @@ test("a successful resolved match never imports every driver", async () => { calls.push("load"); return { module: "uv5r" }; }, - importAllDrivers: () => { + importDriversForDetection: () => { calls.push("sweep"); return Promise.resolve(); }, @@ -483,7 +483,7 @@ test("an unresolved image sweeps first, and a failure there is surfaced", async calls.push("load"); throw new Error("Unable to detect radio from image"); }, - importAllDrivers: () => { + importDriversForDetection: () => { calls.push("sweep"); return Promise.resolve(); }, diff --git a/scripts/test-metadataless-image-load.mjs b/scripts/test-metadataless-image-load.mjs index b46bf51..6b26403 100644 --- a/scripts/test-metadataless-image-load.mjs +++ b/scripts/test-metadataless-image-load.mjs @@ -20,10 +20,30 @@ const imagesDir = path.join(repoRoot, "chirp/tests/images"); const METADATA_LESS_IMAGE = "Baofeng_UV-3R.img"; const METADATA_IMAGE = "Baofeng_UV-5R.img"; +// Two metadata-less images whose owning driver sits at opposite ends of the +// alphabetical module list (baofeng_uv3r is 13th of ~191, kguv8d is 94th), so +// the early exit is exercised near the start and around the middle of a sweep. +const PARITY_IMAGES = ["Baofeng_UV-3R.img", "Wouxun_KG-UV8D.img"]; + +// No driver claims a payload this small: it matches no _memsize, and every +// custom match_model rejects it. +const UNCLAIMABLE_IMAGE_BYTES = 7; + async function readImage(name) { return new Uint8Array(await fs.readFile(path.join(imagesDir, name))); } +async function detectIncrementally(harness, image, modules, progressCb = null) { + return harness.runPythonJson( + "json.dumps(detect_image_driver_incremental(_image_b64, _mods, _progress_cb))", + { + _image_b64: Buffer.from(image).toString("base64"), + _mods: modules, + _progress_cb: progressCb, + }, + ); +} + test("image with a metadata trailer needs no full driver import", async () => { const harness = await createTestRadioHarness({ repoRoot }); const metadata = await harness.runPythonJson( @@ -85,7 +105,7 @@ test("clone image whose metadata resolves to a live-mode driver still loads", as assert.ok(loaded.rows.length > 0, "expected channels to be populated"); }); -// The browser retries the ~20 s all-drivers sweep only when detection is what +// The browser retries the driver sweep only when detection is what // failed, and it decides that by reading the Python class name out of the // traceback Pyodide hands it. Nothing else pins the two together: rename the // Python class and the backstop goes quietly dead, while widening the predicate @@ -112,6 +132,119 @@ test("the retry gate recognises a real detection failure and nothing else", asyn ); }); +// The incremental sweep only saves work if it never changes the answer. It is +// safe because get_radio_by_image returns the first match in DRV_TO_RADIO +// insertion order, and insertion order is decided by import order alone: import +// the same list in the same order and stop at the first hit, and the winner is +// the one the full sweep would have found. That equivalence is what these tests +// pin down — reordering the module list for speed would break it silently, +// because the default match_model is a bare memory-size comparison that several +// drivers can satisfy at once. +test("incremental detection picks the full sweep's driver without importing every module", async () => { + const reference = await createTestRadioHarness({ repoRoot }); + const modules = await listDriverModules(reference.pythonSource); + await reference.runPythonJson("json.dumps(import_all_driver_modules(_mods))", { + _mods: modules, + }); + + for (const name of PARITY_IMAGES) { + const image = await readImage(name); + const full = await reference.loadCodeplugBinary(image); + + // A fresh runtime per image: with nothing imported yet, the early exit is + // real rather than an artifact of drivers a previous case left registered. + const harness = await createTestRadioHarness({ repoRoot }); + const reported = []; + const detected = await detectIncrementally(harness, image, modules, (done, total, mod) => + reported.push([done, total, mod]), + ); + + assert.equal(detected.matched, true, `${name}: expected a driver to claim the image`); + assert.equal(detected.module, full.module, `${name}: driver module`); + assert.equal(detected.className, full.className, `${name}: driver class`); + assert.equal(detected.exhausted, false, `${name}: expected an early exit`); + assert.ok( + detected.imported < detected.total, + `${name}: imported ${detected.imported} of ${detected.total} modules`, + ); + + // Progress has to stop where detection stopped. A report past the match + // would mean the sweep kept fetching modules it no longer needed. + assert.equal(reported.length, detected.imported, `${name}: progress reports`); + assert.deepEqual(reported.at(-1).slice(0, 2), [detected.imported, detected.total]); + + // Loading still goes through get_radio_by_image, now over a partially + // populated directory, and must produce the same radio and the same rows. + const loaded = await harness.loadCodeplugBinary(image); + assert.equal(loaded.module, full.module, `${name}: loaded module`); + assert.equal(loaded.className, full.className, `${name}: loaded class`); + assert.deepEqual(loaded.rows, full.rows, `${name}: loaded channels`); + } +}); + +// A driver imported earlier in the session (the radio the user had selected) +// sits at the front of DRV_TO_RADIO, so the full sweep would consider it before +// anything it imports. The incremental sweep has to check the already-registered +// classes first for the same reason. +test("detection considers drivers imported earlier in the session first", async () => { + const harness = await createTestRadioHarness({ repoRoot }); + await harness.pyodide.runPythonAsync("ensure_radio_module('baofeng_uv3r')"); + + const detected = await detectIncrementally( + harness, + await readImage(METADATA_LESS_IMAGE), + ["uv5r", "ft60"], + ); + + assert.equal(detected.matched, true); + assert.equal(detected.module, "baofeng_uv3r"); + assert.equal(detected.className, "UV3RRadio"); + assert.equal(detected.imported, 0, "expected no module imports at all"); +}); + +test("an image no driver claims imports every module and reports the list exhausted", async () => { + const harness = await createTestRadioHarness({ repoRoot }); + const modules = await listDriverModules(harness.pythonSource); + + const detected = await detectIncrementally( + harness, + new Uint8Array(UNCLAIMABLE_IMAGE_BYTES), + modules, + ); + + assert.equal(detected.matched, false); + assert.equal(detected.module, ""); + // Only an exhausted list means every driver is registered, which is the one + // thing the caller may cache for the rest of the session. + assert.equal(detected.exhausted, true); + assert.equal(detected.imported, modules.length); + assert.ok(detected.registered > 500, `expected many radio classes, got ${detected.registered}`); +}); + +// Images with a metadata trailer normally skip detection entirely, but the +// TS-480 trailer resolves to a live-mode driver in the catalog, so it reaches +// this path. Detection then has to match on the vendor/model/variant aliases +// rather than match_model. +test("image whose metadata the catalog cannot match is detected incrementally too", async () => { + const harness = await createTestRadioHarness({ repoRoot }); + const modules = await listDriverModules(harness.pythonSource); + const image = await readImage("Kenwood_TS-480_CloneMode.img"); + + const detected = await detectIncrementally(harness, image, modules); + + assert.equal(detected.matched, true); + assert.equal(detected.module, "ts480"); + assert.equal(detected.className, "TS480_CRadio"); + assert.ok( + detected.imported < detected.total, + `imported ${detected.imported} of ${detected.total} modules`, + ); + + const loaded = await harness.loadCodeplugBinary(image); + assert.equal(loaded.className, "TS480_CRadio"); + assert.ok(loaded.rows.length > 0, "expected channels to be populated"); +}); + test("import_all_driver_modules reports unimportable drivers instead of hiding them", async () => { const harness = await createTestRadioHarness({ repoRoot }); const result = await harness.runPythonJson( diff --git a/web/js/image-metadata.mjs b/web/js/image-metadata.mjs index ba44682..8275b5f 100644 --- a/web/js/image-metadata.mjs +++ b/web/js/image-metadata.mjs @@ -82,30 +82,33 @@ export function findCatalogRadioForImageMetadata(radioCatalog, metadata) { } // Only a detection failure is worth a retry. `runtime_bridge.ImageDetectionError` -// means no imported driver claimed the image, which importing the rest can fix; -// every other failure (not a clone-mode image, a bad payload, a driver blowing -// up while reading memories) is about the image itself and would still fail -// after the sweep — so retrying would just cost ~20 s in the browser before -// surfacing the same error. Pyodide surfaces the Python traceback as the error -// message, so the class name is the contract; see the Python docstring. +// means no imported driver claimed the image, which importing more drivers can +// fix; every other failure (not a clone-mode image, a bad payload, a driver +// blowing up while reading memories) is about the image itself and would still +// fail after the sweep — so retrying would just cost seconds of CDN fetches in +// the browser before surfacing the same error. Pyodide surfaces the Python +// traceback as the error message, so the class name is the contract; see the +// Python docstring. export function isImageDetectionFailure(error) { return /\bImageDetectionError\b/.test(String(error?.message || error || "")); } -// Detection after a fast-path resolve, with the all-drivers sweep as a -// backstop. Matching can be wrong in ways the catalog cannot see — a driver -// whose match_model rejects an image its metadata claims, a future CHIRP that -// records something new — and without this retry a wrong match is worse than -// no match at all, because it skips the sweep that would have succeeded. -// Injectable rather than inlined so it can be tested without a Pyodide runtime. +// Detection after a fast-path resolve, with the driver sweep as a backstop. +// Matching can be wrong in ways the catalog cannot see — a driver whose +// match_model rejects an image its metadata claims, a future CHIRP that records +// something new — and without this retry a wrong match is worse than no match at +// all, because it skips the sweep that would have succeeded. +// `importDriversForDetection` imports driver modules until one claims the image +// (or the list runs out); it is injectable rather than inlined so this can be +// tested without a Pyodide runtime. export async function loadImageWithDriverFallback({ resolvedDriver, loadImage, - importAllDrivers, + importDriversForDetection, log, }) { if (!resolvedDriver) { - await importAllDrivers(); + await importDriversForDetection(); return loadImage(); } try { @@ -116,9 +119,9 @@ export async function loadImageWithDriverFallback({ } log?.( `IMAGE detection failed with ${resolvedDriver.module}.${resolvedDriver.className} ` - + `(${error?.message || error}); retrying against all drivers`, + + `(${error?.message || error}); retrying against the remaining drivers`, ); - await importAllDrivers(); + await importDriversForDetection(); return loadImage(); } } diff --git a/web/js/runtime-rpc.js b/web/js/runtime-rpc.js index e5283d8..41bb3a2 100644 --- a/web/js/runtime-rpc.js +++ b/web/js/runtime-rpc.js @@ -23,7 +23,11 @@ const pythonSource = createBrowserCdnPythonSource({ let pyodide; let bootstrapPromise; let radioCatalogCache = null; -let allDriverModulesPromise = null; +// Set once a detection sweep has run out of modules to try. Detection stops at +// the first driver that claims the image, so "we imported some drivers" is not +// a reusable fact; only "every driver is registered" lets a later load skip the +// sweep entirely. +let allDriverModulesImported = false; let handleSerialRpc = null; let bootstrapFailed = false; let debugLog = null; @@ -90,66 +94,75 @@ async function ensureSelectedRadioModules(moduleShortName) { await pyodide.runPythonAsync("ensure_radio_module(_sel_module_short)"); } -// Import every driver module once per session. Only the metadata-less image -// path needs this: it is the one case where nothing identifies the driver up -// front, so detection has to try them all. Each module is fetched individually -// by the Python import hook, so this is deliberately not done eagerly. -async function ensureAllDriverModules() { - if (!allDriverModulesPromise) { - allDriverModulesPromise = (async () => { - const modules = await listDriverModules(pythonSource); - await ensurePyodide(); - - // In the browser each module is a separate CDN fetch, so this is by far - // the longest operation the app runs. Report it: an unannounced multi- - // second freeze is indistinguishable from a hang. - const progress = beginProgress - ? beginProgress("Identifying radio: loading CHIRP drivers", modules.length) - : null; - if (debugLog) { - debugLog( - `DRIVERS image identifies no driver; importing all ${modules.length} ` - + "driver modules so match_model can run", - ); - } +// Import driver modules until one of them claims this image. Only the +// metadata-less (or metadata-unmatched) image path needs this: it is the one +// case where nothing identifies the driver up front, so detection has to try +// drivers until it finds the owner. Each module is fetched individually by the +// Python import hook, so this is both expensive and deliberately not eager — +// stopping at the first match typically skips well over half the fetches. +// +// Call only from a queued runtime method: two overlapping sweeps would import +// the same module twice and re-register every radio class in it (see +// scripts/test-driver-import-race.mjs). +async function importDriverModulesUntilImageMatches(imageBase64) { + if (allDriverModulesImported) { + return { matched: false, imported: 0, total: 0, exhausted: true, skipped: true }; + } + const modules = await listDriverModules(pythonSource); + await ensurePyodide(); - const reportProgress = (done, total, moduleShort) => { - progress?.update(done); - if (debugLog && (done % DRIVER_LOG_INTERVAL === 0 || done === total)) { - debugLog(`DRIVERS ${done}/${total} imported (latest ${moduleShort})`); - } - }; + // In the browser each module is a separate CDN fetch, so this is by far the + // longest operation the app runs. Report it: an unannounced multi-second + // freeze is indistinguishable from a hang. The total is an upper bound — + // detection usually ends the strip early. + const progress = beginProgress + ? beginProgress("Identifying radio: loading CHIRP drivers", modules.length) + : null; + if (debugLog) { + debugLog( + `DRIVERS image identifies no driver; importing up to ${modules.length} ` + + "driver modules until one matches", + ); + } - pyodide.globals.set("_all_driver_modules", modules); - pyodide.globals.set("_driver_progress_cb", reportProgress); - try { - const result = await runPythonJson( - "json.dumps(import_all_driver_modules(_all_driver_modules, _driver_progress_cb))", - ); - if (debugLog) { - const failed = Object.entries(result.failed || {}); - debugLog( - `DRIVERS imported ${result.imported}/${modules.length} modules, ` - + `${result.registered} radio classes registered`, - ); - for (const [moduleName, error] of failed) { - debugLog(`DRIVERS SKIP ${moduleName}: ${error}`); - } - } - return result; - } finally { - // The strip must come down on the failure path too, or a failed sweep - // leaves a frozen bar on screen for the rest of the session. - progress?.end(); - pyodide.globals.set("_driver_progress_cb", null); + const reportProgress = (done, total, moduleShort) => { + progress?.update(done); + if (debugLog && (done % DRIVER_LOG_INTERVAL === 0 || done === total)) { + debugLog(`DRIVERS ${done}/${total} imported (latest ${moduleShort})`); + } + }; + + pyodide.globals.set("_detect_image_b64", imageBase64 || ""); + pyodide.globals.set("_all_driver_modules", modules); + pyodide.globals.set("_driver_progress_cb", reportProgress); + try { + const result = await runPythonJson( + "json.dumps(detect_image_driver_incremental(" + + "_detect_image_b64, _all_driver_modules, _driver_progress_cb))", + ); + if (result.exhausted) { + allDriverModulesImported = true; + } + if (debugLog) { + debugLog( + result.matched + ? `DRIVERS ${result.module}.${result.className} claims the image after ` + + `${result.imported}/${result.total} modules ` + + `(${result.registered} radio classes registered)` + : `DRIVERS no driver claimed the image after all ${result.imported} modules ` + + `(${result.registered} radio classes registered)`, + ); + for (const [moduleName, error] of Object.entries(result.failed || {})) { + debugLog(`DRIVERS SKIP ${moduleName}: ${error}`); } - })().catch((error) => { - // Let a later load retry rather than caching the failure for the session. - allDriverModulesPromise = null; - throw error; - }); + } + return result; + } finally { + // The strip must come down on the failure path too, or a failed sweep + // leaves a frozen bar on screen for the rest of the session. + progress?.end(); + pyodide.globals.set("_driver_progress_cb", null); } - return allDriverModulesPromise; } function sortRadioCatalog(radios) { @@ -346,17 +359,21 @@ async function handleLoadImage(payload = {}) { if (!resolvedDriver && debugLog) { // Nothing identified the driver: either the image predates the metadata // trailer, or its metadata names a model the catalog does not list. Either - // way the only route left is match_model against every driver, which can - // only match drivers that have been imported. + // way the only route left is CHIRP's own detection, which can only consider + // drivers that have been imported — so import until one of them claims it. debugLog( `IMAGE ${metadata?.hasMetadata ? "metadata unmatched" : "metadata absent"}; ` - + "importing all drivers for detection", + + "importing drivers until one claims the image", ); } + // Detection below only decides how many drivers to register; the load itself + // still goes through get_radio_by_image, so there is one code path that picks + // the driver and one that reads the image. return loadImageWithDriverFallback({ resolvedDriver, loadImage: () => runPythonJson("json.dumps(load_image_base64(_image_b64))"), - importAllDrivers: () => ensureAllDriverModules(), + importDriversForDetection: () => + importDriverModulesUntilImageMatches(payload.imageBase64 || ""), log: debugLog, }); } diff --git a/web/python/runtime_bridge.py b/web/python/runtime_bridge.py index e9a964c..72d90aa 100644 --- a/web/python/runtime_bridge.py +++ b/web/python/runtime_bridge.py @@ -1,6 +1,7 @@ import asyncio import base64 import builtins +import contextlib import importlib import importlib.abc import json @@ -255,6 +256,132 @@ def import_all_driver_modules(module_short_names, progress_cb=None): } +def _image_class_matches(rclass, filedata, image_path, metadata) -> bool: + """Return whether ``get_radio_by_image`` would pick ``rclass`` for this image. + + This mirrors the per-class body of ``directory.get_radio_by_image``, + including both branches (``match_model`` for metadata-less images, + vendor/model/variant alias comparison for images with a trailer) and the + swallowing of driver exceptions during detection. Any divergence here would + make an incremental sweep resolve a different driver than the full one. + + The one deliberate difference: upstream falls through into the alias + comparison even for metadata-less images, where ``meta_vendor`` and + ``meta_model`` are both ``None``. No registered class declares a ``None`` + VENDOR or MODEL, so that comparison can never match and returning early is + equivalent — while keeping the two branches actually distinct. + """ + if not issubclass(rclass, chirp_common.FileBackedRadio): + return False + + if not metadata: + try: + return bool(rclass.match_model(filedata, image_path)) + except Exception as exc: + _log_debug(f"DETECT driver {rclass.__name__} failed during detection: {exc}") + return False + + meta_vendor = metadata.get("vendor") + meta_model = metadata.get("model") + meta_variant = metadata.get("variant") + meta_vendor, meta_model = directory.MODEL_COMPAT.get( + (meta_vendor, meta_model), (meta_vendor, meta_model) + ) + for alias in list(rclass.ALIASES) + [rclass]: + if ( + alias.VENDOR == meta_vendor + and alias.MODEL == meta_model + and (meta_variant is None or alias.VARIANT == meta_variant) + ): + return True + return False + + +def _first_matching_new_radio_class(seen_keys, filedata, image_path, metadata): + """Check radio classes registered since the last call, in registration order. + + ``DRV_TO_RADIO`` is insertion-ordered and ``get_radio_by_image`` returns the + first match in that order, so checking each newly registered batch in order + and stopping at the first hit yields the same class the full sweep would. + """ + for key, rclass in list(directory.DRV_TO_RADIO.items()): + if key in seen_keys: + continue + seen_keys.add(key) + if _image_class_matches(rclass, filedata, image_path, metadata): + return rclass + return None + + +def detect_image_driver_incremental(image_b64, module_short_names, progress_cb=None): + """Import drivers one at a time, stopping at the first one that claims the image. + + The full ``import_all_driver_modules`` sweep is the slowest thing the app + does — in the browser every module is its own CDN round trip — yet detection + only ever needs the modules up to the winning driver. Importing in the same + order and stopping early therefore costs nothing in correctness: registration + order, and so the class ``get_radio_by_image`` picks, is unchanged. Ordering + the list any other way would not be safe, because the default ``match_model`` + is a bare memory-size comparison that several drivers can satisfy at once. + + Classes already registered before this call are checked first, for the same + reason: a driver imported earlier in the session (a radio the user selected) + sits at the front of ``DRV_TO_RADIO``, and the full sweep would have + considered it first too. + + ``progress_cb(done, total, module_short)`` is optional and reports after each + module, exactly as in ``import_all_driver_modules``. + """ + raw_image = _decode_image_b64(image_b64) + _, metadata = chirp_common.CloneModeRadio._strip_metadata(raw_image) + + names = [str(name or "").strip() for name in module_short_names or []] + names = [name for name in names if name] + total = len(names) + seen_keys = set() + failed = {} + imported = 0 + match = None + + with _temp_image_file(raw_image) as image_path: + match = _first_matching_new_radio_class( + seen_keys, raw_image, image_path, metadata + ) + for module_short in names: + if match is not None: + break + try: + ensure_radio_module(module_short) + except Exception as exc: + failed[module_short] = f"{type(exc).__name__}: {exc}" + imported += 1 + match = _first_matching_new_radio_class( + seen_keys, raw_image, image_path, metadata + ) + if progress_cb is not None: + try: + progress_cb(imported, total, module_short) + except Exception: + pass # Progress reporting must never abort the sweep. + + result = { + "matched": match is not None, + "module": "", + "className": "", + "imported": imported, + "total": total, + # True only when the loop ran out of modules, i.e. the caller now has + # every driver registered and never needs this sweep again. + "exhausted": match is None, + "failed": failed, + "registered": len(directory.DRV_TO_RADIO), + } + if match is not None: + result["module"] = str(match.__module__).rsplit(".", 1)[-1] + result["className"] = str(match.__name__) + return result + + def list_registered_radios(module_short_names): """Import drivers and return radios from CHIRP's registration directory.""" loaded_modules = set() @@ -1409,13 +1536,40 @@ def export_image_base64(module_name: str, class_name: str, rows, settings_groups } -def read_image_metadata_base64(image_b64: str): - """Parse the CHIRP metadata trailer from a .img payload without importing drivers.""" +def _decode_image_b64(image_b64: str) -> bytes: + """Decode a base64 .img payload into bytes, with a UI-facing error on failure.""" try: - raw_image = base64.b64decode(str(image_b64 or ""), validate=True) + return base64.b64decode(str(image_b64 or ""), validate=True) except Exception as exc: raise RuntimeUnsupportedError("Invalid image base64 payload") from exc + +@contextlib.contextmanager +def _temp_image_file(raw_image: bytes): + """Materialize an image on disk for CHIRP APIs that take a filename. + + The suffix matters: drivers receive this path in ``match_model`` and some of + them key off the extension, so detection has to see the same ``.img`` name + shape the real load does. + """ + with tempfile.NamedTemporaryFile( + mode="wb", suffix=".img", prefix="webchirp-", delete=False + ) as f: + image_path = f.name + f.write(raw_image) + try: + yield image_path + finally: + try: + os.unlink(image_path) + except Exception: + pass # A leaked temp file must never fail the load itself. + + +def read_image_metadata_base64(image_b64: str): + """Parse the CHIRP metadata trailer from a .img payload without importing drivers.""" + raw_image = _decode_image_b64(image_b64) + _, metadata = chirp_common.CloneModeRadio._strip_metadata(raw_image) if not metadata: return {"hasMetadata": False} @@ -1439,26 +1593,15 @@ def read_image_metadata_base64(image_b64: str): def load_image_base64(image_b64: str): """Load a CHIRP .img payload, detect driver, and return rows + radio identity.""" - try: - raw_image = base64.b64decode(str(image_b64 or ""), validate=True) - except Exception as exc: - raise RuntimeUnsupportedError("Invalid image base64 payload") from exc - - with tempfile.NamedTemporaryFile( - mode="wb", suffix=".img", prefix="webchirp-", delete=False - ) as f: - image_path = f.name - f.write(raw_image) + raw_image = _decode_image_b64(image_b64) - try: - radio = directory.get_radio_by_image(image_path) - except Exception as exc: - raise ImageDetectionError(f"Unable to detect radio from image: {exc}") from exc - finally: + with _temp_image_file(raw_image) as image_path: try: - os.unlink(image_path) - except Exception: - pass + radio = directory.get_radio_by_image(image_path) + except Exception as exc: + raise ImageDetectionError( + f"Unable to detect radio from image: {exc}" + ) from exc if not isinstance(radio, chirp_common.CloneModeRadio): raise RuntimeUnsupportedError("Loaded image is not a clone-mode CHIRP image")