* [PATCH BlueZ 1/2] shared/gatt-client: Don't send Exchange MTU for default value @ 2016-03-04 12:24 Luiz Augusto von Dentz 2016-03-04 12:24 ` [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral Luiz Augusto von Dentz 2016-03-11 14:44 ` [PATCH BlueZ 1/2] shared/gatt-client: Don't send Exchange MTU for default value Luiz Augusto von Dentz 0 siblings, 2 replies; 16+ messages in thread From: Luiz Augusto von Dentz @ 2016-03-04 12:24 UTC (permalink / raw) To: linux-bluetooth From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> If the MTU to be exchange is the default there is no pointing in sending it since the remote already assumes it anyway. --- src/shared/gatt-client.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c index b255175..45acf7b 100644 --- a/src/shared/gatt-client.c +++ b/src/shared/gatt-client.c @@ -1544,6 +1544,11 @@ static bool gatt_client_init(struct bt_gatt_client *client, uint16_t mtu) if (!op) return false; + /* Check if MTU needs to be send */ + mtu = MAX(BT_ATT_DEFAULT_LE_MTU, mtu); + if (mtu == BT_ATT_DEFAULT_LE_MTU) + goto discover; + /* Configure the MTU */ client->mtu_req_id = bt_gatt_exchange_mtu(client->att, MAX(BT_ATT_DEFAULT_LE_MTU, mtu), @@ -1558,6 +1563,20 @@ static bool gatt_client_init(struct bt_gatt_client *client, uint16_t mtu) client->in_init = true; return true; + +discover: + client->discovery_req = bt_gatt_discover_all_primary_services( + client->att, NULL, + discover_primary_cb, + discovery_op_ref(op), + discovery_op_unref); + if (!client->discovery_req) { + discovery_op_free(op); + return false; + } + + client->in_init = true; + return true; } struct pdu_data { -- 2.5.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral 2016-03-04 12:24 [PATCH BlueZ 1/2] shared/gatt-client: Don't send Exchange MTU for default value Luiz Augusto von Dentz @ 2016-03-04 12:24 ` Luiz Augusto von Dentz 2016-03-04 12:51 ` Łukasz Rymanowski 2016-03-11 14:44 ` [PATCH BlueZ 1/2] shared/gatt-client: Don't send Exchange MTU for default value Luiz Augusto von Dentz 1 sibling, 1 reply; 16+ messages in thread From: Luiz Augusto von Dentz @ 2016-03-04 12:24 UTC (permalink / raw) To: linux-bluetooth From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> Some Android versions don't seem to cope well with the fact that the peripheral can actually set a MTU thus leave the MTU as the default. --- src/attrib-server.c | 2 +- src/device.c | 8 +++++--- src/device.h | 2 +- src/gatt-database.c | 2 +- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/attrib-server.c b/src/attrib-server.c index 4439c27..e92ca5c 100644 --- a/src/attrib-server.c +++ b/src/attrib-server.c @@ -1281,7 +1281,7 @@ static void connect_event(GIOChannel *io, GError *gerr, void *user_data) if (!device) return; - device_attach_att(device, io); + device_attach_att(device, io, false); } static gboolean register_core_services(struct gatt_server *server) diff --git a/src/device.c b/src/device.c index 14e850e..b4bc593 100644 --- a/src/device.c +++ b/src/device.c @@ -4664,7 +4664,7 @@ static bool remote_counter(uint32_t *sign_cnt, void *user_data) return true; } -bool device_attach_att(struct btd_device *dev, GIOChannel *io) +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator) { GError *gerr = NULL; GAttrib *attrib; @@ -4699,7 +4699,9 @@ bool device_attach_att(struct btd_device *dev, GIOChannel *io) } } - dev->att_mtu = MIN(mtu, BT_ATT_MAX_LE_MTU); + /* Only attempt to set MTU if initiator/central */ + dev->att_mtu = initiator ? MIN(mtu, BT_ATT_MAX_LE_MTU) : + BT_ATT_DEFAULT_LE_MTU; attrib = g_attrib_new(io, dev->att_mtu, false); if (!attrib) { error("Unable to create new GAttrib instance"); @@ -4768,7 +4770,7 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) goto done; } - if (!device_attach_att(device, io)) + if (!device_attach_att(device, io, true)) goto done; if (attcb->success) diff --git a/src/device.h b/src/device.h index db10827..5c01ed0 100644 --- a/src/device.h +++ b/src/device.h @@ -72,7 +72,7 @@ struct bt_gatt_client *btd_device_get_gatt_client(struct btd_device *device); struct bt_gatt_server *btd_device_get_gatt_server(struct btd_device *device); void btd_device_gatt_set_service_changed(struct btd_device *device, uint16_t start, uint16_t end); -bool device_attach_att(struct btd_device *dev, GIOChannel *io); +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator); void btd_device_add_uuid(struct btd_device *device, const char *uuid); void device_add_eir_uuids(struct btd_device *dev, GSList *uuids); void device_set_manufacturer_data(struct btd_device *dev, GSList *list); diff --git a/src/gatt-database.c b/src/gatt-database.c index d906e2b..4df4a16 100644 --- a/src/gatt-database.c +++ b/src/gatt-database.c @@ -499,7 +499,7 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) if (!device) return; - device_attach_att(device, io); + device_attach_att(device, io, false); } static void gap_device_name_read_cb(struct gatt_db_attribute *attrib, -- 2.5.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral 2016-03-04 12:24 ` [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral Luiz Augusto von Dentz @ 2016-03-04 12:51 ` Łukasz Rymanowski 2016-03-04 13:12 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 16+ messages in thread From: Łukasz Rymanowski @ 2016-03-04 12:51 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org Hi Luiz, On 4 March 2016 at 13:24, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > Some Android versions don't seem to cope well with the fact that the > peripheral can actually set a MTU thus leave the MTU as the default. Just tested that and for me connection with Android works OK even BlueZ started Exchange MTU procedure. Tested on Android 5 and 6. I saw once the issue that Android could not retrieve services after connection to BlueZ but from the logs it looked like BlueZ start search for primary services and Android not. If Android make to sent search request for primary services first then it was fine. Maybe Android don't like BlueZ to start search primary if Android is initiator? But this is their bug BTW. I saw it on Android 5 not yet on 6. > --- > src/attrib-server.c | 2 +- > src/device.c | 8 +++++--- > src/device.h | 2 +- > src/gatt-database.c | 2 +- > 4 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/src/attrib-server.c b/src/attrib-server.c > index 4439c27..e92ca5c 100644 > --- a/src/attrib-server.c > +++ b/src/attrib-server.c > @@ -1281,7 +1281,7 @@ static void connect_event(GIOChannel *io, GError *gerr, void *user_data) > if (!device) > return; > > - device_attach_att(device, io); > + device_attach_att(device, io, false); > } > > static gboolean register_core_services(struct gatt_server *server) > diff --git a/src/device.c b/src/device.c > index 14e850e..b4bc593 100644 > --- a/src/device.c > +++ b/src/device.c > @@ -4664,7 +4664,7 @@ static bool remote_counter(uint32_t *sign_cnt, void *user_data) > return true; > } > > -bool device_attach_att(struct btd_device *dev, GIOChannel *io) > +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator) > { > GError *gerr = NULL; > GAttrib *attrib; > @@ -4699,7 +4699,9 @@ bool device_attach_att(struct btd_device *dev, GIOChannel *io) > } > } > > - dev->att_mtu = MIN(mtu, BT_ATT_MAX_LE_MTU); > + /* Only attempt to set MTU if initiator/central */ Keep in mind that MTU echange procedure is bonded to Client/Server role not central/peripheral. > + dev->att_mtu = initiator ? MIN(mtu, BT_ATT_MAX_LE_MTU) : > + BT_ATT_DEFAULT_LE_MTU; > attrib = g_attrib_new(io, dev->att_mtu, false); > if (!attrib) { > error("Unable to create new GAttrib instance"); > @@ -4768,7 +4770,7 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) > goto done; > } > > - if (!device_attach_att(device, io)) > + if (!device_attach_att(device, io, true)) > goto done; > > if (attcb->success) > diff --git a/src/device.h b/src/device.h > index db10827..5c01ed0 100644 > --- a/src/device.h > +++ b/src/device.h > @@ -72,7 +72,7 @@ struct bt_gatt_client *btd_device_get_gatt_client(struct btd_device *device); > struct bt_gatt_server *btd_device_get_gatt_server(struct btd_device *device); > void btd_device_gatt_set_service_changed(struct btd_device *device, > uint16_t start, uint16_t end); > -bool device_attach_att(struct btd_device *dev, GIOChannel *io); > +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator); > void btd_device_add_uuid(struct btd_device *device, const char *uuid); > void device_add_eir_uuids(struct btd_device *dev, GSList *uuids); > void device_set_manufacturer_data(struct btd_device *dev, GSList *list); > diff --git a/src/gatt-database.c b/src/gatt-database.c > index d906e2b..4df4a16 100644 > --- a/src/gatt-database.c > +++ b/src/gatt-database.c > @@ -499,7 +499,7 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) > if (!device) > return; > > - device_attach_att(device, io); > + device_attach_att(device, io, false); Not sure if it works when you use white list for autoconnect because iirc we got this callback when BlueZ is initiator then. But still initiator or not, the important thing is if we act as client or server. > } > > static void gap_device_name_read_cb(struct gatt_db_attribute *attrib, > -- > 2.5.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- BR / Pozdrawiam Łukasz ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral 2016-03-04 12:51 ` Łukasz Rymanowski @ 2016-03-04 13:12 ` Luiz Augusto von Dentz 2016-03-04 13:35 ` Szymon Janc 2016-03-04 15:46 ` Łukasz Rymanowski 0 siblings, 2 replies; 16+ messages in thread From: Luiz Augusto von Dentz @ 2016-03-04 13:12 UTC (permalink / raw) To: Łukasz Rymanowski; +Cc: linux-bluetooth@vger.kernel.org Hi Łukasz, On Fri, Mar 4, 2016 at 2:51 PM, Łukasz Rymanowski <lukasz.rymanowski@codecoup.pl> wrote: > Hi Luiz, > > On 4 March 2016 at 13:24, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> >> >> Some Android versions don't seem to cope well with the fact that the >> peripheral can actually set a MTU thus leave the MTU as the default. > > Just tested that and for me connection with Android works OK even BlueZ > started Exchange MTU procedure. Tested on Android 5 and 6. > > I saw once the issue that Android could not retrieve services after > connection to BlueZ > but from the logs it looked like BlueZ start search for primary > services and Android not. > If Android make to sent search request for primary services first then > it was fine. > > Maybe Android don't like BlueZ to start search primary if Android is > initiator? But this is their bug > BTW. I saw it on Android 5 not yet on 6. With 4.4 it actually made a difference. > >> --- >> src/attrib-server.c | 2 +- >> src/device.c | 8 +++++--- >> src/device.h | 2 +- >> src/gatt-database.c | 2 +- >> 4 files changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/src/attrib-server.c b/src/attrib-server.c >> index 4439c27..e92ca5c 100644 >> --- a/src/attrib-server.c >> +++ b/src/attrib-server.c >> @@ -1281,7 +1281,7 @@ static void connect_event(GIOChannel *io, GError *gerr, void *user_data) >> if (!device) >> return; >> >> - device_attach_att(device, io); >> + device_attach_att(device, io, false); >> } >> >> static gboolean register_core_services(struct gatt_server *server) >> diff --git a/src/device.c b/src/device.c >> index 14e850e..b4bc593 100644 >> --- a/src/device.c >> +++ b/src/device.c >> @@ -4664,7 +4664,7 @@ static bool remote_counter(uint32_t *sign_cnt, void *user_data) >> return true; >> } >> >> -bool device_attach_att(struct btd_device *dev, GIOChannel *io) >> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator) >> { >> GError *gerr = NULL; >> GAttrib *attrib; >> @@ -4699,7 +4699,9 @@ bool device_attach_att(struct btd_device *dev, GIOChannel *io) >> } >> } >> >> - dev->att_mtu = MIN(mtu, BT_ATT_MAX_LE_MTU); >> + /* Only attempt to set MTU if initiator/central */ > > Keep in mind that MTU echange procedure is bonded to Client/Server > role not central/peripheral. Well actually is not either GAP or GATT role related since both side are in fact allowed to do that, so this is just a workaround. > >> + dev->att_mtu = initiator ? MIN(mtu, BT_ATT_MAX_LE_MTU) : >> + BT_ATT_DEFAULT_LE_MTU; >> attrib = g_attrib_new(io, dev->att_mtu, false); >> if (!attrib) { >> error("Unable to create new GAttrib instance"); >> @@ -4768,7 +4770,7 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) >> goto done; >> } >> >> - if (!device_attach_att(device, io)) >> + if (!device_attach_att(device, io, true)) >> goto done; >> >> if (attcb->success) >> diff --git a/src/device.h b/src/device.h >> index db10827..5c01ed0 100644 >> --- a/src/device.h >> +++ b/src/device.h >> @@ -72,7 +72,7 @@ struct bt_gatt_client *btd_device_get_gatt_client(struct btd_device *device); >> struct bt_gatt_server *btd_device_get_gatt_server(struct btd_device *device); >> void btd_device_gatt_set_service_changed(struct btd_device *device, >> uint16_t start, uint16_t end); >> -bool device_attach_att(struct btd_device *dev, GIOChannel *io); >> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator); >> void btd_device_add_uuid(struct btd_device *device, const char *uuid); >> void device_add_eir_uuids(struct btd_device *dev, GSList *uuids); >> void device_set_manufacturer_data(struct btd_device *dev, GSList *list); >> diff --git a/src/gatt-database.c b/src/gatt-database.c >> index d906e2b..4df4a16 100644 >> --- a/src/gatt-database.c >> +++ b/src/gatt-database.c >> @@ -499,7 +499,7 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) >> if (!device) >> return; >> >> - device_attach_att(device, io); >> + device_attach_att(device, io, false); > > Not sure if it works when you use white list for autoconnect because > iirc we got this callback when BlueZ is initiator > then. But still initiator or not, the important thing is if we act as > client or server. Probably not, I will have to add more logic to detect how it got connected, maybe we can actually check who is the master so that the master should be the one controlling the MTU. >> } >> >> static void gap_device_name_read_cb(struct gatt_db_attribute *attrib, >> -- >> 2.5.0 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > BR / Pozdrawiam > Łukasz -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral 2016-03-04 13:12 ` Luiz Augusto von Dentz @ 2016-03-04 13:35 ` Szymon Janc 2016-03-04 14:38 ` Luiz Augusto von Dentz 2016-03-04 15:46 ` Łukasz Rymanowski 1 sibling, 1 reply; 16+ messages in thread From: Szymon Janc @ 2016-03-04 13:35 UTC (permalink / raw) To: Luiz Augusto von Dentz Cc: Łukasz Rymanowski, linux-bluetooth@vger.kernel.org Hi Luiz, On Friday 04 March 2016 15:12:39 Luiz Augusto von Dentz wrote: > Hi Łukasz, > > On Fri, Mar 4, 2016 at 2:51 PM, Łukasz Rymanowski > > <lukasz.rymanowski@codecoup.pl> wrote: > > Hi Luiz, > > > > On 4 March 2016 at 13:24, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > >> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > >> > >> Some Android versions don't seem to cope well with the fact that the > >> peripheral can actually set a MTU thus leave the MTU as the default. > > > > Just tested that and for me connection with Android works OK even BlueZ > > started Exchange MTU procedure. Tested on Android 5 and 6. > > > > I saw once the issue that Android could not retrieve services after > > connection to BlueZ > > but from the logs it looked like BlueZ start search for primary > > services and Android not. > > If Android make to sent search request for primary services first then > > it was fine. > > > > Maybe Android don't like BlueZ to start search primary if Android is > > initiator? But this is their bug > > BTW. I saw it on Android 5 not yet on 6. > > With 4.4 it actually made a difference. > > >> --- > >> > >> src/attrib-server.c | 2 +- > >> src/device.c | 8 +++++--- > >> src/device.h | 2 +- > >> src/gatt-database.c | 2 +- > >> 4 files changed, 8 insertions(+), 6 deletions(-) > >> > >> diff --git a/src/attrib-server.c b/src/attrib-server.c > >> index 4439c27..e92ca5c 100644 > >> --- a/src/attrib-server.c > >> +++ b/src/attrib-server.c > >> @@ -1281,7 +1281,7 @@ static void connect_event(GIOChannel *io, GError > >> *gerr, void *user_data)>> > >> if (!device) > >> > >> return; > >> > >> - device_attach_att(device, io); > >> + device_attach_att(device, io, false); > >> > >> } > >> > >> static gboolean register_core_services(struct gatt_server *server) > >> > >> diff --git a/src/device.c b/src/device.c > >> index 14e850e..b4bc593 100644 > >> --- a/src/device.c > >> +++ b/src/device.c > >> @@ -4664,7 +4664,7 @@ static bool remote_counter(uint32_t *sign_cnt, void > >> *user_data)>> > >> return true; > >> > >> } > >> > >> -bool device_attach_att(struct btd_device *dev, GIOChannel *io) > >> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool > >> initiator)>> > >> { > >> > >> GError *gerr = NULL; > >> GAttrib *attrib; > >> > >> @@ -4699,7 +4699,9 @@ bool device_attach_att(struct btd_device *dev, > >> GIOChannel *io)>> > >> } > >> > >> } > >> > >> - dev->att_mtu = MIN(mtu, BT_ATT_MAX_LE_MTU); > >> + /* Only attempt to set MTU if initiator/central */ > > > > Keep in mind that MTU echange procedure is bonded to Client/Server > > role not central/peripheral. > > Well actually is not either GAP or GATT role related since both side > are in fact allowed to do that, so this is just a workaround. > > >> + dev->att_mtu = initiator ? MIN(mtu, BT_ATT_MAX_LE_MTU) : > >> + > >> BT_ATT_DEFAULT_LE_MTU;>> > >> attrib = g_attrib_new(io, dev->att_mtu, false); > >> if (!attrib) { > >> > >> error("Unable to create new GAttrib instance"); > >> > >> @@ -4768,7 +4770,7 @@ static void att_connect_cb(GIOChannel *io, GError > >> *gerr, gpointer user_data)>> > >> goto done; > >> > >> } > >> > >> - if (!device_attach_att(device, io)) > >> + if (!device_attach_att(device, io, true)) > >> > >> goto done; > >> > >> if (attcb->success) > >> > >> diff --git a/src/device.h b/src/device.h > >> index db10827..5c01ed0 100644 > >> --- a/src/device.h > >> +++ b/src/device.h > >> @@ -72,7 +72,7 @@ struct bt_gatt_client > >> *btd_device_get_gatt_client(struct btd_device *device);>> > >> struct bt_gatt_server *btd_device_get_gatt_server(struct btd_device > >> *device); void btd_device_gatt_set_service_changed(struct btd_device > >> *device,>> > >> uint16_t start, uint16_t > >> end); > >> > >> -bool device_attach_att(struct btd_device *dev, GIOChannel *io); > >> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool > >> initiator);>> > >> void btd_device_add_uuid(struct btd_device *device, const char *uuid); > >> void device_add_eir_uuids(struct btd_device *dev, GSList *uuids); > >> void device_set_manufacturer_data(struct btd_device *dev, GSList *list); > >> > >> diff --git a/src/gatt-database.c b/src/gatt-database.c > >> index d906e2b..4df4a16 100644 > >> --- a/src/gatt-database.c > >> +++ b/src/gatt-database.c > >> @@ -499,7 +499,7 @@ static void connect_cb(GIOChannel *io, GError *gerr, > >> gpointer user_data)>> > >> if (!device) > >> > >> return; > >> > >> - device_attach_att(device, io); > >> + device_attach_att(device, io, false); > > > > Not sure if it works when you use white list for autoconnect because > > iirc we got this callback when BlueZ is initiator > > then. But still initiator or not, the important thing is if we act as > > client or server. > > Probably not, I will have to add more logic to detect how it got > connected, maybe we can actually check who is the master so that the > master should be the one controlling the MTU. Both sides can initiate MTU exchange as you can be both server and client. I really fail to see how mixing that with central vs peripheral would help. And if there is a bug in Android then we would have to first understand it eg. if problem is just with MTU or any other request would result in same issue. -- pozdrawiam Szymon Janc ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral 2016-03-04 13:35 ` Szymon Janc @ 2016-03-04 14:38 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 16+ messages in thread From: Luiz Augusto von Dentz @ 2016-03-04 14:38 UTC (permalink / raw) To: Szymon Janc; +Cc: Łukasz Rymanowski, linux-bluetooth@vger.kernel.org Hi Szymon, On Fri, Mar 4, 2016 at 3:35 PM, Szymon Janc <szymon.janc@codecoup.pl> wrote: > Hi Luiz, > > On Friday 04 March 2016 15:12:39 Luiz Augusto von Dentz wrote: >> Hi Łukasz, >> >> On Fri, Mar 4, 2016 at 2:51 PM, Łukasz Rymanowski >> >> <lukasz.rymanowski@codecoup.pl> wrote: >> > Hi Luiz, >> > >> > On 4 March 2016 at 13:24, Luiz Augusto von Dentz <luiz.dentz@gmail.com> > wrote: >> >> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> >> >> >> >> Some Android versions don't seem to cope well with the fact that the >> >> peripheral can actually set a MTU thus leave the MTU as the default. >> > >> > Just tested that and for me connection with Android works OK even BlueZ >> > started Exchange MTU procedure. Tested on Android 5 and 6. >> > >> > I saw once the issue that Android could not retrieve services after >> > connection to BlueZ >> > but from the logs it looked like BlueZ start search for primary >> > services and Android not. >> > If Android make to sent search request for primary services first then >> > it was fine. >> > >> > Maybe Android don't like BlueZ to start search primary if Android is >> > initiator? But this is their bug >> > BTW. I saw it on Android 5 not yet on 6. >> >> With 4.4 it actually made a difference. >> >> >> --- >> >> >> >> src/attrib-server.c | 2 +- >> >> src/device.c | 8 +++++--- >> >> src/device.h | 2 +- >> >> src/gatt-database.c | 2 +- >> >> 4 files changed, 8 insertions(+), 6 deletions(-) >> >> >> >> diff --git a/src/attrib-server.c b/src/attrib-server.c >> >> index 4439c27..e92ca5c 100644 >> >> --- a/src/attrib-server.c >> >> +++ b/src/attrib-server.c >> >> @@ -1281,7 +1281,7 @@ static void connect_event(GIOChannel *io, GError >> >> *gerr, void *user_data)>> >> >> if (!device) >> >> >> >> return; >> >> >> >> - device_attach_att(device, io); >> >> + device_attach_att(device, io, false); >> >> >> >> } >> >> >> >> static gboolean register_core_services(struct gatt_server *server) >> >> >> >> diff --git a/src/device.c b/src/device.c >> >> index 14e850e..b4bc593 100644 >> >> --- a/src/device.c >> >> +++ b/src/device.c >> >> @@ -4664,7 +4664,7 @@ static bool remote_counter(uint32_t *sign_cnt, void >> >> *user_data)>> >> >> return true; >> >> >> >> } >> >> >> >> -bool device_attach_att(struct btd_device *dev, GIOChannel *io) >> >> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool >> >> initiator)>> >> >> { >> >> >> >> GError *gerr = NULL; >> >> GAttrib *attrib; >> >> >> >> @@ -4699,7 +4699,9 @@ bool device_attach_att(struct btd_device *dev, >> >> GIOChannel *io)>> >> >> } >> >> >> >> } >> >> >> >> - dev->att_mtu = MIN(mtu, BT_ATT_MAX_LE_MTU); >> >> + /* Only attempt to set MTU if initiator/central */ >> > >> > Keep in mind that MTU echange procedure is bonded to Client/Server >> > role not central/peripheral. >> >> Well actually is not either GAP or GATT role related since both side >> are in fact allowed to do that, so this is just a workaround. >> >> >> + dev->att_mtu = initiator ? MIN(mtu, BT_ATT_MAX_LE_MTU) : >> >> + >> >> BT_ATT_DEFAULT_LE_MTU;>> >> >> attrib = g_attrib_new(io, dev->att_mtu, false); >> >> if (!attrib) { >> >> >> >> error("Unable to create new GAttrib instance"); >> >> >> >> @@ -4768,7 +4770,7 @@ static void att_connect_cb(GIOChannel *io, GError >> >> *gerr, gpointer user_data)>> >> >> goto done; >> >> >> >> } >> >> >> >> - if (!device_attach_att(device, io)) >> >> + if (!device_attach_att(device, io, true)) >> >> >> >> goto done; >> >> >> >> if (attcb->success) >> >> >> >> diff --git a/src/device.h b/src/device.h >> >> index db10827..5c01ed0 100644 >> >> --- a/src/device.h >> >> +++ b/src/device.h >> >> @@ -72,7 +72,7 @@ struct bt_gatt_client >> >> *btd_device_get_gatt_client(struct btd_device *device);>> >> >> struct bt_gatt_server *btd_device_get_gatt_server(struct btd_device >> >> *device); void btd_device_gatt_set_service_changed(struct btd_device >> >> *device,>> >> >> uint16_t start, uint16_t >> >> end); >> >> >> >> -bool device_attach_att(struct btd_device *dev, GIOChannel *io); >> >> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool >> >> initiator);>> >> >> void btd_device_add_uuid(struct btd_device *device, const char *uuid); >> >> void device_add_eir_uuids(struct btd_device *dev, GSList *uuids); >> >> void device_set_manufacturer_data(struct btd_device *dev, GSList *list); >> >> >> >> diff --git a/src/gatt-database.c b/src/gatt-database.c >> >> index d906e2b..4df4a16 100644 >> >> --- a/src/gatt-database.c >> >> +++ b/src/gatt-database.c >> >> @@ -499,7 +499,7 @@ static void connect_cb(GIOChannel *io, GError *gerr, >> >> gpointer user_data)>> >> >> if (!device) >> >> >> >> return; >> >> >> >> - device_attach_att(device, io); >> >> + device_attach_att(device, io, false); >> > >> > Not sure if it works when you use white list for autoconnect because >> > iirc we got this callback when BlueZ is initiator >> > then. But still initiator or not, the important thing is if we act as >> > client or server. >> >> Probably not, I will have to add more logic to detect how it got >> connected, maybe we can actually check who is the master so that the >> master should be the one controlling the MTU. > > Both sides can initiate MTU exchange as you can be both server and client. > I really fail to see how mixing that with central vs peripheral would help. > And if there is a bug in Android then we would have to first understand it eg. > if problem is just with MTU or any other request would result in same issue. As I said I already tested this, with 4.4 at least it makes a difference since Android stop any client request if you attempt to do ATT Exchange: http://ix.io/oMm Other request do work as you can see from the logs we do manage to continue with our own request, now with these changes I got the following: http://ix.io/oOB I don't think there is any doubt that it is the ATT Exchange command causing the issue, perhaps you can check if that bug still exist but we got a thread with a few other people with similar problem. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral 2016-03-04 13:12 ` Luiz Augusto von Dentz 2016-03-04 13:35 ` Szymon Janc @ 2016-03-04 15:46 ` Łukasz Rymanowski 2016-03-09 12:13 ` Luiz Augusto von Dentz 1 sibling, 1 reply; 16+ messages in thread From: Łukasz Rymanowski @ 2016-03-04 15:46 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org Hi Luiz, On 4 March 2016 at 14:12, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > Hi Łukasz, > > On Fri, Mar 4, 2016 at 2:51 PM, Łukasz Rymanowski > <lukasz.rymanowski@codecoup.pl> wrote: >> Hi Luiz, >> >> On 4 March 2016 at 13:24, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> >>> >>> Some Android versions don't seem to cope well with the fact that the >>> peripheral can actually set a MTU thus leave the MTU as the default. >> >> Just tested that and for me connection with Android works OK even BlueZ >> started Exchange MTU procedure. Tested on Android 5 and 6. >> >> I saw once the issue that Android could not retrieve services after >> connection to BlueZ >> but from the logs it looked like BlueZ start search for primary >> services and Android not. >> If Android make to sent search request for primary services first then >> it was fine. >> >> Maybe Android don't like BlueZ to start search primary if Android is >> initiator? But this is their bug >> BTW. I saw it on Android 5 not yet on 6. > > With 4.4 it actually made a difference. > >> >>> --- >>> src/attrib-server.c | 2 +- >>> src/device.c | 8 +++++--- >>> src/device.h | 2 +- >>> src/gatt-database.c | 2 +- >>> 4 files changed, 8 insertions(+), 6 deletions(-) >>> >>> diff --git a/src/attrib-server.c b/src/attrib-server.c >>> index 4439c27..e92ca5c 100644 >>> --- a/src/attrib-server.c >>> +++ b/src/attrib-server.c >>> @@ -1281,7 +1281,7 @@ static void connect_event(GIOChannel *io, GError *gerr, void *user_data) >>> if (!device) >>> return; >>> >>> - device_attach_att(device, io); >>> + device_attach_att(device, io, false); >>> } >>> >>> static gboolean register_core_services(struct gatt_server *server) >>> diff --git a/src/device.c b/src/device.c >>> index 14e850e..b4bc593 100644 >>> --- a/src/device.c >>> +++ b/src/device.c >>> @@ -4664,7 +4664,7 @@ static bool remote_counter(uint32_t *sign_cnt, void *user_data) >>> return true; >>> } >>> >>> -bool device_attach_att(struct btd_device *dev, GIOChannel *io) >>> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator) >>> { >>> GError *gerr = NULL; >>> GAttrib *attrib; >>> @@ -4699,7 +4699,9 @@ bool device_attach_att(struct btd_device *dev, GIOChannel *io) >>> } >>> } >>> >>> - dev->att_mtu = MIN(mtu, BT_ATT_MAX_LE_MTU); >>> + /* Only attempt to set MTU if initiator/central */ >> >> Keep in mind that MTU echange procedure is bonded to Client/Server >> role not central/peripheral. > > Well actually is not either GAP or GATT role related since both side > are in fact allowed to do that, so this is just a workaround. According to spec Exchange MTU procedure can be started by Client or devices supporting Client/Server role. BlueZ always acts as client/server this is why you can always start this procedure. It is really not related to central/peripheral. > >> >>> + dev->att_mtu = initiator ? MIN(mtu, BT_ATT_MAX_LE_MTU) : >>> + BT_ATT_DEFAULT_LE_MTU; >>> attrib = g_attrib_new(io, dev->att_mtu, false); >>> if (!attrib) { >>> error("Unable to create new GAttrib instance"); >>> @@ -4768,7 +4770,7 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) >>> goto done; >>> } >>> >>> - if (!device_attach_att(device, io)) >>> + if (!device_attach_att(device, io, true)) >>> goto done; >>> >>> if (attcb->success) >>> diff --git a/src/device.h b/src/device.h >>> index db10827..5c01ed0 100644 >>> --- a/src/device.h >>> +++ b/src/device.h >>> @@ -72,7 +72,7 @@ struct bt_gatt_client *btd_device_get_gatt_client(struct btd_device *device); >>> struct bt_gatt_server *btd_device_get_gatt_server(struct btd_device *device); >>> void btd_device_gatt_set_service_changed(struct btd_device *device, >>> uint16_t start, uint16_t end); >>> -bool device_attach_att(struct btd_device *dev, GIOChannel *io); >>> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator); >>> void btd_device_add_uuid(struct btd_device *device, const char *uuid); >>> void device_add_eir_uuids(struct btd_device *dev, GSList *uuids); >>> void device_set_manufacturer_data(struct btd_device *dev, GSList *list); >>> diff --git a/src/gatt-database.c b/src/gatt-database.c >>> index d906e2b..4df4a16 100644 >>> --- a/src/gatt-database.c >>> +++ b/src/gatt-database.c >>> @@ -499,7 +499,7 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) >>> if (!device) >>> return; >>> >>> - device_attach_att(device, io); >>> + device_attach_att(device, io, false); >> >> Not sure if it works when you use white list for autoconnect because >> iirc we got this callback when BlueZ is initiator >> then. But still initiator or not, the important thing is if we act as >> client or server. > > Probably not, I just run the test: 1) add device to autoconnect list. 2) Simulate disconnect by power off. 3) After then power on device and wait until auto connect do its job: I got this: Mar 4 16:30:36 t450s bluetoothd[31211]: src/adapter.c:connected_callback() hci0 device XX:XX:XX:XX:XX:XX connected eir_len 14 Mar 4 16:30:36 t450s bluetoothd[31211]: src/gatt-database.c:connect_cb() New incoming LE ATT connection That means that with this patch after reconnect a default MTU will be used unless remote device starts exchange MTU procedure. I will have to add more logic to detect how it got > connected, maybe we can actually check who is the master so that the > master should be the one controlling the MTU. > Maybe we can do exchange MTU later after discovery ? For discovery default MTU should be enough. >>> } >>> >>> static void gap_device_name_read_cb(struct gatt_db_attribute *attrib, >>> -- >>> 2.5.0 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> >> -- >> BR / Pozdrawiam >> Łukasz > > > > -- > Luiz Augusto von Dentz -- BR / Pozdrawiam Łukasz ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral 2016-03-04 15:46 ` Łukasz Rymanowski @ 2016-03-09 12:13 ` Luiz Augusto von Dentz 2016-03-10 7:59 ` Łukasz Rymanowski 0 siblings, 1 reply; 16+ messages in thread From: Luiz Augusto von Dentz @ 2016-03-09 12:13 UTC (permalink / raw) To: Łukasz Rymanowski; +Cc: linux-bluetooth@vger.kernel.org Hi Lukasz, On Fri, Mar 4, 2016 at 5:46 PM, Łukasz Rymanowski <lukasz.rymanowski@codecoup.pl> wrote: > Hi Luiz, > > On 4 March 2016 at 14:12, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >> Hi Łukasz, >> >> On Fri, Mar 4, 2016 at 2:51 PM, Łukasz Rymanowski >> <lukasz.rymanowski@codecoup.pl> wrote: >>> Hi Luiz, >>> >>> On 4 March 2016 at 13:24, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> >>>> >>>> Some Android versions don't seem to cope well with the fact that the >>>> peripheral can actually set a MTU thus leave the MTU as the default. >>> >>> Just tested that and for me connection with Android works OK even BlueZ >>> started Exchange MTU procedure. Tested on Android 5 and 6. >>> >>> I saw once the issue that Android could not retrieve services after >>> connection to BlueZ >>> but from the logs it looked like BlueZ start search for primary >>> services and Android not. >>> If Android make to sent search request for primary services first then >>> it was fine. >>> >>> Maybe Android don't like BlueZ to start search primary if Android is >>> initiator? But this is their bug >>> BTW. I saw it on Android 5 not yet on 6. >> >> With 4.4 it actually made a difference. >> >>> >>>> --- >>>> src/attrib-server.c | 2 +- >>>> src/device.c | 8 +++++--- >>>> src/device.h | 2 +- >>>> src/gatt-database.c | 2 +- >>>> 4 files changed, 8 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/src/attrib-server.c b/src/attrib-server.c >>>> index 4439c27..e92ca5c 100644 >>>> --- a/src/attrib-server.c >>>> +++ b/src/attrib-server.c >>>> @@ -1281,7 +1281,7 @@ static void connect_event(GIOChannel *io, GError *gerr, void *user_data) >>>> if (!device) >>>> return; >>>> >>>> - device_attach_att(device, io); >>>> + device_attach_att(device, io, false); >>>> } >>>> >>>> static gboolean register_core_services(struct gatt_server *server) >>>> diff --git a/src/device.c b/src/device.c >>>> index 14e850e..b4bc593 100644 >>>> --- a/src/device.c >>>> +++ b/src/device.c >>>> @@ -4664,7 +4664,7 @@ static bool remote_counter(uint32_t *sign_cnt, void *user_data) >>>> return true; >>>> } >>>> >>>> -bool device_attach_att(struct btd_device *dev, GIOChannel *io) >>>> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator) >>>> { >>>> GError *gerr = NULL; >>>> GAttrib *attrib; >>>> @@ -4699,7 +4699,9 @@ bool device_attach_att(struct btd_device *dev, GIOChannel *io) >>>> } >>>> } >>>> >>>> - dev->att_mtu = MIN(mtu, BT_ATT_MAX_LE_MTU); >>>> + /* Only attempt to set MTU if initiator/central */ >>> >>> Keep in mind that MTU echange procedure is bonded to Client/Server >>> role not central/peripheral. >> >> Well actually is not either GAP or GATT role related since both side >> are in fact allowed to do that, so this is just a workaround. > > According to spec Exchange MTU procedure can be started by Client or > devices supporting Client/Server role. > BlueZ always acts as client/server this is why you can always start > this procedure. > It is really not related to central/peripheral. Well the working of client/server role seems very odd to say the least, why the use of client and server role if they could always be both, but it doesn't change the fact that either side can send MTU Exchange. >> >>> >>>> + dev->att_mtu = initiator ? MIN(mtu, BT_ATT_MAX_LE_MTU) : >>>> + BT_ATT_DEFAULT_LE_MTU; >>>> attrib = g_attrib_new(io, dev->att_mtu, false); >>>> if (!attrib) { >>>> error("Unable to create new GAttrib instance"); >>>> @@ -4768,7 +4770,7 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) >>>> goto done; >>>> } >>>> >>>> - if (!device_attach_att(device, io)) >>>> + if (!device_attach_att(device, io, true)) >>>> goto done; >>>> >>>> if (attcb->success) >>>> diff --git a/src/device.h b/src/device.h >>>> index db10827..5c01ed0 100644 >>>> --- a/src/device.h >>>> +++ b/src/device.h >>>> @@ -72,7 +72,7 @@ struct bt_gatt_client *btd_device_get_gatt_client(struct btd_device *device); >>>> struct bt_gatt_server *btd_device_get_gatt_server(struct btd_device *device); >>>> void btd_device_gatt_set_service_changed(struct btd_device *device, >>>> uint16_t start, uint16_t end); >>>> -bool device_attach_att(struct btd_device *dev, GIOChannel *io); >>>> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator); >>>> void btd_device_add_uuid(struct btd_device *device, const char *uuid); >>>> void device_add_eir_uuids(struct btd_device *dev, GSList *uuids); >>>> void device_set_manufacturer_data(struct btd_device *dev, GSList *list); >>>> diff --git a/src/gatt-database.c b/src/gatt-database.c >>>> index d906e2b..4df4a16 100644 >>>> --- a/src/gatt-database.c >>>> +++ b/src/gatt-database.c >>>> @@ -499,7 +499,7 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) >>>> if (!device) >>>> return; >>>> >>>> - device_attach_att(device, io); >>>> + device_attach_att(device, io, false); >>> >>> Not sure if it works when you use white list for autoconnect because >>> iirc we got this callback when BlueZ is initiator >>> then. But still initiator or not, the important thing is if we act as >>> client or server. >> >> Probably not, > > I just run the test: > > 1) add device to autoconnect list. > 2) Simulate disconnect by power off. > 3) After then power on device and wait until auto connect do its job: > > I got this: > > Mar 4 16:30:36 t450s bluetoothd[31211]: > src/adapter.c:connected_callback() hci0 device XX:XX:XX:XX:XX:XX > connected eir_len 14 > Mar 4 16:30:36 t450s bluetoothd[31211]: > src/gatt-database.c:connect_cb() New incoming LE ATT connection > > That means that with this patch after reconnect a default MTU will be > used unless remote device starts > exchange MTU procedure. > > I will have to add more logic to detect how it got >> connected, maybe we can actually check who is the master so that the >> master should be the one controlling the MTU. >> > > Maybe we can do exchange MTU later after discovery ? For discovery > default MTU should be enough. Well we still need to detect the role to do that anyway, and I wouldn't bother with MTU request being the slave since the MTU shall be symmetric: BLUETOOTH SPECIFICATION Version 4.2 [Vol 3, Part F] page 484: 1. A device's Exchange MTU Request shall contain the same MTU as the device's Exchange MTU Response (i.e. the MTU shall be symmetric). ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral 2016-03-09 12:13 ` Luiz Augusto von Dentz @ 2016-03-10 7:59 ` Łukasz Rymanowski 2016-03-10 8:30 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 16+ messages in thread From: Łukasz Rymanowski @ 2016-03-10 7:59 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org Hi Luiz, On 9 March 2016 at 13:13, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > Hi Lukasz, > > On Fri, Mar 4, 2016 at 5:46 PM, Łukasz Rymanowski > <lukasz.rymanowski@codecoup.pl> wrote: >> Hi Luiz, >> >> On 4 March 2016 at 14:12, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >>> Hi Łukasz, >>> >>> On Fri, Mar 4, 2016 at 2:51 PM, Łukasz Rymanowski >>> <lukasz.rymanowski@codecoup.pl> wrote: >>>> Hi Luiz, >>>> >>>> On 4 March 2016 at 13:24, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> >>>>> >>>>> Some Android versions don't seem to cope well with the fact that the >>>>> peripheral can actually set a MTU thus leave the MTU as the default. >>>> >>>> Just tested that and for me connection with Android works OK even BlueZ >>>> started Exchange MTU procedure. Tested on Android 5 and 6. >>>> >>>> I saw once the issue that Android could not retrieve services after >>>> connection to BlueZ >>>> but from the logs it looked like BlueZ start search for primary >>>> services and Android not. >>>> If Android make to sent search request for primary services first then >>>> it was fine. >>>> >>>> Maybe Android don't like BlueZ to start search primary if Android is >>>> initiator? But this is their bug >>>> BTW. I saw it on Android 5 not yet on 6. >>> >>> With 4.4 it actually made a difference. >>> >>>> >>>>> --- >>>>> src/attrib-server.c | 2 +- >>>>> src/device.c | 8 +++++--- >>>>> src/device.h | 2 +- >>>>> src/gatt-database.c | 2 +- >>>>> 4 files changed, 8 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/src/attrib-server.c b/src/attrib-server.c >>>>> index 4439c27..e92ca5c 100644 >>>>> --- a/src/attrib-server.c >>>>> +++ b/src/attrib-server.c >>>>> @@ -1281,7 +1281,7 @@ static void connect_event(GIOChannel *io, GError *gerr, void *user_data) >>>>> if (!device) >>>>> return; >>>>> >>>>> - device_attach_att(device, io); >>>>> + device_attach_att(device, io, false); >>>>> } >>>>> >>>>> static gboolean register_core_services(struct gatt_server *server) >>>>> diff --git a/src/device.c b/src/device.c >>>>> index 14e850e..b4bc593 100644 >>>>> --- a/src/device.c >>>>> +++ b/src/device.c >>>>> @@ -4664,7 +4664,7 @@ static bool remote_counter(uint32_t *sign_cnt, void *user_data) >>>>> return true; >>>>> } >>>>> >>>>> -bool device_attach_att(struct btd_device *dev, GIOChannel *io) >>>>> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator) >>>>> { >>>>> GError *gerr = NULL; >>>>> GAttrib *attrib; >>>>> @@ -4699,7 +4699,9 @@ bool device_attach_att(struct btd_device *dev, GIOChannel *io) >>>>> } >>>>> } >>>>> >>>>> - dev->att_mtu = MIN(mtu, BT_ATT_MAX_LE_MTU); >>>>> + /* Only attempt to set MTU if initiator/central */ >>>> >>>> Keep in mind that MTU echange procedure is bonded to Client/Server >>>> role not central/peripheral. >>> >>> Well actually is not either GAP or GATT role related since both side >>> are in fact allowed to do that, so this is just a workaround. >> >> According to spec Exchange MTU procedure can be started by Client or >> devices supporting Client/Server role. >> BlueZ always acts as client/server this is why you can always start >> this procedure. >> It is really not related to central/peripheral. > > Well the working of client/server role seems very odd to say the > least, why the use of client and server role if they could always be > both, but it doesn't change the fact that either side can send MTU > Exchange. > Server only device cannot start Exchange MTU procedure. But since BlueZ cannot be configured this way that is not a case. >>> >>>> >>>>> + dev->att_mtu = initiator ? MIN(mtu, BT_ATT_MAX_LE_MTU) : >>>>> + BT_ATT_DEFAULT_LE_MTU; >>>>> attrib = g_attrib_new(io, dev->att_mtu, false); >>>>> if (!attrib) { >>>>> error("Unable to create new GAttrib instance"); >>>>> @@ -4768,7 +4770,7 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) >>>>> goto done; >>>>> } >>>>> >>>>> - if (!device_attach_att(device, io)) >>>>> + if (!device_attach_att(device, io, true)) >>>>> goto done; >>>>> >>>>> if (attcb->success) >>>>> diff --git a/src/device.h b/src/device.h >>>>> index db10827..5c01ed0 100644 >>>>> --- a/src/device.h >>>>> +++ b/src/device.h >>>>> @@ -72,7 +72,7 @@ struct bt_gatt_client *btd_device_get_gatt_client(struct btd_device *device); >>>>> struct bt_gatt_server *btd_device_get_gatt_server(struct btd_device *device); >>>>> void btd_device_gatt_set_service_changed(struct btd_device *device, >>>>> uint16_t start, uint16_t end); >>>>> -bool device_attach_att(struct btd_device *dev, GIOChannel *io); >>>>> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator); >>>>> void btd_device_add_uuid(struct btd_device *device, const char *uuid); >>>>> void device_add_eir_uuids(struct btd_device *dev, GSList *uuids); >>>>> void device_set_manufacturer_data(struct btd_device *dev, GSList *list); >>>>> diff --git a/src/gatt-database.c b/src/gatt-database.c >>>>> index d906e2b..4df4a16 100644 >>>>> --- a/src/gatt-database.c >>>>> +++ b/src/gatt-database.c >>>>> @@ -499,7 +499,7 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) >>>>> if (!device) >>>>> return; >>>>> >>>>> - device_attach_att(device, io); >>>>> + device_attach_att(device, io, false); >>>> >>>> Not sure if it works when you use white list for autoconnect because >>>> iirc we got this callback when BlueZ is initiator >>>> then. But still initiator or not, the important thing is if we act as >>>> client or server. >>> >>> Probably not, >> >> I just run the test: >> >> 1) add device to autoconnect list. >> 2) Simulate disconnect by power off. >> 3) After then power on device and wait until auto connect do its job: >> >> I got this: >> >> Mar 4 16:30:36 t450s bluetoothd[31211]: >> src/adapter.c:connected_callback() hci0 device XX:XX:XX:XX:XX:XX >> connected eir_len 14 >> Mar 4 16:30:36 t450s bluetoothd[31211]: >> src/gatt-database.c:connect_cb() New incoming LE ATT connection >> >> That means that with this patch after reconnect a default MTU will be >> used unless remote device starts >> exchange MTU procedure. >> >> I will have to add more logic to detect how it got >>> connected, maybe we can actually check who is the master so that the >>> master should be the one controlling the MTU. >>> >> >> Maybe we can do exchange MTU later after discovery ? For discovery >> default MTU should be enough. > > Well we still need to detect the role to do that anyway, and I > wouldn't bother with MTU request being the slave since the MTU shall > be symmetric: > > BLUETOOTH SPECIFICATION Version 4.2 [Vol 3, Part F] page 484: > 1. A device's Exchange MTU Request shall contain the same MTU as the > device's Exchange MTU Response (i.e. the MTU shall be symmetric). Yes, MTU is symmetric and this is not a discussion point. Bottom line here is like this: 1. Current BlueZ acts correctly in MTU exchange context 2. We try to workaround for not correctly working Android 4.4 which has issue with BlueZ sending Exchange MTU request 3. role on the link is not relevant. There can be devices being Peripheral/Clienr or Central/Server. Central/Server will never send Exchange MTU Request and if BlueZ rely on the role on the link we will operate on Default MTU even though we can do better. -- BR / Pozdrawiam Łukasz ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral 2016-03-10 7:59 ` Łukasz Rymanowski @ 2016-03-10 8:30 ` Luiz Augusto von Dentz 2016-03-10 9:17 ` Łukasz Rymanowski 0 siblings, 1 reply; 16+ messages in thread From: Luiz Augusto von Dentz @ 2016-03-10 8:30 UTC (permalink / raw) To: Łukasz Rymanowski; +Cc: linux-bluetooth@vger.kernel.org Hi Lukasz, On Thu, Mar 10, 2016 at 9:59 AM, Łukasz Rymanowski <lukasz.rymanowski@codecoup.pl> wrote: > Hi Luiz, > > On 9 March 2016 at 13:13, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >> Hi Lukasz, >> >> On Fri, Mar 4, 2016 at 5:46 PM, Łukasz Rymanowski >> <lukasz.rymanowski@codecoup.pl> wrote: >>> Hi Luiz, >>> >>> On 4 March 2016 at 14:12, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >>>> Hi Łukasz, >>>> >>>> On Fri, Mar 4, 2016 at 2:51 PM, Łukasz Rymanowski >>>> <lukasz.rymanowski@codecoup.pl> wrote: >>>>> Hi Luiz, >>>>> >>>>> On 4 March 2016 at 13:24, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >>>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> >>>>>> >>>>>> Some Android versions don't seem to cope well with the fact that the >>>>>> peripheral can actually set a MTU thus leave the MTU as the default. >>>>> >>>>> Just tested that and for me connection with Android works OK even BlueZ >>>>> started Exchange MTU procedure. Tested on Android 5 and 6. >>>>> >>>>> I saw once the issue that Android could not retrieve services after >>>>> connection to BlueZ >>>>> but from the logs it looked like BlueZ start search for primary >>>>> services and Android not. >>>>> If Android make to sent search request for primary services first then >>>>> it was fine. >>>>> >>>>> Maybe Android don't like BlueZ to start search primary if Android is >>>>> initiator? But this is their bug >>>>> BTW. I saw it on Android 5 not yet on 6. >>>> >>>> With 4.4 it actually made a difference. >>>> >>>>> >>>>>> --- >>>>>> src/attrib-server.c | 2 +- >>>>>> src/device.c | 8 +++++--- >>>>>> src/device.h | 2 +- >>>>>> src/gatt-database.c | 2 +- >>>>>> 4 files changed, 8 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/src/attrib-server.c b/src/attrib-server.c >>>>>> index 4439c27..e92ca5c 100644 >>>>>> --- a/src/attrib-server.c >>>>>> +++ b/src/attrib-server.c >>>>>> @@ -1281,7 +1281,7 @@ static void connect_event(GIOChannel *io, GError *gerr, void *user_data) >>>>>> if (!device) >>>>>> return; >>>>>> >>>>>> - device_attach_att(device, io); >>>>>> + device_attach_att(device, io, false); >>>>>> } >>>>>> >>>>>> static gboolean register_core_services(struct gatt_server *server) >>>>>> diff --git a/src/device.c b/src/device.c >>>>>> index 14e850e..b4bc593 100644 >>>>>> --- a/src/device.c >>>>>> +++ b/src/device.c >>>>>> @@ -4664,7 +4664,7 @@ static bool remote_counter(uint32_t *sign_cnt, void *user_data) >>>>>> return true; >>>>>> } >>>>>> >>>>>> -bool device_attach_att(struct btd_device *dev, GIOChannel *io) >>>>>> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator) >>>>>> { >>>>>> GError *gerr = NULL; >>>>>> GAttrib *attrib; >>>>>> @@ -4699,7 +4699,9 @@ bool device_attach_att(struct btd_device *dev, GIOChannel *io) >>>>>> } >>>>>> } >>>>>> >>>>>> - dev->att_mtu = MIN(mtu, BT_ATT_MAX_LE_MTU); >>>>>> + /* Only attempt to set MTU if initiator/central */ >>>>> >>>>> Keep in mind that MTU echange procedure is bonded to Client/Server >>>>> role not central/peripheral. >>>> >>>> Well actually is not either GAP or GATT role related since both side >>>> are in fact allowed to do that, so this is just a workaround. >>> >>> According to spec Exchange MTU procedure can be started by Client or >>> devices supporting Client/Server role. >>> BlueZ always acts as client/server this is why you can always start >>> this procedure. >>> It is really not related to central/peripheral. >> >> Well the working of client/server role seems very odd to say the >> least, why the use of client and server role if they could always be >> both, but it doesn't change the fact that either side can send MTU >> Exchange. >> > > Server only device cannot start Exchange MTU procedure. But since BlueZ cannot > be configured this way that is not a case. Not only we cannot be configured as server, but even if we do the roles are only relevant for PTS because otherwise there is no way to tell the roles of the remote device. >>>> >>>>> >>>>>> + dev->att_mtu = initiator ? MIN(mtu, BT_ATT_MAX_LE_MTU) : >>>>>> + BT_ATT_DEFAULT_LE_MTU; >>>>>> attrib = g_attrib_new(io, dev->att_mtu, false); >>>>>> if (!attrib) { >>>>>> error("Unable to create new GAttrib instance"); >>>>>> @@ -4768,7 +4770,7 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) >>>>>> goto done; >>>>>> } >>>>>> >>>>>> - if (!device_attach_att(device, io)) >>>>>> + if (!device_attach_att(device, io, true)) >>>>>> goto done; >>>>>> >>>>>> if (attcb->success) >>>>>> diff --git a/src/device.h b/src/device.h >>>>>> index db10827..5c01ed0 100644 >>>>>> --- a/src/device.h >>>>>> +++ b/src/device.h >>>>>> @@ -72,7 +72,7 @@ struct bt_gatt_client *btd_device_get_gatt_client(struct btd_device *device); >>>>>> struct bt_gatt_server *btd_device_get_gatt_server(struct btd_device *device); >>>>>> void btd_device_gatt_set_service_changed(struct btd_device *device, >>>>>> uint16_t start, uint16_t end); >>>>>> -bool device_attach_att(struct btd_device *dev, GIOChannel *io); >>>>>> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator); >>>>>> void btd_device_add_uuid(struct btd_device *device, const char *uuid); >>>>>> void device_add_eir_uuids(struct btd_device *dev, GSList *uuids); >>>>>> void device_set_manufacturer_data(struct btd_device *dev, GSList *list); >>>>>> diff --git a/src/gatt-database.c b/src/gatt-database.c >>>>>> index d906e2b..4df4a16 100644 >>>>>> --- a/src/gatt-database.c >>>>>> +++ b/src/gatt-database.c >>>>>> @@ -499,7 +499,7 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) >>>>>> if (!device) >>>>>> return; >>>>>> >>>>>> - device_attach_att(device, io); >>>>>> + device_attach_att(device, io, false); >>>>> >>>>> Not sure if it works when you use white list for autoconnect because >>>>> iirc we got this callback when BlueZ is initiator >>>>> then. But still initiator or not, the important thing is if we act as >>>>> client or server. >>>> >>>> Probably not, >>> >>> I just run the test: >>> >>> 1) add device to autoconnect list. >>> 2) Simulate disconnect by power off. >>> 3) After then power on device and wait until auto connect do its job: >>> >>> I got this: >>> >>> Mar 4 16:30:36 t450s bluetoothd[31211]: >>> src/adapter.c:connected_callback() hci0 device XX:XX:XX:XX:XX:XX >>> connected eir_len 14 >>> Mar 4 16:30:36 t450s bluetoothd[31211]: >>> src/gatt-database.c:connect_cb() New incoming LE ATT connection >>> >>> That means that with this patch after reconnect a default MTU will be >>> used unless remote device starts >>> exchange MTU procedure. >>> >>> I will have to add more logic to detect how it got >>>> connected, maybe we can actually check who is the master so that the >>>> master should be the one controlling the MTU. >>>> >>> >>> Maybe we can do exchange MTU later after discovery ? For discovery >>> default MTU should be enough. >> >> Well we still need to detect the role to do that anyway, and I >> wouldn't bother with MTU request being the slave since the MTU shall >> be symmetric: >> >> BLUETOOTH SPECIFICATION Version 4.2 [Vol 3, Part F] page 484: >> 1. A device's Exchange MTU Request shall contain the same MTU as the >> device's Exchange MTU Response (i.e. the MTU shall be symmetric). > > > Yes, MTU is symmetric and this is not a discussion point. > Bottom line here is like this: > > 1. Current BlueZ acts correctly in MTU exchange context > 2. We try to workaround for not correctly working Android 4.4 which > has issue with BlueZ sending Exchange MTU request Im fine dropping this if it only affects 4.4, but we need to check later version if that was really fixed. > 3. role on the link is not relevant. There can be devices being > Peripheral/Clienr or Central/Server. Central/Server will never send > Exchange MTU Request and if BlueZ rely on the role on the link we will > operate on Default MTU even though we can do better. Indeed, different profiles may have different roles, I don't think I have anything otherwise. Anyway most devices respond with Default MTU anyway so chances are this doesn't change anything. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral 2016-03-10 8:30 ` Luiz Augusto von Dentz @ 2016-03-10 9:17 ` Łukasz Rymanowski 2016-03-10 9:35 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 16+ messages in thread From: Łukasz Rymanowski @ 2016-03-10 9:17 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org Hi Luiz, On 10 March 2016 at 09:30, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > Hi Lukasz, > > On Thu, Mar 10, 2016 at 9:59 AM, Łukasz Rymanowski > <lukasz.rymanowski@codecoup.pl> wrote: >> Hi Luiz, >> >> On 9 March 2016 at 13:13, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >>> Hi Lukasz, >>> >>> On Fri, Mar 4, 2016 at 5:46 PM, Łukasz Rymanowski >>> <lukasz.rymanowski@codecoup.pl> wrote: >>>> Hi Luiz, >>>> >>>> On 4 March 2016 at 14:12, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >>>>> Hi Łukasz, >>>>> >>>>> On Fri, Mar 4, 2016 at 2:51 PM, Łukasz Rymanowski >>>>> <lukasz.rymanowski@codecoup.pl> wrote: >>>>>> Hi Luiz, >>>>>> >>>>>> On 4 March 2016 at 13:24, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >>>>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> >>>>>>> >>>>>>> Some Android versions don't seem to cope well with the fact that the >>>>>>> peripheral can actually set a MTU thus leave the MTU as the default. >>>>>> >>>>>> Just tested that and for me connection with Android works OK even BlueZ >>>>>> started Exchange MTU procedure. Tested on Android 5 and 6. >>>>>> >>>>>> I saw once the issue that Android could not retrieve services after >>>>>> connection to BlueZ >>>>>> but from the logs it looked like BlueZ start search for primary >>>>>> services and Android not. >>>>>> If Android make to sent search request for primary services first then >>>>>> it was fine. >>>>>> >>>>>> Maybe Android don't like BlueZ to start search primary if Android is >>>>>> initiator? But this is their bug >>>>>> BTW. I saw it on Android 5 not yet on 6. >>>>> >>>>> With 4.4 it actually made a difference. >>>>> >>>>>> >>>>>>> --- >>>>>>> src/attrib-server.c | 2 +- >>>>>>> src/device.c | 8 +++++--- >>>>>>> src/device.h | 2 +- >>>>>>> src/gatt-database.c | 2 +- >>>>>>> 4 files changed, 8 insertions(+), 6 deletions(-) >>>>>>> >>>>>>> diff --git a/src/attrib-server.c b/src/attrib-server.c >>>>>>> index 4439c27..e92ca5c 100644 >>>>>>> --- a/src/attrib-server.c >>>>>>> +++ b/src/attrib-server.c >>>>>>> @@ -1281,7 +1281,7 @@ static void connect_event(GIOChannel *io, GError *gerr, void *user_data) >>>>>>> if (!device) >>>>>>> return; >>>>>>> >>>>>>> - device_attach_att(device, io); >>>>>>> + device_attach_att(device, io, false); >>>>>>> } >>>>>>> >>>>>>> static gboolean register_core_services(struct gatt_server *server) >>>>>>> diff --git a/src/device.c b/src/device.c >>>>>>> index 14e850e..b4bc593 100644 >>>>>>> --- a/src/device.c >>>>>>> +++ b/src/device.c >>>>>>> @@ -4664,7 +4664,7 @@ static bool remote_counter(uint32_t *sign_cnt, void *user_data) >>>>>>> return true; >>>>>>> } >>>>>>> >>>>>>> -bool device_attach_att(struct btd_device *dev, GIOChannel *io) >>>>>>> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator) >>>>>>> { >>>>>>> GError *gerr = NULL; >>>>>>> GAttrib *attrib; >>>>>>> @@ -4699,7 +4699,9 @@ bool device_attach_att(struct btd_device *dev, GIOChannel *io) >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> - dev->att_mtu = MIN(mtu, BT_ATT_MAX_LE_MTU); >>>>>>> + /* Only attempt to set MTU if initiator/central */ >>>>>> >>>>>> Keep in mind that MTU echange procedure is bonded to Client/Server >>>>>> role not central/peripheral. >>>>> >>>>> Well actually is not either GAP or GATT role related since both side >>>>> are in fact allowed to do that, so this is just a workaround. >>>> >>>> According to spec Exchange MTU procedure can be started by Client or >>>> devices supporting Client/Server role. >>>> BlueZ always acts as client/server this is why you can always start >>>> this procedure. >>>> It is really not related to central/peripheral. >>> >>> Well the working of client/server role seems very odd to say the >>> least, why the use of client and server role if they could always be >>> both, but it doesn't change the fact that either side can send MTU >>> Exchange. >>> >> >> Server only device cannot start Exchange MTU procedure. But since BlueZ cannot >> be configured this way that is not a case. > > Not only we cannot be configured as server, but even if we do the > roles are only relevant for PTS because otherwise there is no way to > tell the roles of the remote device. If device sends request that means it is a Client. If not that means it is a Server. > >>>>> >>>>>> >>>>>>> + dev->att_mtu = initiator ? MIN(mtu, BT_ATT_MAX_LE_MTU) : >>>>>>> + BT_ATT_DEFAULT_LE_MTU; >>>>>>> attrib = g_attrib_new(io, dev->att_mtu, false); >>>>>>> if (!attrib) { >>>>>>> error("Unable to create new GAttrib instance"); >>>>>>> @@ -4768,7 +4770,7 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) >>>>>>> goto done; >>>>>>> } >>>>>>> >>>>>>> - if (!device_attach_att(device, io)) >>>>>>> + if (!device_attach_att(device, io, true)) >>>>>>> goto done; >>>>>>> >>>>>>> if (attcb->success) >>>>>>> diff --git a/src/device.h b/src/device.h >>>>>>> index db10827..5c01ed0 100644 >>>>>>> --- a/src/device.h >>>>>>> +++ b/src/device.h >>>>>>> @@ -72,7 +72,7 @@ struct bt_gatt_client *btd_device_get_gatt_client(struct btd_device *device); >>>>>>> struct bt_gatt_server *btd_device_get_gatt_server(struct btd_device *device); >>>>>>> void btd_device_gatt_set_service_changed(struct btd_device *device, >>>>>>> uint16_t start, uint16_t end); >>>>>>> -bool device_attach_att(struct btd_device *dev, GIOChannel *io); >>>>>>> +bool device_attach_att(struct btd_device *dev, GIOChannel *io, bool initiator); >>>>>>> void btd_device_add_uuid(struct btd_device *device, const char *uuid); >>>>>>> void device_add_eir_uuids(struct btd_device *dev, GSList *uuids); >>>>>>> void device_set_manufacturer_data(struct btd_device *dev, GSList *list); >>>>>>> diff --git a/src/gatt-database.c b/src/gatt-database.c >>>>>>> index d906e2b..4df4a16 100644 >>>>>>> --- a/src/gatt-database.c >>>>>>> +++ b/src/gatt-database.c >>>>>>> @@ -499,7 +499,7 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data) >>>>>>> if (!device) >>>>>>> return; >>>>>>> >>>>>>> - device_attach_att(device, io); >>>>>>> + device_attach_att(device, io, false); >>>>>> >>>>>> Not sure if it works when you use white list for autoconnect because >>>>>> iirc we got this callback when BlueZ is initiator >>>>>> then. But still initiator or not, the important thing is if we act as >>>>>> client or server. >>>>> >>>>> Probably not, >>>> >>>> I just run the test: >>>> >>>> 1) add device to autoconnect list. >>>> 2) Simulate disconnect by power off. >>>> 3) After then power on device and wait until auto connect do its job: >>>> >>>> I got this: >>>> >>>> Mar 4 16:30:36 t450s bluetoothd[31211]: >>>> src/adapter.c:connected_callback() hci0 device XX:XX:XX:XX:XX:XX >>>> connected eir_len 14 >>>> Mar 4 16:30:36 t450s bluetoothd[31211]: >>>> src/gatt-database.c:connect_cb() New incoming LE ATT connection >>>> >>>> That means that with this patch after reconnect a default MTU will be >>>> used unless remote device starts >>>> exchange MTU procedure. >>>> >>>> I will have to add more logic to detect how it got >>>>> connected, maybe we can actually check who is the master so that the >>>>> master should be the one controlling the MTU. >>>>> >>>> >>>> Maybe we can do exchange MTU later after discovery ? For discovery >>>> default MTU should be enough. >>> >>> Well we still need to detect the role to do that anyway, and I >>> wouldn't bother with MTU request being the slave since the MTU shall >>> be symmetric: >>> >>> BLUETOOTH SPECIFICATION Version 4.2 [Vol 3, Part F] page 484: >>> 1. A device's Exchange MTU Request shall contain the same MTU as the >>> device's Exchange MTU Response (i.e. the MTU shall be symmetric). >> >> >> Yes, MTU is symmetric and this is not a discussion point. >> Bottom line here is like this: >> >> 1. Current BlueZ acts correctly in MTU exchange context >> 2. We try to workaround for not correctly working Android 4.4 which >> has issue with BlueZ sending Exchange MTU request > > Im fine dropping this if it only affects 4.4, but we need to check > later version if that was really fixed. > >> 3. role on the link is not relevant. There can be devices being >> Peripheral/Clienr or Central/Server. Central/Server will never send >> Exchange MTU Request and if BlueZ rely on the role on the link we will >> operate on Default MTU even though we can do better. > > Indeed, different profiles may have different roles, I don't think I > have anything otherwise. Anyway most devices respond with Default MTU > anyway so chances are this doesn't change anything. > > > -- > Luiz Augusto von Dentz -- BR / Pozdrawiam Łukasz ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral 2016-03-10 9:17 ` Łukasz Rymanowski @ 2016-03-10 9:35 ` Luiz Augusto von Dentz 2016-03-10 9:42 ` Szymon Janc 0 siblings, 1 reply; 16+ messages in thread From: Luiz Augusto von Dentz @ 2016-03-10 9:35 UTC (permalink / raw) To: Łukasz Rymanowski; +Cc: linux-bluetooth@vger.kernel.org Hi Lukasz, On Thu, Mar 10, 2016 at 11:17 AM, Łukasz Rymanowski <lukasz.rymanowski@codecoup.pl> wrote: >> Not only we cannot be configured as server, but even if we do the >> roles are only relevant for PTS because otherwise there is no way to >> tell the roles of the remote device. > > If device sends request that means it is a Client. If not that means > it is a Server. Well if it send it can be a Client/Server as well, btw some systems including Android appear to have support to send Exchange MTU at any point so detecting a server only pretty hard, besides our current design split Server and Client functionality so waiting to see if anything has been received before starting sending anything is not so trivial and we had still to choose an arbitrary timeout to wait for detecting the role but if both are based on BlueZ this would just delay each other. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral 2016-03-10 9:35 ` Luiz Augusto von Dentz @ 2016-03-10 9:42 ` Szymon Janc 2016-03-10 10:26 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 16+ messages in thread From: Szymon Janc @ 2016-03-10 9:42 UTC (permalink / raw) To: Luiz Augusto von Dentz Cc: Łukasz Rymanowski, linux-bluetooth@vger.kernel.org Hi Luiz, On Thursday 10 of March 2016 11:35:23 Luiz Augusto von Dentz wrote: > Hi Lukasz, > > On Thu, Mar 10, 2016 at 11:17 AM, Łukasz Rymanowski > > <lukasz.rymanowski@codecoup.pl> wrote: > >> Not only we cannot be configured as server, but even if we do the > >> roles are only relevant for PTS because otherwise there is no way to > >> tell the roles of the remote device. > > > > If device sends request that means it is a Client. If not that means > > it is a Server. > > Well if it send it can be a Client/Server as well, btw some systems > including Android appear to have support to send Exchange MTU at any > point so detecting a server only pretty hard, besides our current > design split Server and Client functionality so waiting to see if > anything has been received before starting sending anything is not so > trivial and we had still to choose an arbitrary timeout to wait for > detecting the role but if both are based on BlueZ this would just > delay each other. Maybe we could simply delay MTU exchange until we are done with discovery? For discovery we shouldn't need larger MTU. And if in the meantime remote initiates MTU exchange we can skip MTU exchange from our side. -- BR Szymon Janc ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral 2016-03-10 9:42 ` Szymon Janc @ 2016-03-10 10:26 ` Luiz Augusto von Dentz 2016-03-11 14:46 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 16+ messages in thread From: Luiz Augusto von Dentz @ 2016-03-10 10:26 UTC (permalink / raw) To: Szymon Janc; +Cc: Łukasz Rymanowski, linux-bluetooth@vger.kernel.org Hi Szymon, On Thu, Mar 10, 2016 at 11:42 AM, Szymon Janc <ext.szymon.janc@tieto.com> wrote: > Hi Luiz, > > On Thursday 10 of March 2016 11:35:23 Luiz Augusto von Dentz wrote: >> Hi Lukasz, >> >> On Thu, Mar 10, 2016 at 11:17 AM, Łukasz Rymanowski >> >> <lukasz.rymanowski@codecoup.pl> wrote: >> >> Not only we cannot be configured as server, but even if we do the >> >> roles are only relevant for PTS because otherwise there is no way to >> >> tell the roles of the remote device. >> > >> > If device sends request that means it is a Client. If not that means >> > it is a Server. >> >> Well if it send it can be a Client/Server as well, btw some systems >> including Android appear to have support to send Exchange MTU at any >> point so detecting a server only pretty hard, besides our current >> design split Server and Client functionality so waiting to see if >> anything has been received before starting sending anything is not so >> trivial and we had still to choose an arbitrary timeout to wait for >> detecting the role but if both are based on BlueZ this would just >> delay each other. > > Maybe we could simply delay MTU exchange until we are done with discovery? > For discovery we shouldn't need larger MTU. And if in the meantime remote > initiates MTU exchange we can skip MTU exchange from our side. Actually discovery is where it makes most of the difference for devices with big databases which delays quite a lot the first connection (sometimes more than 10 seconds to discover everything). We do have a cache to mitigate that in the subsequent connections, but I wouldn't like to receive complains again that discovery takes too long because the MTU was not changed. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral 2016-03-10 10:26 ` Luiz Augusto von Dentz @ 2016-03-11 14:46 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 16+ messages in thread From: Luiz Augusto von Dentz @ 2016-03-11 14:46 UTC (permalink / raw) To: Szymon Janc; +Cc: Łukasz Rymanowski, linux-bluetooth@vger.kernel.org Hi Szymon, Lukasz, On Thu, Mar 10, 2016 at 12:26 PM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > Hi Szymon, > > On Thu, Mar 10, 2016 at 11:42 AM, Szymon Janc <ext.szymon.janc@tieto.com> wrote: >> Hi Luiz, >> >> On Thursday 10 of March 2016 11:35:23 Luiz Augusto von Dentz wrote: >>> Hi Lukasz, >>> >>> On Thu, Mar 10, 2016 at 11:17 AM, Łukasz Rymanowski >>> >>> <lukasz.rymanowski@codecoup.pl> wrote: >>> >> Not only we cannot be configured as server, but even if we do the >>> >> roles are only relevant for PTS because otherwise there is no way to >>> >> tell the roles of the remote device. >>> > >>> > If device sends request that means it is a Client. If not that means >>> > it is a Server. >>> >>> Well if it send it can be a Client/Server as well, btw some systems >>> including Android appear to have support to send Exchange MTU at any >>> point so detecting a server only pretty hard, besides our current >>> design split Server and Client functionality so waiting to see if >>> anything has been received before starting sending anything is not so >>> trivial and we had still to choose an arbitrary timeout to wait for >>> detecting the role but if both are based on BlueZ this would just >>> delay each other. >> >> Maybe we could simply delay MTU exchange until we are done with discovery? >> For discovery we shouldn't need larger MTU. And if in the meantime remote >> initiates MTU exchange we can skip MTU exchange from our side. > > Actually discovery is where it makes most of the difference for > devices with big databases which delays quite a lot the first > connection (sometimes more than 10 seconds to discover everything). We > do have a cache to mitigate that in the subsequent connections, but I > wouldn't like to receive complains again that discovery takes too long > because the MTU was not changed. Im actually abandoning this one as it seems Androind 5.1 and later seems to have this fixed. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH BlueZ 1/2] shared/gatt-client: Don't send Exchange MTU for default value 2016-03-04 12:24 [PATCH BlueZ 1/2] shared/gatt-client: Don't send Exchange MTU for default value Luiz Augusto von Dentz 2016-03-04 12:24 ` [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral Luiz Augusto von Dentz @ 2016-03-11 14:44 ` Luiz Augusto von Dentz 1 sibling, 0 replies; 16+ messages in thread From: Luiz Augusto von Dentz @ 2016-03-11 14:44 UTC (permalink / raw) To: linux-bluetooth@vger.kernel.org Hi, On Fri, Mar 4, 2016 at 2:24 PM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > If the MTU to be exchange is the default there is no pointing in sending > it since the remote already assumes it anyway. > --- > src/shared/gatt-client.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c > index b255175..45acf7b 100644 > --- a/src/shared/gatt-client.c > +++ b/src/shared/gatt-client.c > @@ -1544,6 +1544,11 @@ static bool gatt_client_init(struct bt_gatt_client *client, uint16_t mtu) > if (!op) > return false; > > + /* Check if MTU needs to be send */ > + mtu = MAX(BT_ATT_DEFAULT_LE_MTU, mtu); > + if (mtu == BT_ATT_DEFAULT_LE_MTU) > + goto discover; > + > /* Configure the MTU */ > client->mtu_req_id = bt_gatt_exchange_mtu(client->att, > MAX(BT_ATT_DEFAULT_LE_MTU, mtu), > @@ -1558,6 +1563,20 @@ static bool gatt_client_init(struct bt_gatt_client *client, uint16_t mtu) > client->in_init = true; > > return true; > + > +discover: > + client->discovery_req = bt_gatt_discover_all_primary_services( > + client->att, NULL, > + discover_primary_cb, > + discovery_op_ref(op), > + discovery_op_unref); > + if (!client->discovery_req) { > + discovery_op_free(op); > + return false; > + } > + > + client->in_init = true; > + return true; > } > > struct pdu_data { > -- > 2.5.0 Applied (1/2 only). -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-03-11 14:46 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-03-04 12:24 [PATCH BlueZ 1/2] shared/gatt-client: Don't send Exchange MTU for default value Luiz Augusto von Dentz 2016-03-04 12:24 ` [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral Luiz Augusto von Dentz 2016-03-04 12:51 ` Łukasz Rymanowski 2016-03-04 13:12 ` Luiz Augusto von Dentz 2016-03-04 13:35 ` Szymon Janc 2016-03-04 14:38 ` Luiz Augusto von Dentz 2016-03-04 15:46 ` Łukasz Rymanowski 2016-03-09 12:13 ` Luiz Augusto von Dentz 2016-03-10 7:59 ` Łukasz Rymanowski 2016-03-10 8:30 ` Luiz Augusto von Dentz 2016-03-10 9:17 ` Łukasz Rymanowski 2016-03-10 9:35 ` Luiz Augusto von Dentz 2016-03-10 9:42 ` Szymon Janc 2016-03-10 10:26 ` Luiz Augusto von Dentz 2016-03-11 14:46 ` Luiz Augusto von Dentz 2016-03-11 14:44 ` [PATCH BlueZ 1/2] shared/gatt-client: Don't send Exchange MTU for default value Luiz Augusto von Dentz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.