Field import: opt-in dirty sub-window tile publishing (2026-08-25) — tests unrun - #136
Merged
Conversation
added 7 commits
August 25, 2026 09:10
The incremental coverage publish knows WHICH tiles changed (the publish-dirty set, ADR-0001 section 4) but not WHERE inside them, so it can only send whole 960x960 tiles. Add the cell-resolution half. GeoGrid::insert already evaluates, per cell, whether the CUBE node accepted the sounding; fold that per-cell result into a CellBox instead of collapsing it straight into the per-grid bool. GeoGrid is the only place that knows this: GeoMapSheet sees one bool per grid, and re-deriving a box from the sounding bounds would duplicate the influence-radius math the two deliberately share (#104). CellBox is a bounding box, not a per-cell mask: four uint16 per tile against a 960x960 bitset, O(1) to accumulate on the profiler-hot insert path (#63/#107), and the wire contract carries a rectangular window anyway. The cost is over-coverage on a diagonal or two-lobed change set, which degrades to the whole tile -- today's behaviour, never worse. GeoMapSheet::clearPublishDirtyGrids() resets the boxes of exactly the tiles it clears, so the set and the bounds can never drift. The save cadence (clearDirtyGrids) does not touch them; the seed/reload accessors, which carry an explicit no-dirty-mark contract, do not expand them. No behaviour change yet -- nothing reads the box.
SonarVisualizationTile has carried window_col/window_row/window_width/ window_height since the transport was designed (uma#230), with the contract that bands cover only the dirty sub-window and the consumer patches it in at the offset. The producer never implemented it: quantize_tile.cpp hardcoded the window to the whole tile, so every live coverage message is 960x960 x 3 bands = 3,686,400 bytes -- more than twice the operator link's entire 1,500,000 B/s budget, for one message. Add quantizeTileWindow(grid, stamp, window): same bands, dtypes and quantization, but width/height still carry the full tile size while the window fields and the band arrays cover exactly the given CellBox, row-major. quantizeTile() is now that function called with CellBox::wholeTile(), so the full-tile heal path and the patch path cannot drift apart and the default output stays byte-for-byte what it was. Two deliberate choices: - The backscatter auto-range is scoped to the WINDOW. That is what makes the whole-tile case byte-identical, and it gives a narrow patch better dynamic range -- but successive patches to one tile can then carry different scale/offset, so a consumer must dequantize on receipt (value = raw*scale + offset, uma ADR-0008 D1) rather than storing raw counts. Depth and uncertainty are fixed-scale and unaffected. - A window with no finite-depth cell returns nullopt rather than an all-nodata patch, which would blank cells the consumer holds. Windows are clamped to the tile so a malformed box can neither violate the wire's extent bound nor read past the record vector. Nothing calls quantizeTileWindow yet.
Wire the sub-window quantizer into publishDirtyTiles -- the incremental push, and the only site that changes. The catalog/TileRequest serve and the from-disk serve keep sending whole tiles: they exist to repair a consumer that has diverged, and a patch cannot repair divergence. The parameter DEFAULTS TO FALSE. The wire contract has documented sub-window patching since uma#230, but a documented contract is not a shipped consumer, and CAMP (camp#121) is not in this workspace, so nothing here can verify it. Publishing partial windows to a consumer that ignores the window fields would silently corrupt the operator's coverage display mid-survey. Opt-in only, with a WARN on enable. The in-tree second consumer, marine_web_view's coverage_renderer (uma#345), does implement the patch path correctly -- and shows the other half of the prerequisite: it records catalog possession only for whole-tile messages, on purpose, so partial patches cannot mask a hole. Against that consumer, enabling this makes every patched tile stay re-requestable and be re-served in full, costing MORE than leaving it off. The parameter description says so. Also count and log, throttled, the cells actually sent against the whole-tile equivalent, so the enable/disable call can be made from this survey's measured ratio rather than an assumed one -- the win depends on track geometry, and a diagonal line's bounding box degrades toward the full tile.
Section 4 described the incremental stream as per-tile, which the sub-window work falsifies: that is now only the default. Add the pointer there and the addendum itself, covering the measurement (3,686,400 B per tile against a 1,500,000 B/s link budget), the CellBox choice and why the bounds live in GeoGrid, the window-scoped backscatter auto-range and what it obliges of a consumer, why the heal paths stay whole-tile, the computed saving for four track geometries including the no-saving diagonal worst case, and why the parameter is opt-in. Records plainly that CAMP (camp#121) could not be verified from this workspace, and what the in-tree marine_web_view consumer (uma#345) shows about the possession prerequisite.
The suite name shadowed the type under test in the same namespace, which reads as a collision even though gtest tolerates it.
The descriptor promised "Read at configure" and publish_dirty_subwindow_ is
assigned once in on_configure and never re-read, but the descriptor never set
read_only. A runtime
ros2 param set <node> publish_dirty_subwindow true
therefore SUCCEEDED, read back true via `ros2 param get`, and changed nothing
on the wire -- accepted, reads back, inert.
That exact trap cost a wrong diagnosis and an operator-link outage (reported at
~3 minutes) on udp_bridge earlier today. It is worse here: under rmw_zenoh a
`ros2 param set` can additionally drop silently while reporting success, so the
operator's standard set -> get verification habit cannot catch either failure.
read_only = true makes the runtime set fail loudly instead, which is what the
descriptor already claimed. Launch-time and YAML overrides are unaffected --
read_only only rejects set_parameters after declaration, not the initial value.
The descriptor text now says read_only explicitly, so `ros2 param describe`
tells the operator the enforcement exists rather than just the intent.
Honest scope note: no other parameter in this node sets read_only. This closes
an inherited convention gap for the one parameter whose descriptor makes a
configure-time-only promise -- it is not a repo-wide sweep, and converting the
node's other parameters is deliberately out of scope here.
Part of cube_bathymetry#112
…iptor and WARN
catalog_builder_.update(index, version) in publishDirtyTiles() runs identically
for a whole tile and for a sub-window patch, so after a patch the catalog's
per-tile version means "when the newest patch was sent", not "at this version
you hold the whole tile". Meanwhile TileCatalogEntry.msg still documents that
field as a per-tile version, and SonarVisualizationTile.msg tells the consumer
that header.stamp is "this tile's version time".
A consumer that does the documented thing -- record header.stamp as its held
version -- therefore matches the catalog after applying a patch and never
re-requests. Lose one best-effort patch, receive the next, and the coverage
hole is permanent, invisible, and the anti-entropy protocol reports
convergence. SonarVisualizationTile.msg's own promise that "a lost or reordered
sub-window patch is healed by the catalog/TileRequest path" does not hold for
such a consumer, because it never asks.
The ADR-0001 addendum already states this as the enablement prerequisite ("a
consumer that ... accounts for patch possession"). The parameter descriptor and
the configure-time WARN did not: both warned only about the WASTEFUL mode -- a
consumer that gets possession right and re-requests each patched tile in full,
costing more bandwidth than leaving the parameter off. The descriptor is what
an operator reads via `ros2 param describe` before flipping the switch, so it
has to name the silent mode too. Both now do, alongside the wasteful mode: the
consumer requirement list gains an explicit (c) "does NOT advance its held tile
version from a patch", and each mode is tied to getting (c) right or wrong.
Wording only -- no behaviour change, no test change.
Deliberately NOT changed here: the catalog-bump behaviour itself. Not bumping
the catalog version on a patch, or adding a whole-tile refresh cadence, is the
right eventual fix. It is out of scope while publish_dirty_subwindow defaults
to false and no consumer enables it, and is recorded here so the deferral is on
the record rather than forgotten.
Part of cube_bathymetry#112
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The PR states the new/updated tests were compiled but not run, and test execution is needed to validate the behavioral and contract changes before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces an opt-in publishing mode for ~/coverage_tiles that sends only the dirty sub-window (patch) of each changed tile instead of always sending the full 960×960 tile, reducing per-message bandwidth while preserving the existing default behavior (full-tile stream) and keeping heal paths full-tile for divergence recovery.
Changes:
- Add per-tile dirty cell bounding boxes (
CellBox) tracked onGeoGrid::insert()and cleared alongside the publish-dirty grid set. - Add
quantizeTileWindow(...)to pack only the window’s cells intoSonarVisualizationTilebands; refactorquantizeTile(...)to delegate to the window path withCellBox::wholeTile(). - Add lifecycle parameter
publish_dirty_subwindow(configure-time, read-only) to gate sub-window publishing, plus tests and ADR documentation for the new semantics.
File summaries
| File | Description |
|---|---|
| cube_bathymetry/src/geo_grid.cpp | Tracks per-cell writes during insert() to expand the publish-dirty CellBox. |
| cube_bathymetry/include/cube_bathymetry/geo_grid.h | Introduces CellBox and exposes publishDirtyCells() / clearPublishDirtyCells(). |
| cube_bathymetry/src/geo_map_sheet.cpp | Clears each dirty grid’s CellBox in the same operation as clearing the publish-dirty set. |
| cube_bathymetry/include/cube_bathymetry/geo_map_sheet.h | Documents the strengthened semantics of clearPublishDirtyGrids() (set + cell boxes clear together). |
| cube_bathymetry/src/quantize_tile.cpp | Implements quantizeTileWindow(...) and makes quantizeTile(...) delegate to the whole-tile window case. |
| cube_bathymetry/include/cube_bathymetry/quantize_tile.h | Documents window-patch contract, clamping/nullopt behavior, and backscatter autorange scoping. |
| cube_bathymetry/src/cube_bathymetry_node.cpp | Adds publish_dirty_subwindow parameter (read-only at configure) and uses it to select full-tile vs patch publish for ~/coverage_tiles, plus throttled telemetry. |
| cube_bathymetry/test/test_geo_grid.cpp | Adds tests for dirty-cell box accumulation, reset, and non-dirty seed/reload paths; renames CellBox suite. |
| cube_bathymetry/test/test_geo_map_sheet.cpp | Adds tests ensuring publish-dirty set and per-grid cell boxes clear together and don’t interfere with save dirty state. |
| cube_bathymetry/test/test_quantize_tile.cpp | Adds tests for window packing order/geometry, whole-tile equality, clamping/nullopt cases, and band-size invariants. |
| cube_bathymetry/docs/decisions/0001-tile-eviction-and-incremental-publish.md | Adds an ADR addendum documenting the motivation, decision, constraints, and verification points for sub-window publishing. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+62
to
+65
| std::size_t dtypeSize(uint8_t dtype) | ||
| { | ||
| return dtype == 3 ? sizeof(int16_t) : sizeof(uint8_t); | ||
| } |
added 13 commits
August 25, 2026 21:35
…ttice The three GeoGridDirtyCellsTest cases put their soundings at the cell CENTRE (row + 0.5, col + 0.5). Nodes sit on the cells' SW-CORNER lattice and GeoGrid::insert gates each node on hypot(dlat, dlon) < influenceRadius, which for these test errors (vertical 0.5 m, horizontal 0.1 m) is under half a cell. A centre touchdown is therefore ~0.71 cells from every surrounding node, writes NOTHING, and insert() returns false -- so all three ASSERT_TRUE(insert(...)) failed on their first run. Offset by a quarter cell instead, which is what the neighbouring GeoGridPredictedSurfaceTest cases already do. Probed to confirm the boundary rather than assumed: at this grid and these errors, offsets 0.0, 0.1, 0.25 and 0.4 accept 20 of 20 soundings and 0.5 accepts 0 of 20. The fixture comment now records that, so the next author does not rediscover it. RejectedSoundingLeavesTheBoxEmpty is moved too. It passed before, but for two reasons at once -- the door gate rejects its zero vertical error AND the centre position writes nothing -- so it was not testing the gate it names. Fixture-only: no production code changes and no assertion is weakened. Part of cube_bathymetry#112
BackscatterAutoRangeIsScopedToTheWindow placed its two soundings at hand-typed positions 0.0010 deg apart. A 1 m GGGS tile spans only ~0.0078 deg, and the tile containing 43.07, -70.76 runs from 43.0625 to 43.0703 -- so the "high" touchdown at 43.0710 fell in the NEXT tile north and never entered the grid. The tile's auto-range then saw one intensity, degenerated to the unit span, and matched the patch's scale exactly, which is what the test failed on. Place both touchdowns through lattice helpers (cells 200 and 600, a quarter cell off the node, 400 cells apart) so the separation is expressed in the units the property is about. THE SAME DEFECT WAS ALREADY IN THE PRE-EXISTING FULL-TILE TEST. BackscatterBandAutoRangesOverInsertedIntensities uses those same two positions, so its "well-separated spots with distinct backscatter" have only ever been ONE spot, and it passed anyway: offset was the low value, scale > 0 held for the degenerate 1.0/254 span, and one touchdown spreads over enough cells to satisfy the >= 2 finite-cell count. It is moved onto the lattice too and gains the assertion that would have caught this -- the scale spans the real 30 dB interval, not the unit fallback. Fixture-only: no production code changes. Part of cube_bathymetry#112
…the wire The case for sub-window publishing rested on a cell count. Cells are not what the operator link carries: udp_bridge zlib-compresses every packet before fragmenting it (packet.cpp:25), and a coverage tile is mostly nodata, which compresses to almost nothing. Nobody had measured the compressed figure, so the size of the win -- and the size of the worst-case message -- were both assumptions. Adds an offline measurement to the importer, which already runs the exact production chain (rosbag2 -> tf2 -> DetectionsProjector -> GeoMapSheet). Every --tile-report-interval seconds of BAG time, each dirty tile is quantized BOTH ways from the SAME dirty set -- whole tile and dirty sub-window -- serialized, and compressed with zlib at the default level, the same call udp_bridge makes. One CSV row per tile per cycle, then the dirty set is cleared exactly as cube_bathymetry_node::publishDirtyTiles clears it. The default interval is 5.0 s because that is the live node's own coverage cadence: publishBounded runs when a ping's stamp is more than 5 s past the last publish (cube_bathymetry_node.cpp). Reporting on any other interval would measure a dirty region the boat never sends as one message. The bridge's own `period: 1.0` throttle sits downstream of this and can only drop messages, not add them. Off unless --tile-size-report names a file; no existing invocation changes. ZLIB is a new link dependency of the import_bag target only. Run against both of today's Appledore bags (2026-08-25, the deployment where coverage tiles collapsed the operator link), the report reproduces the boat's own measured source rate -- 997 kB/s uncompressed against the 962,899 B/s recorded live on gabby at 08:17 -- which is the evidence that it is measuring the real thing. Part of cube_bathymetry#112
The sub-window stream trades a large periodic message for a small incremental one, and in doing so it makes message loss UNDISCOVERABLE by the consumer. publishDirtyTiles bumps a tile's catalog version on a patch exactly as on a whole tile, so a consumer that records header.stamp as its held version -- which SonarVisualizationTile explicitly instructs it to do -- matches the catalog and never re-requests. One dropped best-effort patch is then a permanent, invisible hole in the operator's coverage display while anti-entropy reports convergence. That is not hypothetical for the consumer we actually have. CAMP's SonarLiveTile::applyPatch does implement the patch path correctly -- bounds-checked window, per-band dequantize, GGGS row flip (camp#121) -- and then advances its held version from ANY message including a patch (sonar_live_tile.cpp:145), which sonar_live_cache_layer.cpp marks as possession. It is exactly the consumer the silent mode was described for. Heals it at the PRODUCER, so correctness does not depend on the consumer noticing anything: every tile that has received a patch is re-sent whole within subwindow_refresh_interval (default 60 s). A lost patch is then a gap of bounded duration instead of a permanent one. Two parts, because a timer alone is not enough: - A tile still being surveyed heals on its own dirty cycle: when its last whole send has aged past the interval it goes out whole rather than as a patch, which costs one message instead of two. - A tile the vessel has MOVED OFF is not in the dirty set at all, so publishDirtyTiles would never revisit it and its lost patch would outlive the survey. drainRefreshQueue re-sends those, oldest debt first, capped at subwindow_refresh_tiles_per_cycle (default 2) so the heal can never become the burst it exists to prevent. Cost measured, not assumed -- both 2026-08-25 Appledore bags replayed through this chain with import_bag --tile-size-report. Against the whole-tile stream's 56.0 kB/s (transit) and 85.7 kB/s (on station), a 60 s refresh costs 11.2 and 9.1 kB/s: 80% and 89% less. 300 s costs 7.9 and 3.5 kB/s. The interval is a parameter so the trade can be made in the field without a rebuild. Policy lives in cube::CoverageRefreshTracker (coverage_refresh.h), node-free on purpose. The node's publish path has no test harness -- a limitation test_anti_entropy_disk_serve.cpp already records -- so putting the decisions in a pure class is what lets them be tested directly rather than guarded at arm's length through the library primitives. 13 tests cover the interval boundary, that continuous patching cannot postpone its own heal, that a whole send discharges the debt, the quiet-tile drain, the per-cycle bound and oldest-first ordering, and both disable paths. One case is handled but NOT healed, and is surfaced rather than hidden: a tile evicted from RAM while owing a refresh cannot be quantized, so its debt is unpayable and it is dropped from the set (an unbounded set of unpayable debts would be a slow leak) with a throttled WARN naming the count. Rare by construction -- a just-patched tile is the warmest thing in the sheet and eviction takes the coldest -- and the durable fix is to serve the refresh from the draft store the way a TileRequest already is (#106). The tracker is cleared on_cleanup: carried across a reconfigure it would claim tiles had been sent whole that the new sheet has never sent at all, suppressing the heal for exactly the tiles a restart is most likely to have left a consumer stale on. publish_dirty_subwindow stays false here. This commit makes it SAFE to turn on; turning it on is its own decision. Part of cube_bathymetry#112
Adding --tile-size-report inline pushed main() to 504 non-comment lines, four past the cpplint readability/fn_size limit, and put a C system header after the C++ block. main() was clean before the flag, so this is the flag's regression to pay for -- not a limit to raise. The reporter becomes a TileSizeReporter class in the anonymous namespace with the interval bookkeeping, the serialize-and-compress measurement and the CSV writing as its own members, plus a small factory so the open-failure path is not four more lines of main. Behaviour is unchanged: same cadence, same columns, same clearPublishDirtyGrids at the end of a cycle. Part of cube_bathymetry#112
…eals
publish_dirty_subwindow shipped false because a dropped patch was a
permanent, invisible coverage hole against a consumer that advances its
held version from a patch -- which is what CAMP does. The whole-tile
refresh queue removed that failure mode: a lost patch is now a gap of at
most subwindow_refresh_interval (60 s), and healing it does not depend on
the consumer noticing anything. What is left is a bandwidth decision, and
the numbers are measured rather than assumed.
Both 2026-08-25 Appledore bags, replayed through this exact chain with
import_bag --tile-size-report:
transit on station
whole tiles 56.0 kB/s 85.7 kB/s
patches + 60 s heal 11.2 kB/s 9.1 kB/s
saving 80% 89%
against a 1,500,000 B/s VPN connection shared with telemetry, costmap,
video and TF. The replay reproduces the boat's own live measurement --
997 kB/s uncompressed against the 962,899 B/s recorded on gabby at 08:17
-- which is the evidence it is measuring the real stream.
The peak matters as much as the mean. The largest single whole-tile
message was 1,120,845 B compressed; the largest sub-window was 172,040 B.
Connection::send admits a message only when the WHOLE thing fits the
instantaneous budget, so the biggest messages are exactly the ones a
congested link stops carrying: on 2026-08-05 coverage_tiles measured 100%
dropped while /tf and mavros telemetry were still getting through at
~55%. Sub-windowing turns the coverage stream from the topic most likely
to be shed into one of the least.
It also corrects a stale assumption in the field config, which sizes a
tile at 1,843,200 B (INT16 depth only; the message carries three bands,
so 3,686,400 B) and at "~183 KB compressed". Measured today the median is
145-345 KB and the peak 1.12 MB -- which is why raising the connection
cap to 1.5 MB/s never rescued this topic.
A deployment that needs the old behaviour sets publish_dirty_subwindow
false and gets the previous stream byte for byte; setting
subwindow_refresh_interval higher trades healing latency for bandwidth
(300 s measured at 7.9 and 3.5 kB/s).
Part of cube_bathymetry#112
Review found that the sub-window default flip was unsafe for the OTHER consumer, and in a way that made the link worse rather than merely imperfect. marine_web_view's coverage_renderer takes possession only from a whole tile -- deliberately, so a partial patch cannot mask a hole -- and its reconciler re-requests any tile whose catalog version exceeds what it holds. publishDirtyTiles bumped the catalog on every patch. So every patched tile was re-requested IN FULL on every 5 s catalog round, served immediately and unthrottled by tileRequestCallback: on the order of 110 kB/s of request-driven whole tiles on top of the patch stream, more than the 56-86 kB/s whole-tile stream sub-windowing exists to replace. The renderer's own comment says its rule "costs nothing today" precisely BECAUSE the producer serves whole tiles; the flip is what invalidates that. CAMP fails the opposite way, which is how the bump survived review the first time: it advances possession from a patch, so its held version matched the bumped catalog and it never re-requested -- a dropped patch became a permanent invisible hole. The refresh queue was written for that consumer, and reasoning from it alone is what hid the web view's case. Bumping only on a whole send fixes both. The web view's held version now equals the catalog, so it stops asking. CAMP's runs ahead of it, so it does not ask either. And if EITHER misses a whole-tile refresh its held version falls behind the catalog and the documented catalog/TileRequest heal fires -- which also makes SonarVisualizationTile.msg's promise true again, by the mechanism the message actually names, rather than only by the producer-side timer. The catalog version consequently recovers its documented meaning: "at this version you hold the WHOLE tile", not "this is when the newest patch went out". That was the TileCatalogEntry.msg divergence recorded as a deferral while the parameter defaulted to false -- a precondition the default flip had removed without revisiting. Part of cube_bathymetry#112
The heal did not run in the case it exists for. drainRefreshQueue was reachable from exactly one place: publishDirtyTiles <- publishBounded <- pingCallback, gated on a ping arriving, on 5 s of PING-STAMP time having passed, and on a TF fix -- publishBounded early-returns on a transform miss before ever reaching the drain. Every one of those gates fails in precisely the situation a lost patch needs healing: the vessel has finished a line, the sonar is off, it is in transit, the node was deactivated, or TF has gapped. So the tile whose last patch was dropped is the tile that will never be visited again, and the operator keeps a permanent invisible hole in FINISHED coverage -- which is the failure mode the refresh queue was written to eliminate, surviving at the end of every survey line. The TF half was already known here and half-fixed: the file's own comment records that eviction was deliberately moved OUT of publishBounded so a publish-time TF miss could not stall RAM bounding. The drain was then put back inside it. Now on refresh_timer_, a wall timer beside save_timer_ / catalog_timer_ / disk_serve_timer_, created on_activate and cancelled on_deactivate. The tick is 5 s, matching the coverage publish cadence, so subwindow_refresh_tiles_per_cycle keeps meaning "per publish cycle" on either path. quantizeTile needs no transform, so a TF gap cannot stall this either. The in-cycle refresh stays where it was: a tile that is STILL dirty and whose debt is due goes out whole on its own publish cycle, which costs one message instead of two. Only the quiet-tile drain moved. Part of cube_bathymetry#112
… them inert
subwindow_refresh_interval and subwindow_refresh_tiles_per_cycle were
read once into refresh_tracker_.configure() at configure and never looked
at again, with no read_only on either descriptor and no
OnSetParameters callback anywhere in the node. A field
ros2 param set <node> subwindow_refresh_interval 300
therefore SUCCEEDED, read back 300 via param get, and changed nothing:
accepted, reads back, inert. All three review passes flagged it
independently.
That is the identical defect commit 1665651 closed on this node's
publish_dirty_subwindow one commit earlier in this same branch, citing
that it "cost a wrong diagnosis and an operator-link outage" on
udp_bridge. Reintroducing it here is worse than inheriting it, because
the descriptor ACTIVELY INVITES the operator to retune: it tabulates the
measured cost of 300/60/30 s so a live decision can be made on a
struggling link. The one person most likely to try it is the one it would
have silently failed.
read_only would have been the wrong fix for the same reason -- it
contradicts the advice the descriptor gives. These parameters are made
genuinely live instead: applyRefreshParameters validates the WHOLE
proposed batch before applying any of it, then reconfigures the live
tracker. read_only stays on publish_dirty_subwindow, which IS
configure-time and where the promise is true.
Registration goes at the END of on_configure. rclcpp fires an
OnSetParameters callback for declare_parameter() as well as
set_parameter(), so a callback registered earlier would see every YAML
override on its way in, and a rejection there throws out of on_configure
where rclcpp_lifecycle swallows it -- leaving a half-configured node that
looks to the operator like it transitioned. on_cleanup resets the handle
BEFORE clearing the tracker, so a set arriving mid-teardown cannot
reconfigure state that is about to be reset.
The budget also gains the upper bound it never had: refused above 8. At
~183 kB compressed per whole tile and a 5 s tick that is already
~293 kB/s of a 1.5 MB/s operator link, and a field YAML typo of 20 would
put 20 whole tiles on the wire in one tick -- exactly the burst the
budget exists to prevent, on the link this whole branch exists to
protect.
Part of cube_bathymetry#112
refreshDue returned false for a tile with no recorded whole send unless it was already in the patched set, so a brand-new tile's FIRST coverage message was a sub-window patch. A consumer cannot apply a patch to a tile it has never received: it lands as a patch over nothing. Two ways that bites, both reachable: - A tile first touched on the last ping before pings stop (end of a line, sonar off, transit) is left at the consumer as a fragment of a tile it never had, with no later cycle coming to correct it. - Now that the catalog version is bumped only on a whole send, such a tile carries NO catalog version at all -- so the consumer cannot even discover it is missing something. Before that change the patch's bump at least advertised the tile's existence. A tile with no recorded whole send is now due on sight, gated on neither the interval nor an outstanding patch, because this is a possession requirement rather than the periodic heal. Two existing tests asserted the old behaviour and are updated, not weakened -- both were asserting the absence of the guarantee this commit adds: - ZeroIntervalDisablesTheRefresh now sends the tile whole once before patching it, so it tests what it names (the periodic heal being off) rather than the never-sent-whole case. - ClearForgetsEverything now expects a cleared tile to be due IMMEDIATELY. That is the correct post-clear behaviour and worth stating plainly: after a reconfigure the tracker describes a sheet that has sent nothing whole, and treating a cleared tile as satisfied would suppress the heal for exactly the tiles a restart is most likely to have left a consumer stale on. Part of cube_bathymetry#112
"Re-sent WHOLE within subwindow_refresh_interval" was stated flatly in the descriptor, in the tracker's header and in a commit message, and it is not what the code guarantees. The drain clears at most subwindow_refresh_tiles_per_cycle tiles per 5 s tick -- 24 per 60 s interval at the defaults -- so a line-end turn, or a smaller cell_size that puts more tiles under one swath, can quiet more tiles at once than the budget clears. Oldest-debt-first ordering means no tile is starved and the backlog does drain, but the interval is a TARGET UNDER LOAD, not a bound, and the only signal that it is not being met was an RCLCPP_DEBUG line that is off in the field. Two changes, both about not letting an operator infer a guarantee that is not in force: - CoverageRefreshTracker::backlogExceedsBudget(cycles_per_interval) asks whether the outstanding debt is larger than the drain can clear in one interval. The tracker deliberately does not know the tick period; the caller does, so it passes the ratio in. - A throttled WARN naming the owed count, what the budget actually clears, and the two ways out (raise the budget and pay link bandwidth, or accept the longer heal). The descriptor now says the same thing. Also records what the WARN cannot: the drain's own saturation ceiling is 2 tiles x ~183 kB / 5 s = ~73 kB/s, which EXCEEDS the 56.0 kB/s transit whole-tile stream this mode replaces. Sustained saturation is therefore not merely slower healing -- it is worse than not sub-windowing at all. That is what the WARN is for. Two tests pin the signal, including that it stays quiet when the heal is disabled by either parameter: a permanently-firing warning about a latency nobody asked for would be noise, and noise is how a real warning gets ignored. Part of cube_bathymetry#112
Two documentation defects, both of the kind that mislead precisely because the document is otherwise trustworthy. ADR-0001's addendum 1 now contradicts the shipped code. It states the parameter is "off by default" and that "false is the correct value", one commit before the default was flipped to true; it never mentions the refresh queue or the two parameters that came with it; and it still leads with "~958 kB/s, 64% of the budget", an UNCOMPRESSED cell-count figure that this branch's own measurement commit establishes is not what the link carries. Measured: 56.0 and 85.7 kB/s, 4-6%. Corrected in ADR form rather than by editing the record. The falsified measurement gets an inline correction (it is a factual error about the link, not a decision); the two superseded conclusions get pointers; and a new addendum 2 carries what actually changed. Addendum 2 is worth reading for one thing in particular: addendum 1 named the answer without noticing it did not have to be the consumer's. It said enabling required "a consumer that ... accounts for patch possession (a per-tile hole map, or an explicit whole-tile refresh cadence)", and then waited for one. The refresh cadence moved to the PRODUCER works for every consumer at once and waits for none. It also records why a consumer gate could not have worked at all: the two in-tree consumers fail in OPPOSITE directions -- CAMP takes possession from a patch and so never re-requests, the web view refuses to and so re-requests everything -- so no single consumer-side rule satisfies both, and enabling on the strength of one ships a defect to the other. The addendum also records what is deliberately NOT claimed: the heal latency is a target under load rather than a bound, an evicted tile's debt is unpayable, and fixed-size chunking -- which is what #112 actually asks for -- is still not done, so #112 stays open on its own terms. Second defect: the README had no parameter section at all, so all three parameters, their shipped defaults, the heal semantics and the consumer contract existed only in source and in an ADR that was wrong about two of them. Adds one, with the measured costs an operator would actually use to choose an interval, and states plainly that a consumer need NOT track patch possession -- that is the producer's job now, which is the whole point of the change. Related pre-existing gap, NOT introduced here and not fixed here: this repo has no `.agents/README.md`, so there is no verified-parameter table for a parameter change to update. Worth its own issue. Part of cube_bathymetry#112
added 3 commits
August 26, 2026 08:34
Both the tracker's class comment and its test file's header still state the premise the tracker was written under -- "the producer bumps the tile's catalog version on a patch exactly as on a whole tile" -- one commit after 673c23e stopped doing that. Left standing, the two documents that explain WHY this heal exists argue from a mechanism the code no longer has, which is the kind of stale rationale that gets a correct guard removed later by someone who checks the claim and finds it false. The reason the heal is still necessary is narrower and worth stating exactly: bumping only on a whole send fixes what anti-entropy CAN see -- marine_web_view stops re-requesting every patched tile in full -- but CAMP takes possession from a patch, so its held version sits at or ahead of the catalog whether or not the patch it lost ever arrived. A consumer already level with the catalog cannot discover a message it never received, and that is the gap the producer-side refresh covers. No behaviour change. Part of cube_bathymetry#112
tileRequestCallback quantizes the resident tile and publishes it WHOLE, on the same topic every consumer is listening to -- it is exactly the message the refresh drain would have sent, and it closes exactly the gap the drain exists to close. The tracker was never told, so the tile stayed in the owed set and the drain re-sent the same ~183 kB within the interval: a second whole tile, on the operator link this entire mode exists to protect, repairing a hole the request had already repaired. Worse than a wasted message in the case that produces it. A consumer requests tiles when it has diverged -- a restart, a reconcile after a link outage -- so the requests arrive in bursts, and every tile in the burst then owed a duplicate whole send inside one interval, against a link that had just been down. The tracker contract this leans on is the same one the drain uses: notePublished(whole) means "a whole tile reached the wire", not "the drain sent one". Pinned by AWholeSendFromAnyPathDischargesTheDebt, at tracker level -- the node's private methods still have no harness (see test_anti_entropy_disk_serve.cpp's known limitation), so what is testable is the invariant the call site depends on, not the call site. The catalog version is deliberately NOT bumped here. A request serve is addressed to a consumer that already knows it is behind; bumping would make every OTHER consumer's held version fall behind the catalog and re-request a tile nothing was wrong with. Part of cube_bathymetry#112
…say so The owed set had no bound that did not depend on the heal being switched on. Its only reaping was lazy, inside dueForRefresh's residency check -- and dueForRefresh early-returns whole on interval 0 or budget 0, both documented values, both of which an operator is invited to set to turn the heal off over a tight link. With the heal off, every patched tile entered the set and nothing ever removed one: unbounded growth for the life of the sheet, in the node whose entire eviction design exists to bound RAM for long-duration operation. Eviction is the right place to reap, and not only because it bounds the set deterministically instead of lazily. A tile that leaves RAM cannot be quantized, so a whole-tile refresh it still owed is unpayable from that moment -- and eviction is the last moment anything in the node knows the tile existed, which makes it the only moment the operator can be told. The consumer keeps whatever gap it has until the tile is revisited, and that is now stated at WARN rather than inferred from a set that quietly stopped growing. The drain's own dropped-tile path stays as the backstop for a tile that leaves RAM by any route this one does not cover. CoverageRefreshTracker::owesRefresh answers for one tile, which is what this site needs: it is dropping a specific tile and can only report the debt it is making unpayable. owedCount describes the whole set and cannot say anything about the tile in hand. Part of cube_bathymetry#112
added 5 commits
August 26, 2026 08:35
drainRefreshQueue asks the tracker what is due using now() -- the NODE clock, which under use_sim_time is the sim clock -- while the timer that called it ran on the wall clock. The drain's cadence and its own due-test were on two different clocks, and nothing reconciled them. That is incoherent on precisely the route this feature has to be validated on before it can be trusted over a survey link: replay a bag. Played slower than real time, the sim clock lags the wall clock, no tile ever reaches subwindow_refresh_interval, and the heal silently never fires -- the validation run then shows a saving the boat will not reproduce, because on the boat the heal it left out does fire. Paused, or played from a bag whose clock does not advance at all, the timer keeps ticking over a due-test that can never come true. On the boat use_sim_time is false, the node clock IS the system clock, and this is the same 5 s timer it has always been. The other lifecycle timers here (save, catalog, disk-serve) stay on wall timers deliberately: none of them compares a stamp against now() to decide whether to act, so none of them can disagree with itself. Part of cube_bathymetry#112
The one line an operator uses to decide whether to leave sub-window mode on over a constrained link reported the dirty cycle's patch cells against the whole tiles those cycles would have sent -- and omitted the refresh drain's whole-tile re-sends entirely. At a 60 s interval those re-sends are the DOMINANT cost of this mode, and they are a cost the whole-tile mode does not have at all: it has no heal to pay for. A ratio that leaves out the larger half of the bill is not a saving, and it overstated it in the direction that keeps the mode switched on. Three changes, all so the number means what it says: - The heal's re-sends are counted onto the same ledger as the patches. Sent = patched + re-sent whole; the comparison is against what the whole-tile mode would have put on the wire over the same period. - Accumulated over a fixed 30 s window rather than sampled per cycle. The patches and the heal arrive on different cadences, so no single cycle is representative of both, and a throttled per-cycle log reports whichever cycle happened to survive the throttle. - Reported from the drain as well as the publish path, because once the pings stop the drain is the only path still running and its re-sends are exactly what the report exists to expose. A report that goes quiet when the vessel finishes a line hides the mode's steady-state cost. The byte figures are still uncompressed cell bytes and now say so, along with what to run for real ones: udp_bridge zlib-compresses every packet and coverage tiles compress hard, so the link carries substantially less than the printed bytes. The RATIO is the transferable number; import_bag --tile-size-report measures the wire. The ledger resets on configure with the tracker, for the same reason: carried across a reconfigure it would state a ratio computed over two different configurations. Part of cube_bathymetry#112
…ails loud The CSV is the evidence base for a fleet-wide default, and it measured a mode the node does not run. It quantized each dirty tile both ways -- whole and sub-window -- and reported the two extremes, but the node sends neither exclusively: a tile whose last whole send has aged past subwindow_refresh_interval goes out WHOLE even though it is dirty, and a tile that has gone quiet still owing a whole send is re-sent whole by the refresh drain. Both are real traffic. At a 60 s interval the second is the dominant cost. A report that omits them answers "what would pure sub-window mode cost?", which is not a question anyone is deciding. Four new columns -- source, sent, sent_serialized, sent_compressed -- carry what the boat would actually put on the wire, produced by the SAME CoverageRefreshTracker the node runs, driven by BAG time so the heal fires on the survey's duration rather than the import's. The full_* and window_* columns stay: the two extremes are still the right bracket to read the shipped number against. Two options expose the policy (--tile-refresh-interval, --tile-refresh-budget, defaulting to the node's own parameter defaults) so an interval can be chosen from measurement rather than from a cost table. Measured over 6000 pings (602 s) of bag_2026-06-09T14.51.50_m3_detections, 2 tiles, zlib-compressed as udp_bridge sends it: whole-tile mode 2,957,492 B 4,913 B/s pure sub-window 358,714 B 596 B/s 12.1% as shipped 600,126 B 997 B/s 20.3% The heal is 40% of the traffic of the mode that ships. Reporting the 12.1% would have overstated the saving by 1.7x to whoever decides whether to leave it on over a survey link. (Two tiles over ten minutes is a sample, not a fleet number -- what it establishes is that the omitted half is the same order as the reported half, not what the ratio is.) Second defect, same theme: the stream was checked once at construction and never again. No check after a row, none at close -- so a full disk truncated the file silently, and a short CSV still parses, still plots, and still looks like a complete survey that happened to be quieter. Every write is now checked, the failure is named once, and a run asked for with --tile-size-report exits non-zero if its report is incomplete. Tiles that left RAM still owing a refresh are counted and stated too: their re-sends are absent, so the modelled traffic is a lower bound by that many whole tiles. Part of cube_bathymetry#112
main() was one line under cpplint's 500-non-comment-line ceiling, so the coverage-report options tipped it over -- a real failure in colcon test that the previous commit shipped, since the pre-commit hooks here do not run cpplint. Two extractions, both of blocks that were already self-contained: - reportPersisted(): what the pass wrote and the two empty-output cases worth a warning. It reads better out of line anyway -- it is reporting, not importing, and it is the paragraph a reader skips to when a build looks slow or a store comes out empty. - The two modelled-policy options validate inside makeTileReporter rather than at their parse sites. They are one concern with one rule (0 is legal for both and means "model the heal switched off"), and it was stated twice, six lines apart, in the middle of the argument loop. No behaviour change. Verified by re-running the measurement from the previous commit over the same 6000 pings: byte-identical CSV rows but for 20 of 233 compressed sizes differing by 1-2 bytes, and the aggregate ratio unchanged at 20.29%. That jitter is not from this refactor -- it reproduces across runs of the same binary, and the SERIALIZED sizes are identical while only the compressed ones move, which is CDR alignment padding that the serializer does not zero. Worth knowing before anyone diffs two reports and reads a 1-byte delta as a finding. Part of cube_bathymetry#112
rolker
marked this pull request as ready for review
August 26, 2026 13:00
added 4 commits
August 26, 2026 09:09
…ved key Every PR in this repo has been failing in setup_upstream_workspace, before cube_bathymetry is built at all: marine_web_view: Cannot locate rosdep definition for [marine_ais_msgs] marine_web_view was added to the unh_marine_autonomy monorepo after this prune list was last touched. industrial_ci resolves and builds the WHOLE monorepo, so a package cube does not ship, and does not depend on, now decides whether cube's CI runs. Its marine_ais_msgs dep comes from rolker/marine_ais and has no rosdep entry. This is the same case the block already documents for mission_manager* and the integration tests, and it takes the same two-part treatment: COLCON_IGNORE so it is never built, plus the skip key, because rosdep does not honour COLCON_IGNORE and still reads the package.xml. Cloning marine_ais into upstream.repos would also make CI green, and is the wrong fix: it would have cube CI compile a package it never ships, which is precisely what the comment above this line says not to do. The comment now names marine_web_view too, so the next package to arrive is matched against a rule rather than against a list someone has to guess the shape of. Not a workaround waiting on upstream: marine_web_view builds fine in its own repo's CI, which clones marine_ais deliberately. It is simply not cube's package. This is also a prerequisite for merging by EITHER verification route under ADR-0018 -- ci_local.sh builds upstream.repos as an underlay too, so a local attestation run hits the identical failure. Part of #112
…g one byte dtypeSize() returned sizeof(int16_t) for INT16 and sizeof(uint8_t) for literally everything else, so an unexpected dtype was silently sized at one byte. The band-coverage assertion then failed with "band '<name>' does not cover the window" -- accusing the message of breaking the wire contract when the helper is what is wrong. A test that misidentifies which side of a contract broke costs more than the bug it catches. Not a hypothetical dtype either, which is what makes this worth fixing rather than noting: VisualizationBand.msg already DECLARES UINT16 = 4, "reserved for sidescan source rasters". A band using a dtype the contract names by number would have been measured at half its width. Every declared dtype is now handled by name and anything else is an explicit ADD_FAILURE naming the value, so the next dtype added upstream announces itself here instead of arriving as a confusing size mismatch. Raised by Copilot on PR #136. Its argument was future-proofing; the UINT16 constant makes it current. Part of #112
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
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.
Bound the live coverage-tile message to the dirty sub-window instead of
re-sending the whole 3.7 MB tile, and heal a lost patch from the producer.
Started as a field import from the 2026-08-25 BizzyBoat Appledore deployment
(rolker/unh_echoboats_project11#459),
pushed from
gitcloud/feature/tile-subwindow-publishwith SHAs preserved. Ithas since gone through a Deep local review and two rounds of fixes, and the
default is now ON.
The problem
A single
SonarVisualizationTileon the wire is 3.7 MB — 960×960 cells ×three bands, verified both at source (
quantize_tile.cpphardcoded the wholetile) and by echoing a live message on station. Against a 1.5 MB/s operator
link that is more than twice the entire connection budget for one message.
The bridge has no lever for message size, only rate, and the source already
publishes at 0.26 tiles/s — so
period: 1.0was never throttling anything.The problem is size.
What ships
tile patch (
a0199a8,0d42b62).CoverageRefreshTracker): any tileholding an unconfirmed patch is re-sent whole within
subwindow_refresh_interval, budget permitting, on a timer of its own.This is what lets the default be ON. A consumer-side gate could not have
worked: the two in-tree consumers fail in OPPOSITE directions — CAMP takes
possession from a patch and so never re-requests, marine_web_view refuses to
and so re-requests everything — and the producer's cadence satisfies both at
once.
"at this version you hold the WHOLE tile".
costs:
publish_dirty_subwindow,subwindow_refresh_interval,subwindow_refresh_tiles_per_cycle.claimed.
The catalog/TileRequest heal path and the from-disk serve path stay whole-tile
on purpose: they exist to repair divergence, and a patch cannot repair
divergence.
Measured, not asserted
import_bag --tile-size-reportnow MODELS the shipped policy — sameCoverageRefreshTrackerthe node runs, driven by bag time — so the CSVdescribes what the boat would actually send rather than a mode nothing runs.
Over 6000 pings (602 s) of
bag_2026-06-09T14.51.50_m3_detections, 2 tiles,zlib-compressed as udp_bridge sends it:
The heal is 40% of the shipped mode's traffic. Reporting the 12.1% — which is
what both the old CSV and the old in-node INFO line did — overstated the
saving by 1.7× to whoever decides whether to leave the mode on over a survey
link. (Two tiles over ten minutes is a sample, not a fleet number: what it
establishes is that the omitted half is the same order as the reported half.)
Review
A Deep
/review-codepass on the water raised 7 must-fixes and 10suggestions. All 17 are now dispositioned — see
.agent/work-plans/issue-112/progress.mdfor the full timeline. The onesworth naming:
a patch made marine_web_view re-request every patched tile in full, more
traffic than the whole-tile stream this replaces. Fixed by bumping only on a
whole send (
673c23e).reachable only from the ping path, inside a TF-miss early return. Now on its
own timer (
1bc78cc), and on the node clock (34c60d1) so a bag replayunder
use_sim_timedoes not silently skip it and report a saving the boatwill not reproduce.
never received (
7bb1fcb).b589811).post-outage burst of requests owed a duplicate whole send inside one interval
(
981249b).growth on the node whose eviction design exists to bound RAM (
9daa9e2).Build and test
colcon testEXECUTED, not merely compiled: 602 tests, 0 errors, 0failures, 70 skipped (all 70 cppcheck's own file skips, pre-existing).
cpplint and uncrustify clean.
Hosted CI is currently red for an unrelated upstream reason:
rosdep installfails withmarine_web_view: Cannot locate rosdep definition for [marine_ais_msgs], before this package is built at all.Not fixed here
side.
bizzyboat.yamlhas the three coverage topics commented out of theVPN
topics_listwith the restore condition written in — "RESTORE oncetiles are patched or the link budget grows". This branch satisfies it, so
the operator will not see the fix until that config lands. Field-mode
/gitcloud repo, and the gabby field agent owns the change; it is not a
blocker on this PR. Its sizing comments are stale too (1,843,200 B and
"~183 KB compressed" against a measured 3,686,400 B payload).
SonarVisualizationTile.msg/TileCatalogEntry.msgneed comment-onlycontract corrections in
unh_marine_autonomy. No fields change, notype-hash change.
title actually asks for — is still not done. This bounds the message to the
dirty window, a large practical reduction but not a bound: a diagonal
track's bounding box degrades toward the full tile.
Part of #112. Part of
rolker/unh_echoboats_project11#459.
Test plan
colcon teston the bench: 602 tests, 0 failures (done, above).import_bag --tile-size-reportover a survey bag to choose the intervalfrom measurement (done for one 10-minute segment; a full-survey run is the
number to quote).
Sub-window coverage push:INFO linefor the shipped ratio, and confirm CAMP's coverage display fills without
holes across a line-end turn.
Authored-By:
Claude Code AgentModel:
Claude Opus 5 (1M context)