Skip to content

Conversation

@fnde-ot
Copy link
Contributor

@fnde-ot fnde-ot commented Nov 21, 2019

When set, the BT_GATT_SUBSCRIBE_FLAG_NO_RESUB flag indicates that the
subscription should not be renewed when reconnecting with the server.

This is useful if the application layer knows that the GATT server
persists subscription information.

Signed-off-by: François Delawarde [email protected]

Copy link
Contributor

@Vudentz Vudentz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer that this is configurable via Kconfig which should probably default to no if we want to keep current behavior, or perhaps default to yes instead since this was done to deal with misbehaving devices.

@fnde-ot
Copy link
Contributor Author

fnde-ot commented Nov 21, 2019

I would prefer that this is configurable via Kconfig which should probably default to no if we want to keep current behavior, or perhaps default to yes instead since this was done to deal with misbehaving devices.

The current way of resubscribing after reconnection is a workaround for the gatt servers that do not persist subscription information. I preferred the flag approach as we may need to deal with both types of gatt servers (those that persist and those that do not).

The application can then give the appropriate flag to bt_gatt_subscribe given the type of peer device it connects with.

@joerchan
Copy link
Contributor

Do we know when this workaround is needed? This seems to expose a workaround as a feature at the API level. I think this should have a better description of what this option is if the application should have the responsibility to apply the workaround.

@fnde-ot
Copy link
Contributor Author

fnde-ot commented Nov 22, 2019

Do we know when this workaround is needed? This seems to expose a workaround as a feature at the API level. I think this should have a better description of what this option is if the application should have the responsibility to apply the workaround.

The previous workaround is needed if we know the device we subscribe to (gatt server) does not save/persist subscription data. In that case we should resubscribe every time we reconnect to it just in case.

This flag adds the option to not resubscribe on reconnect.

@joerchan
Copy link
Contributor

The previous workaround is needed if we know the device we subscribe to (gatt server) does not save/persist subscription data. In that case we should resubscribe every time we reconnect to it just in case.

This flag adds the option to not resubscribe on reconnect.

That I understand. It's not what I'm wondering about.

With this PR you are giving the application the option to not resubscribe on reconnect dynamically.
So in what way can the application at runtime decide if it wants to apply this workaround or not. How can the application know if it is dealing with a GATT server that persists or not?

@fnde-ot
Copy link
Contributor Author

fnde-ot commented Nov 25, 2019

With this PR you are giving the application the option to not resubscribe on reconnect dynamically.
So in what way can the application at runtime decide if it wants to apply this workaround or not. How can the application know if it is dealing with a GATT server that persists or not?

In our case the application can query the device type from GATT characteristics to know if it persists subscriptions.

Also if one wants to write a test to verify that persistence actually works (in that case the test application knows in advance that the peer device should persist), it would be nice to have the option to not resubscribe immediately when reconnecting.

@joerchan
Copy link
Contributor

@fnde-ot I have no objection to adding this flag. I just think it should be explained in the header file that it is to disable the workaround. That can give the application a better overview of why this would be needed.

When set, the BT_GATT_SUBSCRIBE_FLAG_NO_RESUB flag indicates that the
subscription should not be renewed when reconnecting with the server.

This is useful if the application layer knows that the GATT server
persists subscription information.

Signed-off-by: François Delawarde <[email protected]>
@fnde-ot
Copy link
Contributor Author

fnde-ot commented Nov 27, 2019

@fnde-ot I have no objection to adding this flag. I just think it should be explained in the header file that it is to disable the workaround. That can give the application a better overview of why this would be needed.

Of course! Latest push improves the description in the header file.

@fnde-ot fnde-ot force-pushed the ccc_client_no_resub branch from 006a04f to fd45b04 Compare November 27, 2019 10:08
Copy link
Contributor

@joerchan joerchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. That is a very clear description.

@Vudentz I'd prefer not to change the default behaviour

@fnde-ot fnde-ot requested a review from Vudentz November 27, 2019 10:19
Copy link
Contributor

@Vudentz Vudentz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Id would have it the other way around but there could be code depending on the resubscription logic.

@aescolar aescolar added this to the v2.2.0 milestone Dec 9, 2019
@jhedberg jhedberg merged commit db106a2 into zephyrproject-rtos:master Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Changes to public APIs area: Bluetooth

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants