I was talking to BlueZ folks this morning and we've noticed while looking at https://chromium.googlesource.com/chromiumos/third_party/bluez/+log/chromeos-5.37 that some of these patches were not submitted for review upstream and that others were not relevant or fixed by a new feature in BlueZ such as a new option for remember powered states in main.conf.

Could we make sure to address these issues?

- https://chromium.googlesource.com/chromiumos/third_party/bluez/+/e3b137d8374c40c8a7cfde4e1b99b71f32884fb6: It might be possible to use AutoEnable in main.conf to do that, anyway I don't see a problem to incorporate that in the policy plugin so we could do for example AutoEnable=auto.

- https://chromium.googlesource.com/chromiumos/third_party/bluez/+/ce415a42dcfe87e3d4cd678980c477505744abb2: Is this a chromium or actually chrome OS specific? In case of the later I would recommend it to be renamed chromeos. If it indeed used by chromium in Linux I would like to have it upstream, but there are a few things that needs rework like how it enumerates devices is very inefficient since it uses D-Bus to receives signal while it could just register a driver directly with no IPC involved. I actually manage to use the web bluetooth with chromium + bluez without this plugin so I assume it is chrome OS specific thus we can ignore upstream. Btw, pooling connection info is not very efficient but we could in theory do that when reading the RSSI/TX power property for example so the code can check if connected and then issue the HCI command to read the connection info and then update the RSSI/TX power property.

- https://chromium.googlesource.com/chromiumos/third_party/bluez/+/87773b0e0c433a760aab5712381c3b8ded04a6e0: This could actually be reported upstream since it seems to be fixing a real bug where the old storage was not converted.

-https://chromium.googlesource.com/chromiumos/third_party/bluez/+/1ec8b33f0763070096e34e1abca2f24943ccd4be: This could be removed if the following options are set properly for HFP/HSP UUID:
#[Policy]
#
# The ReconnectUUIDs defines the set of remote services that should try
# to be reconnected to in case of a link loss (link supervision
# timeout). The policy plugin should contain a sane set of values by
# default, but this list can be overridden here. By setting the list to
# empty the reconnection feature gets disabled.
#ReconnectUUIDs=00001112-0000-1000-8000-00805f9b34fb, 0000111f-0000-1000-8000-00805f9b34fb, 0000110a-0000-1000-8000-00805f9b34fb

# ReconnectAttempts define the number of attempts to reconnect after a link
# lost. Setting the value to 0 disables reconnecting feature.
#ReconnectAttempts=7

# ReconnectIntervals define the set of intervals in seconds to use in between
# attempts.
# If the number of attempts defined in ReconnectAttempts is bigger than the
# set of intervals the last interval is repeated until the last attempt.
#ReconnectIntervals=1, 2, 4, 8, 16, 32, 64

- https://chromium.googlesource.com/chromiumos/third_party/bluez/+/dac51d02485dd5d9c0634887397340b41c37860c: We could consider this upstream but the SDP local socket is considered a deprecated interface, instead we recommend to use the Profile API: https://git.kernel.org/cgit/bluetooth/bluez.git/tree/doc/profile-api.txt

For the remaining patches marked with CHROMIUM: I considered either Chrome OS specific or subject to change upstream like in case of making some API stable which in case of GATT we will be doing quite soon.

- https://chromium.googlesource.com/chromiumos/third_party/bluez/+/87773b0e0c433a760aab5712381c3b8ded04a6e0: This could actually be reported upstream since it seems to be fixing a real bug where the old storage was not converted.

According to Jakub, "It was required for some chrome devices that come with pre-paired keyboards, and came with bluez 4.x initially, and were upgraded for X years in a row, with no option to re-pair your keyboard. You can send the patch to upstream, but I don't think it have any value."

Well then please disregard the comments regarding that patch.

This comment is the same as https://bugs.chromium.org/p/chromium/issues/detail?id=600655#c4 but annotated with numbers so that we can keep discuss separately of each item.

[1] https://chromium.googlesource.com/chromiumos/third_party/bluez/+/e3b137d8374c40c8a7cfde4e1b99b71f32884fb6: It might be possible to use AutoEnable in main.conf to do that, anyway I don't see a problem to incorporate that in the policy plugin so we could do for example AutoEnable=auto.

