Re: [PATCH BlueZ 2/2] core/device: Don't set MTU when acting as a peripheral

[Szymon Janc <szymon.janc@codecoup.pl>]

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