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

bluetooth: host: smp: Add bondable flag overlay per connection #59992

Merged

Conversation

mkapala-nordic
Copy link
Contributor

The current API for changing the bondable mode uses the global flag. With Zephyr support for multiple Bluetooth identities, the API for changing the bondable mode should be more fine-grained. The bondable requirements of one identity should not have an impact on another identity which can have a different set of requirements. This change introduces function to overlay bondable flag per connection.

Fixes: #57070

@mkapala-nordic
Copy link
Contributor Author

CC: @kapi-no @MarekPieta

Comment on lines 1093 to 1094
* If the bonding flag is not overlaid, the value will depend on
* BT_BONDABLE Kconfig setting.
Copy link
Collaborator

Choose a reason for hiding this comment

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

...depend on global configuration which is set using bt_set_bondable.
The default value of the global configuration is defined using CONFIG_BT_BONDABLE Kconfig option.

@@ -288,6 +291,11 @@ static K_SEM_DEFINE(sc_local_pkey_ready, 0, 1);
*/
#define BT_SMP_AUTH_CB_UNINITIALIZED ((atomic_ptr_val_t)bt_smp_pool)

/* Value used to mark that bondable flag is not initialized.
Copy link
Collaborator

Choose a reason for hiding this comment

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

per-connection bondable flag?

Copy link
Collaborator

@kapi-no kapi-no left a comment

Choose a reason for hiding this comment

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

Looking good (apart from small doc improvements suggested by @MarekPieta)

@kapi-no
Copy link
Collaborator

kapi-no commented Jul 4, 2023

The naming convention and the implementation for this new bt_bondable_overlay API are similar to the bt_smp_auth_cb_overlay API function.

@mkapala-nordic
Copy link
Contributor Author

Adressed @MarekPieta comments.

* @param conn Connection object.
* @param enable Value allowing/disallowing to be bondable.
*/
int bt_bondable_overlay(struct bt_conn *conn, bool enable);
Copy link
Member

Choose a reason for hiding this comment

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

I feel like bt_conn_set_bondable() would be the more appropriate name. I know we have bt_smp_auth_cb_overlay() as an internal API, but public APIs operating on a struct bt_conn * should preferably be prefixed with bt_conn_.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, name changed.

jhedberg
jhedberg previously approved these changes Jul 5, 2023
Copy link
Collaborator

@jori-nordic jori-nordic left a comment

Choose a reason for hiding this comment

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

could you add a bsim test checking the new functionality?
and also mark this new API as experimental (in case we later find some better way to achieve the same thing)

@mkapala-nordic
Copy link
Contributor Author

Marked as experimental + added bsim test as per @jori-nordic request.

Comment on lines 607 to 608
Enable support for setting/clearing the bonding flag on a
per-connection basis.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Enable support for setting/clearing the bonding flag on a
per-connection basis.
Enable support for the bt_conn_set_bondable API function that is
used to set/clear the bonding flag on a per-connection basis.

Copy link
Collaborator

@MarekPieta MarekPieta left a comment

Choose a reason for hiding this comment

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

Change in host looks good


SET_FLAG(flag_is_connected);

if (bt_conn_set_bondable(conn, bondable)) {
Copy link
Collaborator

@jori-nordic jori-nordic Jul 6, 2023

Choose a reason for hiding this comment

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

could you add another round of bonding without calling bt_conn_set_bondable?
like have a bondable by default device, try bonding and see that it works and then continue with the two test rounds you already have.
This is to check the default way of bonding is not broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added such case.

jhedberg
jhedberg previously approved these changes Jul 6, 2023
The current API for changing the bondable mode uses the global flag.
With Zephyr support for multiple Bluetooth identities, the API for
changing the bondable mode should be more fine-grained.
The bondable requirements of one identity should not have an impact on
another identity which can have a different set of requirements.
This change introduces function to overlay bondable flag per
connection.

Signed-off-by: Mateusz Kapala <mateusz.kapala@nordicsemi.no>
Added test for the bt_conn_set_bondable API function.
Check if we can pair without setting the bonding flag on the
per-connection basis if the device was already bonded on the
other identity.

Signed-off-by: Mateusz Kapala <mateusz.kapala@nordicsemi.no>
@carlescufi
Copy link
Member

@jhedberg would you mind taking another look?

@jhedberg jhedberg merged commit 8e5673c into zephyrproject-rtos:main Jul 7, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bluetooth: Host: SMP: set bondable mode per identity or connection object
8 participants