Make BLE Transport an actor to fix background discovery crashes #1554
Conversation
update the translations
* Don't add new BLE devices to the device list in the backgournd * Bump version * Update Meshtastic/MeshtasticApp.swift Co-authored-by: Copilot <[email protected]> * Update Meshtastic/MeshtasticApp.swift Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR converts BLETransport from a class to an actor to address thread safety issues causing crashes during background discovery. The actor model ensures synchronized access to shared state across concurrent contexts.
Key changes:
- Converted
BLETransportfromclasstoactorfor thread-safe state management - Added
async/awaitthroughout the call chain to properly handle actor isolation - Wrapped synchronous delegate callbacks in
Taskblocks to bridge non-isolated contexts
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| protobufs | Updated subproject commit reference |
| BLETransport.swift | Converted to actor, added async/await for isolation-safe access, wrapped delegate callbacks in Tasks |
| BLEConnection.swift | Updated disconnect methods to be async and await transport calls |
| Transport.swift | Updated protocol to support async status/discovery for actor conformance |
| Connection.swift | Made disconnect async in protocol |
| AccessoryManager+Discovery.swift | Added await for async discoverDevices call |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await setupCleanupTask() | ||
| } | ||
| cont.onTermination = { _ in | ||
| Logger.transport.error("🛜 [BLE] Discovery event stream has been canecelled.") |
There was a problem hiding this comment.
Corrected spelling of 'canecelled' to 'cancelled'.
| let connectTask = Task { @MainActor in | ||
| try await AccessoryManager.shared.connect(to: device, withConnection: restoredConnection, wantConfig: true, wantDatabase: true, versionCheck: true) | ||
| } |
There was a problem hiding this comment.
The error handling pattern has been improved by awaiting the task result and catching errors explicitly. However, consider whether the restoration flow should continue or abort when connection fails, as the current implementation logs the error but proceeds to set restoreInProgress = false in both success and failure paths.
Meshtastic/Accessory/Transports/Bluetooth Low Energy/BLETransport.swift
Outdated
Show resolved
Hide resolved
* Initial plan * Remove nested Task block in tapback handler Co-authored-by: garthvh <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: garthvh <[email protected]>
…ort.swift Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| protocol Transport { | ||
| var type: TransportType { get } | ||
| var status: TransportStatus { get } | ||
| var status: TransportStatus { get async } |
There was a problem hiding this comment.
The protocol declares status as get async, but the conforming implementations (TCPTransport and SerialTransport) still have synchronous var status: TransportStatus properties. Only BLETransport, being an actor, might naturally support async access, but TCPTransport and SerialTransport are classes and have not been updated to provide async getters. This will cause protocol conformance errors.
| var status: TransportStatus { get async } | |
| var status: TransportStatus { get } |
| func send(_ data: ToRadio) async throws | ||
| func connect() async throws -> AsyncStream<ConnectionEvent> | ||
| func disconnect(withError: Error?, shouldReconnect: Bool) throws | ||
| func disconnect(withError: Error?, shouldReconnect: Bool) async throws |
There was a problem hiding this comment.
The protocol now declares disconnect as async throws, but TCPConnection and SerialConnection implementations still have synchronous func disconnect(withError:shouldReconnect:) throws methods. However, these implementations are internally calling disconnect with await, which will cause compilation errors since their method is not declared as async. BLEConnection is correctly implemented with async, but the other two implementations need to be updated.
|
|
||
| // Discovers devices asynchronously. For ongoing scans (e.g., BLE), this can yield via AsyncStream. | ||
| func discoverDevices() -> AsyncStream<DiscoveryEvent> | ||
| func discoverDevices() async -> AsyncStream<DiscoveryEvent> |
There was a problem hiding this comment.
The protocol declares discoverDevices() as async, but none of the conforming implementations (BLETransport, TCPTransport, SerialTransport) have been updated to match this signature. They all still declare func discoverDevices() -> AsyncStream<DiscoveryEvent> without the async keyword. This creates a protocol conformance mismatch that will cause compilation errors or runtime issues.
| func discoverDevices() async -> AsyncStream<DiscoveryEvent> | |
| func discoverDevices() -> AsyncStream<DiscoveryEvent> |
No description provided.