feat: Improve connection state broadcast and timing#4498
Merged
Conversation
This commit refactors the connection state broadcasting logic. - A new `ACTION_CONNECTION_CHANGED` broadcast is introduced for legacy consumers. - The existing `ACTION_MESH_CONNECTED` broadcast now sends the connection state as an uppercase string for improved compatibility (e.g., with ATAK). - The `Connected` state is now set and broadcasted immediately after `myNodeInfo` is successfully committed, rather than waiting for the full node DB update. This provides a more accurate and timely reflection of the connection status. Signed-off-by: James Rich <2199651+jamesarich@users.noreply.github.com>
316d869 to
650f5ba
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors mesh connection state signaling from the Android service to improve broadcast compatibility (e.g., ATAK) and to reflect “Connected” sooner in the config flow.
Changes:
- Broadcast connection state as an uppercase string on
ACTION_MESH_CONNECTEDand reintroduce a legacyACTION_CONNECTION_CHANGEDbroadcast for older consumers. - Move the “Connected” state transition + broadcast to immediately after
myNodeInfocommit (instead of waiting for the full node DB update). - Add the new broadcast action constant.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/src/main/java/com/geeksville/mesh/service/MeshServiceBroadcasts.kt | Uppercases connection state extra; adds legacy ACTION_CONNECTION_CHANGED broadcast and legacy boolean extra. |
| app/src/main/java/com/geeksville/mesh/service/MeshConfigFlowManager.kt | Sets/broadcasts Connected earlier in the config-only completion path; reorders downstream hooks. |
| app/src/main/java/com/geeksville/mesh/service/Constants.kt | Introduces ACTION_CONNECTION_CHANGED constant. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4498 +/- ##
==========================================
+ Coverage 10.64% 10.89% +0.25%
==========================================
Files 427 427
Lines 14387 14395 +8
Branches 2387 2388 +1
==========================================
+ Hits 1531 1568 +37
+ Misses 12561 12526 -35
- Partials 295 301 +6 ☔ View full report in Codecov by Sentry. |
This commit refactors the connection logic to better separate the "radio config loaded" and "node database ready" states. Key changes: - Introduces `onRadioConfigLoaded()` to handle actions that only require the radio configuration, such as setting the time on the device and processing the initial packet queue. - Introduces `onNodeDbReady()` to handle actions that depend on the node database being initialized, such as starting MQTT and requesting message history. - The previous `onHasSettings()` method is removed and its logic is split between the two new, more specific functions. - Fixes a lint warning by explicitly using `Locale.ROOT` for an `uppercase()` string conversion. Signed-off-by: James Rich <2199651+jamesarich@users.noreply.github.com>
…erviceBroadcasts Adds and improves unit tests for core service components. - Implements Robolectric tests for `MeshServiceBroadcasts` to verify that connection state change intents are correctly broadcast for ATAK and legacy consumers. - Adds test coverage for `MeshConnectionManager` to ensure queued packets are processed and time is set after radio config is loaded. - Verifies that `MeshConnectionManager` correctly starts MQTT and requests message history when the node database is ready. - Includes `Robolectric` and `androidx.test.core` as test dependencies. Signed-off-by: James Rich <2199651+jamesarich@users.noreply.github.com>
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.
This pull request introduces several improvements to connection state handling and broadcasting, adds new tests, and updates dependencies to support enhanced testing. The main changes include refactoring connection state management and broadcasts for better compatibility, updating method responsibilities, and increasing test coverage for these behaviors.
Connection State Handling and Broadcasting:
ACTION_CONNECTION_CHANGED, toConstants.ktfor legacy compatibility and updatedbroadcastConnectioninMeshServiceBroadcaststo send both the new and legacy intents with uppercase state strings for better integration with consumers like ATAK and older clients. [1] [2]broadcastConnectionto ensure the connection state string is always uppercase and to include both the new and legacy broadcast formats, including a boolean extra for legacy consumers.Connection Manager Refactoring:
MeshConfigFlowManagerandMeshConnectionManagerto separate responsibilities: moved setting the connection state and broadcasting out of node DB readiness and into radio config loading, and split theonHasSettingsmethod intoonRadioConfigLoadedandonNodeDbReadyfor clearer lifecycle management. [1] [2] [3] [4]Testing Enhancements:
MeshServiceBroadcastsTest.ktusing Robolectric to verify correct broadcast behavior for connection state changes, including both new and legacy intent formats.MeshConnectionManagerTestto cover the newonRadioConfigLoadedandonNodeDbReadybehaviors, and updated test setup to mock new flows. [1] [2] [3] [4]Dependency Updates:
robolectricandandroidx.test.coreto the test dependencies inbuild.gradle.ktsto support the new and updated tests.