| Message ID | 20230725084431.640332-6-simon.mikuda@streamunlimited.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Various fixes and refactors | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | warning | WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line) #83: even when we have connection to LE bearer only. In these situation we should /github/workspace/src/src/13326123.patch total: 0 errors, 1 warnings, 46 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13326123.patch has style problems, please review. NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
Hi Simon, On Tue, Jul 25, 2023 at 1:56 AM Simon Mikuda <simon.mikuda@streamunlimited.com> wrote: > > We'll prefer to pair and discover services on connected bearer first. > > There was a problem with pairing, that select_conn_bearer returned BR/EDR > even when we have connection to LE bearer only. In these situation we should > pair over connected bearer, since connection to another bearer can fail. > > Similar problem with discovery that after connection on LE bearer discovery > was requested on BR/EDR bearer which can cause connection error (e.g. Page > Timeout). > --- > src/device.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/src/device.c b/src/device.c > index 6f28e261e..446e978ee 100644 > --- a/src/device.c > +++ b/src/device.c > @@ -2504,6 +2504,16 @@ static uint8_t select_conn_bearer(struct btd_device *dev) > return dev->bdaddr_type; > } > > +static uint8_t select_active_bearer(struct btd_device *dev) > +{ > + if (dev->bredr_state.connected) > + return BDADDR_BREDR; > + else if (dev->le_state.connected) > + return dev->bdaddr_type == BDADDR_BREDR > + ? BDADDR_LE_PUBLIC : dev->bdaddr_type; The code above assumes BR/EDR has the priority in case both are connected, perhaps we should have a clear policy in case both are connected. > + return select_conn_bearer(dev); > +} > + > static DBusMessage *dev_connect(DBusConnection *conn, DBusMessage *msg, > void *user_data) > { > @@ -3018,7 +3028,7 @@ static DBusMessage *pair_device(DBusConnection *conn, DBusMessage *msg, > else if (device->le_state.bonded) > bdaddr_type = BDADDR_BREDR; > else > - bdaddr_type = select_conn_bearer(device); > + bdaddr_type = select_active_bearer(device); > > state = get_state(device, bdaddr_type); > > @@ -3055,7 +3065,7 @@ static DBusMessage *pair_device(DBusConnection *conn, DBusMessage *msg, > err = device_connect_le(device); > else > err = adapter_create_bonding(adapter, &device->bdaddr, > - device->bdaddr_type, > + bdaddr_type, > io_cap); > } else { > err = adapter_create_bonding(adapter, &device->bdaddr, > @@ -6207,12 +6217,9 @@ static bool start_discovery_cb(gpointer user_data) > { > struct btd_device *device = user_data; > > - if (device->bredr) > - device_browse_sdp(device, NULL); > - else > - device_browse_gatt(device, NULL); > - > device->discov_timer = 0; > + device_discover_services(device, select_active_bearer(device), > + NULL); Perhaps in case of discovery we could do both in parallel, although if the remote side supports GATT services as part of SDP we may end up with redundant discovery but bt_gatt_client/gatt_db shall be able to handle that. > return FALSE; > } > -- > 2.34.1 >
diff --git a/src/device.c b/src/device.c index 6f28e261e..446e978ee 100644 --- a/src/device.c +++ b/src/device.c @@ -2504,6 +2504,16 @@ static uint8_t select_conn_bearer(struct btd_device *dev) return dev->bdaddr_type; } +static uint8_t select_active_bearer(struct btd_device *dev) +{ + if (dev->bredr_state.connected) + return BDADDR_BREDR; + else if (dev->le_state.connected) + return dev->bdaddr_type == BDADDR_BREDR + ? BDADDR_LE_PUBLIC : dev->bdaddr_type; + return select_conn_bearer(dev); +} + static DBusMessage *dev_connect(DBusConnection *conn, DBusMessage *msg, void *user_data) { @@ -3018,7 +3028,7 @@ static DBusMessage *pair_device(DBusConnection *conn, DBusMessage *msg, else if (device->le_state.bonded) bdaddr_type = BDADDR_BREDR; else - bdaddr_type = select_conn_bearer(device); + bdaddr_type = select_active_bearer(device); state = get_state(device, bdaddr_type); @@ -3055,7 +3065,7 @@ static DBusMessage *pair_device(DBusConnection *conn, DBusMessage *msg, err = device_connect_le(device); else err = adapter_create_bonding(adapter, &device->bdaddr, - device->bdaddr_type, + bdaddr_type, io_cap); } else { err = adapter_create_bonding(adapter, &device->bdaddr, @@ -6207,12 +6217,9 @@ static bool start_discovery_cb(gpointer user_data) { struct btd_device *device = user_data; - if (device->bredr) - device_browse_sdp(device, NULL); - else - device_browse_gatt(device, NULL); - device->discov_timer = 0; + device_discover_services(device, select_active_bearer(device), + NULL); return FALSE; }