Repository navigation
Wire interface mode into reticulum-kt interfaces (Kotlin backend) - #1188
Merged
Merged
Conversation
The Kotlin backend persisted the user-selected interface mode to the DB and wrote it to the RNS config (honored by the Python flavor) but never applied it to the constructed reticulum-kt interface objects, so every interface silently ran as the default FULL mode. See #1169. Add InterfaceModeMapper with a withMode() extension that maps Columba's string-valued InterfaceMode onto reticulum-kt's InterfaceMode and applies it via modeOverride at every construction site: AutoInterface, TCPClient, UDP, TCPServer, RNode (SPP/USB/TCP), and BLE. INTERNAL is intentionally left unmapped: the pinned reticulum-kt InterfaceMode enum has no INTERNAL value and its stray POINT_TO_POINT corresponds to no RNS config mode string. Per #1169 it must stay a no-op (leave the interface at its default, log a warning) until reticulum-kt #91 reconciles the enum - coercing it to POINT_TO_POINT or FULL would change announce re-broadcast behavior without conformance proof. Tighten createInterface() to return Interface? and mark it @VisibleForTesting so the regression test drives the real factory wiring directly (no start(), no sockets). InterfaceModeWiringTest asserts the constructed object reports the configured mode across all mappable modes, that the mode field is actually consumed (gateway != full), and that internal is not silently coerced.
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+64
to
+66
| /** Build the interface via the production factory wiring (no start(), no sockets). */ | ||
| private fun buildInterface(config: InterfaceConfig): network.reticulum.interfaces.Interface { | ||
| val iface = NativeInterfaceFactory.createInterface(config) |
Contributor
There was a problem hiding this comment.
The new tests cover only AutoInterface, TCPClient, and UDP. None checks the new mode calls for TCPServer, RNode, or BLE. Removing those calls would leave the suite green while restoring the original bug for those interfaces.
Add coverage for those construction paths, using fake connections where hardware is required.
Prompt To Fix With AI
This is a comment left during a code review.
Path: rns-backend-kt/src/test/kotlin/network/columba/app/rns/backend/kt/InterfaceModeWiringTest.kt
Line: 64-66
Comment:
**Three interface types lack tests**
The new tests cover only `AutoInterface`, `TCPClient`, and `UDP`. None checks the new mode calls for `TCPServer`, `RNode`, or BLE. Removing those calls would leave the suite green while restoring the original bug for those interfaces.
Add coverage for those construction paths, using fake connections where hardware is required.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Owner
Author
There was a problem hiding this comment.
Addressed on the new head: added TCPServer (via the real factory), RNode (driving the real RNodeConnectionHelper with a mocked host bridge), and BLE (real BLEInterface + the same .withMode call from startBleInterface). 8/8 green.
Owner
Author
There was a problem hiding this comment.
All six interface types now include internal in their mode-iteration lists: autoInterface (line 87), tcpClient (line 101), tcpServer (line 124), udp (line 138), rnode (line 166), and ble (line 201). Each asserts effectiveMode matches expectedReticulumMode(mode) which returns InterfaceMode.INTERNAL for the internal string. Verified locally: 11/11 InterfaceModeWiringTest tests pass including the per-type INTERNAL coverage.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
… greploop P2) Greploop 4/5 flagged that InterfaceModeWiringTest only covered AutoInterface, TCPClient, UDP. Close the gap: - TCPServer via the real factory (binds only in start(), safe in JVM) - RNode by driving the real RNodeConnectionHelper with a mocked host bridge - BLE by constructing a real BLEInterface + the same .withMode call from startBleInterface 8/8 green. P1 (live mode edit on a running interface) is a false positive - Torlando confirmed it works in practice.
Owner
Author
|
@greptile Please review the new commit (addresses P2: added RNode, BLE, TCPServer mode-wiring coverage). |
…1188 P1) Greploop 4/5 (P1): syncInterfaces is diff-based and only starts names not already running, so saving a new mode for a running interface was a silent no-op - the object kept the mode it was started with. Track the applied mode per running interface (runningModes) and, during sync, restart any already-running interface whose saved mode changed. A same-mode sync does not restart, so an unrelated field edit does not churn the connection. registerAndTrack now takes the config so the mode is recorded at the single funnel where interfaces become running; stopInterface clears it. Adds 2 regression tests: savedModeEditOnRunningInterface_restartsAndAppliesMode and unchangedRunningInterface_isNotRestartedOnSync. 10/10 green.
Owner
Author
|
@greptile Please re-review the new commit - fixes the saved-mode-edit issue (P1) by restarting running interfaces whose mode changed during sync. |
…#1188 4/5) Greploop 4/5: saving a disable/delete for a running RNode started a replacement in the background; if the user disabled or deleted the interface before the async start opened its connection, the pending start still called registerAndTrack, reviving the interface and letting it carry traffic. Add a per-name start-generation counter (startGenerations). An async RNode/BLE start advances and captures the generation synchronously on the calling thread (before the coroutine runs), so a stop/restart/delete that lands after the start is launched but before it registers sees a higher generation. registerAndTrack refuses to register when the captured generation no longer matches the current one. stopInterface bumps the generation (invalidating any in-flight start for that name) and syncInterfaces invalidates in-flight starts whose name is no longer desired - the case the runningInterfaces stop loop does not reach. The RNode start is launched (not suspended) so the sync thread is never blocked on a radio connection and a disable from that thread can interleave. Adds a deterministic regression test: disabledInFlightAsyncStart_doesNotRevive Interface parks an in-flight RNode start in a mock host bridge, disables it while parked, releases it, and asserts the interface never registers. 11/11 green.
Owner
Author
|
@greptile Please re-review the new commit - fixes the async-start supersede race (4/5 Issue 1) with a per-name start-generation guard. |
#1188) Greploop 3/5 Issue 1 (P1): the start paths call iface.start() before registerAndTrack's generation check, so when a start is rejected (the interface was stopped/restarted/deleted mid-connection) the object had already opened a live radio connection and spawned background coroutines, but never entered runningInterfaces - so stopInterface and shutdownAll could not find or stop it. The rejected radio kept running as an orphan. Detach the rejected interface in the rejection path so a superseded start releases its connection and coroutines (mirrors the cleanup stopInterface does for tracked interfaces). Combined with the per-name start-generation guard this also closes the disable-mid-connection case: the stale start neither registers nor leaks. 11/11 green.
Owner
Author
|
@greptile Please re-review the new commit - detaches the superseded radio interface on rejection (fixes 3/5 Issue 1 resource leak). |
reticulum-kt #91 is resolved (v0.0.24): the InterfaceMode enum now has INTERNAL with the RNS 1.3.6+ semi-transport semantics (relays + path discovery, announce re-broadcast gated on the next-hop mode). - Bump reticulumKt v0.0.22 -> v0.0.24 - InterfaceModeMapper: INTERNAL maps 1:1 to ReticulumMode.INTERNAL (was -> null, left at FULL default pending #91); remove the stale warning path - InterfaceModeWiringTest: internal is now wired across all 6 interface types (Auto, TCPClient, TCPServer, UDP, RNode, BLE); the dedicated INTERNAL test asserts modeOverride and effectiveMode are INTERNAL
Owner
Author
|
@greptile review |
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.
Fixes #1169
The Kotlin backend persisted the user-selected interface mode to the DB and
wrote it to the RNS config file (honored by the Python flavor) but never applied
it to the constructed reticulum-kt interface objects, so every interface
silently ran as the default FULL mode regardless of what the user selected.
What's done
InterfaceModeMapperwith awithMode(name, mode)extension thatmaps Columba's string-valued
InterfaceModeonto reticulum-kt'sInterfaceModeand applies it viamodeOverrideat every constructionsite: AutoInterface, TCPClient, UDP, TCPServer, RNode (SPP/USB/TCP), and BLE.
createInterface()to returnInterface?and marked it@VisibleForTestingso the regression test drives the real factory wiringdirectly (no
start(), no sockets).InterfaceModeWiringTestasserting the constructed object reports theconfigured mode across all mappable modes, that the mode field is actually
consumed (gateway != full), and that internal is not silently coerced.
INTERNAL is intentionally left unmapped
The pinned reticulum-kt
InterfaceModeenum has no INTERNAL value, and itsstray
POINT_TO_POINTcorresponds to no RNS config mode string. Per #1169 itmust stay a no-op (leave the interface at its default, log a warning) until
reticulum-kt #91 reconciles the enum - coercing it to
POINT_TO_POINTorFULLwould change announce re-broadcast behavior without conformance proof.Verification
rns-backend-ktunit suite: 23 tests, 0 failures.announces are not re-broadcast) is deferred until reticulum-kt Support for Encrypted Paper Messages (lxm:// URI Handler) #91 adds a
real INTERNAL.