refactor: Remove AIDL API and modernize service architecture#5586
Conversation
2777fe8 to
3fde9cc
Compare
3fde9cc to
b304b0d
Compare
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
✅ Docs staleness check passedThis PR includes updates to |
🖼️ Preview staleness check — advisoryThis PR modifies UI composables but does not update any
Changed UI files: What to check:
Adding previews checklist:
If this PR does not require preview updates (e.g., logic-only change, non-visual refactor), add the |
51fff3c to
6f447c8
Compare
237f5e1 to
059ee97
Compare
f475045 to
66ae840
Compare
Remove the deprecated AIDL/IPC API surface and perform deep architectural
modernization of the radio command pipeline, aligning with the meshtastic-sdk
AdminApiImpl pattern for future SDK migration.
Key changes:
1. AIDL Removal & Infrastructure Cleanup
- Delete core:api module and all AIDL interfaces
- Remove ServiceBroadcasts + CommonParcelable infrastructure
- Remove core:api from CI workflow lint/publish steps
2. Model Modernization
- Introduce NodeAddress sealed class with type-safe addressing
- Remove deprecated DataPacket constants in favor of NodeAddress
- Consolidate dual node maps into single source with getNodeById
- Split large model files, deduplicate NodeEntity, flatten RadioController
3. Service Layer Refactoring (SDK-aligned)
- Remove ServiceAction sealed class, use direct suspend calls
- Convert CommandSender & MeshActionHandler to suspend APIs
- Merge MeshActionHandler into DirectRadioControllerImpl
(ViewModel → RadioController → CommandSender, no intermediate layer)
- Build AdminMessage protos directly with typed protos end-to-end
- Apply structured concurrency to NodeRequestActions/NodeManagementActions
- Fix CancellationException handling throughout
Architecture (before → after):
ViewModel → RadioController → Handler → CommandSender → PacketHandler
ViewModel → RadioController → CommandSender → PacketHandler
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- NodeAddressTest: 37 tests covering sealed class parser, roundtrip, extensions (ContactKey, DataPacket), edge cases - CommandSenderImplTest: 20 tests covering packet ID generation, address resolution, sendData validation, admin messages, position - DirectRadioControllerImplTest: +8 tests for reboot/shutdown/factory reset, importContact, refreshMetadata - NodeManagerImplTest: +7 tests for getMyNodeInfo aggregation, getMyId, null telemetry handling Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
With AGP 9's built-in Kotlin, org.jetbrains.kotlin.android is no longer
applied to Android modules. Convention plugin hooks using
withPlugin("org.jetbrains.kotlin.android") were dead code — replaced
with withPlugin("com.android.application") and
withPlugin("com.android.library") which correctly trigger for
Android-only modules using built-in Kotlin.
- KoinConventionPlugin: split into app + library hooks
- AndroidRoomConventionPlugin: use com.android.library hook
- KotlinXSerializationConventionPlugin: use app + library hooks
- Remove kotlin-android from version catalog and root build.gradle.kts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
No modules are published externally anymore (core:api was removed).
Remove all publishing-related configuration:
- Delete PublishingConventionPlugin and its registration
- Remove publish-core.yml workflow
- Strip publishing{} blocks from core/model and core/proto
- Remove PUBLISHED_MODULES set and Java 17 compatibility logic
- Unify all modules to JDK 21 toolchain and JVM target
- Remove unused imports (JavaToolchainService, JavaLanguageVersion, Test)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove x86/x86_64 from ABI filters (armeabi-v7a/arm64-v8a only) - Remove unused takpacket-sdk-jvm from version catalog - Raise core/proto minSdk from 21 to 26 (ATAK compat no longer needed) - Remove stale FIXME comment about foreground service in manifest - Replace Executors.newSingleThreadExecutor with Dispatchers.Default.asExecutor in BarcodeScannerProvider (removes manual thread pool management) - Convert formatAgo() to @composable with stringResource(), eliminating runBlocking from the UI rendering path. Non-composable callers (map views, accessibility) use a 3-arg overload with pre-resolved strings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Wrap StateFlow.value read in remember{} to satisfy composition lint
- Wrap derivedStateOf in remember{} (MapView PurgeTileSourceDialog)
- Use mutableIntStateOf to avoid autoboxing (MapStyleDialog)
- Use ResourcesCompat.getDrawable() instead of deprecated getDrawable()
- Fix mixed indentation in MarkerClusterer.java
Lint result: 0 errors, 10 warnings (down from 2 errors, 12 warnings)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This feature flag is now enabled by default in the Compose compiler, making the explicit opt-in unnecessary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- NodeAddress.idToNum: reject >32-bit hex instead of silently truncating; use toLongOrNull rather than runCatching; drop redundant double prefix-strip. - DirectRadioControllerImpl: compare destNum against the nullable myNodeNum.value (not the ?:0 getter) so local-side-effect branches don't fire spuriously for destNum=0 before connect. - DirectRadioControllerImpl.setModuleConfig: move node_status optimistic write inside the local-node guard so we don't overwrite remote status. - DirectRadioControllerImpl.sendReaction: use ContactKey wrapper instead of manual [0].digitToInt()/substring(1). - DirectRadioControllerImpl.importContact: respect incoming manually_verified flag; early-return on node_num==0 or null user. - DirectRadioControllerImpl.requestPosition: consolidate when-chain; document Position(0,0,0) as protocol "no position" sentinel. - ContactKey: accessors safe against empty value; non-digit first char defaults channel to 0 rather than throwing. - NodeDetailViewModel.openRemoteAdmin: atomic compareAndSet replaces check-then-act TOCTOU that allowed double-tap to queue two passkey exchanges + duplicate navigation events. - MessageViewModel.frequentEmojis: toIntOrNull + mapNotNull so a corrupted pref entry no longer crashes every recomposition. - AndroidMeshLocationManager.restart: log the silent-bail case (no-op until MeshConnectionManagerImpl wires us up). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- RadioController.setDeviceAddress is now suspend. Callers can rely on the
device switch completing (DB switched, node DB cleared, transport
reconfigured) before their next call — the old non-suspend impl wrapped
the work in scope.launch{} and returned immediately, racing against
follow-up operations like OTA disconnect-then-delay.
- DirectRadioControllerImpl: drops the scope.launch wrapper.
- FakeRadioController: matches the new suspend contract so tests no longer
hide ordering bugs that production would exhibit.
- UIViewModel / ScannerViewModel: wrap the suspend call in safeLaunch.
- Esp32OtaUpdateHandler.disconnectMeshService: marked suspend (caller is
already inside a withContext block).
- MeshServiceOrchestrator: replace plain `var scope: CoroutineScope?` with
an atomicfu AtomicRef and compareAndSet on start(). Concurrent start()
invocations could otherwise both observe the slot empty, both allocate
scopes, and the second overwrite the first — orphaning the first scope's
collectors on radioInterfaceService.receivedData.
- PacketHandlerImpl.sendToRadio: document the FIFO invariant. Mutex is
not strictly fair across coroutines, but the only ordering callers rely
on is within a single coroutine (e.g. InstallProfileUseCase issues
beginEditSettings → install* → commitEditSettings sequentially in one
suspend), where sequential mutex acquisition preserves order.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add a "Radio Control" section to the developer architecture guide covering how features issue radio commands: the RadioController composite, its four sub-interfaces (Admin/Messaging/Node/Request), and DirectRadioControllerImpl as the in-process composition root that assembles them via interface delegation. Notes the fire-and-forget admin model and the deliberate alignment with the meshtastic-sdk API shape. Additive only (no stale-reference fix); bumps last_updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Apply Interface Segregation Principle to ServiceRepository and RadioController consumers: New interfaces extracted from ServiceRepository: - ConnectionStateProvider: read-only connectionState access - TracerouteResponseProvider: traceroute response state + clear - NeighborInfoResponseProvider: neighbor info response state + clear - ServiceStateWriter: write-side for handlers (set*, emit*, clear*) RadioController now extends ConnectionStateProvider, and sub-controller interfaces (AdminController, MessagingController, NodeController, RequestController) are bound in DI for fine-grained injection. ViewModel narrowing: - MessageViewModel: RadioController+ServiceRepository → MessagingController+ConnectionStateProvider - NodeDetailViewModel: RadioController → RequestController - NodeListViewModel: RadioController+ServiceRepository → AdminController+ConnectionStateProvider - ContactsViewModel: ServiceRepository → ConnectionStateProvider - MetricsViewModel: ServiceRepository → TracerouteResponseProvider All tests updated to use narrowed interfaces. Koin DI bindings updated for both Android (@single binds) and Desktop (manual single<> declarations). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update core/repository README and architecture docs to reflect the new focused provider interfaces (ConnectionStateProvider, TracerouteResponseProvider, NeighborInfoResponseProvider, ServiceStateWriter) and ViewModel narrowing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ateProvider Following the ServiceRepository ISP decomposition, the two consumers that only read connectionState now inject ConnectionStateProvider instead of the full ServiceRepository: - ConnectionsViewModel (core:ui) - LocalStatsWidgetStateProvider (feature:widget) The remaining three full-ServiceRepository injections are left as-is because they use members not carried by any narrow provider: RadioConfigViewModel (meshPacketFlow), ScannerViewModel (connectionProgress), and UIViewModel (clientNotification + errorMessage reads). Extracting single-consumer providers for those would be over-segregation. Koin graph verifies on Android + desktop; ConnectionsViewModelTest green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The ISP decomposition extracted ServiceStateWriter but left it with zero adopters — every write-side handler still injected the full ServiceRepository, able to read state it only writes. Complete the adoption: - 9 pure writers now inject ServiceStateWriter (param renamed serviceStateWriter): TracerouteHandlerImpl, NeighborInfoHandlerImpl, MeshMessageProcessorImpl, MeshConfigFlowManagerImpl, MeshConfigHandlerImpl, MeshDataHandlerImpl, FromRadioPacketHandlerImpl, MqttManagerImpl, MeshServiceOrchestrator. - PacketHandlerImpl reads only connectionState -> ConnectionStateProvider. The two genuinely mixed read+write holders (MeshConnectionManagerImpl, DirectRadioControllerImpl) keep the full ServiceRepository. Read-side flows with a single consumer (connectionProgress/clientNotification/ errorMessage/meshPacketFlow, mostly UIViewModel) are intentionally not extracted into providers — that would be over-segregation. Koin graph verifies on Android + desktop; core:data/core:service suites green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Audit-driven renames: - DirectRadioControllerImpl -> RadioControllerImpl: the "Direct" prefix was vestigial (it once contrasted with the AIDL-routed AndroidRadioControllerImpl, now deleted); it is the sole impl and now matches its parts (AdminControllerImpl, etc.). - RadioController.getPacketId() -> generatePacketId(): a `get`-prefixed function that generates a fresh id each call violates the getter-is-idempotent convention; also aligns with CommandSender.generatePacketId(). - RequestController -> QueryController (+ Impl, + the `requestController` params/mocks): clearer intent for the pull/query surface; "request" was generic. - RequestTimer param `label` -> `logLabel`. - AdminControllerImpl DEFAULT_REBOOT_DELAY -> DEFAULT_DELAY_SECONDS (shared by reboot + shutdown; conveys the unit). Interface-consumer-safe; docs/READMEs/architecture guide updated to match. Koin graph verifies on Android + desktop; affected test suites green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…l-api # Conflicts: # .agent_memory/session_context.md
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Squash merge of PR #5586 into release/2.8.0. Remove the deprecated AIDL API and all associated infrastructure. Replaces with a modern, testable, KMP-compatible architecture. 264 files changed with a net reduction of ~2,559 lines. Introduces RadioController, MessageQueue, and typed ContactKey abstractions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🔗 Release 2.8.0 Integration ReportBranch: Integration Changes Required
Merge Guidance
|
Wrap database write operations (clearUnreadCount, clearAllUnreadCounts, updateLastReadMessage, insertRoomPacket) with NonCancellable to prevent connection pool leaks when the calling coroutine scope is cancelled mid-write. Found during release/2.8.0 integration testing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sort imports alphabetically and format single-line lambdas properly to pass spotlessCheck and detekt. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolved conflicts: - SendMessageUseCase: keep #5585's Int return + #5586's NodeAddress/ContactKey parsing - core/proto/build.gradle.kts: keep release's ATAK minSdk=21 override - proto submodule pinned to 6b1ded4 (master HEAD) - docs (copilot-instructions, session_context): take release/2.8.0 Post-merge API fixups (ID_BROADCAST/ID_LOCAL moved DataPacket -> NodeAddress): - AiFunctionProviderImpl (#5585), PacketRepositoryImpl - car: HomeScreen, ConversationShortcutManager, CarScreenDataBuilder (#5633)
… AiFunctionProvider Final post-AIDL-removal API fixup: PKC_CHANNEL_INDEX also moved to NodeAddress. Plus ktfmt reformat of CarScreenDataBuilder. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
All DataPacket constant refs migrated to NodeAddress; the import is dead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolved conflicts:
- Room schema renumbered v39 -> v42 (FTS5=39, air-quality=40, msh.to=41): version=42,
AutoMigration(41->42), 3 discovery entities, regenerated 42.json, kept release's 39.json
- MeshtasticDatabase imports: union Discovery{Dao,entities} + DeviceLink{Dao,Entity}
- SettingsScreen / DesktopSettingsScreen: union Discovery + DeviceLinks (+ AppFunctions) entries
- proto pinned to 6b1ded4 (master HEAD); docs: take release/2.8.0
Post-merge API fixups (#5586 AIDL removal):
- DiscoveryScanEngine: RadioController core.model -> core.repository; getPacketId() -> generatePacketId()
- discovery tests: DataPacket.ID_BROADCAST -> NodeAddress.ID_BROADCAST
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Summary
Remove the deprecated AIDL API and all associated infrastructure, unlocking a comprehensive modernization of the radio communication architecture. This PR eliminates thousands of lines of legacy code and replaces it with a modern, testable, KMP-compatible architecture.
262 files changed: +3,978 / −6,537 (net −2,559 lines)
Before: AIDL-based Architecture
graph TD subgraph "Client Apps / UI" VM[ViewModels] end subgraph "core:api (AIDL)" AIDL[IMeshService.aidl] Binder[Bound Service Binder] BC[Broadcast Receivers] end subgraph "Service Layer" MS[MeshService<br/>God-class] MR[MeshRouter<br/>Service Locator] end subgraph "Handlers" FRP[FromRadioPacketHandler] MMP[MeshMessageProcessor] MDH[MeshDataHandler] MCH[MeshConfigHandler] NIH[NeighborInfoHandler] TRH[TracerouteHandler] end subgraph "Radio" DRC[DirectRadioControllerImpl<br/>~450 lines, 17 deps, 44 functions] end VM -->|"IPC / Binder"| AIDL AIDL --> Binder Binder --> MS MS -->|"broadcasts"| BC BC -->|"intents"| VM MS --> MR MR -->|"lazy getters"| FRP MR -->|"lazy getters"| MMP MR -->|"lazy getters"| MDH MR -->|"lazy getters"| MCH MR -->|"lazy getters"| NIH MR -->|"lazy getters"| TRH MS --> DRC style AIDL fill:#ff6b6b,stroke:#c92a2a style Binder fill:#ff6b6b,stroke:#c92a2a style BC fill:#ff6b6b,stroke:#c92a2a style MR fill:#ff6b6b,stroke:#c92a2a style MS fill:#ffa94d,stroke:#e67700 style DRC fill:#ffa94d,stroke:#e67700After: Modern KMP Architecture
graph TD subgraph "UI Layer" VM[ViewModels] end subgraph "core:repository — segregated interfaces" CMD["Command interfaces<br/>Admin · Messaging · Node · Query"] READ["Read providers<br/>ConnectionState · Traceroute · NeighborInfo"] WRITE["ServiceStateWriter<br/>(write-only)"] end subgraph "core:service / core:data" DRC[RadioControllerImpl<br/>delegates to 4 sub-controllers] SR[ServiceRepositoryImpl] CS[CommandSender] H[Packet handlers<br/>direct injection, no service locator] end VM -->|"suspend calls"| CMD VM -->|"observe StateFlow"| READ DRC -.implements.-> CMD SR -.implements.-> READ SR -.implements.-> WRITE DRC --> CS H -->|"set* / emit*"| WRITE CS -->|"sends to radio"| HWhat was removed
core:apimodule — AIDL interfaces, bound service, broadcast-based command dispatchServiceBroadcasts/ServiceAction— intent-based command routing and status broadcastsMeshActionHandler— folded into the radio controllerMeshRouter— service-locator facade with zero routing logicrunBlockingin UI paths, redundant compiler flags, stalekotlin-androidplugin references, Parcelable plumbing incore:modelArchitecture improvements
Radio controller redesign
RadioControllersplit into 4 focused sub-interfaces:AdminController,MessagingController,NodeController,QueryControllerRadioControllerImpl, a thin composition root that assembles four focused impls (AdminControllerImpl,MessagingControllerImpl,NodeControllerImpl,QueryControllerImpl) via Kotlin interface delegation; its constructor is unchanged so DI + the integration test are untouchedServiceRepository ISP decomposition
ServiceRepositorydecomposed into focused sub-interfaces:ConnectionStateProvider,TracerouteResponseProvider,NeighborInfoResponseProvider,ServiceStateWriter(ServiceRepositoryextends all for compatibility)MessageViewModel→MessagingController+ConnectionStateProviderNodeListViewModel→AdminController+ConnectionStateProviderNodeDetailViewModel→QueryControllerContactsViewModel,ConnectionsViewModel,LocalStatsWidgetStateProvider→ConnectionStateProviderMetricsViewModel→TracerouteResponseProviderServiceStateWriterinstead of the full repository (TracerouteHandlerImpl,NeighborInfoHandlerImpl,MeshMessageProcessorImpl,MeshConfigFlowManagerImpl,MeshConfigHandlerImpl,MeshDataHandlerImpl,FromRadioPacketHandlerImpl,MqttManagerImpl,MeshServiceOrchestrator);PacketHandlerImpl(read-only) injectsConnectionStateProvider. The genuinely mixed read+write holders (MeshConnectionManagerImpl,RadioControllerImpl) keep the full interface.@Single(binds=[...])) and Desktop (single<>) Koin modules; verified byKoinVerificationTest(Android) andDesktopKoinTestNew abstractions
NodeAddresssealed class — type-safe node addressing (Local, Broadcast, ByNum, ById)ContactKeyvalue class — consolidated contact-key parsing (was duplicated in 6 places), with a nullable-channel accessor that preserves legacy-DM semanticsCommandSender— replaces stringly-typed sender identificationDeviceVersion@JvmInline value class— zero-overhead version comparison with pre-compiled regexRequestTimer— shared request-timing utility extracted from NeighborInfo/Traceroute handlerseditSettings { }DSL (AdminEditScope) — replaces begin/commitEditSettings ceremony and removes destNum/packetId boilerplateBehavioral improvements
setFavorite/setIgnored(Boolean)skip the radio command when state is unchanged (replacing toggles; mirrors the SDK and fixes a latent un-favorite bug inSendMessageUseCase)start()eliminates a race conditionSDK alignment
The interface shapes deliberately mirror meshtastic-sdk (
AdminApi/TelemetryApi/RoutingApi, layered controllers) to ease a future migration. Two further SDK-alignment items —AdminResult<T>return types (R4) and aNodeIdvalue class (R5) — were investigated and deferred to the SDK migration: R4 would reimplement the SDK's admin RPC engine and reverse the intentional fire-and-forget design; R5 is a ~350-site change that overlapsNodeAddress.ByNum. Both come naturally with the SDK swap.Testing
RadioControllerImplTest,NodeAddressTest,CommandSenderImplTest,NeighborInfoHandlerImplTest,RequestTimerTest,ConnectionsViewModelTestatLeast(0)withexactly(0); added idempotency no-op assertions)Build
OptimizeNonSkippingGroupscompiler flagHttpClienttagDocumentation
core:service,core:repository,core:model); module dependency graphs regenerated