[2] https://chromium.googlesource.com/chromiumos/third_party/bluez/+/ce415a42dcfe87e3d4cd678980c477505744abb2: Is this a chromium or actually chrome OS specific? In case of the later I would recommend it to be renamed chromeos. If it indeed used by chromium in Linux I would like to have it upstream, but there are a few things that needs rework like how it enumerates devices is very inefficient since it uses D-Bus to receives signal while it could just register a driver directly with no IPC involved. I actually manage to use the web bluetooth with chromium + bluez without this plugin so I assume it is chrome OS specific thus we can ignore upstream. Btw, pooling connection info is not very efficient but we could in theory do that when reading the RSSI/TX power property for example so the code can check if connected and then issue the HCI command to read the connection info and then update the RSSI/TX power property.

[3] https://chromium.googlesource.com/chromiumos/third_party/bluez/+/87773b0e0c433a760aab5712381c3b8ded04a6e0: This could actually be reported upstream since it seems to be fixing a real bug where the old storage was not converted.

[4] https://chromium.googlesource.com/chromiumos/third_party/bluez/+/1ec8b33f0763070096e34e1abca2f24943ccd4be: This could be removed if the following options are set properly for HFP/HSP UUID:
#[Policy]
#
# The ReconnectUUIDs defines the set of remote services that should try
# to be reconnected to in case of a link loss (link supervision
# timeout). The policy plugin should contain a sane set of values by
# default, but this list can be overridden here. By setting the list to
# empty the reconnection feature gets disabled.
#ReconnectUUIDs=00001112-0000-1000-8000-00805f9b34fb, 0000111f-0000-1000-8000-00805f9b34fb, 0000110a-0000-1000-8000-00805f9b34fb

# ReconnectAttempts define the number of attempts to reconnect after a link
# lost. Setting the value to 0 disables reconnecting feature.
#ReconnectAttempts=7

# ReconnectIntervals define the set of intervals in seconds to use in between
# attempts.
# If the number of attempts defined in ReconnectAttempts is bigger than the
# set of intervals the last interval is repeated until the last attempt.
#ReconnectIntervals=1, 2, 4, 8, 16, 32, 64

[5] https://chromium.googlesource.com/chromiumos/third_party/bluez/+/dac51d02485dd5d9c0634887397340b41c37860c: We could consider this upstream but the SDP local socket is considered a deprecated interface, instead we recommend to use the Profile API: https://git.kernel.org/cgit/bluetooth/bluez.git/tree/doc/profile-api.txt

For the remaining patches marked with CHROMIUM: I considered either Chrome OS specific or subject to change upstream like in case of making some API stable which in case of GATT we will be doing quite soon.

hychao@ will take care of [4] next week.

 Issue 596050  has been merged into this issue.

The discussion of reducing the Chromium patch should be tracked in a separate issue, since the patches needed to be sent to upstream will be landed in a newer release but not 5.39. Therefore the discussion is moved to https://bugs.chromium.org/p/chromium/issues/detail?id=602353.

https://bugs.chromium.org/p/chromium/issues/detail?id=602348 is created to track the creation of branch chromeos-5.39.

Re [4] of #4:
It has been a while since  crbug.com/527575  so I spent some time look into https://chromium-review.googlesource.com/#/c/330103/3 and recall why I did this instead of using the policy options.

The problem we tried to solve is that: for given BT headset, A2DP was connected but HFP/HSP failed and there was no reconnect triggered.
According to plugins/policy.c the 'default_reconnect' already has the UUIDs of HFP and HSP but it didn't work as I expect. The reason is that the reconnect timer is set at two events: (1) dev disconnect, and (2) dev connect fail.
However in our case the btd_device stays 'connected' for all the time because A2DP was connected, which causes no reconnect timer ever scheduled for HFP or HSP.

The sink_cb() function of policy.c tracks the state change for each btd_service to synchronize the connection of A2DP and AVRCP. I figured this is the approach we need for HFP/HSP reconnect on Chromebook, but might not be okay to merge into policy.c

I didn't spend too much time look into other possible solution to this. I am happy to adopt a cleaner fix if anyone could suggestion one. Also please feel free to correct me if I understand the code wrong.

Yes, I think we should have a similar approach to HFP/HSP  and to A2DP and AVRCP, so if we detect A2DP is connected but HFP/HSP is not then it should trigger an attempt to connect them. Note though that some cars actually have media only mode which could prevent HFP/HSP to connect, in that case it is not an error but anyway it should not disrupt anything in case the retries fails.

Btw, the UUID in default_reconnect[] are HSP_AG_UUID, HFP_AG_UUID and A2DP_SOURCE_UUID, these are the opposite roles that you would normally find in headsets/carkits so chances are this is no even active in Chrome OS. In fact using the reconnection policy is perhaps misleading here since it was intended for link loss recovery not for profile retries, so perhaps we need a different entry for profiles.

