google / bumble

Apache License 2.0
266 stars 78 forks source link

Subscribing to characteristic with both notify and indicate returns ATT error #67

Closed mogenson closed 2 years ago

mogenson commented 2 years ago

Some BT stacks do not handle setting the notify and indicate CCCD bits simultaneously when subscribing to a characteristic.

For example, Cordio returns an ATT error of 0x80. In the Cordio stack this is defined as ATT_ERR_VALUE_RANGE, and in the Bumble stack this is defined as WRITE_REQUIRES_AUTHORIZATION.

https://github.com/packetcraft-inc/stacks/blob/3656312d6b73e2a2c1c8b33ee0385bc199dd97e6/ble-host/sources/stack/att/atts_ccc.c#L231

Should the GATT client subscribe() method prefer notify over indicate or indicate over notify? Should we let the caller choose with a separate subscribe with notify and subscribe with indicate method like the notify_subscribers()/indicate_subscribers() methods? Maybe we can have a keyword argument to subscribe that's an enum of values notify, indicate, or either.

Also, is it invalid to have a characteristic that supports both notify and indicate properties?

https://github.com/google/bumble/blob/main/bumble/gatt_client.py#L562

mogenson commented 2 years ago

@AlanRosenthal @Arrowbox FYI

barbibulle commented 2 years ago

The 0x80 error from Cordio shouldn't be seen in an ATT error returned over the air. From the code:

/** \name ATT Application Error Codes
 * These codes may be sent to application but are not present in any ATT PDU.
 */
/**@{*/
#define ATT_ERR_VALUE_RANGE           0x80      /*!< \brief Value out of range */

It is legal for a characteristic to support both indications and notifications, and it is up to the client to decide what flags to write to the CCCD. It would be odd, however, for a client to ask for both. The iOS API says: "...If the specified characteristic’s configuration allows both notifications and indications, calling this method enables notifications only...", so on that platform the client doesn't have a choice.

I think it would make sense for the subscribe() method to take an optional preference, which would default to 'prefer notifications'. It should be a preference rather than a hard choice, so that the subscriber would end up subscribing regardless of the server config. A client that really wants just one mode and not the other can always inspect the properties and decide.

mogenson commented 2 years ago

Huh strange, I was seeing error 0x80 in the write response:

│[2022-11-09 15:07:32,072][DEBUG] GATT Response to client: [0x0040] ATT_ERROR_RESPONSE:                                                        
│  request_opcode_in_error:   ATT_WRITE_REQUEST
│  attribute_handle_in_error: 0x0073
│  error_code:                [0x80]

It looks like Cordio may take the returned status for the cccCallback and directly send it back as an ATT error response.

That seems like a Cordio specific quirk. There's nothing to be done from the Bumble side.

I'll write up a PR that adds mode preference option to the subscribe() method.