Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2421713002:
arc: bluetooth: Expose missing advertise data. (Closed)
|
Created:
4 years, 2 months ago by puthik_chromium Modified:
4 years, 1 month ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, ortuno+watch_chromium.org, scheib+watch_chromium.org, yusukes+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Expose missing advertise data
This CL does the following.
- Make dbus Property support unorder_map<uint16_t, vector<uint8_t>>
for Manufacturer Data.
- Expose Manufacturer Data and Bluetooth Flags from BlueZ device
property in bluez::BluetoothDeviceClient.
- Add new BluetoothDevice and BluetoothDeviceBluZ API to query
Manufacturer Data and Bluetooth Flags.
This required upstream BlueZ patch at
http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=f96e4a9f
http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=9caa0457
And the system_api patch at http://crosreview.com/398418
BUG=618442
TEST=nRF Connect App in minnie can see all advertise data
Committed: https://crrev.com/23399e4540ec7e6e17fcf03fd45248d7e947caf5
Cr-Commit-Position: refs/heads/master@{#429980}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Split arc++ patch / Fix ortuno comment #Patch Set 3 : fix comment #
Total comments: 16
Patch Set 4 : Make dbus interface match upstream #Patch Set 5 : Fix comment #
Total comments: 5
Patch Set 6 : Add BlueZ unittests / more comment #
Dependent Patchsets: Messages
Total messages: 34 (17 generated)
puthik@chromium.org changed reviewers: + hashimoto@chromium.org, lhchavez@chromium.org, ortuno@chromium.org, steel@chromium.org
BlueZ patch at http://crosreview.com/398225 Another approach to this is to make BlueZ expose raw advertising data. But I think it might be too much overkill for this. Will write unit test in next version of this patch.
Please separate adding new features to device/bluetooth from arc++ changes. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:323: // Returns advertised Manufacturer Data. Keys are 16 bits Manufacturer ID s/ID/IDs/ https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:327: // Note that only BlueZ support this now. This method returns an empty map s/support/supports/ https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:328: // on platform without BlueZ. nit: on platforms that don't use BlueZ. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:329: virtual std::unordered_map<uint16_t, std::vector<uint8_t>> Please match the pattern of ServiceData i.e. same set of functions, manufacturer_data_ member in BluetoothDevice, etc. Also consider adding a typedef for std::unordered_map<...>. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:344: // Returns Bluetooth flags in Advertise Data. s/Advertise/Advertising/ and also I think you can drop 'Bluetooth'. Could you add more detail so that we know how to implement this on other platforms? For example: Does this reflect the latest advertising packet? What happens if a packet stops advertising flags? Does GetFlags() still return the last value? Does this get invalidated when we stop scanning? https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:346: // Note that only BlueZ support this now. This method returns base::nullopt s/support/supports/ https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:347: // on platform without BlueZ. nit: on platforms that don't use BlueZ. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:348: virtual base::Optional<uint8_t> GetFlags() const; There are a lot of flags in Bluetooth ^.^ so you might want to call this GetAdvertisingDataFlags() https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluez/blue... File device/bluetooth/bluez/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluez/blue... device/bluetooth/bluez/bluetooth_adapter_bluez.cc:607: property_name == properties->manufacturer_data.name() || Add these to the DeviceChanged docs. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluez/blue... File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluez/blue... device/bluetooth/bluez/bluetooth_device_bluez.cc:397: // BlueZ always have flags available. Can you DCHECK this? Also I don't think this is true on Linux. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/dbus/bluet... File device/bluetooth/dbus/bluetooth_device_client.cc (right): https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/dbus/bluet... device/bluetooth/dbus/bluetooth_device_client.cc:210: RegisterProperty(bluetooth_device::kFlagsProperty, &flags); I might be missing something but I don't see this property in the BlueZ docs. Is it done in an internal BlueZ patch? If so can you mention it in the description? What's the plan for upstreaming it? Does this break Linux? rkc: Are you OK with Chrome OS and Linux implementations starting to work differently.
As ortuno@ mentioned; please split the ARC++ changes from the BlueZ changes using dependent patches. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluez/blue... File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluez/blue... device/bluetooth/bluez/bluetooth_device_bluez.cc:397: // BlueZ always have flags available. This is not true. Depending on the version of BlueZ being used (and whether your patch gets upstreamed), we may not have flags at all. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/dbus/bluet... File device/bluetooth/dbus/bluetooth_device_client.cc (right): https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/dbus/bluet... device/bluetooth/dbus/bluetooth_device_client.cc:210: RegisterProperty(bluetooth_device::kFlagsProperty, &flags); On 2016/10/14 00:15:10, ortuno wrote: > I might be missing something but I don't see this property in the BlueZ docs. Is > it done in an internal BlueZ patch? If so can you mention it in the description? > What's the plan for upstreaming it? Does this break Linux? > > rkc: Are you OK with Chrome OS and Linux implementations starting to work > differently. I've asked Opal to upstream the patch to BlueZ. I would prefer Linux and Chrome OS to work similarly; that being said, that won't always be possible. Certain features we add purely for Chrome OS will just not work on Linux.
Description was changed from ========== arc: bluetooth: Expose missing advertise data. This CL does the following. - Make dbus Property support unorder_map<uint16_t, vector<uint8_t>> for Manufacturer Data. - Expose Manufacturer Data and Bluetooth Flags from BlueZ device property in bluez::BluetoothDeviceClient. - Add new BluetoothDevice and BluetoothDeviceBluZ API to query Manufacturer Data and Bluetooth Flags. - Make ArcBluetoothBridge send missing advertise data to Android which are Manufacturer Data, Bluetooth Flags and Tx Power Level. BUG=618442 TEST=nRF Connect App in minnie can see all advertise data ========== to ========== arc: bluetooth: Expose missing advertise data This CL does the following. - Make dbus Property support unorder_map<uint16_t, vector<uint8_t>> for Manufacturer Data. - Expose Manufacturer Data and Bluetooth Flags from BlueZ device property in bluez::BluetoothDeviceClient. - Add new BluetoothDevice and BluetoothDeviceBluZ API to query Manufacturer Data and Bluetooth Flags. This requied upstream BlueZ patch stating at http://marc.info/?l=linux-bluetooth&m=147674326322110&w=2 And the system_api patch at http://crosreview.com/398418 BUG=618442 TEST=nRF Connect App in minnie can see all advertise data ==========
puthik@chromium.org changed reviewers: - lhchavez@chromium.org
https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:323: // Returns advertised Manufacturer Data. Keys are 16 bits Manufacturer ID On 2016/10/14 00:15:10, ortuno wrote: > s/ID/IDs/ Done. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:327: // Note that only BlueZ support this now. This method returns an empty map On 2016/10/14 00:15:10, ortuno wrote: > s/support/supports/ Done. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:328: // on platform without BlueZ. On 2016/10/14 00:15:10, ortuno wrote: > nit: on platforms that don't use BlueZ. Done. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:329: virtual std::unordered_map<uint16_t, std::vector<uint8_t>> On 2016/10/14 00:15:10, ortuno wrote: > Please match the pattern of ServiceData i.e. same set of functions, > manufacturer_data_ member in BluetoothDevice, etc. Also consider adding a > typedef for std::unordered_map<...>. Done. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:344: // Returns Bluetooth flags in Advertise Data. On 2016/10/14 00:15:10, ortuno wrote: > s/Advertise/Advertising/ and also I think you can drop 'Bluetooth'. > > Could you add more detail so that we know how to implement this on other > platforms? For example: Does this reflect the latest advertising packet? What > happens if a packet stops advertising flags? Does GetFlags() still return the > last value? Does this get invalidated when we stop scanning? Done. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:346: // Note that only BlueZ support this now. This method returns base::nullopt On 2016/10/14 00:15:10, ortuno wrote: > s/support/supports/ Done. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:347: // on platform without BlueZ. On 2016/10/14 00:15:10, ortuno wrote: > nit: on platforms that don't use BlueZ. Done. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:348: virtual base::Optional<uint8_t> GetFlags() const; On 2016/10/14 00:15:10, ortuno wrote: > There are a lot of flags in Bluetooth ^.^ so you might want to call this > GetAdvertisingDataFlags() Done. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluez/blue... File device/bluetooth/bluez/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluez/blue... device/bluetooth/bluez/bluetooth_adapter_bluez.cc:607: property_name == properties->manufacturer_data.name() || On 2016/10/14 00:15:10, ortuno wrote: > Add these to the DeviceChanged docs. I can't find the DeviceChanged docs that mention each property changed. Can you give me a pointer to this? https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluez/blue... File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluez/blue... device/bluetooth/bluez/bluetooth_device_bluez.cc:397: // BlueZ always have flags available. On 2016/10/17 20:41:21, Rahul Chaturvedi wrote: > This is not true. Depending on the version of BlueZ being used (and whether your > patch gets upstreamed), we may not have flags at all. I added the check if it is valid or not. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/dbus/bluet... File device/bluetooth/dbus/bluetooth_device_client.cc (right): https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/dbus/bluet... device/bluetooth/dbus/bluetooth_device_client.cc:210: RegisterProperty(bluetooth_device::kFlagsProperty, &flags); On 2016/10/17 20:41:21, Rahul Chaturvedi wrote: > On 2016/10/14 00:15:10, ortuno wrote: > > I might be missing something but I don't see this property in the BlueZ docs. > Is > > it done in an internal BlueZ patch? If so can you mention it in the > description? > > What's the plan for upstreaming it? Does this break Linux? > > > > rkc: Are you OK with Chrome OS and Linux implementations starting to work > > differently. > > I've asked Opal to upstream the patch to BlueZ. I would prefer Linux and Chrome > OS to work similarly; that being said, that won't always be possible. Certain > features we add purely for Chrome OS will just not work on Linux. I sent it to upstream http://marc.info/?l=linux-bluetooth&m=147674326322110&w=2 http://marc.info/?l=linux-bluetooth&m=147674326522113&w=2 http://marc.info/?l=linux-bluetooth&m=147674326322111&w=2
Description was changed from ========== arc: bluetooth: Expose missing advertise data This CL does the following. - Make dbus Property support unorder_map<uint16_t, vector<uint8_t>> for Manufacturer Data. - Expose Manufacturer Data and Bluetooth Flags from BlueZ device property in bluez::BluetoothDeviceClient. - Add new BluetoothDevice and BluetoothDeviceBluZ API to query Manufacturer Data and Bluetooth Flags. This requied upstream BlueZ patch stating at http://marc.info/?l=linux-bluetooth&m=147674326322110&w=2 And the system_api patch at http://crosreview.com/398418 BUG=618442 TEST=nRF Connect App in minnie can see all advertise data ========== to ========== bluetooth: Expose missing advertise data This CL does the following. - Make dbus Property support unorder_map<uint16_t, vector<uint8_t>> for Manufacturer Data. - Expose Manufacturer Data and Bluetooth Flags from BlueZ device property in bluez::BluetoothDeviceClient. - Add new BluetoothDevice and BluetoothDeviceBluZ API to query Manufacturer Data and Bluetooth Flags. This requied upstream BlueZ patch stating at http://marc.info/?l=linux-bluetooth&m=147674326322110&w=2 And the system_api patch at http://crosreview.com/398418 BUG=618442 TEST=nRF Connect App in minnie can see all advertise data ==========
https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluez/blue... File device/bluetooth/bluez/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluez/blue... device/bluetooth/bluez/bluetooth_adapter_bluez.cc:607: property_name == properties->manufacturer_data.name() || On 2016/10/17 at 23:30:32, puthik_chromium wrote: > On 2016/10/14 00:15:10, ortuno wrote: > > Add these to the DeviceChanged docs. > > I can't find the DeviceChanged docs that mention each property changed. > Can you give me a pointer to this? https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.h?q=f... I think we missed service data, could you add that one as well? https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:106: typedef uint16_t ManufacturerID; nit: s/ID/Id/ here and below. Though it really doesn't matter. https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:330: // Returns cached value if the adapter is not discovering. What about changing Manufacturer Data? Does BlueZ persist that as well? If so you could say: // Note: On ChromeOS and Linux, BlueZ persists all manufacturer data meaning if // a device stops advertising manufacturer data for a Manufacturer Id, this function will // still return the cached value for that Id. https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:333: // on platforms that don't use BlueZ. Could you open an issue to implement this on other platforms? https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:362: // Note that only BlueZ supports this now. This method returns base::nullopt s/BlueZ/Chrome OS/ Also could you open an issue regarding upstreaming Advertising Flags and add a TODO mentioning that Linux will support it once that issue is resolved? https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_adapter_bluez.cc:611: property_name == properties->advertising_data_flags.name()) { Should flags be included here? https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_device_bluez.cc:378: base::Optional<uint8_t> BluetoothDeviceBlueZ::GetAdvertisingDataFlags() const { Also please follow the same pattern as ManufacturerData. All advertising data should follow that pattern. https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_device_bluez.cc:626: DCHECK(properties->manufacturer_data.is_valid()); manufacturer data is optional so this would is not always true. Also seems we missed this detail with service data. Could you open an issue regarding this and if possible send out a patch to fix? https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/dbus/b... File device/bluetooth/dbus/bluetooth_device_client.h (right): https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/dbus/b... device/bluetooth/dbus/bluetooth_device_client.h:125: // Bluetooth Flag in Advertise data. Read-only. s/Flag/Flags/
Chrome OS switch to BlueZ 5.41 branch now which supports this feature. https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluez/blue... File device/bluetooth/bluez/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/2421713002/diff/1/device/bluetooth/bluez/blue... device/bluetooth/bluez/bluetooth_adapter_bluez.cc:607: property_name == properties->manufacturer_data.name() || On 2016/10/18 01:41:18, ortuno wrote: > On 2016/10/17 at 23:30:32, puthik_chromium wrote: > > On 2016/10/14 00:15:10, ortuno wrote: > > > Add these to the DeviceChanged docs. > > > > I can't find the DeviceChanged docs that mention each property changed. > > Can you give me a pointer to this? > > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.h?q=f... > > I think we missed service data, could you add that one as well? Done. https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:106: typedef uint16_t ManufacturerID; On 2016/10/18 01:41:18, ortuno wrote: > nit: s/ID/Id/ here and below. Though it really doesn't matter. Done. https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:330: // Returns cached value if the adapter is not discovering. On 2016/10/18 01:41:18, ortuno wrote: > What about changing Manufacturer Data? Does BlueZ persist that as well? If so > you could say: > > // Note: On ChromeOS and Linux, BlueZ persists all manufacturer data meaning if > // a device stops advertising manufacturer data for a Manufacturer Id, this > function will > // still return the cached value for that Id. Done. https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:333: // on platforms that don't use BlueZ. On 2016/10/18 01:41:18, ortuno wrote: > Could you open an issue to implement this on other platforms? Done. https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:362: // Note that only BlueZ supports this now. This method returns base::nullopt On 2016/10/18 01:41:18, ortuno wrote: > s/BlueZ/Chrome OS/ Also could you open an issue regarding upstreaming > Advertising Flags and add a TODO mentioning that Linux will support it once that > issue is resolved? Done. https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_adapter_bluez.cc:611: property_name == properties->advertising_data_flags.name()) { On 2016/10/18 01:41:18, ortuno wrote: > Should flags be included here? Yes https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_device_bluez.cc:378: base::Optional<uint8_t> BluetoothDeviceBlueZ::GetAdvertisingDataFlags() const { On 2016/10/18 01:41:19, ortuno wrote: > Also please follow the same pattern as ManufacturerData. All advertising data > should follow that pattern. Done. https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_device_bluez.cc:626: DCHECK(properties->manufacturer_data.is_valid()); On 2016/10/18 01:41:18, ortuno wrote: > manufacturer data is optional so this would is not always true. Also seems we > missed this detail with service data. Could you open an issue regarding this and > if possible send out a patch to fix? Done. https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/dbus/b... File device/bluetooth/dbus/bluetooth_device_client.h (right): https://codereview.chromium.org/2421713002/diff/40001/device/bluetooth/dbus/b... device/bluetooth/dbus/bluetooth_device_client.h:125: // Bluetooth Flag in Advertise data. Read-only. On 2016/10/18 01:41:19, ortuno wrote: > s/Flag/Flags/ Make it match upstream doc instead.
Description was changed from ========== bluetooth: Expose missing advertise data This CL does the following. - Make dbus Property support unorder_map<uint16_t, vector<uint8_t>> for Manufacturer Data. - Expose Manufacturer Data and Bluetooth Flags from BlueZ device property in bluez::BluetoothDeviceClient. - Add new BluetoothDevice and BluetoothDeviceBluZ API to query Manufacturer Data and Bluetooth Flags. This requied upstream BlueZ patch stating at http://marc.info/?l=linux-bluetooth&m=147674326322110&w=2 And the system_api patch at http://crosreview.com/398418 BUG=618442 TEST=nRF Connect App in minnie can see all advertise data ========== to ========== bluetooth: Expose missing advertise data This CL does the following. - Make dbus Property support unorder_map<uint16_t, vector<uint8_t>> for Manufacturer Data. - Expose Manufacturer Data and Bluetooth Flags from BlueZ device property in bluez::BluetoothDeviceClient. - Add new BluetoothDevice and BluetoothDeviceBluZ API to query Manufacturer Data and Bluetooth Flags. This requied upstream BlueZ patch at http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=f96e4a9f http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=9caa0457 And the system_api patch at http://crosreview.com/398418 BUG=618442 TEST=nRF Connect App in minnie can see all advertise data ==========
https://codereview.chromium.org/2421713002/diff/80001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2421713002/diff/80001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_device_bluez.cc:631: advertising_data_flags_ = properties->advertising_data_flags.value()[0]; Could you comment why we only need the first element in the vector? https://codereview.chromium.org/2421713002/diff/80001/device/bluetooth/dbus/b... File device/bluetooth/dbus/bluetooth_device_client.h (right): https://codereview.chromium.org/2421713002/diff/80001/device/bluetooth/dbus/b... device/bluetooth/dbus/bluetooth_device_client.h:126: dbus::Property<std::vector<uint8_t>> advertising_data_flags; q: Why is this a vector?
https://codereview.chromium.org/2421713002/diff/80001/device/bluetooth/dbus/b... File device/bluetooth/dbus/bluetooth_device_client.h (right): https://codereview.chromium.org/2421713002/diff/80001/device/bluetooth/dbus/b... device/bluetooth/dbus/bluetooth_device_client.h:126: dbus::Property<std::vector<uint8_t>> advertising_data_flags; On 2016/11/02 23:36:48, ortuno wrote: > q: Why is this a vector? It's what upstream want for future proof. Only the first element is matter now. https://www.spinics.net/lists/linux-bluetooth/msg68753.html
lgtm but please add a comment to bluetooth_device_bluez to indicate why you only use the first element. https://codereview.chromium.org/2421713002/diff/80001/device/bluetooth/dbus/b... File device/bluetooth/dbus/bluetooth_device_client.h (right): https://codereview.chromium.org/2421713002/diff/80001/device/bluetooth/dbus/b... device/bluetooth/dbus/bluetooth_device_client.h:126: dbus::Property<std::vector<uint8_t>> advertising_data_flags; On 2016/11/02 at 23:44:13, puthik_chromium wrote: > On 2016/11/02 23:36:48, ortuno wrote: > > q: Why is this a vector? > > It's what upstream want for future proof. Only the first element is matter now. > https://www.spinics.net/lists/linux-bluetooth/msg68753.html I see.
I also added BTDeviceBlueZ unittests for the new methods. https://codereview.chromium.org/2421713002/diff/80001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2421713002/diff/80001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_device_bluez.cc:631: advertising_data_flags_ = properties->advertising_data_flags.value()[0]; On 2016/11/02 23:36:48, ortuno wrote: > Could you comment why we only need the first element in the vector? Done.
puthik@chromium.org changed reviewers: + stevenjb@chromium.org
+stevenjb for dbus Look like hashimoto@ will be OoO this week and next week. So add stevenjb for dbus review instead.
Description was changed from ========== bluetooth: Expose missing advertise data This CL does the following. - Make dbus Property support unorder_map<uint16_t, vector<uint8_t>> for Manufacturer Data. - Expose Manufacturer Data and Bluetooth Flags from BlueZ device property in bluez::BluetoothDeviceClient. - Add new BluetoothDevice and BluetoothDeviceBluZ API to query Manufacturer Data and Bluetooth Flags. This requied upstream BlueZ patch at http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=f96e4a9f http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=9caa0457 And the system_api patch at http://crosreview.com/398418 BUG=618442 TEST=nRF Connect App in minnie can see all advertise data ========== to ========== bluetooth: Expose missing advertise data This CL does the following. - Make dbus Property support unorder_map<uint16_t, vector<uint8_t>> for Manufacturer Data. - Expose Manufacturer Data and Bluetooth Flags from BlueZ device property in bluez::BluetoothDeviceClient. - Add new BluetoothDevice and BluetoothDeviceBluZ API to query Manufacturer Data and Bluetooth Flags. This required upstream BlueZ patch at http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=f96e4a9f http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=9caa0457 And the system_api patch at http://crosreview.com/398418 BUG=618442 TEST=nRF Connect App in minnie can see all advertise data ==========
The CQ bit was checked by puthik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
dbus/ owner lgtm
The CQ bit was checked by puthik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org Link to the patchset: https://codereview.chromium.org/2421713002/#ps70012 (title: "Add BlueZ unittests / more comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by puthik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Expose missing advertise data This CL does the following. - Make dbus Property support unorder_map<uint16_t, vector<uint8_t>> for Manufacturer Data. - Expose Manufacturer Data and Bluetooth Flags from BlueZ device property in bluez::BluetoothDeviceClient. - Add new BluetoothDevice and BluetoothDeviceBluZ API to query Manufacturer Data and Bluetooth Flags. This required upstream BlueZ patch at http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=f96e4a9f http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=9caa0457 And the system_api patch at http://crosreview.com/398418 BUG=618442 TEST=nRF Connect App in minnie can see all advertise data ========== to ========== bluetooth: Expose missing advertise data This CL does the following. - Make dbus Property support unorder_map<uint16_t, vector<uint8_t>> for Manufacturer Data. - Expose Manufacturer Data and Bluetooth Flags from BlueZ device property in bluez::BluetoothDeviceClient. - Add new BluetoothDevice and BluetoothDeviceBluZ API to query Manufacturer Data and Bluetooth Flags. This required upstream BlueZ patch at http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=f96e4a9f http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=9caa0457 And the system_api patch at http://crosreview.com/398418 BUG=618442 TEST=nRF Connect App in minnie can see all advertise data ==========
Message was sent while issue was closed.
Committed patchset #6 (id:70012)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Expose missing advertise data This CL does the following. - Make dbus Property support unorder_map<uint16_t, vector<uint8_t>> for Manufacturer Data. - Expose Manufacturer Data and Bluetooth Flags from BlueZ device property in bluez::BluetoothDeviceClient. - Add new BluetoothDevice and BluetoothDeviceBluZ API to query Manufacturer Data and Bluetooth Flags. This required upstream BlueZ patch at http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=f96e4a9f http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=9caa0457 And the system_api patch at http://crosreview.com/398418 BUG=618442 TEST=nRF Connect App in minnie can see all advertise data ========== to ========== bluetooth: Expose missing advertise data This CL does the following. - Make dbus Property support unorder_map<uint16_t, vector<uint8_t>> for Manufacturer Data. - Expose Manufacturer Data and Bluetooth Flags from BlueZ device property in bluez::BluetoothDeviceClient. - Add new BluetoothDevice and BluetoothDeviceBluZ API to query Manufacturer Data and Bluetooth Flags. This required upstream BlueZ patch at http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=f96e4a9f http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=9caa0457 And the system_api patch at http://crosreview.com/398418 BUG=618442 TEST=nRF Connect App in minnie can see all advertise data Committed: https://crrev.com/23399e4540ec7e6e17fcf03fd45248d7e947caf5 Cr-Commit-Position: refs/heads/master@{#429980} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/23399e4540ec7e6e17fcf03fd45248d7e947caf5 Cr-Commit-Position: refs/heads/master@{#429980} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