Thanks for the detailed explanation Luiz!

Francois: In that case we'll need to keep the chromium specific patch for now, to define the profile retry behavior on Chrome OS devices.

To verify BlueZ5.39+Chromium specific changes do not break the builds and the basic BT functionalities, I ran Bluetooth autotest against the images from *-paladin.

Here is the summary of the tests:

[Cyan]
trybot-cyan-paladin/R52-8248.0.0-b956 Total PASS: 8/22 (36%)
[Falco]
trybot-falco-paladin/R52-8248.0.0-b955 Total PASS: 8/22 (36%)
[Lumpy]
trybot-lumpy-paladin/R52-8248.0.0-b954 Total PASS: 2/22 (9%)
[Ninja]
 trybot-ninja-paladin/R52-8248.0.0-b953 Total PASS: 18/22 (81%
[Veyron_minnie]
trybot-veyron_minnie-paladin/R52-8248.0.0-b952 Total PASS: 22/22 (100%)
[Peach_pi]
trybot-peach_pi-paladin/R52-8248.0.0-b950 Total PASS: 22/22 (100%)
[Nyan_kitty]
 trybot-nyan_kitty-paladin/R52-8248.0.0-b949 ERROR  [stderr] ssh: connect to host chromeos1-row6-shelf1-host3.cros port 22: Connection timed out
[Chell]
trybot-chell-paladin/R52-8248.0.0-b948 Total PASS: 8/22 (36%)
[Skate]
trybot-daisy_skate-paladin/R52-8248.0.0-b947 Total PASS: 22/22 (100%)
[Samus]
trybot-samus-paladin/R52-8248.0.0-b946 Total PASS: 22/22 (100%)
[Monroe]
trybot-monroe-paladin/R52-8248.0.0-b945 Total PASS: 18/22 (81%)
[Lulu]
trybot-lulu-paladin/R52-8248.0.0-b943 Total PASS: 22/22 (100%)

Comparing the test passing rates with the ones listed in WMatrix(R51), the rates are better, so I landed all Chromium CLs. Please see the manifest changes as follows.
https://chromium-review.googlesource.com/#/c/341052/
https://chrome-internal-review.googlesource.com/#/c/257177/

After coordinating with users of Chrome OS BlueZ, and they tested their build against the new branch for 5.39. Everything works fine except for the following D-Bus warnings of the StartNotify/StopNotify methods:

NOTICE dbus[293]: [system] Rejected send message, 3 matched rules; type="method_return", sender=":1.61" (uid=0 pid=6005 comm="python /tmp/example-gatt-server ") interface="(unset)" member="(unset)" error name="(unset)" requested_reply="0" destination=":1.9" (uid=218 pid=548 comm="/usr/libexec/bluetooth/bluetoothd --nodetach ")
b/28547563 is filed for tracking the above warning message.

The plan is to land the manifest changes by the end of 05/04(UTC+08:00).

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/manifest-internal/+/866413b44448016fe248cfd919e92693bc62d75b

commit 866413b44448016fe248cfd919e92693bc62d75b
Author: Miao Chou <mcchou@google.com>
Date: Tue Apr 26 13:52:36 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/manifest/+/3161eade6ce7d68dcaa3f1515c27dadac03694b1

commit 3161eade6ce7d68dcaa3f1515c27dadac03694b1
Author: Miao Chou <mcchou@google.com>
Date: Tue Apr 26 13:55:03 2016

manifest: adopt branch chromeos-5.39 for chromiumos/third_party/bluez

Branch chromeos-5.39 is based on BlueZ 5.39 release with Chromium-specific
patches on top. BlueZ will be updated to 5.39 version by switching the branch
to chromeos-5.39.

BUG= chromium:600655 
TEST=run bluetooth autotest suite against the following platform(passing
rate): Cyan (8/22), Falco (8/22), Lumpy (2/22), Ninja (18/22), Veyron_minnie
(22/22), Peach_pi (22/22), Chell (8/22), Skate (22/22), Samus (22/22), Monroe
(18/22), Lulu (22/22)

Change-Id: Ibe018f6bcb00972b7526a5e1dc19938387006586
Reviewed-on: https://chromium-review.googlesource.com/341052
Commit-Ready: Miao-chen Chou <mcchou@chromium.org>
Tested-by: Miao-chen Chou <mcchou@chromium.org>
Reviewed-by: Rahul Chaturvedi <rkc@chromium.org>
Reviewed-by: Miao-chen Chou <mcchou@chromium.org>

[modify] https://crrev.com/3161eade6ce7d68dcaa3f1515c27dadac03694b1/full.xml

https://chromium-review.googlesource.com/#/c/341052/ and https://chrome-internal-review.googlesource.com/#/c/257177/ landed. The new cros/master is now pointing to cros/chromeos-5.39.

@kathrelkeld, can you help with verifying the BlueZ uprev? Thanks.:)

