Repository navigation
fix(coverage): Site Planner estimate placement, floor, offline terrain, threads and credit - #7758
Conversation
Band rectangles now extend half a cell beyond their outermost sample centers, and iteration and merging include every row and column. A regression test checks that a uniform 2×2 grid merges into one closed rectangle with the expected half-cell bounds. Fixes #7752
`lonAt` and `latAt` divide by `width - 1` and `height - 1`, and the bands now reach every row and column, so a one-wide grid would emit non-finite coordinates.
The style used fixed -130..-80 dBm that the form never changed, so LongFast lost its -139..-130 edge and a -120 dBm receiver was drawn hearing where it cannot. The dead `minDbm`/`maxDbm` params go with it. fixes #7756
The sweep queued every radial on `Dispatchers.Default` with no limit, and the mesh service shares it, so incoming radio data waited behind the estimate. Radials and tile decodes now run on a view of Default one worker short. fixes #7754
The estimate read only its own cache and the network, so a site inside a downloaded region failed offline. Region terrain stores are now read after the cache and before the network, and a region downloaded below the estimate zoom stands in with its coarser parent tile. Stand-ins are dropped from the shared cache when the estimate ends, so a later online run downloads the real tile. fixes #7753
The overlay is built from Mapterhorn terrain, but the credit only showed while hillshade or contours drew. The Google map now credits it whenever a coverage layer is visible, and the Site Planner form carries the credit on every flavor. MapLibre has no map-side credit for it: `GeoJsonOptions` takes no attribution. fixes #7755
Every region holds the planet tile, so an unbounded walk gave any site anywhere a stand-in a pixel wide instead of failing. The tile scope is now canceled before stand-ins are forgotten, so a decode finishing in between cannot leave one in the shared cache.
The corrupt-tile retry now sits inside the same fallback, and decode and the ancestor walk are reshaped for detekt.
Walkthrough
Priority: Severity of issue fixed: Medium Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Tests Prove The Path, Not The End State |  |
The prior failure remains unresolved. OfflineTerrainTest.aRegionTileIsReadWithoutTheNetwork() writes bytes with store.writeTile(...), then asserts that MapterhornTiles.bytes(...) returns those b… |
Replace the populated-region round-trip with an instrumented filesystem or tile-read wrapper. Assert that the offline store read path was invoked and that the HTTP client received zero requests. Keep the missing-region request-count test as… |
| Regression Coverage For Changed Behavior | |
Regression coverage is incomplete for changed behavior. - MapterhornTiles.bytes and offlineBytes now combine the coverage cache, offline regions, and network. OfflineTerrainTest uses `store = nu… |
Add focused common tests for tile-source precedence, fresh lookup, failing and multiple offline regions, edge-only bounds, ancestor fallback, the z6/no-ancestor cases, stand-in and TerrainCache cleanup, and estimate cancellation. Add a te… |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title accurately summarizes the main changes: Site Planner estimate placement, signal floor, offline terrain, dispatcher usage, and attribution credit. It is specific and concise enough for projec… Linked Issues check The PR meets the coding requirements for all active direct issues. [ #7752]CoverageGrid.bandsincludes the final row and column and places boundaries half a cell around each sample; grids below 2×2 …Out of Scope Changes check The changes stay within the linked issue scope. Dispatcher abstractions, offline-region wiring, cache cleanup, string resources, dependency updates, and tests directly support the concurrency, offline… Sibling Call Sites And Presence Semantics The diff does not introduce a nullable physical measurement or a new zero-valued default. It removes the unused SitePlannerParams minDbm/maxDbm fields and adds optional offline terrain represented by … Moved Code Diffed Against Its Original PASS. The authoritative diff shows no type or function moved to another file. New files add coverageComputeDispatcher, platformprocessorCount(), andRegionTerrain; they do not replace deleted d…Full details: Title check
-
Explanation
The title accurately summarizes the main changes: Site Planner estimate placement, signal floor, offline terrain, dispatcher usage, and attribution credit. It is specific and concise enough for project history.
Full details: Linked Issues check
-
Explanation
The PR meets the coding requirements for all active direct issues. [
#7752]CoverageGrid.bandsincludes the final row and column and places boundaries half a cell around each sample; grids below 2×2 are rejected. [#7753]MapterhornTileschecks the coverage cache, region terrain, and then the network. Coarser region tiles are cropped, limited by region bounds, and removed after estimation. [#7754] Radials and tile decoding use aDispatchers.Defaultview with one worker reserved. [#7755] Google coverage attribution and the Site Planner form include Mapterhorn credit. [#7756] Coverage bands start at receiver sensitivity and span 50 dB. The added tests cover these behaviors.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within the linked issue scope. Dispatcher abstractions, offline-region wiring, cache cleanup, string resources, dependency updates, and tests directly support the concurrency, offline terrain, attribution, and coverage objectives. No unrelated product behavior is established by the change summary.
Full details: Sibling Call Sites And Presence Semantics
-
Explanation
The diff does not introduce a nullable physical measurement or a new zero-valued default. It removes the unused SitePlannerParams minDbm/maxDbm fields and adds optional offline terrain represented by an empty list. All three rememberCoverageEstimate call sites pass the updated terrain list, and all RegionTerrain constructions are present in the F-Droid, Google, and desktop paths. No unfixed sibling call site or in-scope 0 default is shown.
Full details: Moved Code Diffed Against Its Original
-
Explanation
PASS. The authoritative diff shows no type or function moved to another file. New files add
coverageComputeDispatcher, platformprocessorCount(), andRegionTerrain; they do not replace deleted declarations. TheMapterhornElevation.decodesplit preserves the original fetch/decode body, while the added fallback and limited dispatcher are explicit changes documented in code and the PR.ElevationSource,AutoCloseable, and both overrides remain. Existing callers use the new signatures correctly, and no caller still references removedSitePlannerParams.minDbmormaxDbmfields.
-
Full details: Tests Prove The Path, Not The End State
-
Explanation
The prior failure remains unresolved.
OfflineTerrainTest.aRegionTileIsReadWithoutTheNetwork()writes bytes withstore.writeTile(...), then asserts thatMapterhornTiles.bytes(...)returns those bytes and that requests equal zero. This is a fake backing-store round trip, not an independent proof that the offline fallback read path ran.aRegionsCoarseTileIsOnlyUsedForGroundTheRegionCovers()uses the same seeded-store pattern throughofflineBytes(...). The missing-region request test is only a fallback guard because the base implementation also issued that request. The geometry, grid validation, dispatcher, descendant, and sensitivity tests assert changed behavior that would fail when their covered production changes are reverted.Resolution
Replace the populated-region round-trip with an instrumented filesystem or tile-read wrapper. Assert that the offline store read path was invoked and that the HTTP client received zero requests. Keep the missing-region request-count test as a fallback regression guard, and make the populated-region assertion fail when the offline fallback is reverted.
Full details: Regression Coverage For Changed Behavior
-
Explanation
Regression coverage is incomplete for changed behavior. -
MapterhornTiles.bytesandofflineBytesnow combine the coverage cache, offline regions, and network.OfflineTerrainTestusesstore = nulland one successful region, so it does not prove cache precedence,fresh = truebypassing both stores, failed regional reads being skipped, multiple-region selection, or the positive-area boundary rule. Missing test: use separate stores with conflicting bytes, a failing first region and valid second region, fresh reads, and edge-only bounds. -MapterhornElevation.decode/fromCoarserRegionTile,forgetStandIns,TerrainCache.forget, and theestimateCoverageGeoJsoncleanup path have no tests. The existing tests only cover raw lookup andElevationTile.descendant. Missing test: make the requested and fresh tile decodes fail, provide a valid ancestor, assert fallback down toMIN_ZOOMand failure with no ancestor, then verify stand-ins are removed after success and cancellation. Verify cancellation still runs cleanup and thatTerrainCache.forgetremoves resolved and in-flight entries. -coverageComputeDispatcheris the production default, butPolarCoverageTestonly supplies a custom dispatcher. The new Android, JVM, and iOSprocessorCount()actuals are also untested. Missing test: add a controllable processor-count seam, assert the default limit ismaxOf(1, count - 1), and cover each platform actual. -CoverageGridnow rejects both dimensions below two, butCoverageContoursTestchecks onlywidth = 1. Missing test: assert a2 x 1grid is rejected. - Google attribution and terrain wiring have no UI or extracted-state tests.MapViewchanged attribution when a visible coverage layer exists,SitePlannerSheetadded the form credit, and Google/F-Droid/Desktop now pass offline terrain stores. Missing tests: assert attribution appears and disappears with coverage visibility, assert the credit renders, assert Google filtershasTerrainregions and passes their stores and bounds, and assert F-Droid and Desktop pass their region terrain.Resolution
Add focused common tests for tile-source precedence, fresh lookup, failing and multiple offline regions, edge-only bounds, ancestor fallback, the z6/no-ancestor cases, stand-in and
TerrainCachecleanup, and estimate cancellation. Add a test seam forprocessorCount()with default-dispatcher assertions plus Android/JVM/iOS actual coverage. Add the height-one grid test. Extract or test the Google attribution and terrain-selection state, add a Compose test for the form credit, and add flavor-specific propagation tests for Google, F-Droid, and Desktop.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
@coderabbitai full review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: meshtastic/Meshtastic-Android
- Branch: fix/coverage-garth-stack
- File: .coderabbit.yaml
- Commit: c524499
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Advanced
- Run ID:
d179d143-c079-407f-b3e4-5c80d0259888
-
.skills/compose-ui/strings-index.txtandroidApp/src/fdroid/kotlin/org/meshtastic/app/map/component/SitePlannerSlot.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/MapView.ktcore/resources/src/commonMain/composeResources/values/strings.xmldesktopApp/src/main/kotlin/org/meshtastic/desktop/map/DesktopSitePlannerSlot.ktfeature/coverage/build.gradle.ktsfeature/coverage/src/androidMain/kotlin/org/meshtastic/feature/coverage/CoverageCompute.android.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/CoverageCompute.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/CoverageContours.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/CoverageGrid.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/MapterhornElevation.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/MapterhornTiles.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/PolarCoverage.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/SitePlannerEstimate.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/TerrainCache.ktfeature/coverage/src/commonTest/kotlin/org/meshtastic/feature/coverage/CoverageContoursTest.ktfeature/coverage/src/commonTest/kotlin/org/meshtastic/feature/coverage/OfflineTerrainTest.ktfeature/coverage/src/commonTest/kotlin/org/meshtastic/feature/coverage/PolarCoverageTest.ktfeature/coverage/src/commonTest/kotlin/org/meshtastic/feature/coverage/SitePlannerEstimateTest.ktfeature/coverage/src/iosMain/kotlin/org/meshtastic/feature/coverage/CoverageCompute.ios.ktfeature/coverage/src/jvmMain/kotlin/org/meshtastic/feature/coverage/CoverageCompute.jvm.ktfeature/map/src/commonMain/kotlin/org/meshtastic/feature/map/component/SitePlannerParams.ktfeature/map/src/commonMain/kotlin/org/meshtastic/feature/map/component/SitePlannerSheet.kt
-
- feature/map/src/commonMain/kotlin/org/meshtastic/feature/map/component/SitePlannerParams.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
-
✅ Action performed
|
There was a problem hiding this comment.
Actionable comments posted: 3
-
-
- Source mode: Auto
- Repository: meshtastic/Meshtastic-Android
- Branch: fix/coverage-garth-stack
- File: .coderabbit.yaml
- Commit: c524499
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Advanced
- Run ID:
e898ac0a-df4c-4ca8-b99a-b622a61a738d
-
.skills/compose-ui/strings-index.txtandroidApp/src/fdroid/kotlin/org/meshtastic/app/map/component/SitePlannerSlot.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/MapView.ktcore/resources/src/commonMain/composeResources/values/strings.xmldesktopApp/src/main/kotlin/org/meshtastic/desktop/map/DesktopSitePlannerSlot.ktfeature/coverage/build.gradle.ktsfeature/coverage/src/androidMain/kotlin/org/meshtastic/feature/coverage/CoverageCompute.android.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/CoverageCompute.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/CoverageContours.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/CoverageGrid.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/MapterhornElevation.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/MapterhornTiles.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/PolarCoverage.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/SitePlannerEstimate.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/TerrainCache.ktfeature/coverage/src/commonTest/kotlin/org/meshtastic/feature/coverage/CoverageContoursTest.ktfeature/coverage/src/commonTest/kotlin/org/meshtastic/feature/coverage/OfflineTerrainTest.ktfeature/coverage/src/commonTest/kotlin/org/meshtastic/feature/coverage/PolarCoverageTest.ktfeature/coverage/src/commonTest/kotlin/org/meshtastic/feature/coverage/SitePlannerEstimateTest.ktfeature/coverage/src/iosMain/kotlin/org/meshtastic/feature/coverage/CoverageCompute.ios.ktfeature/coverage/src/jvmMain/kotlin/org/meshtastic/feature/coverage/CoverageCompute.jvm.ktfeature/map/src/commonMain/kotlin/org/meshtastic/feature/map/component/SitePlannerParams.ktfeature/map/src/commonMain/kotlin/org/meshtastic/feature/map/component/SitePlannerSheet.kt
-
- feature/map/src/commonMain/kotlin/org/meshtastic/feature/map/component/SitePlannerParams.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
-
A region keeps every z6-z12 tile it touches, and a z6 tile spans hundreds of km, so a site far outside the region still got stand-in terrain offline. Each region now comes with its bounds, and a tile is read from it only for ground inside them.
There was a problem hiding this comment.
Actionable comments posted: 2
-
-
- Source mode: Auto
- Repository: meshtastic/Meshtastic-Android
- Branch: fix/coverage-garth-stack
- File: .coderabbit.yaml
- Commit: 4d14ad6
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Advanced
- Run ID:
7f45c84e-7798-4c43-814b-bb405fbf9725
-
androidApp/src/fdroid/kotlin/org/meshtastic/app/map/component/SitePlannerSlot.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/MapView.ktdesktopApp/src/main/kotlin/org/meshtastic/desktop/map/DesktopSitePlannerSlot.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/MapterhornElevation.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/MapterhornTiles.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/RegionTerrain.ktfeature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/SitePlannerEstimate.ktfeature/coverage/src/commonTest/kotlin/org/meshtastic/feature/coverage/OfflineTerrainTest.kt
-
- androidApp/src/fdroid/kotlin/org/meshtastic/app/map/component/SitePlannerSlot.kt
- androidApp/src/google/kotlin/org/meshtastic/app/map/MapView.kt
- desktopApp/src/main/kotlin/org/meshtastic/desktop/map/DesktopSitePlannerSlot.kt
- feature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/MapterhornTiles.kt
- feature/coverage/src/commonTest/kotlin/org/meshtastic/feature/coverage/OfflineTerrainTest.kt
- feature/coverage/src/commonMain/kotlin/org/meshtastic/feature/coverage/MapterhornElevation.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.
-
The on-device Site Planner estimate drew its bands half a cell off, cut the outer edge at a fixed -130 dBm, failed offline over downloaded terrain, tied up every
Dispatchers.Defaultworker and never credited Mapterhorn. Supersedes #7757, whose commit is cherry-picked here.🐛 Bug Fixes
minDbm/maxDbmparams are gone ([Bug]: Coverage overlay floor ignores the receiver sensitivity, so the outer edge is wrong #7756).Dispatchers.Defaultone worker short ([Bug]: Coverage estimate occupies every Dispatchers.Default thread for the whole run #7754).GeoJsonOptionstakes no attribution.Testing Performed
CoverageContoursTest: half-cell ring bounds, 1-wide grid rejected.SitePlannerEstimateTest: -139 dBm sensitivity gives a -139..-89 ramp.PolarCoverageTest: radials dispatch on the dispatcher passed in.OfflineTerrainTest: region tile read with no request, network still asked when no region holds the tile, a region's coarse tile refused for ground outside it, parent-tile crop.fixes #7752
fixes #7753
fixes #7754
fixes #7755
fixes #7756
Summary by CodeRabbit