Web Bluetooth: Expose maxWriteWithoutResponseSize [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 6 comments![]( "Open in Gerrit")
aligned this with the spec PR, the attribute is cached in the renderer, seeded from the live platform MTU when characteristics are enumerated, and updated by MTU-change events on the supported platforms that can report changes. macOS/CoreBluetooth has no change event, but the value is settled before characteristic discovery. tried it without caching but that leads to a synchronous blocking mojo call.
mtu_ = static_cast<uint16_t>(mtu);Helmut JanuschkaFire the `DeviceMTUChanged` event?
Done
if (mtu > std::numeric_limits<uint16_t>::max()) {
return std::nullopt;
}
return static_cast<uint16_t>(mtu);Helmut JanuschkaUse a saturating cast so that this just turns into MAX_UINT16 if it's too large. Or does this really even need to be sent as a uint16 over Mojo?
Done. i think it should stay uint16, esp, android they all handle mtu as uint16, and ideas how mojo could be changed?
RegisterProperty(kMTUProperty, &mtu);Helmut JanuschkaWhy isn't there a constant in the `bluetooth_gatt_characteristic` namespace for this?
Done
if (!GetGatt()->connected()) {Helmut JanuschkaThis should probably still be update the cached value but maybe not fire an event if disconnected.
Done
Related details
- Kinuko Yasuda
- Reilly Grant
- warith Akbar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant voted and added 1 comment![]( "Open in Gerrit")
Votes added by Reilly Grant
| Code-Review | +1 |
1 comment
[CallWith=ScriptState, RaisesException] Promise<unsigned short> getMTU();Helmut JanuschkaThis change needs a corresponding specification PR and Intent to Ship.
In terms of API shape, I suspect we could add this information to `blink.mojom.WebBluetoothRemoteGATTCharacteristic` so that it is available synchronously, depending on when in the device discovering and connection process the value is known.
Reilly Grantspec pr: https://github.com/WebBluetoothCG/web-bluetooth/pull/672
feature entry (i'll pursue getting it through the stages; after the spec PR lands): https://chromestatus.com/feature/5177569273053184also added a default-off flag, and we can flip it after the i2s
> In terms of API shape, I suspect we could add this information to blink.mojom.WebBluetoothRemoteGATTCharacteristic so that it is available synchronously...I looked into this across the four backends and I think the async shape is actually the right one, for a correctness reason rather than just plumbing convenience:
The ATT MTU is per-connection, mutable state, and on most platforms it isn't reliably known at characteristic-discovery time.
Helmut JanuschkaAll the implementations I see in this CL read the value synchronously. It seems like we could at least initialize the characteristic with a value, and then hook up an event to propagate the change to the renderer when it changes.
Does that work to implement the currently proposed API shape in your PR?
Reilly Grantyep, done exactly that. initial maxWriteWithoutResponseSize (att_mtu - 3) carried in the mojom struct so it reads sync, plus a `maxwritewithoutresponsesizechanged` event wired through WebBluetoothCharacteristicClient (BlueZ DeviceMTUChanged fires it). should now align with the spec PR.
The specification change is ready to land once we're sure there aren't any implementation issues. Please send out the "Intent to Prototype" based on your ChromeStatus entry.
Related details
- Helmut Januschka
- Kinuko Yasuda
- warith Akbar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant voted and added 1 comment![]( "Open in Gerrit")
Votes added by Reilly Grant
| Code-Review | +0 |
1 comment
Sorry, that was a mis-click. Not done reviewing.
Related details
- Helmut Januschka
- Kinuko Yasuda
- warith Akbar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant added 5 comments![]( "Open in Gerrit")
return static_cast<uint32_t>(effective_mtu) - kAttWriteCommandHeaderSize;I don't think an explicit cast is necessary here.
// Notify characteristics on this device that have an active client (i.e. a
// notify session). MTU is per-connection, so every such characteristic gets
// the same updated value.A notify session shouldn't be required to get updates to device MTU. This is an artifact of the fact that the event is sent on the `WebBluetoothCharacteristicClient` pipe which is only present when notifications are enabled instead of the `WebBluetoothServerClient` pipe which is always present.
maximumWriteValueLengthForType:CBCharacteristicWriteWithoutResponse];On macOS, consider checking this value on the completion of a read or write to see if it has changed so we can inform the renderer.
return properties->mtu.value();Hook up the event for when this property changes. To keep this change small, feel free to implement one platform at a time, keeping only one platform (e.g. Windows) in the main patch.
constexpr char kMTUProperty[] = "MTU";This should be added to `third_party/cros_system_api/dbus/bluetooth/dbus-constants.h`.
Helmut Januschka (Gerrit)
Helmut Januschka voted and added 7 comments![]( "Open in Gerrit")
Votes added by Helmut Januschka
| Commit-Queue | +1 |
7 comments
done, took a while to build and test on mac, windows, linux (that was the easy one)
thanks for your time, from my pov, this matches the spec PR, but pelase let me know if you want me to address anything.
about the flag, should we put it into stable, and keep it as killswitch?
why i am asking, i am curious if this requires intent-to-ship or not.
return static_cast<uint32_t>(effective_mtu) - kAttWriteCommandHeaderSize;I don't think an explicit cast is necessary here.
Done
// Notify characteristics on this device that have an active client (i.e. a
// notify session). MTU is per-connection, so every such characteristic gets
// the same updated value.A notify session shouldn't be required to get updates to device MTU. This is an artifact of the fact that the event is sent on the `WebBluetoothCharacteristicClient` pipe which is only present when notifications are enabled instead of the `WebBluetoothServerClient` pipe which is always present.
Done. Moved MaxWriteWithoutResponseSizeChanged to the WebBluetoothServerClient pipe so no notify session is needed. DeviceMTUChanged now forwards through FrameConnectedBluetoothDevices to the connection's server client, and on the renderer side BluetoothRemoteGATTServer fans the update out to all live characteristics via BluetoothDevice/BluetoothAttributeInstanceMap
maximumWriteValueLengthForType:CBCharacteristicWriteWithoutResponse];On macOS, consider checking this value on the completion of a read or write to see if it has changed so we can inform the renderer.
Done.
if (mtu > std::numeric_limits<uint16_t>::max()) {
return std::nullopt;
}
return static_cast<uint16_t>(mtu);Helmut JanuschkaUse a saturating cast so that this just turns into MAX_UINT16 if it's too large. Or does this really even need to be sent as a uint16 over Mojo?
Done. i think it should stay uint16, esp, android they all handle mtu as uint16, and ideas how mojo could be changed?
Done
Hook up the event for when this property changes. To keep this change small, feel free to implement one platform at a time, keeping only one platform (e.g. Windows) in the main patch.
Done. BluetoothRemoteGattServiceBlueZ::GattCharacteristicPropertyChanged now handles the characteristic MTU property and fires DeviceMTUChanged. While hooking this up I consolidated the identical per-platform NotifyDeviceMTUChanged helpers (Android/WinRT/Apple from this CL, plus the pre-existing BlueZ one) into a single BluetoothAdapter::NotifyDeviceMTUChanged in the base class.
constexpr char kMTUProperty[] = "MTU";This should be added to `third_party/cros_system_api/dbus/bluetooth/dbus-constants.h`.
https://chromium-review.googlesource.com/c/chromiumos/platform2/+/8224745 - will refactor once the CL lands/rolls
Related details
- Kinuko Yasuda
- Reilly Grant
- warith Akbar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |