fix(ble): stop treating a missing GATT attribute as a dead link (gh-656) - #657
Conversation
characteristicNotFound and serviceNotFound were in _goneDeviceCodes, so any read of an attribute the peripheral does not expose made the transport emit disconnected and drain the queue. The Solo Barista (LSJ-001) routes to EurekaScale but has no 0x180F battery service, so the optional battery read at the end of onConnect dropped the scale microseconds after it connected. Both codes are ambiguous: a live peripheral returns them for an attribute that is not in its GATT database, a dead link returns them from a stale cache. They now log, rethrow the original UniversalBleException, and hand off to _probeAndDeclareIfDead, which asks the OS for the real link state before declaring the link dead. EurekaScale also gates the battery read on the discovered service list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RRSCR8Q7HWcHPwH8qvPap6
tadelv
left a comment
There was a problem hiding this comment.
Thanks for tracking this down — the root cause and general direction make sense. I’d like two changes before merging:
-
Keep
universal_bleexceptions behind the transport boundary.AGENTS.md/doc/AI_BLE_NOTES.mdrequire library-specific BLE errors to be wrapped in domain types at the transport boundary. The new_attributeMissingCodespath currently rethrows the originalUniversalBleException, and the PR explicitly changesread/write/subscribecallers to observe that third-party exception. Please introduce/use a transport-domain exception for “GATT attribute unavailable” (or equivalent) instead. This is particularly important for write paths: a stale-GATTcharacteristicNotFoundcan represent a dead link, while scale implementations are intentionally required to catchDeviceNotConnectedExceptionat their lowest-level write helper. The asynchronous link probe cannot change the exception already returned to the caller. -
Add regression coverage for the actual Eureka/Solo Barista path. Please add a fake-transport
EurekaScaletest showing that when discovery returns the Eureka service but not0x180F,onConnect()registers notifications, does not attempt the battery read, and remains connected. Also please exercise bothcharacteristicNotFoundandserviceNotFoundin the transport regression tests, since production behavior changes for both codes.
The service-presence guard itself and the probe-before-declaring-dead approach look appropriate; this is mainly about preserving the repository’s transport abstraction and covering the user-visible failure path directly.
Review feedback on #657: - `_handleGattError()` no longer rethrows the third-party `UniversalBleException` for `characteristicNotFound` / `serviceNotFound`. It throws `GattAttributeUnavailableException`, which extends `DeviceNotConnectedException` so the lowest-level scale write helpers keep catching it: for a write, a stale-GATT `characteristicNotFound` may still mean a dead link, and the asynchronous OS probe cannot change the exception already handed to the caller. - Transport regression tests now run over both `characteristicNotFound` and `serviceNotFound`, plus a case asserting the error is catchable as `DeviceNotConnectedException`. - New `test/unit/models/eureka_scale_test.dart` covers the Solo Barista path with a fake transport: discovery without 0x180F registers notifications, skips the battery read, and stays connected; discovery with 0x180F still reads the battery level. - `doc/AI_BLE_NOTES.md` documents the domain exception and why it subclasses `DeviceNotConnectedException`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013888xMCNLWCuQFW1RkgYka
|
Both points addressed in 691742d. 1.
class GattAttributeUnavailableException extends DeviceNotConnectedException {
final String operation;
final String path;
const GattAttributeUnavailableException({
required this.operation,
required this.path,
}) : super(DeviceKind.unknown);
}It extends No third-party BLE type crosses the transport boundary any more. The earlier PR-body claim that 2. Regression coverage.
Verification
|
The previous commits reformatted eight test files untouched by this fix. That formatting came from an older local Dart formatter and disagrees with the stable Flutter the CI format gate runs, so `dart format --set-exit-if-changed` over the PR diff failed. Restore those files to their base state; the fix's own files are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015J7EV8fcsjBNH15axyAiwk
Review feedback on #657: - `_handleGattError()` no longer rethrows the third-party `UniversalBleException` for `characteristicNotFound` / `serviceNotFound`. It throws `GattAttributeUnavailableException`, which extends `DeviceNotConnectedException` so the lowest-level scale write helpers keep catching it: for a write, a stale-GATT `characteristicNotFound` may still mean a dead link, and the asynchronous OS probe cannot change the exception already handed to the caller. - Transport regression tests now run over both `characteristicNotFound` and `serviceNotFound`, plus a case asserting the error is catchable as `DeviceNotConnectedException`. - New `test/unit/models/eureka_scale_test.dart` covers the Solo Barista path with a fake transport: discovery without 0x180F registers notifications, skips the battery read, and stays connected; discovery with 0x180F still reads the battery level. - `doc/AI_BLE_NOTES.md` documents the domain exception and why it subclasses `DeviceNotConnectedException`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013888xMCNLWCuQFW1RkgYka
Summary
What changed, and why?
UniversalBleTransport._handleGattError()treatedcharacteristicNotFoundandserviceNotFoundas gone-device codes: it emittedConnectionState.disconnected, drained the queue withdeviceDisconnected, and threwDeviceNotConnectedException. Both codes are ambiguous — a live peripheral returns them when the attribute is simply not in its GATT database, and a dead link returns them from a stale cache.The Solo Barista (LSJ-001) matches to
EurekaScale, which endsonConnect()with an optional battery read of0x180F/0x2A19. That scale has no battery service, so the read failed and the transport dropped the link microseconds after the scale connected. The impl-levelcatch (_)around_readBattery()was useless — the transport had already emitteddisconnected. From a user log:Both codes now log, throw the domain
GattAttributeUnavailableException, and hand off to the existing_probeAndDeclareIfDead(), which asks the OS for the real link state and declares the link dead only when the OS agrees. Real-disconnect detection is preserved through that probe.GattAttributeUnavailableException(new, inlib/src/models/errors.dart) extendsDeviceNotConnectedException, so nouniversal_bletype crosses the transport boundary and the lowest-level scale write helpers that already catchDeviceNotConnectedExceptionkeep swallowing it. That matters for writes: a stale-GATTcharacteristicNotFoundmay still mean a dead link, and the asynchronous OS probe cannot change the exception the caller already received.EurekaScaleadditionally gates the optional battery read onbatteryService.matchesAny(services), using thediscoverServices()result it already holds.doc/AI_BLE_NOTES.md"Gone-Device Error Handling" updated with the split and the Solo Barista case.Linked Issue
Fixes #656
Verification
How did you verify the change? Include relevant tests and any manual or hardware testing.
test/services/ble/universal_ble_transport_mtu_test.dart(reusing its fakeUniversalBlePlatform, extended with areadErrorCodehook), run over bothcharacteristicNotFoundandserviceNotFound:<code> on a live link does not disconnect— fails on the old code, which emitteddisconnectedsynchronously.<code> after the link died reports disconnected— guards the probe path so a real drop is still detected.attribute-missing errors are catchable as not-connected— pins the property the scale write helpers depend on.test/unit/models/eureka_scale_test.dartcovers the Solo Barista path with a fake transport: discovery without0x180Fregisters notifications, skips the battery read, and staysconnected; discovery with0x180Fstill reads the battery level.flutter test test/unit/models/eureka_scale_test.dart test/services/ble/→+16: All tests passed!flutter analyze→No issues found!flutter test→ 3215 passed, 4 failures, all intest/webui_support/webui_token_injection_test.dart(getWifiIPfallback cases). Confirmed pre-existing onorigin/main; unrelated to this PR.Impact
Note any user-visible behavior, compatibility, migration, API/spec, documentation, or security impact. Write
Noneif there is none.read/write/subscribenow surfaceGattAttributeUnavailableExceptionfor these two codes. It is aDeviceNotConnectedExceptionsubtype, so every existing catch site — scale_safeWrite()helpers, controller-level swallows,isBenignFrameworkError(), the telemetry forwarder filter — behaves exactly as before. The link-death outcome for a genuinely dead link is unchanged, just asynchronous via the OS probe.doc/AI_BLE_NOTES.mdupdated.Contributor Responsibility
AI-assisted development is allowed. The submitter remains responsible for the submitted work.
🤖 Generated with Claude Code
https://claude.ai/code/session_01RRSCR8Q7HWcHPwH8qvPap6