Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2376873002:
arc: bluetooth: Use uuid 128 bit for service uuid (Closed)
|
Created:
4 years, 2 months ago by puthik_chromium Modified:
4 years, 2 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: bluetooth: Use uuid 128 bit for service uuid
We used 128 bit uuid for service uuid before. However this
caused buffer overflow problem when encounter with multiple
16 bit uuid for this field as the length of advertising data
would exceed the limit.
We mitigated that by forcing the service uuid to be 16 bit.
But this break the device that decide to advertised custom
128 bit uuids.
This CL properly fixes the above problems by send 128 bit
uuid to Android and let Android side deal with the overflow
problem.
BUG=653310, b:28670943
TEST=Android side can see 128 bit service uuid
Committed: https://crrev.com/81711b78e78640b9ec45cd25cb8c0dbeb252030f
Cr-Commit-Position: refs/heads/master@{#423641}
Patch Set 1 #
Total comments: 6
Patch Set 2 : fix lhchavez comment #Patch Set 3 : Add more code comment #
Total comments: 7
Patch Set 4 : Always send 128 bit uuid to Android instead #Patch Set 5 : fix cherry pick #Patch Set 6 : fix comment #
Messages
Total messages: 31 (14 generated)
puthik@chromium.org changed reviewers: + lhchavez@chromium.org, steel@chromium.org
https://codereview.chromium.org/2376873002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2376873002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1867: // Note that we need to use UUID 16 bits because Android does not support This does not seem to be true anymore? Or at least needs some rewording. https://codereview.chromium.org/2376873002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1891: i++; Ugh, more of this pattern :( Can you try using this change? https://codereview.chromium.org/2380643003 if it works, I'll send it for review. https://codereview.chromium.org/2376873002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1913: service_data->uuid_16bit = GetUUID16(uuid); In which cases |uuid| won't be an uuid16? Will it happen often and cause a lot of logspam? Can we somehow prevent it from happening by e.g. filtering these cases?
https://codereview.chromium.org/2376873002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2376873002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1891: i++; On 2016/09/28 16:11:48, Luis Héctor Chávez wrote: > Ugh, more of this pattern :( > > Can you try using this change? https://codereview.chromium.org/2380643003 if it > works, I'll send it for review. It works. Thanks for doing this.
On 2016/09/29 02:06:08, puthik_chromium wrote: > https://codereview.chromium.org/2376873002/diff/1/components/arc/bluetooth/ar... > File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): > > https://codereview.chromium.org/2376873002/diff/1/components/arc/bluetooth/ar... > components/arc/bluetooth/arc_bluetooth_bridge.cc:1891: i++; > On 2016/09/28 16:11:48, Luis Héctor Chávez wrote: > > Ugh, more of this pattern :( > > > > Can you try using this change? https://codereview.chromium.org/2380643003 if > it > > works, I'll send it for review. > > It works. > Thanks for doing this. awesome, that change has landed and is now in master. You should be able to rebase and use it.
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...
https://codereview.chromium.org/2376873002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2376873002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1867: // Note that we need to use UUID 16 bits because Android does not support On 2016/09/28 16:11:48, Luis Héctor Chávez wrote: > This does not seem to be true anymore? Or at least needs some rewording. Done. https://codereview.chromium.org/2376873002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1913: service_data->uuid_16bit = GetUUID16(uuid); On 2016/09/28 16:11:48, Luis Héctor Chávez wrote: > In which cases |uuid| won't be an uuid16? Will it happen often and cause a lot > of logspam? Can we somehow prevent it from happening by e.g. filtering these > cases? I think it is rare not to have uuid 16 bit for service data. But I will remove the log to prevent log spam.
lgtm defer to rkc. https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1915: // Android only support UUID 16 bit here. nit: s/support/supports/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:197: bool isUuid16(const BluetoothUUID& uuid) { Can't we get this from uuid.Format()? https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1897: service_uuids->set_service_uuids_16(std::move(uuid16s)); Why do 16 bit UUIDs need to be handled differently? Can't we just handle all via the BluetoothUUID type and translate it to the correct type on the Android side? It seems odd that we're handling 16 bit UUIDs one way, but 32 bit and 128 bit UUIDs another way.
https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:197: bool isUuid16(const BluetoothUUID& uuid) { On 2016/09/29 22:10:47, Rahul Chaturvedi wrote: > Can't we get this from uuid.Format()? uuid.Format() return the size of uuid string given in constructor. For example BluetoothUUID a("00001234-0000-1000-8000-00805f9b34fb"); BluetoothUUID b("1234"); Then the following conditions are true a == b a.Format() == BluetoothUUID::kFormat128Bit b.Format() == BluetoothUUID::kFormat16Bit https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1897: service_uuids->set_service_uuids_16(std::move(uuid16s)); On 2016/09/29 22:10:47, Rahul Chaturvedi wrote: > Why do 16 bit UUIDs need to be handled differently? Can't we just handle all via > the BluetoothUUID type and translate it to the correct type on the Android side? > > It seems odd that we're handling 16 bit UUIDs one way, but 32 bit and 128 bit > UUIDs another way. Why do 16 bit UUIDs need to be handled differently? Because for max length of advertising data is only 63 byte. Actually we can implement this in 3 ways 1. Implement it like this. (may be add another case for 32 bit uuid) 2. Always use 128 bit UUIDs in Chrome and translate to correct type in Android. 3. Make BlueZ expose raw advertising data and send it to Android. So you think the best approach is (2)? In my opinion, (1) and (2) are not much different. I can change this to use (2) approach but if (3) is possible I think that might be the best.
https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1897: service_uuids->set_service_uuids_16(std::move(uuid16s)); On 2016/09/29 22:39:25, puthik_chromium wrote: > On 2016/09/29 22:10:47, Rahul Chaturvedi wrote: > > Why do 16 bit UUIDs need to be handled differently? Can't we just handle all > via > > the BluetoothUUID type and translate it to the correct type on the Android > side? > > > > It seems odd that we're handling 16 bit UUIDs one way, but 32 bit and 128 bit > > UUIDs another way. > > Why do 16 bit UUIDs need to be handled differently? > Because for max length of advertising data is only 63 byte. > > Actually we can implement this in 3 ways > 1. Implement it like this. (may be add another case for 32 bit uuid) > 2. Always use 128 bit UUIDs in Chrome and translate to correct type in Android. > 3. Make BlueZ expose raw advertising data and send it to Android. > > So you think the best approach is (2)? > In my opinion, (1) and (2) are not much different. I can change this to use (2) > approach but if (3) is possible I think that might be the best. Android's implementation and APIs can change (in fact, often do with every release). We should keep our interface as flexible as possible, so 3 sounds like the best option. That being said, it does seem a bit like overkill for here, so if you want to do 2, that works too. I would prefer to do any processing of the data we send to Android to be done on Android rather than in ARC code.
On 2016/09/29 22:53:39, Rahul Chaturvedi wrote: > https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... > File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): > > https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... > components/arc/bluetooth/arc_bluetooth_bridge.cc:1897: > service_uuids->set_service_uuids_16(std::move(uuid16s)); > On 2016/09/29 22:39:25, puthik_chromium wrote: > > On 2016/09/29 22:10:47, Rahul Chaturvedi wrote: > > > Why do 16 bit UUIDs need to be handled differently? Can't we just handle all > > via > > > the BluetoothUUID type and translate it to the correct type on the Android > > side? > > > > > > It seems odd that we're handling 16 bit UUIDs one way, but 32 bit and 128 > bit > > > UUIDs another way. > > > > Why do 16 bit UUIDs need to be handled differently? > > Because for max length of advertising data is only 63 byte. > > > > Actually we can implement this in 3 ways > > 1. Implement it like this. (may be add another case for 32 bit uuid) > > 2. Always use 128 bit UUIDs in Chrome and translate to correct type in > Android. > > 3. Make BlueZ expose raw advertising data and send it to Android. > > > > So you think the best approach is (2)? > > In my opinion, (1) and (2) are not much different. I can change this to use > (2) > > approach but if (3) is possible I think that might be the best. > > Android's implementation and APIs can change (in fact, often do with every > release). We should keep our interface as flexible as possible, so 3 sounds like > the best option. > > That being said, it does seem a bit like overkill for here, so if you want to do > 2, that works too. I would prefer to do any processing of the data we send to > Android to be done on Android rather than in ARC code. Actually implement 3 should not be that hard and it would solve a lot of other problem too. I'll look in to that first and see whether it can be easily implement or not.
On 2016/09/30 00:29:21, puthik_chromium wrote: > On 2016/09/29 22:53:39, Rahul Chaturvedi wrote: > > > https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... > > File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): > > > > > https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... > > components/arc/bluetooth/arc_bluetooth_bridge.cc:1897: > > service_uuids->set_service_uuids_16(std::move(uuid16s)); > > On 2016/09/29 22:39:25, puthik_chromium wrote: > > > On 2016/09/29 22:10:47, Rahul Chaturvedi wrote: > > > > Why do 16 bit UUIDs need to be handled differently? Can't we just handle > all > > > via > > > > the BluetoothUUID type and translate it to the correct type on the Android > > > side? > > > > > > > > It seems odd that we're handling 16 bit UUIDs one way, but 32 bit and 128 > > bit > > > > UUIDs another way. > > > > > > Why do 16 bit UUIDs need to be handled differently? > > > Because for max length of advertising data is only 63 byte. > > > > > > Actually we can implement this in 3 ways > > > 1. Implement it like this. (may be add another case for 32 bit uuid) > > > 2. Always use 128 bit UUIDs in Chrome and translate to correct type in > > Android. > > > 3. Make BlueZ expose raw advertising data and send it to Android. > > > > > > So you think the best approach is (2)? > > > In my opinion, (1) and (2) are not much different. I can change this to use > > (2) > > > approach but if (3) is possible I think that might be the best. > > > > Android's implementation and APIs can change (in fact, often do with every > > release). We should keep our interface as flexible as possible, so 3 sounds > like > > the best option. > > > > That being said, it does seem a bit like overkill for here, so if you want to > do > > 2, that works too. I would prefer to do any processing of the data we send to > > Android to be done on Android rather than in ARC code. > > Actually implement 3 should not be that hard and it would solve a lot of other > problem too. > I'll look in to that first and see whether it can be easily implement or not. I digged BlueZ code today and make BlueZ expose advertising data won't working great for us. BlueZ have EIR field which is same as AD but the max length of EIR is exceed the support length in Android. Even if we make BlueZ expose EIR, we still need to parse that and filter out not important part any way. I will continue with (2) approach, always use 128 bit UUIDs in Chrome and translate to correct type in Android.
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...
Description was changed from ========== arc: bluetooth: Use uuid 128 bit for service uuid when need We used 128 bit uuid for service uuid before. However this caused buffer overflow problem when encounter with multiple 16 bit uuid for this field as the length of advertising data would exceed the limit. We mitigated that by forcing the service uuid to be 16 bit. But this break the device that decide to advertised custom 128 bit uuids. This CL properly fixes the above problems by determine the appropriate size of uuid to use. BUG=b:28670943 TEST=Android side can see 128 bit service uuid ========== to ========== arc: bluetooth: Use uuid 128 bit for service uuid when need We used 128 bit uuid for service uuid before. However this caused buffer overflow problem when encounter with multiple 16 bit uuid for this field as the length of advertising data would exceed the limit. We mitigated that by forcing the service uuid to be 16 bit. But this break the device that decide to advertised custom 128 bit uuids. This CL properly fixes the above problems by send 128 bit uuid to Android and let Android side deal with the overflow problem. BUG=b:28670943 TEST=Android side can see 128 bit service uuid ==========
This is now send 128 bit uuids to Android and let Android worry about convert them to shorter uuids. https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2376873002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1915: // Android only support UUID 16 bit here. On 2016/09/29 21:10:15, Luis Héctor Chávez wrote: > nit: s/support/supports/ Done.
Description was changed from ========== arc: bluetooth: Use uuid 128 bit for service uuid when need We used 128 bit uuid for service uuid before. However this caused buffer overflow problem when encounter with multiple 16 bit uuid for this field as the length of advertising data would exceed the limit. We mitigated that by forcing the service uuid to be 16 bit. But this break the device that decide to advertised custom 128 bit uuids. This CL properly fixes the above problems by send 128 bit uuid to Android and let Android side deal with the overflow problem. BUG=b:28670943 TEST=Android side can see 128 bit service uuid ========== to ========== arc: bluetooth: Use uuid 128 bit for service uuid We used 128 bit uuid for service uuid before. However this caused buffer overflow problem when encounter with multiple 16 bit uuid for this field as the length of advertising data would exceed the limit. We mitigated that by forcing the service uuid to be 16 bit. But this break the device that decide to advertised custom 128 bit uuids. This CL properly fixes the above problems by send 128 bit uuid to Android and let Android side deal with the overflow problem. BUG=b:28670943 TEST=Android side can see 128 bit service uuid ==========
Description was changed from ========== arc: bluetooth: Use uuid 128 bit for service uuid We used 128 bit uuid for service uuid before. However this caused buffer overflow problem when encounter with multiple 16 bit uuid for this field as the length of advertising data would exceed the limit. We mitigated that by forcing the service uuid to be 16 bit. But this break the device that decide to advertised custom 128 bit uuids. This CL properly fixes the above problems by send 128 bit uuid to Android and let Android side deal with the overflow problem. BUG=b:28670943 TEST=Android side can see 128 bit service uuid ========== to ========== arc: bluetooth: Use uuid 128 bit for service uuid We used 128 bit uuid for service uuid before. However this caused buffer overflow problem when encounter with multiple 16 bit uuid for this field as the length of advertising data would exceed the limit. We mitigated that by forcing the service uuid to be 16 bit. But this break the device that decide to advertised custom 128 bit uuids. This CL properly fixes the above problems by send 128 bit uuid to Android and let Android side deal with the overflow problem. BUG=653310,b:28670943 TEST=Android side can see 128 bit service uuid ==========
Note that Android side CLs are at http://ag/1512179 http://ag/1512180 http://ag/1512231
lgtm
The CQ bit was checked by puthik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2376873002/#ps100001 (title: "fix comment")
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 ========== arc: bluetooth: Use uuid 128 bit for service uuid We used 128 bit uuid for service uuid before. However this caused buffer overflow problem when encounter with multiple 16 bit uuid for this field as the length of advertising data would exceed the limit. We mitigated that by forcing the service uuid to be 16 bit. But this break the device that decide to advertised custom 128 bit uuids. This CL properly fixes the above problems by send 128 bit uuid to Android and let Android side deal with the overflow problem. BUG=653310,b:28670943 TEST=Android side can see 128 bit service uuid ========== to ========== arc: bluetooth: Use uuid 128 bit for service uuid We used 128 bit uuid for service uuid before. However this caused buffer overflow problem when encounter with multiple 16 bit uuid for this field as the length of advertising data would exceed the limit. We mitigated that by forcing the service uuid to be 16 bit. But this break the device that decide to advertised custom 128 bit uuids. This CL properly fixes the above problems by send 128 bit uuid to Android and let Android side deal with the overflow problem. BUG=653310,b:28670943 TEST=Android side can see 128 bit service uuid ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== arc: bluetooth: Use uuid 128 bit for service uuid We used 128 bit uuid for service uuid before. However this caused buffer overflow problem when encounter with multiple 16 bit uuid for this field as the length of advertising data would exceed the limit. We mitigated that by forcing the service uuid to be 16 bit. But this break the device that decide to advertised custom 128 bit uuids. This CL properly fixes the above problems by send 128 bit uuid to Android and let Android side deal with the overflow problem. BUG=653310,b:28670943 TEST=Android side can see 128 bit service uuid ========== to ========== arc: bluetooth: Use uuid 128 bit for service uuid We used 128 bit uuid for service uuid before. However this caused buffer overflow problem when encounter with multiple 16 bit uuid for this field as the length of advertising data would exceed the limit. We mitigated that by forcing the service uuid to be 16 bit. But this break the device that decide to advertised custom 128 bit uuids. This CL properly fixes the above problems by send 128 bit uuid to Android and let Android side deal with the overflow problem. BUG=653310,b:28670943 TEST=Android side can see 128 bit service uuid Committed: https://crrev.com/81711b78e78640b9ec45cd25cb8c0dbeb252030f Cr-Commit-Position: refs/heads/master@{#423641} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/81711b78e78640b9ec45cd25cb8c0dbeb252030f Cr-Commit-Position: refs/heads/master@{#423641} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
