Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
reillyeon opened this issue Sep 9, 2020 · 5 comments

Comments

@reillyeon
Copy link
Contributor

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
Copy link
Member

👍 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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor

dlech commented Mar 12, 2021

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
Copy link

tthiery commented May 12, 2021

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants