WebBluetoothCG / web-bluetooth

Bluetooth support for the Web.
http://www.w3.org/community/web-bluetooth/
Other
1.38k stars 187 forks source link

Generate 'characteristicvaluechanged' events without waiting for a CCCD update #514

Open reillyeon opened 4 years ago

reillyeon commented 4 years ago

In this StackOverflow question a developer explains that they are trying to use Web Bluetooth to communicate with a device that appears to generate a number of Characteristic Value Notifications immediately after the GATT connection has been established. If they perform the following sequence of calls there is a high probability that some of the notifications will be lost.

const device = await navigator.bluetooth.requestDevice({ filters: [{ services: [serviceUuid] }] });
const server = await device.gatt.connect();
const service = await server.getPrimaryService(serviceUuid);
const characteristic = await service.getCharacteristic(characteristicUuid);
characteristic.addEventListener('characteristicvaluechanged', () => { ... });
await characteristic.startNotifications();

The issue is that each of these is an asynchronous operation which takes some time to complete and the startNotifications() call in particular depends on being able to update the Client Characteristic Configuration descriptor in order to enable notifications. While the device is not complying with the Bluetooth specification by sending notifications before requested the reality is that other Bluetooth APIs pass on these unsolicited notifications.

Properly fixing this seems to require two changes. First, the active notification context set must be removed and the steps for responding to notifications and indications updated to deliver events to all active Bluetooth globals regardless of whether startNotifications() or stopNotifications() have been called. Second, the bubbling of events through the Bluetooth tree needs to be stabilized so that the code above can be rewritten to the following.

const device = await navigator.bluetooth.requestDevice({ filters: [{ services: [serviceUuid] }] });
characteristic.addEventListener('characteristicvaluechanged', () => { ... });
const server = await device.gatt.connect();
const service = await server.getPrimaryService(serviceUuid);
const characteristic = await service.getCharacteristic(characteristicUuid);
await characteristic.startNotifications();

In this example an attempt to update the CCCD is made by calling startNotifications() but an event listener is added to the BluetoothDevice object to catch all characteristic value change notifications that are delivered before that process is complete.

jyasskin commented 4 years ago

👍 for having this API match reality. Do you think it'd make sense to ask developers to explicitly opt into unsolicited notifications and indications? For example:

const device = await navigator.bluetooth.requestDevice({ filters: [{ services: [serviceUuid] }] });
device.addEventListener('characteristicvaluechanged', () => { ... });
device.gatt.deliverUnsolicitedNotifications = true;
const server = await device.gatt.connect();
const service = await server.getPrimaryService(serviceUuid);
const characteristic = await service.getCharacteristic(characteristicUuid);
await characteristic.startNotifications();

Without that, I worry a bit that a device could attack a page by sending unexpected notifications ... maybe if the device runs multiple apps each constrained to their own service...

reillyeon commented 4 years ago

That seems like a reasonable mitigation to put in place. What do you think about a property on the BluetoothRemoteGattServer vs. an option passed to connect()? The advantage of the latter is that for platforms where that is additional setup required to receive notifications and indications from all characteristics it could only be performed if the page explicitly requests it. For example, in the WinRT API a handler for the ValueChanged event must be added to each IGattCharacteristic instance as they are discovered.

jyasskin commented 4 years ago

I don't have a strong opinion. Putting it in .connect() makes it impossible to change later in the lifetime of the connection, which is probably easier implementation-wise, and I don't immediately see any use cases that couldn't handle that.

dlech commented 3 years ago

If an event handler is attached after a device is connected, then there is a race condition where notifications could be missed, so +1 for passing it to the connect() method so that people won't accidentally write racy code. Although it sounds like there could still be a potential for a race condition on WinRT if the ValueChanged event handler can't be connected until after the device has connected.

tthiery commented 3 years ago

@reillyeon @jyasskin Has this ended up being on someone's agenda 😀. I do not know exactly how this WG works in relation to the browser vendors.