Fix notification subscription state reporting - #7
Merged
Conversation
This header writes uint32 length prefixes for strings and uint32 count prefixes for arrays, vectors, and maps. Other implementations of the same binary encoding use uint64 throughout. Reading a uint64 prefix consumes the first four bytes of string data as the high half of the length and produces an absurd value, so the symptom appears far from the cause. Record the widths and that symptom so a codec ported in either direction has its prefix width adjusted first. Signed-off-by: Joel Winarske <[email protected]>
Owner
Author
|
cc @jaydon2020 |
A successful method reply does not prove the notification state changed. BlueZ scopes a notification session to the D-Bus client that requested it, so a client that does not outlive the call sees StartNotify succeed while Notifying stays false and no PropertiesChanged ever arrives. That presents as the peripheral having stopped responding. Read the Notifying property after the reply and post an error when it does not match the requested state. The read is async because the reply handler runs on the event loop thread, where a synchronous D-Bus call can deadlock. When the read itself fails, report success: an unreadable property is not evidence that the operation failed, and failing there would break working callers. Register the PropertiesChanged listener before calling StartNotify. BlueZ emits Notifying=true while servicing the call, so a listener attached in the reply handler is too late to see it. Roll the subscription back if StartNotify fails. On the stop path, defer the unsubscribe until the property read resolves, so a Notifying=false that arrives after the reply is still observed. The reply handler takes its arguments by value because sdbus::apply invokes it that way; reference parameters do not bind and the template fails to instantiate. Signed-off-by: Joel Winarske <[email protected]>
The characteristic PropertiesChanged handler returned early unless the update carried Value, so BlueZGattCharacteristic.notifying kept its discovery-time value even while notifications were arriving. Callers checking that getter were told the wrong thing with no way to detect it. Add GattCharChangedBit and a changedMask field to BlueZGattCharProps. The field is appended last in glz::meta and read last in codec.dart, so the layout of the preceding fields is unchanged; the two must move together. Full snapshots set the mask to ~0. Guard the variant extraction in the signal handler. Variant::get throws when the peer sends an unexpected type, and an exception escaping the handler takes down the D-Bus event loop thread along with every other subscription on it. Drop the malformed update instead. Route 0x03 through BlueZGattCharacteristic.mergeChanged, which updates the cached properties and emits on the value stream only when the update actually carried a value. Without the mask check, a Notifying-only change would push an empty payload to listeners. Add example/notify_regression.dart, which asserts the notifying transitions around subscribing rather than only that bytes arrive. A value-only check passes in both the broken and fixed states. Signed-off-by: Joel Winarske <[email protected]>
The package description, library doc comment, changelog entry, codec header, and glaze_meta.h header referred to an internal channel taxonomy by name. Describe what the code does instead: zero-copy delivery through Dart_PostCObject_DL, and a binary struct encoding for the payloads. Replace the device address in the notify_regression example usage with a documentation placeholder rather than a real adapter address. Signed-off-by: Joel Winarske <[email protected]>
jwinarske
force-pushed
the
gatt-notify-state
branch
from
July 22, 2026 20:53
eccefa0 to
6256745
Compare
Merged
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.
BlueZGattCharacteristic.notifyingreported the wrong value while notifications were arriving. Two independent bugs produced it, and neither is visible to a test that only checks whether bytes show up.Notifying and MTU were never forwarded
The characteristic
PropertiesChangedhandler returned early unless the update carriedValue, sonotifyingkept its discovery-time value for the lifetime of the characteristic.BlueZGattCharPropsgains achangedMaskfield and aGattCharChangedBitenum. The field is appended last inglz::metaand read last incodec.dart, so the layout of the preceding fields is unchanged. Those two must be edited together — a mismatch is silent corruption, not a compile error. The frozen vectors incodec_test.dartcaught the layout change immediately, which is what they exist for.0x03now routes throughBlueZGattCharacteristic.mergeChanged, which updates cached properties and emits on the value stream only when the update actually carried a value. Without that check aNotifying-only change would push an empty payload to listeners.The subscription was registered too late
subscribe_char_notifyran inside theStartNotifyreply handler, but BlueZemits
Notifying=truewhile servicing the call. The listener was attached after the signal had already been delivered. Fixing the first bug alone changednothing, which is how this one surfaced.
The subscription is now registered before the call and rolled back if
StartNotifyfails. On the stop path the unsubscribe is deferred until the property read resolves, so aNotifying=falsearriving after the reply is still observed.Post-condition check on Start/StopNotify
A successful method reply does not prove the notification state changed. BlueZ
scopes a notification session to the D-Bus client that requested it, so a client that does not outlive the call sees
StartNotifysucceed while Notifyingstays false and noPropertiesChanged` ever arrives. That presents as the peripheral having stopped responding.The property is now read back after the reply, asynchronously — the reply handler runs on the event loop thread, where a synchronous D-Bus call can deadlock. If the read itself fails it reports success: an unreadable property is not evidence the operation failed, and failing there would break working callers.
Note the read must use the live property. Checking the cached value would fail
every healthy subscription, since the cache is exactly what was broken.
Other changes
Variant extraction in the signal handler is guarded.
Variant::getthrows when a peer sends an unexpected type, and an exception escaping the handler takesdown the D-Bus event loop thread along with every other subscription on it.
glaze_meta.hrecords that its length and count prefixes are uint32 while other implementations of the same encoding use uint64. Reading a uint64 prefixconsumes the first four bytes of string data as the high half of the length, producing a value far from anything recognizable.
Descriptions in the README, pubspec, changelog, library doc comment, and two headers now describe the delivery mechanism directly rather than naming an internal channel taxonomy.
Testing
dart analyzecleanparameters with a
NOLINT:sdbus::applyinvokes handlers by value, soreference parameters do not bind and the template fails to instantiate
example/notify_regression.dart, which asserts thenotifyingtransitionsrather than only that bytes arrive:
Against the pre-fix build,
notifying is true after subscribingfails whileevery other check passes.