Modem quality & compatibility wave (retroactive review PR for the released v1.3.0) - #21
Merged
Conversation
…iming policy The reconnect backoff was a bare setTimeout promise, so a disconnect() landing mid-backoff left the loop parked for the whole remaining delay — 2s at the default ceiling, unbounded for a caller that raised it. It is now cancellable: disconnect() cuts the sleep short and awaits the loop, so no reconnect loop outlives a close. The reconnect loop is retained rather than fired and forgotten. The single-loop property no longer rests solely on the #handleDrop state guard, and disconnect() has a handle to wait on. Two teardown failures were swallowed outright — a rejecting bus.disconnect() and a throwing connection.end() — so a socket a consumer believed closed could fail to close with no signal anywhere. Both now emit a typed TransportTeardownFailure (phase + step + cause) on the existing 'error' event. It is never thrown, disconnect() never rejects because of it, and emission is guarded against Node's unobserved-'error' re-throw so a report about a failed teardown cannot become the crash it describes. The 2s connect bound and the 30s call bound are now one named TransportTimingPolicy, injectable via options.timing. Both default values are unchanged, and the call bound is sourced from calls.ts's own constant so the policy cannot drift from it. Also fences the case where disconnect() lands while an establish attempt is in flight: the attempt now fails into the shared teardown path instead of publishing a live bus onto a closed transport. Wire-level reconnect semantics and happy-path timing are unchanged. New coverage in lifecycle.test.ts drives an injected fake bus, so the failure paths a real dbus-daemon cannot be made to take on demand are deterministic and need no session bus.
… setup rate
Claim NormalizedSignal.rsrp/rsrq/snr/sinr for MM-managed modems from the
Modem.Signal interface's own per-RAT a{sv} properties, so a ModemManager
device reports the detail the Huawei/ZTE dongles always did.
An a{sv} decodes to [key, variant][], and snapshot.ts's rawValue had no
variant branch — every dict retained as [['rsrp'], ['rsrq'], ...], keeping
the key and dropping the reading. Unwrapping it is what makes the extended
metrics reachable at all; removing that line reddens 3 of the 5 new
provider tests.
rsrp/rsrq/snr read Nr5g then Lte and dbm reads rssi across Lte/Umts/Gsm/
Evdo/Cdma. Nothing is merged or averaged: on an NSA attach both dicts are
populated with different measurements, so the ladder picks one and
provenance names it (Signal.Nr5g.rsrp), while the unchosen dict stays
verbatim in the diagnostics block.
sinr comes from Evdo and from no other dict — verified against MM 1.24.2's
own introspection XML, where Lte/Nr5g publish snr, a different quantity.
So an LTE/5G modem reporting no SINR now answers not-reported rather than
the previous blanket unsupported, which was a false capability claim: the
source can express SINR, this modem did not. The NR SINR available through
Modem.GetCellInfo is a different call on a different interface and is not
folded in here.
Also thread the Signal.Setup rate through ModemManagerProviderOptions. The
backend had accepted signalIntervalSeconds since it was written, but the
provider — what an embedder actually constructs — had no way to pass it.
Setup takes a `u`, so a fractional or non-positive rate is refused at
construction rather than marshalled. The once-per-(epoch, modem) issue
semantics are unchanged and now additionally pinned by test.
…rates Two additive read surfaces, both fetched-and-verified against ModemManager 1.24.2 rather than written from recall. Registration and cell context. `NormalizedRadio` gains `operatorName` and `operatorCode`, read from `Modem3gpp` and never from `Sim` — the first is the operator the modem is registered with, the second is the home operator written into the SIM, and they disagree for the whole time a device is roaming. The code stays text because a two- versus three-digit MNC is a different network. A new `cell` block reports `cellId` and `tac`, decoded together out of the existing `3gpp-lac-ci` source's single five-token value; a value in any other shape fails both fields rather than decoding partially, and the uppercase hex is kept as written so an identifier matches what mmcli shows. The GNSS fence is untouched by design: `signal_location` stays false, `3gpp-lac-ci` stays outside `GNSS_SOURCES`, coarse cell context stays out of the coordinate redaction class, and nothing here enables a location source. One consequence is recorded rather than hidden — MM masks the `Location` property unless `signal_location` is true, so nothing populates the optional `location` input today and an MM observation honestly reads `not-observed` for `cell`. Cell identity that is wired runs through `Modem.GetCellInfo`, where `CellReading` now also carries `tac` and reads MM's real `ci` key ahead of the older `cell-id` spelling it had been reading instead. No EARFCN is claimed anywhere. MM publishes none generically; the only occurrences are per-cell, under `earfcn` for LTE and `nrarfcn` for 5GNR — two keys for two quantities, so one slot would have to merge them or pick a RAT. Counter-reset-aware throughput. `SlotUsageSnapshot` gains `rateBytesPerSecond`, and the key is omitted rather than set to zero whenever there was no interval to measure. Interface counters restart when the interface is re-created, and both obvious repairs report something untrue: clamping the negative delta shows an idle link that was carrying traffic, while dividing the raw post-reset value shows every byte since the interface came up as one interval's traffic. So a backwards counter reports no rate at all and the baseline is rebased in the same pass, which is what makes the next interval correct instead of inheriting the gap. Rates are never persisted — a same-boot reload resumes the cumulative baseline but restarts the rate unmeasured, because this process did not observe that interval's start. The counter-reset concept is credited to irlserver/modem-metrics (MIT); concepts adopted, no source code copied. `accounting.ts`'s private `sameKey` becomes the exported `sameBaselineKey` so the rate and the reducer cannot drift about what "same counter" means. Full workspace 1492 pass / 0 fail (from 1459), typecheck and lint clean, verify:tarball OK with the unchanged seven-entry public surface.
Todo 7 wrapped the companion build in a `run: |` guard that validates RELEASE_VERSION against the tag, so the literal `run: packaging/ci/build-companion.sh` the wiring contract grepped for no longer exists and the contract went red — which fails release.yml's own test job before build-deb can start. Match the invocation anchored at end-of-line instead. A plain fixed-string match on the path would have silently matched the `echo "::error::…"` line inside the guard and passed even with the invocation deleted.
The quirks row claimed no `AT!` form existed anywhere in this repository. The gate it cites (`providers/ufi-himi`) scans that provider's directory, and `usb-mode/runtime-capability.ts` carries Sierra's reviewed `AT!USBCOMP` composition forms — which is what makes the composition switch `implemented` for Sierra. The row is about the password-gated `AT!BAND` / `AT!ENTERCND` surface, which really is absent, so the rung stays `unavailable`; only the scope of the justification was wrong. COMPAT-MATRIX.md recorded this as an open discrepancy and now records the reconciliation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Affected repo & language:
modem-stack— TypeScript (Bun workspace:control/+cli/) and Debian packaging (packaging/, shell).What
Nineteen commits delivering the
modem-stack-quality-compateffort: modem vendor classification and quirk sourcing (Sierra, Telit, u-blox, NETGEAR), a compatibility matrix across vendor × firmware × composition × operation, ModemManager Signal-interface metric normalization, operator/cell registration context with counter-reset-aware usage rates, transport and safety-gate hardening, a decomposition of five oversized modules, and the release plumbing that producedv1.3.0(first-party version unity across the threepackage.jsonfiles plus the companion build, and the differential per-source~ceralive.Nrebuild path).The branch tip is
26db4f9, which is exactly the commit tagv1.3.0points at. Nothing on this branch has been rebased, amended, or force-pushed since the release — the reviewed content is byte-identical to what shipped.Why
The release was cut directly from this branch with maintainer approval, but the code-review PR that should have preceded it was never opened. This PR closes that audit-trail gap after the fact. It does not re-do, re-tag, or re-publish anything:
@ceralive/modem-control@1.3.0on npm and the GitHub release with its 56 assets are final and untouched.It also fixes a history-topology problem. Tag
v1.3.0currently sits on a branch that is not reachable frommain. Merging this PR with a real merge commit makes the tagged commit an ancestor ofmain, so the published release stays reachable from the canonical branch and futuregit merge-basetracking against it keeps working.Merge method is load-bearing, not a preference. A squash merge collapses these 19 commits into one new commit with a new hash and drops the branch's parentage. The published tag would then point at a commit that is not on
mainand never becomes so — permanently orphaningv1.3.0from the canonical history. RootAGENTS.mdrecords the same reasoning for upstream-sync PRs. Please use "Create a merge commit".How to verify
main:bun run lint,bun run typecheck,bun test, and the standalone Node 26 + Bun consumer fixtures against the packed tarball; CI (packaging) —packaging/ci/contract.shin adebian:bookwormcontainer.closure_version: 2,suffix_scheme: per-source-counter, and 19 runtime rows (9 packages × 2 arches + 1Architecture: allcompanion), matching the frozen matrix inapt-worker/scripts/modem-closure-lib.sh.Risks
Merge-method risk — the only real one. Squash- or rebase-merging this PR orphans the published
v1.3.0tag frommainirreversibly. Merge commit only. The post-mergemerge-base --is-ancestorcheck above is the verification.Code risk: low. Every commit here is already running in a published release, so this is not a first exposure to production. The branch is 0 commits behind
main, so the merge is a fast-forward-shaped merge with no conflicts and no content change tomainbeyond what the release already contains.Known, pre-existing, accepted gap (not introduced by this PR):
apt.ceralive.tvdoes not yet serve thev1.3.0closure.modem-stackdispatched the reindex successfully; the receivingapt-workerrun failed its publisher-credential gate before any index mutation, so there is no partial publish and a re-run needs no rebuild and no re-tag. This is owner-side secret provisioning onCERALIVE/apt-workerand is tracked there.Reviewer note — the companion package carries no committed per-release changelog entry, and that is by design.
packaging/ceralive-modem-support/debian/changelogis a permanent0.0.0~dev/UNRELEASEDplaceholder.packaging/ci/build-companion.shrewrites the top entry withdchin a work copy only (the comment at L51 says so explicitly) and then fails closed if the built.debVersion:does not equal the release version. The four upstream sources work identically — their committed changelogs top out at the plain Debian versions (modemmanager 1.24.2-2,libmbim 1.34.0-1,libqmi 1.38.0-1,libqrtr-glib 1.4.0-1), andpackaging/ci/inject-deb-version.shappends the~ceralive.Nsuffix at build time. So no package in this repo commits a release changelog entry, and pre-writing one for the companion would be actively harmful:dchreplaces anUNRELEASEDtop entry in place, so a pre-written1.3.0 stableentry produces two duplicate1.3.0entries — permanently, since the top entry would no longer beUNRELEASED.Checklist
origin/main, deliberately not rebased since the tag was cutbun test1492/0, typecheck 0, lint 0 across 410 files,packaging/ci/contract.shPASS indebian:bookworm,verify:tarballandverify:consumersOK on Node 26 + Bun 1.4.0