Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1116963002:
Bluetooth low energy connection. (Closed)
DescriptionImplementing a proximity_auth::Connection interface for Bluetooth Low Energy devices.
The current version does not handle messages larger than a single characteristic size (around 150 bytes). Message receiving functionality is not implemented yet.
BUG=485123
Committed: https://crrev.com/574c1637312ef98f8fa4bde909c1f040ddf33a4b
Cr-Commit-Position: refs/heads/master@{#329837}
Patch Set 1 #Patch Set 2 : Cleaning up #Patch Set 3 : More cleaning up #Patch Set 4 : Starting notification session #Patch Set 5 : Removing debug messages #
Total comments: 60
Patch Set 6 : Addressing comments and handling control signals #Patch Set 7 : Refactoring the connection callback #
Total comments: 25
Patch Set 8 : Addressing tengs comments #
Total comments: 5
Patch Set 9 : nits #Patch Set 10 : Refactoring #Patch Set 11 : #Messages
Total messages: 25 (6 generated)
sacomoto@chromium.org changed reviewers: + msarda@chromium.org
PTAL.
https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_connection.cc (right): https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:41: if (connection_) { Can |connection_| be null? If not, then it is better to DCHECK: DCHECK(connection_) https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:45: if (adapter_) { DCHECK(adapter_)? https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:69: device->Forget(base::Bind(&base::DoNothing)); Please add a comment that disconnect actually forgets the remote BLE device. Also comment why this is safe (the fact that it is SmarLock device). https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:74: // TODO(sacomoto): Send a Socketeer incoming signal. Implement a sender with Remove all reference to Socketeer. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:96: VLOG(1) << "device removed " << remote_device_address(); Log should state with CAPS: s/device/Device https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:116: VLOG(1) << "Connection error, missing characteristics for service."; s/service/Smart Lock service https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:116: VLOG(1) << "Connection error, missing characteristics for service."; Please also log which one of |to_peripheral_char_id_| and |from_peripheral_char_id_| were not found. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:129: // TODO(sacomoto): Parse the Sockeeter incoming signal. Implement a receiver Remove Socketeer. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:169: SendInviteToConnectSignal(); Should we wait for a reply to invite signal before actually changing the status to CONNECTED? https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_connection.h (right): https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:22: class BluetoothLowEnergyConnection : public Connection, Comment this class. It should ideally describe the strategy used to cut down messages that are sent over BLE. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:33: void Connect(); Are these methods overriding Connection? If so, then they should be void Connect() override; https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:58: std::string remote_device_address() { This is not a trivial method. It should be GetRemoteDeviceAddress and its implementation should be in the .cc file. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:58: std::string remote_device_address() { Change return type to const std::string& https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:62: // Send a invite to connect signal (defined in Socketeer library) to the s/Send a invite/Sends an invite https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:62: // Send a invite to connect signal (defined in Socketeer library) to the Remote reference to Socketeer library. That is not public and people looking at this code review will not understand what that means. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:63: // central. central? Should this be peripheral? https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:66: // Called when there is an error writing the remote characteristic s/writing the/writing to the https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:75: // The connection is complete when: (i) |to_peripheral_char_| and Maybe move (i) and (ii) on different lines (like bullet points). I find it more readable. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:88: // |from_peripheral_char_|. s/|from_peripheral_char_|/|from_peripheral_char_| characteristic. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:92: // |from_peripheral_char_|. Ditto. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:131: const std::string to_peripheral_char_ = Same as for from_peripheral_char_ https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:139: const std::string from_peripheral_char_ = These should be constants received from the outside or defined in an internal anonymous namespace. We do not initialize them in header files. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:139: const std::string from_peripheral_char_ = This is a characteristic UUID. This is not clear from its name. Maybe: s/from_peripheral_char_/from_peripheral_char_characteristic_uuid_. But the const should just be internal or passed down as an argument in the constructor. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc (right): https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:18: #include "components/proximity_auth/ble/bluetooth_low_energy_connection.h" Sort headers. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:213: std::string remote_device_address = gatt_connection->GetDeviceAddress(); Remove remote_device_address as it is not used. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... File components/proximity_auth/ble/fake_wire_message.h (right): https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/fake_wire_message.h:12: Kill empty line. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/fake_wire_message.h:23: bool* is_incomplete_message); is_incomplete_message is an output variable. Mark it as such for clarity. s/is_incomplete_message/out_is_incomplete_message https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... File components/proximity_auth/ble/proximity_auth_ble_system.cc (right): https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/proximity_auth_ble_system.cc:67: connection_.reset(); Destroy the connection before the finder. Just in case the connection may need to talk with the finder. I would also call connection_->Disconnect() to make it clear that the connection is actually closed here. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/proximity_auth_ble_system.cc:78: connection_ = connection.Pass(); We should close the connection when the screen in unlocked. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... File components/proximity_auth/connection.h (right): https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/connection.h:91: std::string received_bytes() { return received_bytes_; } s/std::string received_bytes()/const std::string& received_bytes() const
https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_connection.cc (right): https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:41: if (connection_) { On 2015/05/05 11:56:13, msarda wrote: > Can |connection_| be null? If not, then it is better to DCHECK: > DCHECK(connection_) Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:45: if (adapter_) { On 2015/05/05 11:56:14, msarda wrote: > DCHECK(adapter_)? Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:69: device->Forget(base::Bind(&base::DoNothing)); On 2015/05/05 11:56:14, msarda wrote: > Please add a comment that disconnect actually forgets the remote BLE device. > Also comment why this is safe (the fact that it is SmarLock device). Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:74: // TODO(sacomoto): Send a Socketeer incoming signal. Implement a sender with On 2015/05/05 11:56:13, msarda wrote: > Remove all reference to Socketeer. Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:96: VLOG(1) << "device removed " << remote_device_address(); On 2015/05/05 11:56:13, msarda wrote: > Log should state with CAPS: > s/device/Device Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:116: VLOG(1) << "Connection error, missing characteristics for service."; On 2015/05/05 11:56:13, msarda wrote: > s/service/Smart Lock service Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:116: VLOG(1) << "Connection error, missing characteristics for service."; On 2015/05/05 11:56:13, msarda wrote: > Please also log which one of |to_peripheral_char_id_| and > |from_peripheral_char_id_| were not found. Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:129: // TODO(sacomoto): Parse the Sockeeter incoming signal. Implement a receiver On 2015/05/05 11:56:13, msarda wrote: > Remove Socketeer. Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:169: SendInviteToConnectSignal(); On 2015/05/05 11:56:13, msarda wrote: > Should we wait for a reply to invite signal before actually changing the status > to CONNECTED? Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_connection.h (right): https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:22: class BluetoothLowEnergyConnection : public Connection, On 2015/05/05 11:56:14, msarda wrote: > Comment this class. It should ideally describe the strategy used to cut down > messages that are sent over BLE. Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:33: void Connect(); On 2015/05/05 11:56:14, msarda wrote: > Are these methods overriding Connection? If so, then they should be > void Connect() override; Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:58: std::string remote_device_address() { On 2015/05/05 11:56:14, msarda wrote: > This is not a trivial method. It should be GetRemoteDeviceAddress and its > implementation should be in the .cc file. Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:58: std::string remote_device_address() { On 2015/05/05 11:56:14, msarda wrote: > Change return type to const std::string& Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:62: // Send a invite to connect signal (defined in Socketeer library) to the On 2015/05/05 11:56:14, msarda wrote: > s/Send a invite/Sends an invite Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:62: // Send a invite to connect signal (defined in Socketeer library) to the On 2015/05/05 11:56:14, msarda wrote: > Remote reference to Socketeer library. That is not public and people looking at > this code review will not understand what that means. Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:63: // central. On 2015/05/05 11:56:14, msarda wrote: > central? Should this be peripheral? Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:66: // Called when there is an error writing the remote characteristic On 2015/05/05 11:56:14, msarda wrote: > s/writing the/writing to the Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:75: // The connection is complete when: (i) |to_peripheral_char_| and On 2015/05/05 11:56:14, msarda wrote: > Maybe move (i) and (ii) on different lines (like bullet points). I find it more > readable. Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:88: // |from_peripheral_char_|. On 2015/05/05 11:56:14, msarda wrote: > s/|from_peripheral_char_|/|from_peripheral_char_| characteristic. Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:92: // |from_peripheral_char_|. On 2015/05/05 11:56:14, msarda wrote: > Ditto. Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:131: const std::string to_peripheral_char_ = On 2015/05/05 11:56:14, msarda wrote: > Same as for from_peripheral_char_ Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:139: const std::string from_peripheral_char_ = On 2015/05/05 11:56:14, msarda wrote: > This is a characteristic UUID. This is not clear from its name. Maybe: > s/from_peripheral_char_/from_peripheral_char_characteristic_uuid_. > > But the const should just be internal or passed down as an argument in the > constructor. Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection.h:139: const std::string from_peripheral_char_ = On 2015/05/05 11:56:14, msarda wrote: > These should be constants received from the outside or defined in an internal > anonymous namespace. We do not initialize them in header files. Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc (right): https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:18: #include "components/proximity_auth/ble/bluetooth_low_energy_connection.h" On 2015/05/05 11:56:14, msarda wrote: > Sort headers. Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:213: std::string remote_device_address = gatt_connection->GetDeviceAddress(); On 2015/05/05 11:56:14, msarda wrote: > Remove remote_device_address as it is not used. Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... File components/proximity_auth/ble/fake_wire_message.h (right): https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/fake_wire_message.h:12: On 2015/05/05 11:56:14, msarda wrote: > Kill empty line. Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/fake_wire_message.h:23: bool* is_incomplete_message); On 2015/05/05 11:56:14, msarda wrote: > is_incomplete_message is an output variable. Mark it as such for clarity. > > s/is_incomplete_message/out_is_incomplete_message I'm being consistent with proximity_auth::WireMessage implementation. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... File components/proximity_auth/ble/proximity_auth_ble_system.cc (right): https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/proximity_auth_ble_system.cc:67: connection_.reset(); On 2015/05/05 11:56:15, msarda wrote: > Destroy the connection before the finder. Just in case the connection may need > to talk with the finder. > > I would also call connection_->Disconnect() to make it clear that the connection > is actually closed here. Done. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/ble/proximity_auth_ble_system.cc:78: connection_ = connection.Pass(); On 2015/05/05 11:56:15, msarda wrote: > We should close the connection when the screen in unlocked. There is no need to do it here. When the screen is unlocked ProximityAuthBleSystem::OnScreenDidUnlock is called, and the connection is destroyed. https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... File components/proximity_auth/connection.h (right): https://codereview.chromium.org/1116963002/diff/80001/components/proximity_au... components/proximity_auth/connection.h:91: std::string received_bytes() { return received_bytes_; } On 2015/05/05 11:56:15, msarda wrote: > s/std::string received_bytes()/const std::string& received_bytes() const Done.
sacomoto@chromium.org changed reviewers: + tengs@chromium.org
Hi Tim, I need an owners review for proximity_auth/connection.h and proximity_auth.gypi. Mihai already did a first review, but he's OOO for the next 2 weeks. So, it'd be nice if also could take a look in the rest. I'm currently implementing unit tests for both classes: BluetoothLowEnergyConnection and BluetoothLowEnergyConnectionFinder. I should send the CLs to review next week.
https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection.cc (right): https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:51: adapter_->AddObserver(this); Are all services going to be discovered as soon as the GattConnection is created? Do you need to iterate through all the services currently on the remote device in case they're not discovered later on? https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:141: HandleCharacteristicUpdate(characteristic); Is it correct to call HandleCharacteristicUpdate() here? According to the comments, GattDiscoveryCompleteForService() will be always be called at the end, so you will always be calling HandleCharacteristicUpdate() twice for the service. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:260: const char connect_signal[4] = {}; You should use use the kInviteToConnectSignal value explicitly. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:278: if (from_peripheral_char_uuid_ == canonical_value) { nit : please be consistent and remove the braces for single line conditionals. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection.h (right): https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:29: enum ControlSignal : uint32 { nit: use an enum class instead. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:38: // |remote_device| must be already established and |adapter| already Can you document the flow for establishing the logical connection after being given a device connected through BLE? I noticed you have a similar comment below. You should move that to the class comment or here. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:97: // (ii) |notify_session_| was set for |from_peripheral_char_uuid_|; I would also add that you write an initial hello signal on the peripheral. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:150: const device::BluetoothUUID remote_service_uuid_; nit: Can you create a private struct that encapsulates the (UUID, ID) pair? It would really help with readability. Also, why is the service UUID stored as a device::BluetoothUUID but the characteristic UUIDs as strings? https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:172: bool notify_session_pending_; Instead of having a separate bool for each state to wait for an operation to finish, let's use an enum to track the internal state for what this object is currently waiting on. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc (right): https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:259: return scoped_ptr<BluetoothLowEnergyConnection>( nit: it's a bit shorter to use make_scoped_ptr()
I don't see proximity_auth/connection.h. Did you forget to add it or did you not need the changes anymore?
On 2015/05/11 08:03:18, Tim Song wrote: > I don't see proximity_auth/connection.h. > > Did you forget to add it or did you not need the changes anymore? It's still there on Patch Set 7. It's the last file in the list. Anyway, I touch it again in the next patch.
PTAL. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection.cc (right): https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:51: adapter_->AddObserver(this); On 2015/05/11 08:02:39, Tim Song wrote: > Are all services going to be discovered as soon as the GattConnection is > created? Do you need to iterate through all the services currently on the remote > device in case they're not discovered later on? Yes, you are right. We should scan the services and characteristics here. I added the scan. I also modified the GetRemoteService() method to handle the case where the remote_service_id_ is empty but the service was already discovered (which would be the case here). I already intended to do it, but got delayed because at the moment it doesn't matter: after the a GATT connection is established it takes 10s for the characteristics to be discovered... https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:141: HandleCharacteristicUpdate(characteristic); On 2015/05/11 08:02:39, Tim Song wrote: > Is it correct to call HandleCharacteristicUpdate() here? According to the > comments, GattDiscoveryCompleteForService() will be always be called at the end, > so you will always be calling HandleCharacteristicUpdate() twice for the > service. This was intentional. The idea was to have the connection ready as soon as possible. If the necessary characteristics were added in the very beginning, we don't to wait for a call of GattDiscoveryCompleteForService() to have a complete connection. Moreover, there is no problem calling HandleCharacteristicUpdate() twice. The second call will have (almost) no effect. The notify session is started only once and only one invite to connect signal is sent. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:260: const char connect_signal[4] = {}; On 2015/05/11 08:02:39, Tim Song wrote: > You should use use the kInviteToConnectSignal value explicitly. Done. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:278: if (from_peripheral_char_uuid_ == canonical_value) { On 2015/05/11 08:02:39, Tim Song wrote: > nit : please be consistent and remove the braces for single line conditionals. Done. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection.h (right): https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:29: enum ControlSignal : uint32 { On 2015/05/11 08:02:39, Tim Song wrote: > nit: use an enum class instead. Done. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:38: // |remote_device| must be already established and |adapter| already On 2015/05/11 08:02:39, Tim Song wrote: > Can you document the flow for establishing the logical connection after being > given a device connected through BLE? I noticed you have a similar comment > below. You should move that to the class comment or here. Done. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:97: // (ii) |notify_session_| was set for |from_peripheral_char_uuid_|; On 2015/05/11 08:02:39, Tim Song wrote: > I would also add that you write an initial hello signal on the peripheral. Done. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:150: const device::BluetoothUUID remote_service_uuid_; On 2015/05/11 08:02:39, Tim Song wrote: > nit: Can you create a private struct that encapsulates the (UUID, ID) pair? It > would really help with readability. > > Also, why is the service UUID stored as a device::BluetoothUUID but the > characteristic UUIDs as strings? Done. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:172: bool notify_session_pending_; On 2015/05/11 08:02:39, Tim Song wrote: > Instead of having a separate bool for each state to wait for an operation to > finish, let's use an enum to track the internal state for what this object is > currently waiting on. We could to this way, but I don't think it would make the code cleaner. The problem is that we don't have a total order for all events leading to a connection. We only have a partial order. The events leading to a connection are: (i) |to_peripheral_char| discovered; (ii) |from_peripheral_char| discovered; (iii) notify session for |from_peripheral_char| started; (iv) invite to connect signal sent; (v) response received. The last two events are ordered, (v) immediately happens after (iv). But (iv) can be preceded by (iii) or (i). This happens because we don't know in which order the characteristics will be discovered. A possible ordering is this: |from_peripheral_char| is discovered (i), the notify session is started (iii), |to_peripheral_char| is discovered. In this case, we have (ii) < (iii) < (i) < (iv) < (v). And we could as well have (i) < (ii) < (iii) < (iv) < (v). https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc (right): https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc:259: return scoped_ptr<BluetoothLowEnergyConnection>( On 2015/05/11 08:02:40, Tim Song wrote: > nit: it's a bit shorter to use make_scoped_ptr() Done.
https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection.cc (right): https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:141: HandleCharacteristicUpdate(characteristic); On 2015/05/11 12:04:04, sacomoto wrote: > On 2015/05/11 08:02:39, Tim Song wrote: > > Is it correct to call HandleCharacteristicUpdate() here? According to the > > comments, GattDiscoveryCompleteForService() will be always be called at the > end, > > so you will always be calling HandleCharacteristicUpdate() twice for the > > service. > > This was intentional. The idea was to have the connection ready as soon as > possible. If the necessary characteristics were added in the very beginning, we > don't to wait for a call of GattDiscoveryCompleteForService() to have a complete > connection. > > Moreover, there is no problem calling HandleCharacteristicUpdate() twice. The > second call will have (almost) no effect. The notify session is started only > once and only one invite to connect signal is sent. Thanks for the explanation. In this case, do you even need to override GattDiscoveryCompleteForService()? GattCharacteristicAdded() should be called for all characteristics you haven't handled in the constructor. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection.h (right): https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:172: bool notify_session_pending_; On 2015/05/11 12:04:04, sacomoto wrote: > On 2015/05/11 08:02:39, Tim Song wrote: > > Instead of having a separate bool for each state to wait for an operation to > > finish, let's use an enum to track the internal state for what this object is > > currently waiting on. > > We could to this way, but I don't think it would make the code cleaner. The > problem is that we don't have a total order for all events leading to a > connection. We only have a partial order. > > The events leading to a connection are: > > (i) |to_peripheral_char| discovered; > (ii) |from_peripheral_char| discovered; > (iii) notify session for |from_peripheral_char| started; > (iv) invite to connect signal sent; > (v) response received. > > The last two events are ordered, (v) immediately happens after (iv). But (iv) > can be preceded by (iii) or (i). > > This happens because we don't know in which order the characteristics will be > discovered. A possible ordering is this: |from_peripheral_char| is discovered > (i), the notify session is started (iii), |to_peripheral_char| is discovered. In > this case, we have (ii) < (iii) < (i) < (iv) < (v). And we could as well have > (i) < (ii) < (iii) < (iv) < (v). Thanks, I understand the flow a lot better now. Ideally, we should refactor all the characteristic discovery code to another class, so we will only have two linear states to establish a ProximityAuth connection: WAITING_FOR_CHARACTERISTICS, and WAITING_FOR_INVITATION_RESPONSE. This branching flow can be hard to understand when you're dealing with a bunch of reentrant functions and a bunch of booleans. I won't ask you to do that in this CL, but it maybe something to keep in mind when you write tests. It would be a lot easier to test the characteristic discovery by itself. https://codereview.chromium.org/1116963002/diff/140001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection.h (right): https://codereview.chromium.org/1116963002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:36: // Constructs a Bluetooth low energy connection to the service with I think an diagram would be much more descriptive of the flow: Discover ReaderCharacteristic Discover WriterCharacteristic | | Start Notify Session | | | ---------------- && --------------------- | | | Write InviteToConnect to WriterCharacteristic | | | Read InvitationResponse from ReaderCharacteristic | | | Proximity Auth Connection Established https://codereview.chromium.org/1116963002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:161: const device::BluetoothUUID remote_service_uuid_; You can put |remote_service_uuid_| and |remote_service_id_| in a RemoteCharacteristic struct as well, but you should probably change the name.
https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection.cc (right): https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:141: HandleCharacteristicUpdate(characteristic); On 2015/05/12 03:02:46, Tim Song wrote: > On 2015/05/11 12:04:04, sacomoto wrote: > > On 2015/05/11 08:02:39, Tim Song wrote: > > > Is it correct to call HandleCharacteristicUpdate() here? According to the > > > comments, GattDiscoveryCompleteForService() will be always be called at the > > end, > > > so you will always be calling HandleCharacteristicUpdate() twice for the > > > service. > > > > This was intentional. The idea was to have the connection ready as soon as > > possible. If the necessary characteristics were added in the very beginning, > we > > don't to wait for a call of GattDiscoveryCompleteForService() to have a > complete > > connection. > > > > Moreover, there is no problem calling HandleCharacteristicUpdate() twice. The > > second call will have (almost) no effect. The notify session is started only > > once and only one invite to connect signal is sent. > > Thanks for the explanation. In this case, do you even need to override > GattDiscoveryCompleteForService()? GattCharacteristicAdded() should be called > for all characteristics you haven't handled in the constructor. Yes, we still need to override GattDiscoveryCompleteForService(). That is when we discover that there is an error: all characteristics were discovered but the two characteristics we need were not there. https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection.h (right): https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:172: bool notify_session_pending_; On 2015/05/12 03:02:46, Tim Song wrote: > On 2015/05/11 12:04:04, sacomoto wrote: > > On 2015/05/11 08:02:39, Tim Song wrote: > > > Instead of having a separate bool for each state to wait for an operation to > > > finish, let's use an enum to track the internal state for what this object > is > > > currently waiting on. > > > > We could to this way, but I don't think it would make the code cleaner. The > > problem is that we don't have a total order for all events leading to a > > connection. We only have a partial order. > > > > The events leading to a connection are: > > > > (i) |to_peripheral_char| discovered; > > (ii) |from_peripheral_char| discovered; > > (iii) notify session for |from_peripheral_char| started; > > (iv) invite to connect signal sent; > > (v) response received. > > > > The last two events are ordered, (v) immediately happens after (iv). But (iv) > > can be preceded by (iii) or (i). > > > > This happens because we don't know in which order the characteristics will be > > discovered. A possible ordering is this: |from_peripheral_char| is discovered > > (i), the notify session is started (iii), |to_peripheral_char| is discovered. > In > > this case, we have (ii) < (iii) < (i) < (iv) < (v). And we could as well have > > (i) < (ii) < (iii) < (iv) < (v). > > Thanks, I understand the flow a lot better now. Ideally, we should refactor all > the characteristic discovery code to another class, so we will only have two > linear states to establish a ProximityAuth connection: > WAITING_FOR_CHARACTERISTICS, and WAITING_FOR_INVITATION_RESPONSE. This branching > flow can be hard to understand when you're dealing with a bunch of reentrant > functions and a bunch of booleans. > > I won't ask you to do that in this CL, but it maybe something to keep in mind > when you write tests. It would be a lot easier to test the characteristic > discovery by itself. Ok. https://codereview.chromium.org/1116963002/diff/140001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection.h (right): https://codereview.chromium.org/1116963002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:36: // Constructs a Bluetooth low energy connection to the service with On 2015/05/12 03:02:47, Tim Song wrote: > > > I think an diagram would be much more descriptive of the flow: > > Discover ReaderCharacteristic Discover WriterCharacteristic > | | > Start Notify Session | > | | > ---------------- && --------------------- > | > | > | > Write InviteToConnect to WriterCharacteristic > | > | > | > Read InvitationResponse from ReaderCharacteristic > | > | > | > Proximity Auth Connection Established Done. https://codereview.chromium.org/1116963002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:161: const device::BluetoothUUID remote_service_uuid_; On 2015/05/12 03:02:47, Tim Song wrote: > You can put |remote_service_uuid_| and |remote_service_id_| in a > RemoteCharacteristic struct as well, but you should probably change the name. Done.
On 2015/05/12 07:23:20, sacomoto wrote: > https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... > File components/proximity_auth/ble/bluetooth_low_energy_connection.cc (right): > > https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... > components/proximity_auth/ble/bluetooth_low_energy_connection.cc:141: > HandleCharacteristicUpdate(characteristic); > On 2015/05/12 03:02:46, Tim Song wrote: > > On 2015/05/11 12:04:04, sacomoto wrote: > > > On 2015/05/11 08:02:39, Tim Song wrote: > > > > Is it correct to call HandleCharacteristicUpdate() here? According to the > > > > comments, GattDiscoveryCompleteForService() will be always be called at > the > > > end, > > > > so you will always be calling HandleCharacteristicUpdate() twice for the > > > > service. > > > > > > This was intentional. The idea was to have the connection ready as soon as > > > possible. If the necessary characteristics were added in the very beginning, > > we > > > don't to wait for a call of GattDiscoveryCompleteForService() to have a > > complete > > > connection. > > > > > > Moreover, there is no problem calling HandleCharacteristicUpdate() twice. > The > > > second call will have (almost) no effect. The notify session is started only > > > once and only one invite to connect signal is sent. > > > > Thanks for the explanation. In this case, do you even need to override > > GattDiscoveryCompleteForService()? GattCharacteristicAdded() should be called > > for all characteristics you haven't handled in the constructor. > > Yes, we still need to override GattDiscoveryCompleteForService(). That is when > we discover that there is an error: all characteristics were discovered but the > two characteristics we need were not there. > > https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... > File components/proximity_auth/ble/bluetooth_low_energy_connection.h (right): > > https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... > components/proximity_auth/ble/bluetooth_low_energy_connection.h:172: bool > notify_session_pending_; > On 2015/05/12 03:02:46, Tim Song wrote: > > On 2015/05/11 12:04:04, sacomoto wrote: > > > On 2015/05/11 08:02:39, Tim Song wrote: > > > > Instead of having a separate bool for each state to wait for an operation > to > > > > finish, let's use an enum to track the internal state for what this object > > is > > > > currently waiting on. > > > > > > We could to this way, but I don't think it would make the code cleaner. The > > > problem is that we don't have a total order for all events leading to a > > > connection. We only have a partial order. > > > > > > The events leading to a connection are: > > > > > > (i) |to_peripheral_char| discovered; > > > (ii) |from_peripheral_char| discovered; > > > (iii) notify session for |from_peripheral_char| started; > > > (iv) invite to connect signal sent; > > > (v) response received. > > > > > > The last two events are ordered, (v) immediately happens after (iv). But > (iv) > > > can be preceded by (iii) or (i). > > > > > > This happens because we don't know in which order the characteristics will > be > > > discovered. A possible ordering is this: |from_peripheral_char| is > discovered > > > (i), the notify session is started (iii), |to_peripheral_char| is > discovered. > > In > > > this case, we have (ii) < (iii) < (i) < (iv) < (v). And we could as well > have > > > (i) < (ii) < (iii) < (iv) < (v). > > > > Thanks, I understand the flow a lot better now. Ideally, we should refactor > all > > the characteristic discovery code to another class, so we will only have two > > linear states to establish a ProximityAuth connection: > > WAITING_FOR_CHARACTERISTICS, and WAITING_FOR_INVITATION_RESPONSE. This > branching > > flow can be hard to understand when you're dealing with a bunch of reentrant > > functions and a bunch of booleans. > > > > I won't ask you to do that in this CL, but it maybe something to keep in mind > > when you write tests. It would be a lot easier to test the characteristic > > discovery by itself. > > Ok. BTW, I'll refactor this in a future CL. Probably after I land this and a CL with unit test for this class (that I'm currently working on). > > https://codereview.chromium.org/1116963002/diff/140001/components/proximity_a... > File components/proximity_auth/ble/bluetooth_low_energy_connection.h (right): > > https://codereview.chromium.org/1116963002/diff/140001/components/proximity_a... > components/proximity_auth/ble/bluetooth_low_energy_connection.h:36: // > Constructs a Bluetooth low energy connection to the service with > On 2015/05/12 03:02:47, Tim Song wrote: > > > > > > I think an diagram would be much more descriptive of the flow: > > > > Discover ReaderCharacteristic Discover WriterCharacteristic > > | | > > Start Notify Session | > > | | > > ---------------- && --------------------- > > | > > | > > | > > Write InviteToConnect to WriterCharacteristic > > | > > | > > | > > Read InvitationResponse from ReaderCharacteristic > > | > > | > > | > > Proximity Auth Connection Established > > Done. > > https://codereview.chromium.org/1116963002/diff/140001/components/proximity_a... > components/proximity_auth/ble/bluetooth_low_energy_connection.h:161: const > device::BluetoothUUID remote_service_uuid_; > On 2015/05/12 03:02:47, Tim Song wrote: > > You can put |remote_service_uuid_| and |remote_service_id_| in a > > RemoteCharacteristic struct as well, but you should probably change the name. > > Done.
https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection.cc (right): https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.cc:141: HandleCharacteristicUpdate(characteristic); On 2015/05/12 07:23:19, sacomoto wrote: > On 2015/05/12 03:02:46, Tim Song wrote: > > On 2015/05/11 12:04:04, sacomoto wrote: > > > On 2015/05/11 08:02:39, Tim Song wrote: > > > > Is it correct to call HandleCharacteristicUpdate() here? According to the > > > > comments, GattDiscoveryCompleteForService() will be always be called at > the > > > end, > > > > so you will always be calling HandleCharacteristicUpdate() twice for the > > > > service. > > > > > > This was intentional. The idea was to have the connection ready as soon as > > > possible. If the necessary characteristics were added in the very beginning, > > we > > > don't to wait for a call of GattDiscoveryCompleteForService() to have a > > complete > > > connection. > > > > > > Moreover, there is no problem calling HandleCharacteristicUpdate() twice. > The > > > second call will have (almost) no effect. The notify session is started only > > > once and only one invite to connect signal is sent. > > > > Thanks for the explanation. In this case, do you even need to override > > GattDiscoveryCompleteForService()? GattCharacteristicAdded() should be called > > for all characteristics you haven't handled in the constructor. > > Yes, we still need to override GattDiscoveryCompleteForService(). That is when > we discover that there is an error: all characteristics were discovered but the > two characteristics we need were not there. Can we at least get rid of the for loop iterating over all the characteristics in GattDiscoveryCompleteForService()? https://codereview.chromium.org/1116963002/diff/140001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_connection.h (right): https://codereview.chromium.org/1116963002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_connection.h:161: const device::BluetoothUUID remote_service_uuid_; On 2015/05/12 07:23:19, sacomoto wrote: > On 2015/05/12 03:02:47, Tim Song wrote: > > You can put |remote_service_uuid_| and |remote_service_id_| in a > > RemoteCharacteristic struct as well, but you should probably change the name. > > Done. I don't see this change.
Sorry, I messed up with my git branches here. I had done both modifications, but didn't upload them. It should be ok now. PTAL. On 2015/05/13 02:36:56, Tim Song wrote: > https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... > File components/proximity_auth/ble/bluetooth_low_energy_connection.cc (right): > > https://codereview.chromium.org/1116963002/diff/120001/components/proximity_a... > components/proximity_auth/ble/bluetooth_low_energy_connection.cc:141: > HandleCharacteristicUpdate(characteristic); > On 2015/05/12 07:23:19, sacomoto wrote: > > On 2015/05/12 03:02:46, Tim Song wrote: > > > On 2015/05/11 12:04:04, sacomoto wrote: > > > > On 2015/05/11 08:02:39, Tim Song wrote: > > > > > Is it correct to call HandleCharacteristicUpdate() here? According to > the > > > > > comments, GattDiscoveryCompleteForService() will be always be called at > > the > > > > end, > > > > > so you will always be calling HandleCharacteristicUpdate() twice for the > > > > > service. > > > > > > > > This was intentional. The idea was to have the connection ready as soon as > > > > possible. If the necessary characteristics were added in the very > beginning, > > > we > > > > don't to wait for a call of GattDiscoveryCompleteForService() to have a > > > complete > > > > connection. > > > > > > > > Moreover, there is no problem calling HandleCharacteristicUpdate() twice. > > The > > > > second call will have (almost) no effect. The notify session is started > only > > > > once and only one invite to connect signal is sent. > > > > > > Thanks for the explanation. In this case, do you even need to override > > > GattDiscoveryCompleteForService()? GattCharacteristicAdded() should be > called > > > for all characteristics you haven't handled in the constructor. > > > > Yes, we still need to override GattDiscoveryCompleteForService(). That is when > > we discover that there is an error: all characteristics were discovered but > the > > two characteristics we need were not there. > > Can we at least get rid of the for loop iterating over all the characteristics > in GattDiscoveryCompleteForService()? > > https://codereview.chromium.org/1116963002/diff/140001/components/proximity_a... > File components/proximity_auth/ble/bluetooth_low_energy_connection.h (right): > > https://codereview.chromium.org/1116963002/diff/140001/components/proximity_a... > components/proximity_auth/ble/bluetooth_low_energy_connection.h:161: const > device::BluetoothUUID remote_service_uuid_; > On 2015/05/12 07:23:19, sacomoto wrote: > > On 2015/05/12 03:02:47, Tim Song wrote: > > > You can put |remote_service_uuid_| and |remote_service_id_| in a > > > RemoteCharacteristic struct as well, but you should probably change the > name. > > > > Done. > > I don't see this change.
LGTM. Thank you, Gustavo!
The CQ bit was checked by sacomoto@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1116963002/180001
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...)
The CQ bit was checked by sacomoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tengs@chromium.org Link to the patchset: https://codereview.chromium.org/1116963002/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1116963002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/574c1637312ef98f8fa4bde909c1f040ddf33a4b Cr-Commit-Position: refs/heads/master@{#329837} |