This change landed in 8280.0.0.  We'll start taking a look after we get this build.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ced897af4ba797e3824a615b847701cbe80e0c95

commit ced897af4ba797e3824a615b847701cbe80e0c95
Author: rkc <rkc@chromium.org>
Date: Wed May 04 19:17:25 2016

Update //device/bluetooth/dbus code to match cros_system_api constants.

The method name for GATT service registration has changed in BlueZ. We've
rolled the cros_system_api constants to match. This CL updates the GATT
manager client code to reflect that.

Since this is a required but miniscule mechanical change, TBRing.

TBR=scheib@chromium.org
BUG= 600655 

Review-Url: https://codereview.chromium.org/1947173002
Cr-Commit-Position: refs/heads/master@{#391596}

[modify] https://crrev.com/ced897af4ba797e3824a615b847701cbe80e0c95/device/bluetooth/dbus/bluetooth_gatt_manager_client.cc

Just a heads up that the following patch might be required when using ServicesResolved signal:

https://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=6aa1c4488ee96fd6a209b41b7148b037a888a783

we're not seeing major Bluetooth problems since this update landed.

mcchou, as mentioned in #c29 we might want to apply 
https://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=6aa1c4488ee96fd6a209b41b7148b037a888a783 as well in chromeos-5.39 branch.

fbeaufort, I will put this on the 5.40 cherry-pick list. Thanks for the reminder.

mcchou, can we please apply https://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=6aa1c4488ee96fd6a209b41b7148b037a888a783 to chromeos-5.39 branch and merge it in M52 as we can't discover services anymore on a first-time seen Bluetooth device. 

For info, here's the IRC logs:

[2016-06-14 11:30:21] — fbeaufort vudentz, any idea on what could happen there regarding services_resolved property? http://pastebin.com/raw/6qRft5hJ I can reproduce this  issue 100 % time with last Dev Chrome OS and Chrome OS BlueZ 5.39
[2016-06-14 11:31:32] <vudentz> fbeaufort, is 6aa1c4488ee96fd6a209b41b7148b037a888a783 applied there?
[2016-06-14 11:31:47] ⇐ fdanis quit (~frederic@37.58.183.20): Quit: Leaving.
[2016-06-14 11:32:04] <vudentz> commit 6aa1c4488ee96fd6a209b41b7148b037a888a783
[2016-06-14 11:32:05] <vudentz> Author: Vinicius Costa Gomes <vcgomes@gmail.com>
[2016-06-14 11:32:05] <vudentz> Date:   Tue May 3 14:56:27 2016 -0300
[2016-06-14 11:32:05] <vudentz>     gdbus: Fix the ordering of signals
[2016-06-14 11:32:15] <fbeaufort> I don't think so: https://chromium.googlesource.com/chromiumos/third_party/bluez/+log/chromeos-5.39
[2016-06-14 11:32:40] <vudentz> Then that would explain the signal arriving before the services
[2016-06-14 11:33:41] → fdanis joined (~frederic@37.58.183.20)
[2016-06-14 11:34:15] <fbeaufort> I'll apply this patch locally and see if that fixes it.
[2016-06-14 11:34:30] <fbeaufort> Can I do something to see if that is the issue though before?
[2016-06-14 11:37:30] <vudentz> fbeaufort, Im not following, you want to check if that indeed due to signal ordering? Perhaps with bluetoothctl it would be easier to see as it prints the events in the order they come.
[2016-06-14 11:38:18] <fbeaufort> Does that help? http://pastebin.com/raw/iCs4mBTc
[2016-06-14 11:39:42] <vudentz> Yep, it is indeed the signal ordering as you can see [CHG] Device AC:E6:4B:05:88:2D ServicesResolved: yes is printed before the attributes which is exactly what the patch is fixing
[2016-06-14 11:40:15] <fbeaufort> Yeah!
[2016-06-14 11:40:35] <vudentz> I remember commenting that this patch was needed, perhaps this was not taken care
[2016-06-14 11:40:37] <fbeaufort> It's sad I didn't see it before ;( 
[2016-06-14 11:40:41] <fbeaufort> My bad
[2016-06-14 11:41:12] <fbeaufort> It works well when device is already cached by the way.
[2016-06-14 11:41:26] <fbeaufort> I'll find your comment and ping the appropriate Chrome OS people.