Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1565773002:
Clear the BLE services list on disconnect. (Closed)
|
Created:
4 years, 11 months ago by tommyt Modified:
4 years, 11 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, perja, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClear the BLE services list on disconnect.
This lets us recreate the services list correctly upon the next
reconnect, which fixes some problems with a device we have, which
disconnects automatically after 30 seconds.
Also reset the services_discovered flag for this device on disconnect.
In order to accomplish this, I had to move this flag from the dispatcher
host to the bluetooth device instance.
BUG=570804
Committed: https://crrev.com/11195385967983a0b5fc3865576a9fa826d51711
Cr-Commit-Position: refs/heads/master@{#369593}
Patch Set 1 #Patch Set 2 : Add a unittest #
Total comments: 9
Patch Set 3 : Add requested improvements to the unittest #
Total comments: 1
Patch Set 4 : Rebased onto master to solve conflicts #
Messages
Total messages: 53 (19 generated)
Description was changed from ========== Clear the BLE services list on disconnect. This lets us recreate the services list correctly upon the next reconnect, which fixes some problems with a device we have, which disconnects automatically after 30 seconds. Also reset the services_discovered flag for this device on disconnect. In order to accomplish this, I had to move this flag from the dispatcher host to the bluetooth device instance. ========== to ========== Clear the BLE services list on disconnect. This lets us recreate the services list correctly upon the next reconnect, which fixes some problems with a device we have, which disconnects automatically after 30 seconds. Also reset the services_discovered flag for this device on disconnect. In order to accomplish this, I had to move this flag from the dispatcher host to the bluetooth device instance. ==========
tommyt@opera.com changed reviewers: + ortuno@chromium.org, scheib@chromium.org
Hi! We've been playing around with some BLE devices here in the Opera office, and I had to make this change to the web bluetooth code to be able to reconnect to a device which auto-disconnects after 30 seconds. I hope this can be useful to you, and if you would like the commit with some modifications, don't hesitate to ask me. Best regards, Tommy Thorsen
On 2016/01/06 15:04:45, tommyt wrote: > Hi! > > We've been playing around with some BLE devices here in the Opera office, and I > had to make this change to the web bluetooth code to be able to reconnect to a > device which auto-disconnects after 30 seconds. Just out of curiosity, which device does auto-disconnect after 30 seconds? > > I hope this can be useful to you, and if you would like the commit with some > modifications, don't hesitate to ask me. > > Best regards, > Tommy Thorsen
It's a very custom device that our friends at Nordic Semiconductor has hand-made for us, so unfortunately you won't be able to buy one of these in the shops. It's a candy dispenser machine that can be controlled via a web page. It disconnects after 30 seconds so that one person won't be hogging all the candy :) They made a youtube video featuring this machine here: https://www.youtube.com/watch?v=jcVhkGRs2FQ If you go visit Nordic Semi's stand at CES this week, you should be able to try out an identical machine. Br, Tommy On Wed, Jan 6, 2016 at 4:11 PM, <fbeaufort@chromium.org> wrote: > On 2016/01/06 15:04:45, tommyt wrote: > >> Hi! >> > > We've been playing around with some BLE devices here in the Opera office, >> and >> > I > >> had to make this change to the web bluetooth code to be able to reconnect >> to a >> device which auto-disconnects after 30 seconds. >> > > Just out of curiosity, which device does auto-disconnect after 30 seconds? > > > I hope this can be useful to you, and if you would like the commit with >> some >> modifications, don't hesitate to ask me. >> > > Best regards, >> Tommy Thorsen >> > > https://codereview.chromium.org/1565773002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks! Meta: Would be good to create issues and CC folks for heads up of changes and work in progress. Just in case anything comes up again related to it the issue also ties together any unforseen follow up patch. The content of the patch looks good. But we should have a test for this scenario added to bluetooth_device_unittest.cc.
On 2016/01/07 23:44:47, scheib wrote: > Thanks! > > Meta: Would be good to create issues and CC folks for heads up of changes and > work in progress. Just in case anything comes up again related to it the issue > also ties together any unforseen follow up patch. We have also created https://groups.google.com/a/chromium.org/forum/#!forum/web-bluetooth for coordinating web bluetooth dev work. > > The content of the patch looks good. But we should have a test for this scenario > added to bluetooth_device_unittest.cc.
I'll look into creating a unittest for this scenario, and update my patch. On Fri, Jan 8, 2016 at 12:44 AM, <scheib@chromium.org> wrote: > Thanks! > > Meta: Would be good to create issues and CC folks for heads up of changes > and > work in progress. Just in case anything comes up again related to it the > issue > also ties together any unforseen follow up patch. > > The content of the patch looks good. But we should have a test for this > scenario > added to bluetooth_device_unittest.cc. > > https://codereview.chromium.org/1565773002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by rune@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1565773002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1565773002/20001
I've uploaded a new patchset that includes a unittest
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rune@opera.com changed reviewers: - rune@opera.com
https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:400: TEST_F(BluetoothTest, BluetoothGattConnection_DisconnectGatt_Cleanup) { Thanks, would you add + Connecting again after a disconnect -- ensuring that a device can be connected to again. + Verifying that the services discovered notification is sent See in this file other uses of: TestBluetoothAdapterObserver observer(adapter_); observer.gatt_services_discovered_count()
Description was changed from ========== Clear the BLE services list on disconnect. This lets us recreate the services list correctly upon the next reconnect, which fixes some problems with a device we have, which disconnects automatically after 30 seconds. Also reset the services_discovered flag for this device on disconnect. In order to accomplish this, I had to move this flag from the dispatcher host to the bluetooth device instance. ========== to ========== Clear the BLE services list on disconnect. This lets us recreate the services list correctly upon the next reconnect, which fixes some problems with a device we have, which disconnects automatically after 30 seconds. Also reset the services_discovered flag for this device on disconnect. In order to accomplish this, I had to move this flag from the dispatcher host to the bluetooth device instance. BUG=570804 ==========
I added a reference to the issue to the description as well. https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:398: // Calls CreateGattConnection & DisconnectGatt, then checks that gatt services You also need to add tests for bluez based systems in: https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:420: EXPECT_EQ(0u, device->GetGattServices().size()); Also make sure IsGattServicesDiscovered == false.
Add requested improvements to the unit test. I've intentionally not added the requested bluez unit tests, as that would require also implementing proper disconnect handling for bluez, which I feel falls outside the scope of this simple patch. https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:398: // Calls CreateGattConnection & DisconnectGatt, then checks that gatt services On 2016/01/11 21:48:40, ortuno (OOO until Jan 10th) wrote: > You also need to add tests for bluez based systems in: > > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... While it wouldn't be too hard to add equivalent unit tests to that bluez file, those tests would be failing, as this functionality is not yet correctly implemented on bluez. I think it would make more sense to implement both the bluez tests and functionality as a separate change from this one. https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:400: TEST_F(BluetoothTest, BluetoothGattConnection_DisconnectGatt_Cleanup) { On 2016/01/11 18:40:09, scheib wrote: > Thanks, would you add > + Connecting again after a disconnect -- ensuring that a device can be connected > to again. > + Verifying that the services discovered notification is sent > See in this file other uses of: > TestBluetoothAdapterObserver observer(adapter_); > observer.gatt_services_discovered_count() Done. https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:420: EXPECT_EQ(0u, device->GetGattServices().size()); On 2016/01/11 21:48:40, ortuno (OOO until Jan 10th) wrote: > Also make sure IsGattServicesDiscovered == false. Done.
https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:398: // Calls CreateGattConnection & DisconnectGatt, then checks that gatt services On 2016/01/12 at 08:42:35, tommyt wrote: > On 2016/01/11 21:48:40, ortuno (OOO until Jan 10th) wrote: > > You also need to add tests for bluez based systems in: > > > > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > > While it wouldn't be too hard to add equivalent unit tests to that bluez file, those tests would be failing, as this functionality is not yet correctly implemented on bluez. I think it would make more sense to implement both the bluez tests and functionality as a separate change from this one. btw I'm not talking about clearing the services list but rather setting services_discovered_ to false. Maybe I'm missing something but I don't think you need to touch disconnect at all. You would just need to make a two-line change: Add "device_bluez->SetGattServicesDiscoveryComplete(false);" to https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... if the device disconnected. Removing the map without a proper implementation in Bluez leads to many bugs, for example calling getPrimaryService() after all services have been discovered would return a promise that never resolves.
https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:398: // Calls CreateGattConnection & DisconnectGatt, then checks that gatt services On 2016/01/12 22:28:49, ortuno wrote: > On 2016/01/12 at 08:42:35, tommyt wrote: > > On 2016/01/11 21:48:40, ortuno (OOO until Jan 10th) wrote: > > > You also need to add tests for bluez based systems in: > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > > > > While it wouldn't be too hard to add equivalent unit tests to that bluez file, > those tests would be failing, as this functionality is not yet correctly > implemented on bluez. I think it would make more sense to implement both the > bluez tests and functionality as a separate change from this one. > > btw I'm not talking about clearing the services list but rather setting > services_discovered_ to false. Maybe I'm missing something but I don't think you > need to touch disconnect at all. > > You would just need to make a two-line change: > > Add "device_bluez->SetGattServicesDiscoveryComplete(false);" to > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > if the device disconnected. > > Removing the map without a proper implementation in Bluez leads to many bugs, > for example calling getPrimaryService() after all services have been discovered > would return a promise that never resolves. Hmm. The intention of my change was to fix something for android, and leave all other platforms working the way they did before my change. Introducing this kind of bug is of course not acceptable. That being said, I have a hard time seeing: A) how my change could cause that kind of bug, and B) how calling SetGattServicesDiscoveryComplete(false) on disconnect would fix it. As far as I can see, calling getPrimaryService() after all services have been discovered, should end up in the first clause (labelled "1. & 2.") in BluetoothDispatcherHost::OnGetPrimaryService(). This should cause AddToServicesMapAndSendGetPrimaryServiceSuccess() to be called, which should resolve the promise right away. The only change I did to the bluez code, was to add device->SetGattServicesDiscoveryComplete(true) to make up for the BluetoothDispatcherHost no longer keeping the discovery complete state in that map. As far as I can see, whether this state is kept in a map or in a boolean variable in each device, should not make any difference. What am I missing here?
lgtm https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:398: // Calls CreateGattConnection & DisconnectGatt, then checks that gatt services On 2016/01/13 at 09:42:30, tommyt wrote: > On 2016/01/12 22:28:49, ortuno wrote: > > On 2016/01/12 at 08:42:35, tommyt wrote: > > > On 2016/01/11 21:48:40, ortuno (OOO until Jan 10th) wrote: > > > > You also need to add tests for bluez based systems in: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > > > > > > While it wouldn't be too hard to add equivalent unit tests to that bluez file, > > those tests would be failing, as this functionality is not yet correctly > > implemented on bluez. I think it would make more sense to implement both the > > bluez tests and functionality as a separate change from this one. > > > > btw I'm not talking about clearing the services list but rather setting > > services_discovered_ to false. Maybe I'm missing something but I don't think you > > need to touch disconnect at all. > > > > You would just need to make a two-line change: > > > > Add "device_bluez->SetGattServicesDiscoveryComplete(false);" to > > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > > if the device disconnected. > > > > Removing the map without a proper implementation in Bluez leads to many bugs, > > for example calling getPrimaryService() after all services have been discovered > > would return a promise that never resolves. > > Hmm. The intention of my change was to fix something for android, and leave all other platforms working the way they did before my change. Introducing this kind of bug is of course not acceptable. > > That being said, I have a hard time seeing: A) how my change could cause that kind of bug, and B) how calling SetGattServicesDiscoveryComplete(false) on disconnect would fix it. As far as I can see, calling getPrimaryService() after all services have been discovered, should end up in the first clause (labelled "1. & 2.") in BluetoothDispatcherHost::OnGetPrimaryService(). This should cause AddToServicesMapAndSendGetPrimaryServiceSuccess() to be called, which should resolve the promise right away. > > The only change I did to the bluez code, was to add device->SetGattServicesDiscoveryComplete(true) to make up for the BluetoothDispatcherHost no longer keeping the discovery complete state in that map. As far as I can see, whether this state is kept in a map or in a boolean variable in each device, should not make any difference. What am I missing here? My bad that was an incorrect example. The issue that we currently have and I would like to get fixed is the following: 1. Connect to a device; Its services are discovered and gatt_services_discovered_ becomes true. 2. Disconnect. 3. Connect again. Services haven't been discovered yet but the variable is still set to true so we hit case 3. This can be solved if you set the variable to false when a device disconnects the same way you do in android in onConnectionStateChanged. I see your point in that you only want to fix Android but with that addition you can fix Chrome OS and Linux in time for Chrome 49! Anyway, you are not introducing any new bugs so this LGTM. https://codereview.chromium.org/1565773002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/1565773002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:427: // Set the gatt services discovery complete flag for this device. nit: comment should be descriptive rather than imperative[1] [1] https://google.github.io/styleguide/cppguide.html#Function_Comments
LGTM, if you don't choose to take on the bluez bit would you please file an issue to us for it?
The CQ bit was checked by tommyt@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1565773002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1565773002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by tommyt@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1565773002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1565773002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tommyt@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org, ortuno@chromium.org Link to the patchset: https://codereview.chromium.org/1565773002/#ps60001 (title: "Rebased onto master to solve conflicts")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1565773002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1565773002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Clear the BLE services list on disconnect. This lets us recreate the services list correctly upon the next reconnect, which fixes some problems with a device we have, which disconnects automatically after 30 seconds. Also reset the services_discovered flag for this device on disconnect. In order to accomplish this, I had to move this flag from the dispatcher host to the bluetooth device instance. BUG=570804 ========== to ========== Clear the BLE services list on disconnect. This lets us recreate the services list correctly upon the next reconnect, which fixes some problems with a device we have, which disconnects automatically after 30 seconds. Also reset the services_discovered flag for this device on disconnect. In order to accomplish this, I had to move this flag from the dispatcher host to the bluetooth device instance. BUG=570804 ==========
tommyt@opera.com changed reviewers: + armansito@chromium.org
@armansito: Hi! I had originally added scheib and ortuno as reviewers, but it turns out they are not owners for all of the files I touched with this patch. Would you mind taking a look at my changes? Thanks!
On 2016/01/13 18:36:47, scheib wrote: > LGTM, if you don't choose to take on the bluez bit would you please file an > issue to us for it? I've filed this issue for the bluez problem: https://code.google.com/p/chromium/issues/detail?id=577641 I would be ok with trying to fix this (in a separate commit), but I prefer to be able to test my code before uploading a review. What is the best way to test the bluez code on linux? I tried building a regular chromium browser thing (with "ninja -C out/Linux chrome"), but it says that WebBluetooth is not an available experiment, in chrome://flags.
On 2016/01/14 13:24:07, tommyt wrote: > On 2016/01/13 18:36:47, scheib wrote: > > LGTM, if you don't choose to take on the bluez bit would you please file an > > issue to us for it? > > I've filed this issue for the bluez problem: > https://code.google.com/p/chromium/issues/detail?id=577641 > > I would be ok with trying to fix this (in a separate commit), but I prefer to be > able to test my code before uploading a review. What is the best way to test the > bluez code on linux? I tried building a regular chromium browser thing (with > "ninja -C out/Linux chrome"), but it says that WebBluetooth is not an available > experiment, in chrome://flags. I have now tried to build with target_os="chromeos", which also gives me a working browser on linux. With this browser, I'm able to enable the web bluetooth flag, but in javascript, "navigator.bluetooth" is not defined.
On 2016/01/14 14:20:02, tommyt wrote: > On 2016/01/14 13:24:07, tommyt wrote: > > On 2016/01/13 18:36:47, scheib wrote: > > > LGTM, if you don't choose to take on the bluez bit would you please file an > > > issue to us for it? > > > > I've filed this issue for the bluez problem: > > https://code.google.com/p/chromium/issues/detail?id=577641 > > > > I would be ok with trying to fix this (in a separate commit), but I prefer to > be > > able to test my code before uploading a review. What is the best way to test > the > > bluez code on linux? I tried building a regular chromium browser thing (with > > "ninja -C out/Linux chrome"), but it says that WebBluetooth is not an > available > > experiment, in chrome://flags. > > I have now tried to build with target_os="chromeos", which also gives me a > working browser on linux. With this browser, I'm able to enable the web > bluetooth flag, but in javascript, "navigator.bluetooth" is not defined. Set --enable-web-bluetooth on the commend line - instead of via about:flags. Not sure why, but it seems about:flags alone aren't sufficient for chromeos build on linux.
lgtm
The CQ bit was checked by scheib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1565773002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1565773002/60001
Message was sent while issue was closed.
Description was changed from ========== Clear the BLE services list on disconnect. This lets us recreate the services list correctly upon the next reconnect, which fixes some problems with a device we have, which disconnects automatically after 30 seconds. Also reset the services_discovered flag for this device on disconnect. In order to accomplish this, I had to move this flag from the dispatcher host to the bluetooth device instance. BUG=570804 ========== to ========== Clear the BLE services list on disconnect. This lets us recreate the services list correctly upon the next reconnect, which fixes some problems with a device we have, which disconnects automatically after 30 seconds. Also reset the services_discovered flag for this device on disconnect. In order to accomplish this, I had to move this flag from the dispatcher host to the bluetooth device instance. BUG=570804 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Clear the BLE services list on disconnect. This lets us recreate the services list correctly upon the next reconnect, which fixes some problems with a device we have, which disconnects automatically after 30 seconds. Also reset the services_discovered flag for this device on disconnect. In order to accomplish this, I had to move this flag from the dispatcher host to the bluetooth device instance. BUG=570804 ========== to ========== Clear the BLE services list on disconnect. This lets us recreate the services list correctly upon the next reconnect, which fixes some problems with a device we have, which disconnects automatically after 30 seconds. Also reset the services_discovered flag for this device on disconnect. In order to accomplish this, I had to move this flag from the dispatcher host to the bluetooth device instance. BUG=570804 Committed: https://crrev.com/11195385967983a0b5fc3865576a9fa826d51711 Cr-Commit-Position: refs/heads/master@{#369593} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/11195385967983a0b5fc3865576a9fa826d51711 Cr-Commit-Position: refs/heads/master@{#369593}
Message was sent while issue was closed.
This seems to have caused the bluetooth/getPrimaryService.html layout test to start timing out. I'll set an expectation and file a bug, but consider reverting if you can't figure out why soon.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1587193004/ by scheib@chromium.org. The reason for reverting is: bluetooth/getPrimaryService.html is timing out. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=blu... https://code.google.com/p/chromium/issues/detail?id=577936.
Message was sent while issue was closed.
Reproducing locally should be possible with ninja -C out/Release blink_tests && blink/tools/run_layout_tests.sh bluetooth
Message was sent while issue was closed.
Patchset #5 (id:80001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
