Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2075313002:
Substituting legacy protocol with uWeave protocol (Closed)
DescriptionUsing the uWeave packet generator and receiver to re-implement the BluetoothLowEnergyConnection class
Major changes:
SendMesssageImpl which instead of using the old protocol, it will just generate the message with the WeavePacketGenerator.
GattCharacteristicValueChanged instead of parsing messages received with the old protocol, will now use the WeavePacketReceiver and just switch on the state of the receiver and take appropriate action.
Design Doc for ProximityAuth uWeave Migration:
https://docs.google.com/a/google.com/document/d/1HJ1fipxsNQwIxmKs-5_6TjroCAcs6qRhAYvOnIr6mQw/edit?usp=sharing
BUG=617238
Committed: https://crrev.com/f4f0a5cde8eff464de5359281560444ff442f920
Cr-Commit-Position: refs/heads/master@{#407655}
Patch Set 1 #Patch Set 2 : the old connection class for better diffs #Patch Set 3 : Don't Review Yet, Not built yet, skeleton migration #Patch Set 4 : built version of the migrated connection #Patch Set 5 : tested and working version of the weave connection #
Total comments: 2
Patch Set 6 : renamed to weave client connection #
Total comments: 47
Patch Set 7 : mocks for unittest and sends connection close now #
Total comments: 44
Patch Set 8 : readible reason for close and removed dlog in test #
Total comments: 2
Patch Set 9 : added test for connection close #Patch Set 10 : saved reference to generator/receiver #
Total comments: 2
Patch Set 11 : addressed minor comments #Patch Set 12 : minor change #
Total comments: 8
Patch Set 13 : correct handling of connection close #
Total comments: 28
Patch Set 14 : fixed bug in onWriteRemoteCharacteristicError #Patch Set 15 : added comments for sending connection close, minor change to test #Patch Set 16 : added tests for connection close #
Total comments: 8
Patch Set 17 : actually sends message back #
Total comments: 31
Patch Set 18 : addressed stylistic comments #Patch Set 19 : addressed stylistic comments #Patch Set 20 : added test with OnDidSendMessage observer #
Total comments: 14
Patch Set 21 : fixed minor style problems #
Total comments: 2
Patch Set 22 : made setsubstatus private #Patch Set 23 : fixed onWriteRemoteCharacteristicError bug #Patch Set 24 : delete todo comment #Patch Set 25 : removed extra log messages #Patch Set 26 : updated gyp build files #
Total comments: 30
Patch Set 27 : addressed minor style comments #
Total comments: 2
Patch Set 28 : set status to disconnect when failed to create gatt connection #Patch Set 29 : better ways to pass information to write characteristic callback #Patch Set 30 : delete a space #Patch Set 31 : fix compile error on android #Patch Set 32 : fixed typo #Patch Set 33 : expect true/false instead of equality #Patch Set 34 : fix unittest memory leak #Messages
Total messages: 85 (18 generated)
Description was changed from ========== Substituting legacy protocol with uWeave protocol ========== to ========== Using the uWeave packet generator and receiver to re-implement the BluetoothLowEnergyConnection class Design Doc for ProximityAuth uWeave Migration: https://docs.google.com/a/google.com/document/d/1HJ1fipxsNQwIxmKs-5_6TjroCAcs... BUG=617238 ==========
jingxuy@google.com changed reviewers: + khorimoto@chromium.org, msarda@chromium.org, sacomoto@chromium.org, tengs@chromium.org
Please review the weave connection class.
Before I start reviewing, please change the class name to reflect that it is for the client role. https://codereview.chromium.org/2075313002/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_connection.h (right): https://codereview.chromium.org/2075313002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_connection.h:39: class BluetoothLowEnergyWeaveConnection Please add an in-depth description to this class.
Description was changed from ========== Using the uWeave packet generator and receiver to re-implement the BluetoothLowEnergyConnection class Design Doc for ProximityAuth uWeave Migration: https://docs.google.com/a/google.com/document/d/1HJ1fipxsNQwIxmKs-5_6TjroCAcs... BUG=617238 ========== to ========== Using the uWeave packet generator and receiver to re-implement the BluetoothLowEnergyConnection class Major changes: SendMesssageImpl which instead of using the old protocol, it will just generate the message with the WeavePacketGenerator. GattCharacteristicValueChanged instead of parsing messages received with the old protocol, will now use the WeavePacketReceiver and just switch on the state of the receiver and take appropriate action. Design Doc for ProximityAuth uWeave Migration: https://docs.google.com/a/google.com/document/d/1HJ1fipxsNQwIxmKs-5_6TjroCAcs... BUG=617238 ==========
https://codereview.chromium.org/2075313002/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_connection.h (right): https://codereview.chromium.org/2075313002/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_connection.h:39: class BluetoothLowEnergyWeaveConnection On 2016/06/24 00:14:09, Kyle Horimoto wrote: > Please add an in-depth description to this class. Done.
Haven't fully reviewed the test yet. You'll have to do a bit of work getting the mocks worked in correctly, so I'll wait until that work has been done before reviewing in-depth. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:33: // The UUID of the TX characteristic used to send data to the server. s/send/transmit/ https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:34: const char kTXCharacteristicUUID[] = "977c6674-1239-4e72-993b-502369b8bb5a"; 00000100-0004-1000-8000-001A11000101 https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:37: // The UUID of the RX characteristic used to receiver data from the server. s/receiver/receive/ https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:38: const char kRXCharacteristicUUID[] = "f4b904a2-a030-43b3-98a8-221c536c03cb"; 00000100-0004-1000-8000-001A11000102 https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:45: BluetoothLowEnergyWeaveClientConnection( Your constructor should take a generator and receiver as parameters. In your test, you can pass in a fake generator/receiver which each generate stub values which are easy to test against. If you do not do this, your tests for this class must implicitly know how the uWeave protocol works, meaning that you are essentially testing both this class and the generator/receiver classes. Providing mock classes isolates your unit test to testing only this class, providing greater test isolation and ensuring that a bug in one of the generator/receiver classes does not cause an incorrect test to pass or a correct test to fail. In your mocks, you should do something like the following: define several constant values to be used as messages. For example, the "connection request" packet could just be std::vector<uint8_t>(20, 'a'). It doesn't really matter what the contents of the packets are as long as they are different. Then you could create several fake "data messages" (name them message 1, message 2, etc). Then, for example, your CreateConnectionRequest() would just return the constant value, and your mock receiver would only change states if the mock value for a connection request was received. Likewise, for functions which we expect to be called, we would just set a value which can be observed later. So, for example, SetMaxPacketSize() could just store the packet sized passed as an instance variable for the generator class. Then, you could add a GetLastMaxPacketSizeSet() function to your mock class which could be called after the fact to ensure that the correct value was passed via EXPECT_EQ(expected_value, mock_generator->GetLastMaxPacketSizeSet()). https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:237: // TODO(jingxuy): send a control close over the channel Can you do this in this CL please? https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:241: packet_generator_->SetMaxPacketSize( Please add a comment about what the WAITING state means for the client and why this is done at this time. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:246: case ReceiverState::CONNECTING: Is CONNECTING a valid state? If we have already received a packet, shouldn't we have transitioned to another state? https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:50: class BluetoothLowEnergyWeaveClientConnection This class will also require a Factory for when we test the part of the code which creates this connection. Please add one here which you will be able to use when testing your next milestone. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:55: enum class ControlSignal : uint32_t { This class is not needed anymore. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:72: WAITING_RESPONSE_SIGNAL, It's not called a response signal in uWeave. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:78: // initaalized and ready. The GATT connection may alreaady be established and s/initaalized/initialized/ https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:128: // a kSendSignal + the size of the WireMessage, and the second containing a Doesn't use kSendSignal. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:131: WriteRequest(const std::vector<uint8_t>& val, bool flag); Use Packet. Same below, and same in the .cc, and same in the test. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:180: void SendInviteToConnectSignal(); Again, these "signal" values only exist in the legacy protocol. Please make sure to look around this implementation and remove everything that does not apply to uWeave. The test, in particular, has a ton of references to signal values.
https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:34: const char kTXCharacteristicUUID[] = "977c6674-1239-4e72-993b-502369b8bb5a"; On 2016/06/27 18:06:02, Kyle Horimoto wrote: > 00000100-0004-1000-8000-001A11000101 +1 https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:38: const char kRXCharacteristicUUID[] = "f4b904a2-a030-43b3-98a8-221c536c03cb"; On 2016/06/27 18:06:02, Kyle Horimoto wrote: > 00000100-0004-1000-8000-001A11000102 +1 https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:233: // TODO(jingxuy): what to do with the reason for close? Just log the error for now. This class doesn't pass any errors to its users in the case of a disconnect. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:237: // TODO(jingxuy): send a control close over the channel On 2016/06/27 18:06:02, Kyle Horimoto wrote: > Can you do this in this CL please? +1 You'll need to change the logic here. If try to send the error packet and then immediately call the |Disconnect()|, the error packet will never be sent. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:55: enum class ControlSignal : uint32_t { On 2016/06/27 18:06:02, Kyle Horimoto wrote: > This class is not needed anymore. +1 https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:63: // class nit: kill the empty line. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:72: WAITING_RESPONSE_SIGNAL, On 2016/06/27 18:06:03, Kyle Horimoto wrote: > It's not called a response signal in uWeave. +1 https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:154: const RemoteAttribute& to_peripheral_char, s/to_peripheral_char/tx_chararacteristic/. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:155: const RemoteAttribute& from_peripheral_char); s/from_peripheral_char/rx_characteristic/. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:160: const RemoteAttribute& from_peripheral_char); The same here.
https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:33: // The UUID of the TX characteristic used to send data to the server. On 2016/06/27 18:06:02, Kyle Horimoto wrote: > s/send/transmit/ Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:34: const char kTXCharacteristicUUID[] = "977c6674-1239-4e72-993b-502369b8bb5a"; On 2016/06/29 14:26:58, sacomoto wrote: > On 2016/06/27 18:06:02, Kyle Horimoto wrote: > > 00000100-0004-1000-8000-001A11000101 > > +1 Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:37: // The UUID of the RX characteristic used to receiver data from the server. On 2016/06/27 18:06:02, Kyle Horimoto wrote: > s/receiver/receive/ Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:38: const char kRXCharacteristicUUID[] = "f4b904a2-a030-43b3-98a8-221c536c03cb"; On 2016/06/29 14:26:57, sacomoto wrote: > On 2016/06/27 18:06:02, Kyle Horimoto wrote: > > 00000100-0004-1000-8000-001A11000102 > > +1 Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:45: BluetoothLowEnergyWeaveClientConnection( On 2016/06/27 18:06:02, Kyle Horimoto wrote: > Your constructor should take a generator and receiver as parameters. In your > test, you can pass in a fake generator/receiver which each generate stub values > which are easy to test against. > > If you do not do this, your tests for this class must implicitly know how the > uWeave protocol works, meaning that you are essentially testing both this class > and the generator/receiver classes. Providing mock classes isolates your unit > test to testing only this class, providing greater test isolation and ensuring > that a bug in one of the generator/receiver classes does not cause an incorrect > test to pass or a correct test to fail. > > In your mocks, you should do something like the following: define several > constant values to be used as messages. For example, the "connection request" > packet could just be std::vector<uint8_t>(20, 'a'). It doesn't really matter > what the contents of the packets are as long as they are different. Then you > could create several fake "data messages" (name them message 1, message 2, etc). > Then, for example, your CreateConnectionRequest() would just return the constant > value, and your mock receiver would only change states if the mock value for a > connection request was received. > > Likewise, for functions which we expect to be called, we would just set a value > which can be observed later. So, for example, SetMaxPacketSize() could just > store the packet sized passed as an instance variable for the generator class. > Then, you could add a GetLastMaxPacketSizeSet() function to your mock class > which could be called after the fact to ensure that the correct value was passed > via EXPECT_EQ(expected_value, mock_generator->GetLastMaxPacketSizeSet()). Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:233: // TODO(jingxuy): what to do with the reason for close? On 2016/06/29 14:26:57, sacomoto wrote: > Just log the error for now. > > This class doesn't pass any errors to its users in the case of a disconnect. Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:237: // TODO(jingxuy): send a control close over the channel On 2016/06/29 14:26:58, sacomoto wrote: > On 2016/06/27 18:06:02, Kyle Horimoto wrote: > > Can you do this in this CL please? > > +1 > > You'll need to change the logic here. If try to send the error packet and then > immediately call the |Disconnect()|, the error packet will never be sent. Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:241: packet_generator_->SetMaxPacketSize( On 2016/06/27 18:06:02, Kyle Horimoto wrote: > Please add a comment about what the WAITING state means for the client and why > this is done at this time. Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:246: case ReceiverState::CONNECTING: On 2016/06/27 18:06:02, Kyle Horimoto wrote: > Is CONNECTING a valid state? If we have already received a packet, shouldn't we > have transitioned to another state? Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:50: class BluetoothLowEnergyWeaveClientConnection On 2016/06/27 18:06:03, Kyle Horimoto wrote: > This class will also require a Factory for when we test the part of the code > which creates this connection. Please add one here which you will be able to use > when testing your next milestone. Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:55: enum class ControlSignal : uint32_t { On 2016/06/29 14:26:58, sacomoto wrote: > On 2016/06/27 18:06:02, Kyle Horimoto wrote: > > This class is not needed anymore. > > +1 Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:63: // class On 2016/06/29 14:26:58, sacomoto wrote: > nit: kill the empty line. Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:72: WAITING_RESPONSE_SIGNAL, On 2016/06/29 14:26:58, sacomoto wrote: > On 2016/06/27 18:06:03, Kyle Horimoto wrote: > > It's not called a response signal in uWeave. > > +1 Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:78: // initaalized and ready. The GATT connection may alreaady be established and On 2016/06/27 18:06:02, Kyle Horimoto wrote: > s/initaalized/initialized/ Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:128: // a kSendSignal + the size of the WireMessage, and the second containing a On 2016/06/27 18:06:02, Kyle Horimoto wrote: > Doesn't use kSendSignal. Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:131: WriteRequest(const std::vector<uint8_t>& val, bool flag); On 2016/06/27 18:06:03, Kyle Horimoto wrote: > Use Packet. Same below, and same in the .cc, and same in the test. Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:154: const RemoteAttribute& to_peripheral_char, On 2016/06/29 14:26:58, sacomoto wrote: > s/to_peripheral_char/tx_chararacteristic/. Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:155: const RemoteAttribute& from_peripheral_char); On 2016/06/29 14:26:58, sacomoto wrote: > s/from_peripheral_char/rx_characteristic/. Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:160: const RemoteAttribute& from_peripheral_char); On 2016/06/29 14:26:58, sacomoto wrote: > The same here. Done. https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:180: void SendInviteToConnectSignal(); On 2016/06/27 18:06:02, Kyle Horimoto wrote: > Again, these "signal" values only exist in the legacy protocol. Please make sure > to look around this implementation and remove everything that does not apply to > uWeave. The test, in particular, has a ton of references to signal values. Done.
https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:233: // TODO(jingxuy): what to do with the reason for close? On 2016/06/30 00:27:20, jingxuy wrote: > On 2016/06/29 14:26:57, sacomoto wrote: > > Just log the error for now. > > > > This class doesn't pass any errors to its users in the case of a disconnect. > > Done. Add a function which converts a ReasonForClose to a string so that the logs are readable. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:420: // TODO(jingxuy): I know this is single threaded, but can it actually still Please don't make two separate paths for this. This is unneeded. Just call WriteRemoteCharacteristic and send the queued messages in order. You'll need to handle the disconnect specially, so just add an extra "disconnect_after_sending" boolean to WriteRequest, then in OnRemoteCharacteristicWritten(), look at the message that was just written and check that value. If it is true, then Disconnect().
https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:233: // TODO(jingxuy): what to do with the reason for close? On 2016/06/30 00:42:33, Kyle Horimoto wrote: > On 2016/06/30 00:27:20, jingxuy wrote: > > On 2016/06/29 14:26:57, sacomoto wrote: > > > Just log the error for now. > > > > > > This class doesn't pass any errors to its users in the case of a disconnect. > > > > > Done. > > Add a function which converts a ReasonForClose to a string so that the logs are > readable. Done. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:420: // TODO(jingxuy): I know this is single threaded, but can it actually still On 2016/06/30 00:42:33, Kyle Horimoto wrote: > Please don't make two separate paths for this. This is unneeded. Just call > WriteRemoteCharacteristic and send the queued messages in order. > > You'll need to handle the disconnect specially, so just add an extra > "disconnect_after_sending" boolean to WriteRequest, then in > OnRemoteCharacteristicWritten(), look at the message that was just written and > check that value. If it is true, then Disconnect(). This is done because I am presuming that you want to close the connection immediately. As in that you shouldn't wait for all the writes in the queue to finish sending once you observe an error. Unless we do want to wait on queue, I'm not sure how much code reuse can be achieved through reusing writeRemoteCharacteristic
I'll review the tests more in-depth tomorrow. Going to finish up some other work tonight. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:247: if (sub_status() == SubStatus::WAITING_CONNECTION_RESPONSE) Should this be an assertion? Is there ever a valid time in which this would not be the case? https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:105: const std::vector<Packet> kSmallPackets{Packet{kDataHeader, 'b', 'b'}}; Create a constant for each Packet so that they can be referenced below. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:117: Packet CreateConnectionResponse() override { This function should never be called by the client. Just replace the implementation with NOTIMPLEMENTED(). https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:119: DLOG(ERROR) << "getting small"; Please remove these logs. Alternatively, make the log messages descriptive and log them to INFO instead of ERROR. Same elsewhere in this test. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:128: return Packet{static_cast<uint8_t>(reason_for_close)}; You should use a special header for this just as you did for the others. Otherwise, what if someone sets the "reason for close" to 0, 1, 2, or 3? Your message would be indistinguishable from the other message types you defined above. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:137: std::vector<Packet> EncodeDataMessage(std::string message) override { Please add some string constants which correspond to the Packets which are returned. Otherwise, |message| is ignored. For example, you could have a string "smallMessage" which results in |kSmallPackets| being returned; likewise with "largeMessage". https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:146: uint16_t max_packet_size_; Add a getter for this so that you can test that when a connection response is received with a certain data packet size, you can get it and assert that it is correct. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:162: return ReasonForClose::CLOSE_WITHOUT_ERROR; Don't just return a constant. Add a setter for this and the two getters below so that you can set them in your test. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:195: if (packet[1] == 'b') { Check the entire packet for equality instead of one character. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:214: private: Why is this private? Same below. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:328: BluetoothLowEnergyWeavePacketGenerator::Factory::SetInstanceForTesting( These should be moved to SetUp(). https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:419: Assert that a connection request was sent. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:442: Assert that the correct data packet size was set on the generator.
https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:420: // TODO(jingxuy): I know this is single threaded, but can it actually still On 2016/06/30 01:14:55, jingxuy wrote: > On 2016/06/30 00:42:33, Kyle Horimoto wrote: > > Please don't make two separate paths for this. This is unneeded. Just call > > WriteRemoteCharacteristic and send the queued messages in order. > > > > You'll need to handle the disconnect specially, so just add an extra > > "disconnect_after_sending" boolean to WriteRequest, then in > > OnRemoteCharacteristicWritten(), look at the message that was just written and > > check that value. If it is true, then Disconnect(). > > This is done because I am presuming that you want to close the connection > immediately. As in that you shouldn't wait for all the writes in the queue to > finish sending once you observe an error. Unless we do want to wait on queue, > I'm not sure how much code reuse can be achieved through reusing > writeRemoteCharacteristic All writes that have been queued should still be written. Otherwise, the remote device may not receive all of the data it should have. Any writes that are queued *after* the close connection is queued should not be sent, but any writes that are queued *before* it is queued should be sent. For example, consider this case where a client wants to open a connection, send a large, 100-packet message, then close the connection: 1) Client sends connection request. 2) Server responds with connection response. 3) Client starts sending the large message. The 100 packets are queued, but only 5 have been sent so far. 4) Client enqueues the connection close packet to be sent after all 100 packets are sent. In your solution, only 5 packets would be sent, then the close connection packet would be sent. The server would receive this data and only have 5% of it, so it wouldn't understand what was actually sent. Likewise, maybe some other app is utilizing the Bluetooth hardware, so maybe even the first packet hasn't been sent yet before the connection close packet is queued. There are many other possibilities in which your implementation would cause errors.
https://codereview.chromium.org/2075313002/diff/140001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:222: if (characteristic->GetIdentifier() == rx_characteristic_.id) { It looks like none of this new logic you added is actually tested. Please add tests which receive the various message types and assert that the state machine you've provided here behaves as expected.
https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:247: if (sub_status() == SubStatus::WAITING_CONNECTION_RESPONSE) On 2016/06/30 01:29:48, Kyle Horimoto wrote: > Should this be an assertion? Is there ever a valid time in which this would not > be the case? Done. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:420: // TODO(jingxuy): I know this is single threaded, but can it actually still On 2016/06/30 02:01:25, Kyle Horimoto wrote: > On 2016/06/30 01:14:55, jingxuy wrote: > > On 2016/06/30 00:42:33, Kyle Horimoto wrote: > > > Please don't make two separate paths for this. This is unneeded. Just call > > > WriteRemoteCharacteristic and send the queued messages in order. > > > > > > You'll need to handle the disconnect specially, so just add an extra > > > "disconnect_after_sending" boolean to WriteRequest, then in > > > OnRemoteCharacteristicWritten(), look at the message that was just written > and > > > check that value. If it is true, then Disconnect(). > > > > This is done because I am presuming that you want to close the connection > > immediately. As in that you shouldn't wait for all the writes in the queue to > > finish sending once you observe an error. Unless we do want to wait on queue, > > I'm not sure how much code reuse can be achieved through reusing > > writeRemoteCharacteristic > > All writes that have been queued should still be written. Otherwise, the remote > device may not receive all of the data it should have. Any writes that are > queued *after* the close connection is queued should not be sent, but any writes > that are queued *before* it is queued should be sent. > > For example, consider this case where a client wants to open a connection, send > a large, 100-packet message, then close the connection: > > 1) Client sends connection request. > 2) Server responds with connection response. > 3) Client starts sending the large message. The 100 packets are queued, but only > 5 have been sent so far. > 4) Client enqueues the connection close packet to be sent after all 100 packets > are sent. > > In your solution, only 5 packets would be sent, then the close connection packet > would be sent. The server would receive this data and only have 5% of it, so it > wouldn't understand what was actually sent. > > Likewise, maybe some other app is utilizing the Bluetooth hardware, so maybe > even the first packet hasn't been sent yet before the connection close packet is > queued. There are many other possibilities in which your implementation would > cause errors. I think the misunderstanding is that this connection close is used to kill a connection in a non-friendly manner. This is not we are done with the connection, so no more is needed. This is when the client receiver jumped into error state, which means the server is probably in some weird and non-functional state as well. It does bring up the point that currently there is no way of ending the connection in a friendly manner. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:105: const std::vector<Packet> kSmallPackets{Packet{kDataHeader, 'b', 'b'}}; On 2016/06/30 01:29:48, Kyle Horimoto wrote: > Create a constant for each Packet so that they can be referenced below. Done. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:117: Packet CreateConnectionResponse() override { On 2016/06/30 01:29:48, Kyle Horimoto wrote: > This function should never be called by the client. Just replace the > implementation with NOTIMPLEMENTED(). Done. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:119: DLOG(ERROR) << "getting small"; On 2016/06/30 01:29:48, Kyle Horimoto wrote: > Please remove these logs. Alternatively, make the log messages descriptive and > log them to INFO instead of ERROR. > > Same elsewhere in this test. It's already done in the most recent version. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:128: return Packet{static_cast<uint8_t>(reason_for_close)}; On 2016/06/30 01:29:48, Kyle Horimoto wrote: > You should use a special header for this just as you did for the others. > Otherwise, what if someone sets the "reason for close" to 0, 1, 2, or 3? Your > message would be indistinguishable from the other message types you defined > above. Done. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:137: std::vector<Packet> EncodeDataMessage(std::string message) override { On 2016/06/30 01:29:48, Kyle Horimoto wrote: > Please add some string constants which correspond to the Packets which are > returned. Otherwise, |message| is ignored. > > For example, you could have a string "smallMessage" which results in > |kSmallPackets| being returned; likewise with "largeMessage". Done. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:146: uint16_t max_packet_size_; On 2016/06/30 01:29:48, Kyle Horimoto wrote: > Add a getter for this so that you can test that when a connection response is > received with a certain data packet size, you can get it and assert that it is > correct. I replied in a comment below. A getter is not going to work. This can be a static variable instead. But I think this is implicitly tested by small/large test. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:195: if (packet[1] == 'b') { On 2016/06/30 01:29:48, Kyle Horimoto wrote: > Check the entire packet for equality instead of one character. Done. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:214: private: On 2016/06/30 01:29:48, Kyle Horimoto wrote: > Why is this private? Same below. it only need to be called from the factory class. It was protected initially so that it can be inherited. now there is no point of inhereiting https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:328: BluetoothLowEnergyWeavePacketGenerator::Factory::SetInstanceForTesting( On 2016/06/30 01:29:48, Kyle Horimoto wrote: > These should be moved to SetUp(). I moved them to the constructor since they only needed to be called once. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:419: On 2016/06/30 01:29:48, Kyle Horimoto wrote: > Assert that a connection request was sent. what do you mean? the expect_call on tx_characteristic_ already does that https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:442: On 2016/06/30 01:29:48, Kyle Horimoto wrote: > Assert that the correct data packet size was set on the generator. the test class has no access to generator reference as of now. If you wanted to test this explicitly, there had to be a static variable in the generator that's publicly accessible. But I think this is already tested below as the small/large packet size will return different messages. So if the packet size is set incorrectly, it will fail some tests below. https://codereview.chromium.org/2075313002/diff/140001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:222: if (characteristic->GetIdentifier() == rx_characteristic_.id) { On 2016/06/30 18:00:23, Kyle Horimoto wrote: > It looks like none of this new logic you added is actually tested. Please add > tests which receive the various message types and assert that the state machine > you've provided here behaves as expected. Since certain functions are only called in certain state, the receive small/large packet test most of the important states. The only ones missing is connection closed and error_detected. I can write the connection close real quick. I think error_detected is still pending discussion
https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:420: // TODO(jingxuy): I know this is single threaded, but can it actually still On 2016/06/30 21:59:25, jingxuy wrote: > On 2016/06/30 02:01:25, Kyle Horimoto wrote: > > On 2016/06/30 01:14:55, jingxuy wrote: > > > On 2016/06/30 00:42:33, Kyle Horimoto wrote: > > > > Please don't make two separate paths for this. This is unneeded. Just call > > > > WriteRemoteCharacteristic and send the queued messages in order. > > > > > > > > You'll need to handle the disconnect specially, so just add an extra > > > > "disconnect_after_sending" boolean to WriteRequest, then in > > > > OnRemoteCharacteristicWritten(), look at the message that was just written > > and > > > > check that value. If it is true, then Disconnect(). > > > > > > This is done because I am presuming that you want to close the connection > > > immediately. As in that you shouldn't wait for all the writes in the queue > to > > > finish sending once you observe an error. Unless we do want to wait on > queue, > > > I'm not sure how much code reuse can be achieved through reusing > > > writeRemoteCharacteristic > > > > All writes that have been queued should still be written. Otherwise, the > remote > > device may not receive all of the data it should have. Any writes that are > > queued *after* the close connection is queued should not be sent, but any > writes > > that are queued *before* it is queued should be sent. > > > > For example, consider this case where a client wants to open a connection, > send > > a large, 100-packet message, then close the connection: > > > > 1) Client sends connection request. > > 2) Server responds with connection response. > > 3) Client starts sending the large message. The 100 packets are queued, but > only > > 5 have been sent so far. > > 4) Client enqueues the connection close packet to be sent after all 100 > packets > > are sent. > > > > In your solution, only 5 packets would be sent, then the close connection > packet > > would be sent. The server would receive this data and only have 5% of it, so > it > > wouldn't understand what was actually sent. > > > > Likewise, maybe some other app is utilizing the Bluetooth hardware, so maybe > > even the first packet hasn't been sent yet before the connection close packet > is > > queued. There are many other possibilities in which your implementation would > > cause errors. > > I think the misunderstanding is that this connection close is used to kill a > connection in a non-friendly manner. This is not we are done with the > connection, so no more is needed. This is when the client receiver jumped into > error state, which means the server is probably in some weird and non-functional > state as well. It does bring up the point that currently there is no way of > ending the connection in a friendly manner. The connection close packet can also used to close the connection if no error has occurred. There is even a "reason for close" value called "close without error". IMO, when Disconnect() is called, we should send a connection close packet assuming that the following are true: 1) We are the ones initiating the connection being closed (i.e., we have not received a connection close packet from the remote device). 2) The Bluetooth connection is still valid (i.e., we are able to send things still). 3) We haven't already sent a connection close packet. Then, we would do the rest of the disconnection cleanup. Tim/Gustavo/Mihai, what do you think? In any case, we should still try to send the rest of the packets that were already queued for the reasons I said earlier. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:146: uint16_t max_packet_size_; On 2016/06/30 21:59:26, jingxuy wrote: > On 2016/06/30 01:29:48, Kyle Horimoto wrote: > > Add a getter for this so that you can test that when a connection response is > > received with a certain data packet size, you can get it and assert that it is > > correct. > > I replied in a comment below. A getter is not going to work. This can be a > static variable instead. But I think this is implicitly tested by small/large > test. Please test this explicitly. You should create a new mock generator/receiver in each test and share the reference between the test and the implementation so that the implementation makes calls on the generator/receiver and the test can make assertions on them. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:328: BluetoothLowEnergyWeavePacketGenerator::Factory::SetInstanceForTesting( On 2016/06/30 21:59:26, jingxuy wrote: > On 2016/06/30 01:29:48, Kyle Horimoto wrote: > > These should be moved to SetUp(). > > I moved them to the constructor since they only needed to be called once. You should create a new instance of the generator/receiver for each test so that you can call getters on these instances in order to assert behavior. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:419: On 2016/06/30 21:59:26, jingxuy wrote: > On 2016/06/30 01:29:48, Kyle Horimoto wrote: > > Assert that a connection request was sent. > > what do you mean? the expect_call on tx_characteristic_ already does that Where? It looks like it only asserts that WriteRemoteCharacteristic() was called, not that a connection request was sent. Just move this line from ConnectionResponseReceived() here instead: EXPECT_EQ(last_value_written_on_tx_characteristic_, kConnectionRequest); https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:442: On 2016/06/30 21:59:25, jingxuy wrote: > On 2016/06/30 01:29:48, Kyle Horimoto wrote: > > Assert that the correct data packet size was set on the generator. > > the test class has no access to generator reference as of now. If you wanted to > test this explicitly, there had to be a static variable in the generator that's > publicly accessible. But I think this is already tested below as the > small/large packet size will return different messages. So if the packet size is > set incorrectly, it will fail some tests below. Please test this explicitly. You should create an instance of the mock generator/receiver classes and return these instances from the factories. That way, the class can make calls on the generator/receiver and the test can make calls to retrieve values for assertions from the generator/receiver.
https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:420: // TODO(jingxuy): I know this is single threaded, but can it actually still On 2016/06/30 22:36:11, Kyle Horimoto wrote: > On 2016/06/30 21:59:25, jingxuy wrote: > > On 2016/06/30 02:01:25, Kyle Horimoto wrote: > > > On 2016/06/30 01:14:55, jingxuy wrote: > > > > On 2016/06/30 00:42:33, Kyle Horimoto wrote: > > > > > Please don't make two separate paths for this. This is unneeded. Just > call > > > > > WriteRemoteCharacteristic and send the queued messages in order. > > > > > > > > > > You'll need to handle the disconnect specially, so just add an extra > > > > > "disconnect_after_sending" boolean to WriteRequest, then in > > > > > OnRemoteCharacteristicWritten(), look at the message that was just > written > > > and > > > > > check that value. If it is true, then Disconnect(). > > > > > > > > This is done because I am presuming that you want to close the connection > > > > immediately. As in that you shouldn't wait for all the writes in the queue > > to > > > > finish sending once you observe an error. Unless we do want to wait on > > queue, > > > > I'm not sure how much code reuse can be achieved through reusing > > > > writeRemoteCharacteristic > > > > > > All writes that have been queued should still be written. Otherwise, the > > remote > > > device may not receive all of the data it should have. Any writes that are > > > queued *after* the close connection is queued should not be sent, but any > > writes > > > that are queued *before* it is queued should be sent. > > > > > > For example, consider this case where a client wants to open a connection, > > send > > > a large, 100-packet message, then close the connection: > > > > > > 1) Client sends connection request. > > > 2) Server responds with connection response. > > > 3) Client starts sending the large message. The 100 packets are queued, but > > only > > > 5 have been sent so far. > > > 4) Client enqueues the connection close packet to be sent after all 100 > > packets > > > are sent. > > > > > > In your solution, only 5 packets would be sent, then the close connection > > packet > > > would be sent. The server would receive this data and only have 5% of it, so > > it > > > wouldn't understand what was actually sent. > > > > > > Likewise, maybe some other app is utilizing the Bluetooth hardware, so maybe > > > even the first packet hasn't been sent yet before the connection close > packet > > is > > > queued. There are many other possibilities in which your implementation > would > > > cause errors. > > > > I think the misunderstanding is that this connection close is used to kill a > > connection in a non-friendly manner. This is not we are done with the > > connection, so no more is needed. This is when the client receiver jumped into > > error state, which means the server is probably in some weird and > non-functional > > state as well. It does bring up the point that currently there is no way of > > ending the connection in a friendly manner. > > The connection close packet can also used to close the connection if no error > has occurred. There is even a "reason for close" value called "close without > error". IMO, when Disconnect() is called, we should send a connection close > packet assuming that the following are true: > > 1) We are the ones initiating the connection being closed (i.e., we have not > received a connection close packet from the remote device). > 2) The Bluetooth connection is still valid (i.e., we are able to send things > still). > 3) We haven't already sent a connection close packet. > > Then, we would do the rest of the disconnection cleanup. > > Tim/Gustavo/Mihai, what do you think? > > In any case, we should still try to send the rest of the packets that were > already queued for the reasons I said earlier. Yes, I think queuing is the proper way to send a normal connection close without error. It is actually yet not implemented as I mentioned in the last comment, will be a TODO. (Currently it seems like Disconnect() is the only way to stop a connection). But this writeConnectionClose is actually only called on error. So I might want to change name later so there's no confusion. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:146: uint16_t max_packet_size_; On 2016/06/30 22:36:11, Kyle Horimoto wrote: > On 2016/06/30 21:59:26, jingxuy wrote: > > On 2016/06/30 01:29:48, Kyle Horimoto wrote: > > > Add a getter for this so that you can test that when a connection response > is > > > received with a certain data packet size, you can get it and assert that it > is > > > correct. > > > > I replied in a comment below. A getter is not going to work. This can be a > > static variable instead. But I think this is implicitly tested by small/large > > test. > > Please test this explicitly. You should create a new mock generator/receiver in > each test and share the reference between the test and the implementation so > that the implementation makes calls on the generator/receiver and the test can > make assertions on them. Done. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:162: return ReasonForClose::CLOSE_WITHOUT_ERROR; On 2016/06/30 01:29:48, Kyle Horimoto wrote: > Don't just return a constant. Add a setter for this and the two getters below so > that you can set them in your test. Done. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:328: BluetoothLowEnergyWeavePacketGenerator::Factory::SetInstanceForTesting( On 2016/06/30 22:36:11, Kyle Horimoto wrote: > On 2016/06/30 21:59:26, jingxuy wrote: > > On 2016/06/30 01:29:48, Kyle Horimoto wrote: > > > These should be moved to SetUp(). > > > > I moved them to the constructor since they only needed to be called once. > > You should create a new instance of the generator/receiver for each test so that > you can call getters on these instances in order to assert behavior. instance of generator/receiver is still unique to each test. This is merely just injecting a factory that is only needed once. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:419: On 2016/06/30 22:36:11, Kyle Horimoto wrote: > On 2016/06/30 21:59:26, jingxuy wrote: > > On 2016/06/30 01:29:48, Kyle Horimoto wrote: > > > Assert that a connection request was sent. > > > > what do you mean? the expect_call on tx_characteristic_ already does that > > Where? It looks like it only asserts that WriteRemoteCharacteristic() was > called, not that a connection request was sent. Just move this line from > ConnectionResponseReceived() here instead: > > EXPECT_EQ(last_value_written_on_tx_characteristic_, kConnectionRequest); It's kind of weird because this function merely stores last_value_written_on_tx_characteristic_ but it's other functions's responsibility to test the value of it. If a connection request is sent, this value is tested in WriteRemoteCharacteristic(). But it's not the only instance, it can also be called before a disconnect. I didn't dig too deep into other tests but it seems like the current implementation is fine. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:442: On 2016/06/30 22:36:11, Kyle Horimoto wrote: > On 2016/06/30 21:59:25, jingxuy wrote: > > On 2016/06/30 01:29:48, Kyle Horimoto wrote: > > > Assert that the correct data packet size was set on the generator. > > > > the test class has no access to generator reference as of now. If you wanted > to > > test this explicitly, there had to be a static variable in the generator > that's > > publicly accessible. But I think this is already tested below as the > > small/large packet size will return different messages. So if the packet size > is > > set incorrectly, it will fail some tests below. > > Please test this explicitly. You should create an instance of the mock > generator/receiver classes and return these instances from the factories. That > way, the class can make calls on the generator/receiver and the test can make > calls to retrieve values for assertions from the generator/receiver. Done.
Please ping other reviewers so that we can figure out when the close connection packet should be sent. https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:419: On 2016/07/01 00:00:44, jingxuy wrote: > On 2016/06/30 22:36:11, Kyle Horimoto wrote: > > On 2016/06/30 21:59:26, jingxuy wrote: > > > On 2016/06/30 01:29:48, Kyle Horimoto wrote: > > > > Assert that a connection request was sent. > > > > > > what do you mean? the expect_call on tx_characteristic_ already does that > > > > Where? It looks like it only asserts that WriteRemoteCharacteristic() was > > called, not that a connection request was sent. Just move this line from > > ConnectionResponseReceived() here instead: > > > > EXPECT_EQ(last_value_written_on_tx_characteristic_, kConnectionRequest); > > It's kind of weird because this function merely stores > last_value_written_on_tx_characteristic_ but it's other functions's > responsibility to test the value of it. If a connection request is sent, this > value is tested in WriteRemoteCharacteristic(). But it's not the only instance, > it can also be called before a disconnect. I didn't dig too deep into other > tests but it seems like the current implementation is fine. Please test this explicitly here. Some tests call NotifySessionStarted() without calling ConnectionResponseReceived(), so we should not rely on this happening to test this. It is a testing best practice to explicitly test interactions between the class under test and the other objects which have been injected instead of relying on implicit interactions. https://codereview.chromium.org/2075313002/diff/180001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:221: return most_recent_instance_; Hmm, most_recent_instance_ will be deleted once the Connection class' destructor has run. Are we guaranteeing that we will never touch this instance after the Connection is deleted? If so, this approach won't work. If not, then please write a comment here and below making it very clear that this is the case.
https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:419: On 2016/07/01 17:23:21, Kyle Horimoto wrote: > On 2016/07/01 00:00:44, jingxuy wrote: > > On 2016/06/30 22:36:11, Kyle Horimoto wrote: > > > On 2016/06/30 21:59:26, jingxuy wrote: > > > > On 2016/06/30 01:29:48, Kyle Horimoto wrote: > > > > > Assert that a connection request was sent. > > > > > > > > what do you mean? the expect_call on tx_characteristic_ already does that > > > > > > Where? It looks like it only asserts that WriteRemoteCharacteristic() was > > > called, not that a connection request was sent. Just move this line from > > > ConnectionResponseReceived() here instead: > > > > > > EXPECT_EQ(last_value_written_on_tx_characteristic_, kConnectionRequest); > > > > It's kind of weird because this function merely stores > > last_value_written_on_tx_characteristic_ but it's other functions's > > responsibility to test the value of it. If a connection request is sent, this > > value is tested in WriteRemoteCharacteristic(). But it's not the only > instance, > > it can also be called before a disconnect. I didn't dig too deep into other > > tests but it seems like the current implementation is fine. > > Please test this explicitly here. Some tests call NotifySessionStarted() without > calling ConnectionResponseReceived(), so we should not rely on this happening to > test this. It is a testing best practice to explicitly test interactions between > the class under test and the other objects which have been injected instead of > relying on implicit interactions. Done. https://codereview.chromium.org/2075313002/diff/180001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:221: return most_recent_instance_; On 2016/07/01 17:23:21, Kyle Horimoto wrote: > Hmm, most_recent_instance_ will be deleted once the Connection class' destructor > has run. Are we guaranteeing that we will never touch this instance after the > Connection is deleted? If so, this approach won't work. If not, then please > write a comment here and below making it very clear that this is the case. yes we are guaranteed
https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:420: // TODO(jingxuy): I know this is single threaded, but can it actually still On 2016/07/01 00:00:44, jingxuy wrote: > On 2016/06/30 22:36:11, Kyle Horimoto wrote: > > On 2016/06/30 21:59:25, jingxuy wrote: > > > On 2016/06/30 02:01:25, Kyle Horimoto wrote: > > > > On 2016/06/30 01:14:55, jingxuy wrote: > > > > > On 2016/06/30 00:42:33, Kyle Horimoto wrote: > > > > > > Please don't make two separate paths for this. This is unneeded. Just > > call > > > > > > WriteRemoteCharacteristic and send the queued messages in order. > > > > > > > > > > > > You'll need to handle the disconnect specially, so just add an extra > > > > > > "disconnect_after_sending" boolean to WriteRequest, then in > > > > > > OnRemoteCharacteristicWritten(), look at the message that was just > > written > > > > and > > > > > > check that value. If it is true, then Disconnect(). > > > > > > > > > > This is done because I am presuming that you want to close the > connection > > > > > immediately. As in that you shouldn't wait for all the writes in the > queue > > > to > > > > > finish sending once you observe an error. Unless we do want to wait on > > > queue, > > > > > I'm not sure how much code reuse can be achieved through reusing > > > > > writeRemoteCharacteristic > > > > > > > > All writes that have been queued should still be written. Otherwise, the > > > remote > > > > device may not receive all of the data it should have. Any writes that are > > > > queued *after* the close connection is queued should not be sent, but any > > > writes > > > > that are queued *before* it is queued should be sent. > > > > > > > > For example, consider this case where a client wants to open a connection, > > > send > > > > a large, 100-packet message, then close the connection: > > > > > > > > 1) Client sends connection request. > > > > 2) Server responds with connection response. > > > > 3) Client starts sending the large message. The 100 packets are queued, > but > > > only > > > > 5 have been sent so far. > > > > 4) Client enqueues the connection close packet to be sent after all 100 > > > packets > > > > are sent. > > > > > > > > In your solution, only 5 packets would be sent, then the close connection > > > packet > > > > would be sent. The server would receive this data and only have 5% of it, > so > > > it > > > > wouldn't understand what was actually sent. > > > > > > > > Likewise, maybe some other app is utilizing the Bluetooth hardware, so > maybe > > > > even the first packet hasn't been sent yet before the connection close > > packet > > > is > > > > queued. There are many other possibilities in which your implementation > > would > > > > cause errors. > > > > > > I think the misunderstanding is that this connection close is used to kill a > > > connection in a non-friendly manner. This is not we are done with the > > > connection, so no more is needed. This is when the client receiver jumped > into > > > error state, which means the server is probably in some weird and > > non-functional > > > state as well. It does bring up the point that currently there is no way of > > > ending the connection in a friendly manner. > > > > The connection close packet can also used to close the connection if no error > > has occurred. There is even a "reason for close" value called "close without > > error". IMO, when Disconnect() is called, we should send a connection close > > packet assuming that the following are true: > > > > 1) We are the ones initiating the connection being closed (i.e., we have not > > received a connection close packet from the remote device). > > 2) The Bluetooth connection is still valid (i.e., we are able to send things > > still). > > 3) We haven't already sent a connection close packet. > > > > Then, we would do the rest of the disconnection cleanup. > > > > Tim/Gustavo/Mihai, what do you think? > > > > In any case, we should still try to send the rest of the packets that were > > already queued for the reasons I said earlier. > > Yes, I think queuing is the proper way to send a normal connection close without > error. It is actually yet not implemented as I mentioned in the last comment, > will be a TODO. (Currently it seems like Disconnect() is the only way to stop a > connection). But this writeConnectionClose is actually only called on error. So > I might want to change name later so there's no confusion. This strategy is correct. The close connection packet should be sent (and queued like any other packet) when this class initiates a clean disconnect (no error). In the case of a disconnect after an error (e.g. a packet with the wrong packet counter was received) the close connect packet should be sent right away (no queueing). You should probably implement an InternalDisconnect() method, that does exactly what the public Disconnect() does now. And change the implementation of Disconnect() to send a connection close packet and then call the InternalDisconnect(). https://codereview.chromium.org/2075313002/diff/220001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:237: Disconnect(); The close connection packet won't be sent if you call Disconnect() here. https://codereview.chromium.org/2075313002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:420: // have arbitrary overlay? If so, then this queue clear can race with What do you mean by arbitrary overlay? https://codereview.chromium.org/2075313002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:423: std::swap(write_requests_queue_, empty); I think there is an issue here. The OnRemoteCharacteristicWritten() method assumes that the queue is non-empty. What would happen in this case: 1) You clear |write_requests_queue_| here and there is a pending write request; 2) OnRemoteCharacteristicWritten() gets called. https://codereview.chromium.org/2075313002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:426: while (write_remote_characteristic_pending_) { I don't think this works.
https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:420: // TODO(jingxuy): I know this is single threaded, but can it actually still On 2016/07/06 13:07:07, sacomoto wrote: > On 2016/07/01 00:00:44, jingxuy wrote: > > On 2016/06/30 22:36:11, Kyle Horimoto wrote: > > > On 2016/06/30 21:59:25, jingxuy wrote: > > > > On 2016/06/30 02:01:25, Kyle Horimoto wrote: > > > > > On 2016/06/30 01:14:55, jingxuy wrote: > > > > > > On 2016/06/30 00:42:33, Kyle Horimoto wrote: > > > > > > > Please don't make two separate paths for this. This is unneeded. > Just > > > call > > > > > > > WriteRemoteCharacteristic and send the queued messages in order. > > > > > > > > > > > > > > You'll need to handle the disconnect specially, so just add an extra > > > > > > > "disconnect_after_sending" boolean to WriteRequest, then in > > > > > > > OnRemoteCharacteristicWritten(), look at the message that was just > > > written > > > > > and > > > > > > > check that value. If it is true, then Disconnect(). > > > > > > > > > > > > This is done because I am presuming that you want to close the > > connection > > > > > > immediately. As in that you shouldn't wait for all the writes in the > > queue > > > > to > > > > > > finish sending once you observe an error. Unless we do want to wait on > > > > queue, > > > > > > I'm not sure how much code reuse can be achieved through reusing > > > > > > writeRemoteCharacteristic > > > > > > > > > > All writes that have been queued should still be written. Otherwise, the > > > > remote > > > > > device may not receive all of the data it should have. Any writes that > are > > > > > queued *after* the close connection is queued should not be sent, but > any > > > > writes > > > > > that are queued *before* it is queued should be sent. > > > > > > > > > > For example, consider this case where a client wants to open a > connection, > > > > send > > > > > a large, 100-packet message, then close the connection: > > > > > > > > > > 1) Client sends connection request. > > > > > 2) Server responds with connection response. > > > > > 3) Client starts sending the large message. The 100 packets are queued, > > but > > > > only > > > > > 5 have been sent so far. > > > > > 4) Client enqueues the connection close packet to be sent after all 100 > > > > packets > > > > > are sent. > > > > > > > > > > In your solution, only 5 packets would be sent, then the close > connection > > > > packet > > > > > would be sent. The server would receive this data and only have 5% of > it, > > so > > > > it > > > > > wouldn't understand what was actually sent. > > > > > > > > > > Likewise, maybe some other app is utilizing the Bluetooth hardware, so > > maybe > > > > > even the first packet hasn't been sent yet before the connection close > > > packet > > > > is > > > > > queued. There are many other possibilities in which your implementation > > > would > > > > > cause errors. > > > > > > > > I think the misunderstanding is that this connection close is used to kill > a > > > > connection in a non-friendly manner. This is not we are done with the > > > > connection, so no more is needed. This is when the client receiver jumped > > into > > > > error state, which means the server is probably in some weird and > > > non-functional > > > > state as well. It does bring up the point that currently there is no way > of > > > > ending the connection in a friendly manner. > > > > > > The connection close packet can also used to close the connection if no > error > > > has occurred. There is even a "reason for close" value called "close without > > > error". IMO, when Disconnect() is called, we should send a connection close > > > packet assuming that the following are true: > > > > > > 1) We are the ones initiating the connection being closed (i.e., we have not > > > received a connection close packet from the remote device). > > > 2) The Bluetooth connection is still valid (i.e., we are able to send things > > > still). > > > 3) We haven't already sent a connection close packet. > > > > > > Then, we would do the rest of the disconnection cleanup. > > > > > > Tim/Gustavo/Mihai, what do you think? > > > > > > In any case, we should still try to send the rest of the packets that were > > > already queued for the reasons I said earlier. > > > > Yes, I think queuing is the proper way to send a normal connection close > without > > error. It is actually yet not implemented as I mentioned in the last comment, > > will be a TODO. (Currently it seems like Disconnect() is the only way to stop > a > > connection). But this writeConnectionClose is actually only called on error. > So > > I might want to change name later so there's no confusion. > > This strategy is correct. The close connection packet should be sent (and queued > like any other packet) when this class initiates a clean disconnect (no error). > > In the case of a disconnect after an error (e.g. a packet with the wrong packet > counter was received) the close connect packet should be sent right away (no > queueing). > > You should probably implement an InternalDisconnect() method, that does exactly > what the public Disconnect() does now. And change the implementation of > Disconnect() to send a connection close packet and then call the > InternalDisconnect(). Done. https://codereview.chromium.org/2075313002/diff/220001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:237: Disconnect(); On 2016/07/06 13:07:08, sacomoto wrote: > The close connection packet won't be sent if you call Disconnect() here. Done. https://codereview.chromium.org/2075313002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:420: // have arbitrary overlay? If so, then this queue clear can race with On 2016/07/06 13:07:08, sacomoto wrote: > What do you mean by arbitrary overlay? Done. https://codereview.chromium.org/2075313002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:423: std::swap(write_requests_queue_, empty); On 2016/07/06 13:07:08, sacomoto wrote: > I think there is an issue here. The OnRemoteCharacteristicWritten() method > assumes that the queue is non-empty. What would happen in this case: > > 1) You clear |write_requests_queue_| here and there is a pending write request; > 2) OnRemoteCharacteristicWritten() gets called. Done. https://codereview.chromium.org/2075313002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:426: while (write_remote_characteristic_pending_) { On 2016/07/06 13:07:08, sacomoto wrote: > I don't think this works. Done.
Please add a unit test for these new cases. These are a few of the one off the top of my head, but there should be more: *Call Disconnect(), and assert that the close packet was sent and that the Connection is shut down properly. *Receive an erroneous packet in the middle of sending, and assert that the rest of the packets are discarded and that the control packet is sent instead. https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:127: if (sub_status_ == SubStatus::CONNECTED) { Should this occur under other circumstances as well? For example, what if we have already established a connection and have sent a connection request. Should we wait until a connection response is received before sending? https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:499: break; You should return early here, or else you would continue processing more write requests. Please add a test for this case. https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:519: PA_LOG(ERROR) << "Received erroneous packet."; nit: Also say that you're sending a connection close packet. https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:527: std::queue<WriteRequest> empty; Can't you just call write_requests_queue_.clear()? https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:531: write_requests_queue_.push(empty.front()); Don't you have to add the request to write_requests_queue_?
https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:467: // TODO(sacomoto): Actually pass the current message to the observer. Shouldn't this be done in this CL? https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:495: OnDidSendMessage(WireMessage(std::string(), std::string()), true); Shouldn't this be false?
https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:127: if (sub_status_ == SubStatus::CONNECTED) { On 2016/07/07 18:04:46, Kyle Horimoto wrote: > Should this occur under other circumstances as well? For example, what if we > have already established a connection and have sent a connection request. Should > we wait until a connection response is received before sending? When we are waiting for a connection response, we are not actually connected to the server. So I didn't think we need to send a connection close. And otherwise every other state is when the connection is not established, hence can't send a connection close. https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:467: // TODO(sacomoto): Actually pass the current message to the observer. On 2016/07/07 18:34:20, Kyle Horimoto wrote: > Shouldn't this be done in this CL? Hmmm, I don't know why this is left as a TODO. Let's wait for Gustavo? https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:495: OnDidSendMessage(WireMessage(std::string(), std::string()), true); On 2016/07/07 18:34:20, Kyle Horimoto wrote: > Shouldn't this be false? Done. @Gustavo, why is this function called here? I thought if you get error, you just try again, why invoke the failure callback? https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:499: break; On 2016/07/07 18:04:46, Kyle Horimoto wrote: > You should return early here, or else you would continue processing more write > requests. > > Please add a test for this case. It's a bug. I removed the DestroyConnection(). I think connection_close should be either successfully written before destroyconnection or fail the max_number_of_write_attempts below. https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:519: PA_LOG(ERROR) << "Received erroneous packet."; On 2016/07/07 18:04:46, Kyle Horimoto wrote: > nit: Also say that you're sending a connection close packet. Done. https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:527: std::queue<WriteRequest> empty; On 2016/07/07 18:04:46, Kyle Horimoto wrote: > Can't you just call write_requests_queue_.clear()? somehow that doesn't exist for queues in c++... swapping with empty queue seems to be the shortest way to clear a queue according to stack overflow. https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:531: write_requests_queue_.push(empty.front()); On 2016/07/07 18:04:46, Kyle Horimoto wrote: > Don't you have to add the request to write_requests_queue_? If you are talking about the connection close request, it's added to the queue in WriteRemoteCharacteristic
Could you test the disconnect error cases still? https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:467: // TODO(sacomoto): Actually pass the current message to the observer. On 2016/07/07 23:38:47, jingxuy wrote: > On 2016/07/07 18:34:20, Kyle Horimoto wrote: > > Shouldn't this be done in this CL? > > Hmmm, I don't know why this is left as a TODO. Let's wait for Gustavo? Can you just implement it in this CL? You would just have to assign each string sent an ID, store a mapping from ID to string, then add that ID to the WriteRequest. Then, when you finish writing, use the ID to look up the string and pass it back. https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:527: std::queue<WriteRequest> empty; On 2016/07/07 23:38:47, jingxuy wrote: > On 2016/07/07 18:04:46, Kyle Horimoto wrote: > > Can't you just call write_requests_queue_.clear()? > > somehow that doesn't exist for queues in c++... swapping with empty queue seems > to be the shortest way to clear a queue according to stack overflow. Ah, I see. Can you add comments to make this clear what's happening and why you can't just clear it? https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:531: write_requests_queue_.push(empty.front()); On 2016/07/07 23:38:47, jingxuy wrote: > On 2016/07/07 18:04:46, Kyle Horimoto wrote: > > Don't you have to add the request to write_requests_queue_? > > If you are talking about the connection close request, it's added to the queue > in WriteRemoteCharacteristic I see now that you're just adding the in-progress characteristic back to the queue. Can you add comments to make this clear?
I received your request for tests, they are pending https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:467: // TODO(sacomoto): Actually pass the current message to the observer. On 2016/07/07 23:50:54, Kyle Horimoto wrote: > On 2016/07/07 23:38:47, jingxuy wrote: > > On 2016/07/07 18:34:20, Kyle Horimoto wrote: > > > Shouldn't this be done in this CL? > > > > Hmmm, I don't know why this is left as a TODO. Let's wait for Gustavo? > > Can you just implement it in this CL? You would just have to assign each string > sent an ID, store a mapping from ID to string, then add that ID to the > WriteRequest. Then, when you finish writing, use the ID to look up the string > and pass it back. oh I see what this is used for now. yah, left for TODO for now. Will address it when I get there. https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:527: std::queue<WriteRequest> empty; On 2016/07/07 23:50:54, Kyle Horimoto wrote: > On 2016/07/07 23:38:47, jingxuy wrote: > > On 2016/07/07 18:04:46, Kyle Horimoto wrote: > > > Can't you just call write_requests_queue_.clear()? > > > > somehow that doesn't exist for queues in c++... swapping with empty queue > seems > > to be the shortest way to clear a queue according to stack overflow. > > Ah, I see. Can you add comments to make this clear what's happening and why you > can't just clear it? Done. https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:531: write_requests_queue_.push(empty.front()); On 2016/07/07 23:50:54, Kyle Horimoto wrote: > On 2016/07/07 23:38:47, jingxuy wrote: > > On 2016/07/07 18:04:46, Kyle Horimoto wrote: > > > Don't you have to add the request to write_requests_queue_? > > > > If you are talking about the connection close request, it's added to the queue > > in WriteRemoteCharacteristic > > I see now that you're just adding the in-progress characteristic back to the > queue. Can you add comments to make this clear? Done.
https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:127: if (sub_status_ == SubStatus::CONNECTED) { On 2016/07/07 23:38:47, jingxuy wrote: > On 2016/07/07 18:04:46, Kyle Horimoto wrote: > > Should this occur under other circumstances as well? For example, what if we > > have already established a connection and have sent a connection request. > Should > > we wait until a connection response is received before sending? > > When we are waiting for a connection response, we are not actually connected to > the server. So I didn't think we need to send a connection close. And otherwise > every other state is when the connection is not established, hence can't send a > connection close. I agree with Jing. https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:495: OnDidSendMessage(WireMessage(std::string(), std::string()), true); On 2016/07/07 23:38:47, jingxuy wrote: > On 2016/07/07 18:34:20, Kyle Horimoto wrote: > > Shouldn't this be false? > > Done. @Gustavo, why is this function called here? I thought if you get error, > you just try again, why invoke the failure callback? It used to be |false|: https://cs.chromium.org/chromium/src/components/proximity_auth/ble/bluetooth_... You should try resending the packet a few times (i.e. |max_number_of_write_attempts_|), but then if it fails we should call |OnDidSendMessage(..., ..., false)|. But there was a bug in my previous implementation, when some packet of a message failed to be sent the |OnDidSendMessage()| was only being called if it was the last packet that failed. To fix that you should call |OnDidSendMessage(..., ..., false)| inside the "if" below (line 511), if were are actually trying to send a message. Be careful not call it if a control packet failed to be sent. https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:499: break; On 2016/07/07 23:38:47, jingxuy wrote: > On 2016/07/07 18:04:46, Kyle Horimoto wrote: > > You should return early here, or else you would continue processing more write > > requests. > > > > Please add a test for this case. > > It's a bug. I removed the DestroyConnection(). I think connection_close should > be either successfully written before destroyconnection or fail the > max_number_of_write_attempts below. That's right.
https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:495: OnDidSendMessage(WireMessage(std::string(), std::string()), true); On 2016/07/08 15:17:38, sacomoto wrote: > On 2016/07/07 23:38:47, jingxuy wrote: > > On 2016/07/07 18:34:20, Kyle Horimoto wrote: > > > Shouldn't this be false? > > > > Done. @Gustavo, why is this function called here? I thought if you get error, > > you just try again, why invoke the failure callback? > > It used to be |false|: > > https://cs.chromium.org/chromium/src/components/proximity_auth/ble/bluetooth_... > > You should try resending the packet a few times (i.e. > |max_number_of_write_attempts_|), but then if it fails we should call > |OnDidSendMessage(..., ..., false)|. But there was a bug in my previous > implementation, when some packet of a message failed to be sent the > |OnDidSendMessage()| was only being called if it was the last packet that > failed. > > To fix that you should call |OnDidSendMessage(..., ..., false)| inside the "if" > below (line 511), if were are actually trying to send a message. Be careful not > call it if a control packet failed to be sent. Oh yes that is my mistake. I already fixed it to be false. I am asking why still invoke the callback with false if you are just going to try to send the message again. https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:499: break; On 2016/07/08 15:17:38, sacomoto wrote: > On 2016/07/07 23:38:47, jingxuy wrote: > > On 2016/07/07 18:04:46, Kyle Horimoto wrote: > > > You should return early here, or else you would continue processing more > write > > > requests. > > > > > > Please add a test for this case. > > > > It's a bug. I removed the DestroyConnection(). I think connection_close should > > be either successfully written before destroyconnection or fail the > > max_number_of_write_attempts below. > > That's right. Done.
Added tests for connection close https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:499: break; On 2016/07/08 17:56:09, jingxuy wrote: > On 2016/07/08 15:17:38, sacomoto wrote: > > On 2016/07/07 23:38:47, jingxuy wrote: > > > On 2016/07/07 18:04:46, Kyle Horimoto wrote: > > > > You should return early here, or else you would continue processing more > > write > > > > requests. > > > > > > > > Please add a test for this case. > > > > > > It's a bug. I removed the DestroyConnection(). I think connection_close > should > > > be either successfully written before destroyconnection or fail the > > > max_number_of_write_attempts below. > > > > That's right. > > Done. Test added.
https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:467: // TODO(sacomoto): Actually pass the current message to the observer. On 2016/07/08 00:12:45, jingxuy wrote: > On 2016/07/07 23:50:54, Kyle Horimoto wrote: > > On 2016/07/07 23:38:47, jingxuy wrote: > > > On 2016/07/07 18:34:20, Kyle Horimoto wrote: > > > > Shouldn't this be done in this CL? > > > > > > Hmmm, I don't know why this is left as a TODO. Let's wait for Gustavo? > > > > Can you just implement it in this CL? You would just have to assign each > string > > sent an ID, store a mapping from ID to string, then add that ID to the > > WriteRequest. Then, when you finish writing, use the ID to look up the string > > and pass it back. > > oh I see what this is used for now. yah, left for TODO for now. Will address it > when I get there. Ping. https://codereview.chromium.org/2075313002/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:526: // C++ way of clearing a queue. nit: Explain that std::queue does not have a clear() method and link to the explanation of why this is the way it should be done. https://codereview.chromium.org/2075313002/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:819: TEST_F(ProximityAuthBluetoothLowEnergyWeaveClientConnectionTest, Can you also write a test which does tests this situation except when an existing packet is in the process of being sent? https://codereview.chromium.org/2075313002/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:845: TEST_F(ProximityAuthBluetoothLowEnergyWeaveClientConnectionTest, Can you register for callbacks and check that OnDidSendMessage() was called correctly? Same in the above test (for success).
https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:467: // TODO(sacomoto): Actually pass the current message to the observer. On 2016/07/11 21:34:59, Kyle Horimoto wrote: > On 2016/07/08 00:12:45, jingxuy wrote: > > On 2016/07/07 23:50:54, Kyle Horimoto wrote: > > > On 2016/07/07 23:38:47, jingxuy wrote: > > > > On 2016/07/07 18:34:20, Kyle Horimoto wrote: > > > > > Shouldn't this be done in this CL? > > > > > > > > Hmmm, I don't know why this is left as a TODO. Let's wait for Gustavo? > > > > > > Can you just implement it in this CL? You would just have to assign each > > string > > > sent an ID, store a mapping from ID to string, then add that ID to the > > > WriteRequest. Then, when you finish writing, use the ID to look up the > string > > > and pass it back. > > > > oh I see what this is used for now. yah, left for TODO for now. Will address > it > > when I get there. > > Ping. Done. https://codereview.chromium.org/2075313002/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:526: // C++ way of clearing a queue. On 2016/07/11 21:34:59, Kyle Horimoto wrote: > nit: Explain that std::queue does not have a clear() method and link to the > explanation of why this is the way it should be done. Done. https://codereview.chromium.org/2075313002/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:819: TEST_F(ProximityAuthBluetoothLowEnergyWeaveClientConnectionTest, On 2016/07/11 21:34:59, Kyle Horimoto wrote: > Can you also write a test which does tests this situation except when an > existing packet is in the process of being sent? Done. https://codereview.chromium.org/2075313002/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:845: TEST_F(ProximityAuthBluetoothLowEnergyWeaveClientConnectionTest, On 2016/07/11 21:34:59, Kyle Horimoto wrote: > Can you register for callbacks and check that OnDidSendMessage() was called > correctly? Same in the above test (for success). OnDidSendMessage is only called if a wire message is send. It would not be invoked on sending connection close.
https://codereview.chromium.org/2075313002/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:845: TEST_F(ProximityAuthBluetoothLowEnergyWeaveClientConnectionTest, On 2016/07/11 23:15:17, jingxuy wrote: > On 2016/07/11 21:34:59, Kyle Horimoto wrote: > > Can you register for callbacks and check that OnDidSendMessage() was called > > correctly? Same in the above test (for success). > > OnDidSendMessage is only called if a wire message is send. It would not be > invoked on sending connection close. That's incorrect - it is also called if a message fails to be sent. Please test both the failure and success cases by actually registering for callbacks and ensuring that they are called with the correct values. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:70: map_to_message_[kDefaultMessageCounter] = Comment about why this is needed (non-data packets). https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:71: std::unique_ptr<WireMessage>(nullptr); You should probably use an empty string instead of nullptr. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:189: message_counter_, std::move(message))); This won't work correctly once the WireMessage is deleted; if you try to access this later, you'll get segfaults. You need to allocate new memory, copy the existing WireMessage to it, and store that in the map instead. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:189: message_counter_, std::move(message))); You also need to remove things from the map when they are done. Otherwise, this could grow arbitrarily large without bound. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:191: int message_counter = message_counter_; current_message_counter Also, move these 2 lines above where you add it to the map and use current_message_counter there. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:476: weak_ptr_factory_.GetWeakPtr(), next_request.request_type, nit: Put next_request.request_type on its own line for clarity. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:494: OnDidSendMessage(*map_to_message_[message_counter], true); Please change your tests to assert that the correct value is passed here. Same for error case. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:297: // Counter indicate which message the packet is from. s/indicate/indicating/ https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:298: int message_counter_; next_message_counter_to_use_ https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:871: EXPECT_EQ(last_value_written_on_tx_characteristic_, Also test that the in-flight message is also sent. You might need to add a way to look at the 2nd-to-last value written in order to do this.
https://codereview.chromium.org/2075313002/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:845: TEST_F(ProximityAuthBluetoothLowEnergyWeaveClientConnectionTest, On 2016/07/12 01:03:12, Kyle Horimoto wrote: > On 2016/07/11 23:15:17, jingxuy wrote: > > On 2016/07/11 21:34:59, Kyle Horimoto wrote: > > > Can you register for callbacks and check that OnDidSendMessage() was called > > > correctly? Same in the above test (for success). > > > > OnDidSendMessage is only called if a wire message is send. It would not be > > invoked on sending connection close. > > That's incorrect - it is also called if a message fails to be sent. > > Please test both the failure and success cases by actually registering for > callbacks and ensuring that they are called with the correct values. What I meant is that there are 3 types of write requests. Message_complete is the kind that would call OnDidSendMessage on either success or failure. But connection_close does not invoke the function at all. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:70: map_to_message_[kDefaultMessageCounter] = On 2016/07/12 01:03:12, Kyle Horimoto wrote: > Comment about why this is needed (non-data packets). It's actually not necessary https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:71: std::unique_ptr<WireMessage>(nullptr); On 2016/07/12 01:03:14, Kyle Horimoto wrote: > You should probably use an empty string instead of nullptr. This line is deleted https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:189: message_counter_, std::move(message))); On 2016/07/12 01:03:12, Kyle Horimoto wrote: > This won't work correctly once the WireMessage is deleted; if you try to access > this later, you'll get segfaults. You need to allocate new memory, copy the > existing WireMessage to it, and store that in the map instead. The wire_message is passed in as unique_ptr which means this function is the owner of the pointer. So whoever passed this in should not delete the pointer. If you want to defensive copying, then WireMessage is actually disallow_copy_and_assign so would have to construct a new one. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:189: message_counter_, std::move(message))); On 2016/07/12 01:03:12, Kyle Horimoto wrote: > You also need to remove things from the map when they are done. Otherwise, this > could grow arbitrarily large without bound. Done. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:191: int message_counter = message_counter_; On 2016/07/12 01:03:12, Kyle Horimoto wrote: > current_message_counter > > Also, move these 2 lines above where you add it to the map and use > current_message_counter there. Done. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:476: weak_ptr_factory_.GetWeakPtr(), next_request.request_type, On 2016/07/12 01:03:13, Kyle Horimoto wrote: > nit: Put next_request.request_type on its own line for clarity. this is git cl format's doing, undoing it will fail presubmit check https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:494: OnDidSendMessage(*map_to_message_[message_counter], true); On 2016/07/12 01:03:13, Kyle Horimoto wrote: > Please change your tests to assert that the correct value is passed here. Same > for error case. I tried to do this but it seems like I'm encountering difficulty. The WireMessage is disallow_copy_and_assign, which means .WillOnce( DoAll(SaveArg<0>(&last_completed_wire_message_), SaveArg<1>(&last_wire_message_success_))); does not work to save message arguments https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:297: // Counter indicate which message the packet is from. On 2016/07/12 01:03:15, Kyle Horimoto wrote: > s/indicate/indicating/ Done. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:298: int message_counter_; On 2016/07/12 01:03:15, Kyle Horimoto wrote: > next_message_counter_to_use_ Done. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:871: EXPECT_EQ(last_value_written_on_tx_characteristic_, On 2016/07/12 01:03:16, Kyle Horimoto wrote: > Also test that the in-flight message is also sent. You might need to add a way > to look at the 2nd-to-last value written in order to do this. Done.
https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:189: message_counter_, std::move(message))); On 2016/07/15 18:24:59, jingxuy wrote: > On 2016/07/12 01:03:12, Kyle Horimoto wrote: > > You also need to remove things from the map when they are done. Otherwise, > this > > could grow arbitrarily large without bound. > > Done. Please expose map_to_message_ as a protected variable and write a test that asserts that the map is empty after sending a message (basically, assert that the map doesn't grow without bound). https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:189: message_counter_, std::move(message))); On 2016/07/15 18:24:59, jingxuy wrote: > On 2016/07/12 01:03:12, Kyle Horimoto wrote: > > This won't work correctly once the WireMessage is deleted; if you try to > access > > this later, you'll get segfaults. You need to allocate new memory, copy the > > existing WireMessage to it, and store that in the map instead. > > The wire_message is passed in as unique_ptr which means this function is the > owner of the pointer. So whoever passed this in should not delete the pointer. > If you want to defensive copying, then WireMessage is actually > disallow_copy_and_assign so would have to construct a new one. I didn't mean that the client who passed in the unique_ptr would delete it. This function will delete it. Brief description of how unique_ptr works: when its destructor is called, it deletes the allocated memory. Thus, in this function, |message| is a parameter, so it is on the stack. When the function is finished, the stack is cleaned up, meaning that the destructors of all the objects on the stack are called. Therefore, when this function is complete, the destructor will be called and the memory will be freed. This means that the pointer you're copying into the map will no longer be valid. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:494: OnDidSendMessage(*map_to_message_[message_counter], true); On 2016/07/15 18:24:59, jingxuy wrote: > On 2016/07/12 01:03:13, Kyle Horimoto wrote: > > Please change your tests to assert that the correct value is passed here. Same > > for error case. > > I tried to do this but it seems like I'm encountering difficulty. The > WireMessage is disallow_copy_and_assign, which means > .WillOnce( > DoAll(SaveArg<0>(&last_completed_wire_message_), > SaveArg<1>(&last_wire_message_success_))); > does not work to save message arguments I'm not sure what you mean, but I'm not talking about mocking anything. I'm saying to do this: 1) Create a ConnectionObserver. 2) Call connection.AddObserver(<the observer you created>); 3) Write something. 4) Assert that your observer's OnMessageReceive() was called with the correct value.
https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:189: message_counter_, std::move(message))); On 2016/07/15 20:20:43, Kyle Horimoto wrote: > On 2016/07/15 18:24:59, jingxuy wrote: > > On 2016/07/12 01:03:12, Kyle Horimoto wrote: > > > This won't work correctly once the WireMessage is deleted; if you try to > > access > > > this later, you'll get segfaults. You need to allocate new memory, copy the > > > existing WireMessage to it, and store that in the map instead. > > > > The wire_message is passed in as unique_ptr which means this function is the > > owner of the pointer. So whoever passed this in should not delete the pointer. > > If you want to defensive copying, then WireMessage is actually > > disallow_copy_and_assign so would have to construct a new one. > > I didn't mean that the client who passed in the unique_ptr would delete it. This > function will delete it. > > Brief description of how unique_ptr works: when its destructor is called, it > deletes the allocated memory. Thus, in this function, |message| is a parameter, > so it is on the stack. When the function is finished, the stack is cleaned up, > meaning that the destructors of all the objects on the stack are called. > Therefore, when this function is complete, the destructor will be called and the > memory will be freed. > > This means that the pointer you're copying into the map will no longer be valid. There could be only one object that owns the pointer in unique_ptr so std::move already moved the owned pointer to the unique_ptr in the vector. http://stackoverflow.com/questions/26318506/transferring-the-ownership-of-obj... https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:189: message_counter_, std::move(message))); On 2016/07/15 20:20:43, Kyle Horimoto wrote: > On 2016/07/15 18:24:59, jingxuy wrote: > > On 2016/07/12 01:03:12, Kyle Horimoto wrote: > > > You also need to remove things from the map when they are done. Otherwise, > > this > > > could grow arbitrarily large without bound. > > > > Done. > > Please expose map_to_message_ as a protected variable and write a test that > asserts that the map is empty after sending a message (basically, assert that > the map doesn't grow without bound). TODO https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:494: OnDidSendMessage(*map_to_message_[message_counter], true); On 2016/07/15 20:20:43, Kyle Horimoto wrote: > On 2016/07/15 18:24:59, jingxuy wrote: > > On 2016/07/12 01:03:13, Kyle Horimoto wrote: > > > Please change your tests to assert that the correct value is passed here. > Same > > > for error case. > > > > I tried to do this but it seems like I'm encountering difficulty. The > > WireMessage is disallow_copy_and_assign, which means > > .WillOnce( > > DoAll(SaveArg<0>(&last_completed_wire_message_), > > SaveArg<1>(&last_wire_message_success_))); > > does not work to save message arguments > > I'm not sure what you mean, but I'm not talking about mocking anything. I'm > saying to do this: > > 1) Create a ConnectionObserver. > 2) Call connection.AddObserver(<the observer you created>); > 3) Write something. > 4) Assert that your observer's OnMessageReceive() was called with the correct > value. The way the current tests are written, you can save arguments passed to functions, which is how I was trying to get to what's passed in the OnDidSendMessage and that doesn't work due to copying problems. But I can definitely create an observer for that.
https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:189: message_counter_, std::move(message))); On 2016/07/15 21:30:06, jingxuy wrote: > On 2016/07/15 20:20:43, Kyle Horimoto wrote: > > On 2016/07/15 18:24:59, jingxuy wrote: > > > On 2016/07/12 01:03:12, Kyle Horimoto wrote: > > > > This won't work correctly once the WireMessage is deleted; if you try to > > > access > > > > this later, you'll get segfaults. You need to allocate new memory, copy > the > > > > existing WireMessage to it, and store that in the map instead. > > > > > > The wire_message is passed in as unique_ptr which means this function is the > > > owner of the pointer. So whoever passed this in should not delete the > pointer. > > > If you want to defensive copying, then WireMessage is actually > > > disallow_copy_and_assign so would have to construct a new one. > > > > I didn't mean that the client who passed in the unique_ptr would delete it. > This > > function will delete it. > > > > Brief description of how unique_ptr works: when its destructor is called, it > > deletes the allocated memory. Thus, in this function, |message| is a > parameter, > > so it is on the stack. When the function is finished, the stack is cleaned up, > > meaning that the destructors of all the objects on the stack are called. > > Therefore, when this function is complete, the destructor will be called and > the > > memory will be freed. > > > > This means that the pointer you're copying into the map will no longer be > valid. > > There could be only one object that owns the pointer in unique_ptr so std::move > already moved the owned pointer to the unique_ptr in the vector. > > http://stackoverflow.com/questions/26318506/transferring-the-ownership-of-obj... Thanks for the link! Didn't know this before. Can you please add comments above this line to make it clear that ownership is transferring to the map and that |message| will hold a nullptr after this? Thanks! https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:494: OnDidSendMessage(*map_to_message_[message_counter], true); On 2016/07/15 21:30:06, jingxuy wrote: > On 2016/07/15 20:20:43, Kyle Horimoto wrote: > > On 2016/07/15 18:24:59, jingxuy wrote: > > > On 2016/07/12 01:03:13, Kyle Horimoto wrote: > > > > Please change your tests to assert that the correct value is passed here. > > Same > > > > for error case. > > > > > > I tried to do this but it seems like I'm encountering difficulty. The > > > WireMessage is disallow_copy_and_assign, which means > > > .WillOnce( > > > DoAll(SaveArg<0>(&last_completed_wire_message_), > > > SaveArg<1>(&last_wire_message_success_))); > > > does not work to save message arguments > > > > I'm not sure what you mean, but I'm not talking about mocking anything. I'm > > saying to do this: > > > > 1) Create a ConnectionObserver. > > 2) Call connection.AddObserver(<the observer you created>); > > 3) Write something. > > 4) Assert that your observer's OnMessageReceive() was called with the correct > > value. > > The way the current tests are written, you can save arguments passed to > functions, which is how I was trying to get to what's passed in the > OnDidSendMessage and that doesn't work due to copying problems. But I can > definitely create an observer for that. Yep, let's create an observer - thanks!
https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:189: message_counter_, std::move(message))); On 2016/07/15 21:30:06, jingxuy wrote: > On 2016/07/15 20:20:43, Kyle Horimoto wrote: > > On 2016/07/15 18:24:59, jingxuy wrote: > > > On 2016/07/12 01:03:12, Kyle Horimoto wrote: > > > > You also need to remove things from the map when they are done. Otherwise, > > > this > > > > could grow arbitrarily large without bound. > > > > > > Done. > > > > Please expose map_to_message_ as a protected variable and write a test that > > asserts that the map is empty after sending a message (basically, assert that > > the map doesn't grow without bound). > > TODO Done. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:189: message_counter_, std::move(message))); On 2016/07/15 21:54:54, Kyle Horimoto wrote: > On 2016/07/15 21:30:06, jingxuy wrote: > > On 2016/07/15 20:20:43, Kyle Horimoto wrote: > > > On 2016/07/15 18:24:59, jingxuy wrote: > > > > On 2016/07/12 01:03:12, Kyle Horimoto wrote: > > > > > This won't work correctly once the WireMessage is deleted; if you try to > > > > access > > > > > this later, you'll get segfaults. You need to allocate new memory, copy > > the > > > > > existing WireMessage to it, and store that in the map instead. > > > > > > > > The wire_message is passed in as unique_ptr which means this function is > the > > > > owner of the pointer. So whoever passed this in should not delete the > > pointer. > > > > If you want to defensive copying, then WireMessage is actually > > > > disallow_copy_and_assign so would have to construct a new one. > > > > > > I didn't mean that the client who passed in the unique_ptr would delete it. > > This > > > function will delete it. > > > > > > Brief description of how unique_ptr works: when its destructor is called, it > > > deletes the allocated memory. Thus, in this function, |message| is a > > parameter, > > > so it is on the stack. When the function is finished, the stack is cleaned > up, > > > meaning that the destructors of all the objects on the stack are called. > > > Therefore, when this function is complete, the destructor will be called and > > the > > > memory will be freed. > > > > > > This means that the pointer you're copying into the map will no longer be > > valid. > > > > There could be only one object that owns the pointer in unique_ptr so > std::move > > already moved the owned pointer to the unique_ptr in the vector. > > > > > http://stackoverflow.com/questions/26318506/transferring-the-ownership-of-obj... > > Thanks for the link! Didn't know this before. Can you please add comments above > this line to make it clear that ownership is transferring to the map and that > |message| will hold a nullptr after this? Thanks! Done. https://codereview.chromium.org/2075313002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:494: OnDidSendMessage(*map_to_message_[message_counter], true); On 2016/07/15 21:54:54, Kyle Horimoto wrote: > On 2016/07/15 21:30:06, jingxuy wrote: > > On 2016/07/15 20:20:43, Kyle Horimoto wrote: > > > On 2016/07/15 18:24:59, jingxuy wrote: > > > > On 2016/07/12 01:03:13, Kyle Horimoto wrote: > > > > > Please change your tests to assert that the correct value is passed > here. > > > Same > > > > > for error case. > > > > > > > > I tried to do this but it seems like I'm encountering difficulty. The > > > > WireMessage is disallow_copy_and_assign, which means > > > > .WillOnce( > > > > DoAll(SaveArg<0>(&last_completed_wire_message_), > > > > SaveArg<1>(&last_wire_message_success_))); > > > > does not work to save message arguments > > > > > > I'm not sure what you mean, but I'm not talking about mocking anything. I'm > > > saying to do this: > > > > > > 1) Create a ConnectionObserver. > > > 2) Call connection.AddObserver(<the observer you created>); > > > 3) Write something. > > > 4) Assert that your observer's OnMessageReceive() was called with the > correct > > > value. > > > > The way the current tests are written, you can save arguments passed to > > functions, which is how I was trying to get to what's passed in the > > OnDidSendMessage and that doesn't work due to copying problems. But I can > > definitely create an observer for that. > > Yep, let's create an observer - thanks! Done.
Almost there! :) https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:522: PA_LOG(ERROR) << "I'm here "; nit: Remove test logs. https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:79: DLOG(ERROR) << "I'm here too!"; nit: Remove test logs. https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:878: // TODO(jingxuy): find out whether this is a bug or a feature. This sounds like a bug; observers should be notified of all failures. https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:881: EXPECT_EQ(i + 1, connection_observer_.GetNumSendCompleted()); nit: You can just expect 1, since this is only called when i == 0.
https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:522: PA_LOG(ERROR) << "I'm here "; On 2016/07/16 00:46:56, Kyle Horimoto wrote: > nit: Remove test logs. Done. https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:79: DLOG(ERROR) << "I'm here too!"; On 2016/07/16 00:46:57, Kyle Horimoto wrote: > nit: Remove test logs. Done. https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:878: // TODO(jingxuy): find out whether this is a bug or a feature. On 2016/07/16 00:46:56, Kyle Horimoto wrote: > This sounds like a bug; observers should be notified of all failures. Yes, I chased down this and it's a boolean in OnDidSendMessage that will return early. I want to ask someone about this. https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:881: EXPECT_EQ(i + 1, connection_observer_.GetNumSendCompleted()); On 2016/07/16 00:46:57, Kyle Horimoto wrote: > nit: You can just expect 1, since this is only called when i == 0. Done.
https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:878: // TODO(jingxuy): find out whether this is a bug or a feature. On 2016/07/16 00:54:53, jingxuy wrote: > On 2016/07/16 00:46:56, Kyle Horimoto wrote: > > This sounds like a bug; observers should be notified of all failures. > > Yes, I chased down this and it's a boolean in OnDidSendMessage that will return > early. I want to ask someone about this. I don't think this is a bug. The observers should be notified when the message fails to be send, and the send operation is only considered to be a failure after the characteristic write fails 3 times. https://codereview.chromium.org/2075313002/diff/390001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/390001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:109: void SetSubStatus(SubStatus status); This method no longer used in tests. Please move it to private.
https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:878: // TODO(jingxuy): find out whether this is a bug or a feature. On 2016/07/18 16:07:09, sacomoto wrote: > On 2016/07/16 00:54:53, jingxuy wrote: > > On 2016/07/16 00:46:56, Kyle Horimoto wrote: > > > This sounds like a bug; observers should be notified of all failures. > > > > Yes, I chased down this and it's a boolean in OnDidSendMessage that will > return > > early. I want to ask someone about this. > > I don't think this is a bug. The observers should be notified when the message > fails to be send, and the send operation is only considered to be a failure > after the characteristic write fails 3 times. Doesn't this test imply that the failure was reported after the first failure though, instead of the 3rd?
https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:878: // TODO(jingxuy): find out whether this is a bug or a feature. On 2016/07/18 17:04:14, Kyle Horimoto wrote: > On 2016/07/18 16:07:09, sacomoto wrote: > > On 2016/07/16 00:54:53, jingxuy wrote: > > > On 2016/07/16 00:46:56, Kyle Horimoto wrote: > > > > This sounds like a bug; observers should be notified of all failures. > > > > > > Yes, I chased down this and it's a boolean in OnDidSendMessage that will > > return > > > early. I want to ask someone about this. > > > > I don't think this is a bug. The observers should be notified when the message > > fails to be send, and the send operation is only considered to be a failure > > after the characteristic write fails 3 times. > > Doesn't this test imply that the failure was reported after the first failure > though, instead of the 3rd? yah, I think from the current and the legacy implementation, the first failure immediately caused OnDidSendMessage(false) to be called. https://codereview.chromium.org/2075313002/diff/390001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/390001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:109: void SetSubStatus(SubStatus status); On 2016/07/18 16:07:09, sacomoto wrote: > This method no longer used in tests. Please move it to private. Done.
https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:878: // TODO(jingxuy): find out whether this is a bug or a feature. On 2016/07/18 17:22:25, jingxuy wrote: > On 2016/07/18 17:04:14, Kyle Horimoto wrote: > > On 2016/07/18 16:07:09, sacomoto wrote: > > > On 2016/07/16 00:54:53, jingxuy wrote: > > > > On 2016/07/16 00:46:56, Kyle Horimoto wrote: > > > > > This sounds like a bug; observers should be notified of all failures. > > > > > > > > Yes, I chased down this and it's a boolean in OnDidSendMessage that will > > > return > > > > early. I want to ask someone about this. > > > > > > I don't think this is a bug. The observers should be notified when the > message > > > fails to be send, and the send operation is only considered to be a failure > > > after the characteristic write fails 3 times. > > > > Doesn't this test imply that the failure was reported after the first failure > > though, instead of the 3rd? > > yah, I think from the current and the legacy implementation, the first failure > immediately caused OnDidSendMessage(false) to be called. IMO, only one OnDidSendMessage(false) should be called, but it should only be done after we have failed the maximum number of times. How's that sound?
https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:878: // TODO(jingxuy): find out whether this is a bug or a feature. On 2016/07/18 17:36:07, Kyle Horimoto wrote: > On 2016/07/18 17:22:25, jingxuy wrote: > > On 2016/07/18 17:04:14, Kyle Horimoto wrote: > > > On 2016/07/18 16:07:09, sacomoto wrote: > > > > On 2016/07/16 00:54:53, jingxuy wrote: > > > > > On 2016/07/16 00:46:56, Kyle Horimoto wrote: > > > > > > This sounds like a bug; observers should be notified of all failures. > > > > > > > > > > Yes, I chased down this and it's a boolean in OnDidSendMessage that will > > > > return > > > > > early. I want to ask someone about this. > > > > > > > > I don't think this is a bug. The observers should be notified when the > > message > > > > fails to be send, and the send operation is only considered to be a > failure > > > > after the characteristic write fails 3 times. > > > > > > Doesn't this test imply that the failure was reported after the first > failure > > > though, instead of the 3rd? > > > > yah, I think from the current and the legacy implementation, the first failure > > immediately caused OnDidSendMessage(false) to be called. > > IMO, only one OnDidSendMessage(false) should be called, but it should only be > done after we have failed the maximum number of times. How's that sound? I agree, this is the intended behavior.
https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc (right): https://codereview.chromium.org/2075313002/diff/370001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc:878: // TODO(jingxuy): find out whether this is a bug or a feature. On 2016/07/18 17:42:15, sacomoto wrote: > On 2016/07/18 17:36:07, Kyle Horimoto wrote: > > On 2016/07/18 17:22:25, jingxuy wrote: > > > On 2016/07/18 17:04:14, Kyle Horimoto wrote: > > > > On 2016/07/18 16:07:09, sacomoto wrote: > > > > > On 2016/07/16 00:54:53, jingxuy wrote: > > > > > > On 2016/07/16 00:46:56, Kyle Horimoto wrote: > > > > > > > This sounds like a bug; observers should be notified of all > failures. > > > > > > > > > > > > Yes, I chased down this and it's a boolean in OnDidSendMessage that > will > > > > > return > > > > > > early. I want to ask someone about this. > > > > > > > > > > I don't think this is a bug. The observers should be notified when the > > > message > > > > > fails to be send, and the send operation is only considered to be a > > failure > > > > > after the characteristic write fails 3 times. > > > > > > > > Doesn't this test imply that the failure was reported after the first > > failure > > > > though, instead of the 3rd? > > > > > > yah, I think from the current and the legacy implementation, the first > failure > > > immediately caused OnDidSendMessage(false) to be called. > > > > IMO, only one OnDidSendMessage(false) should be called, but it should only be > > done after we have failed the maximum number of times. How's that sound? > > I agree, this is the intended behavior. Done.
Please update the GYP files too: components/components_tests.gyp components/proximity_auth.gypi. LGTM after that change.
lgtm
On 2016/07/20 11:41:11, sacomoto wrote: > Please update the GYP files too: > > components/components_tests.gyp components/proximity_auth.gypi. > > LGTM after that change. Done.
https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:76: // connection since we might not be connected at all. "it would be unwise to send a connection" What does "send a connection" refer to here? https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:115: if (remote_device) { If remote_device == null, we should call DestroyConnection() https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:287: BluetoothLowEnergyWeaveClientConnection::WriteRequest::WriteRequest( nit: move these WriteRequest definitions below all the parent class methods. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:320: << "error code: " << error_code; nit: extra space or comma before "error code: " https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:334: gatt_connection_ = std::move(gatt_connection); nit: You don't need the std::move() call. By default, unique_ptrs should have move semantics for assignment. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:378: << (tx_characteristic.id.empty() nit: put new lines or spaces between each characteristic. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:390: if (sub_status() == SubStatus::CHARACTERISTICS_FOUND) { DCHECK() this state instead. Can StartNotifySession() be called in any other case? https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:393: DCHECK(characteristic); nit: might as well do a full CHECK here, as we're dereferencing the pointer right after. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:425: notify_session_ = std::move(notify_session); nit: no need for std::move https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:471: PA_LOG(INFO) << "Writing characteristic..."; nit: it would be nice to log the number of bytes written. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:310: std::map<int, std::unique_ptr<WireMessage>> map_to_message_; Do you actually need to store a map of WireMessages that were sent? It seems you are inserting and then erasing each written message. Wouldn't it suffice just to store the last written WireMessage?
https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:76: // connection since we might not be connected at all. On 2016/07/21 20:49:34, Tim Song wrote: > "it would be unwise to send a connection" > > What does "send a connection" refer to here? Done. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:115: if (remote_device) { On 2016/07/21 20:49:35, Tim Song wrote: > If remote_device == null, we should call DestroyConnection() DestroyConnection destroys a gatt connection. so if it's never established, there is nothing to destroy. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:287: BluetoothLowEnergyWeaveClientConnection::WriteRequest::WriteRequest( On 2016/07/21 20:49:35, Tim Song wrote: > nit: move these WriteRequest definitions below all the parent class methods. Done. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:320: << "error code: " << error_code; On 2016/07/21 20:49:34, Tim Song wrote: > nit: extra space or comma before "error code: " Done. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:334: gatt_connection_ = std::move(gatt_connection); On 2016/07/21 20:49:34, Tim Song wrote: > nit: You don't need the std::move() call. By default, unique_ptrs should have > move semantics for assignment. Done. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:378: << (tx_characteristic.id.empty() On 2016/07/21 20:49:35, Tim Song wrote: > nit: put new lines or spaces between each characteristic. Done. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:390: if (sub_status() == SubStatus::CHARACTERISTICS_FOUND) { On 2016/07/21 20:49:34, Tim Song wrote: > DCHECK() this state instead. Can StartNotifySession() be called in any other > case? Done. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:393: DCHECK(characteristic); On 2016/07/21 20:49:34, Tim Song wrote: > nit: might as well do a full CHECK here, as we're dereferencing the pointer > right after. A lot of your comments are on legacy code. Sorry I didn't look too closely through them. But done. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:425: notify_session_ = std::move(notify_session); On 2016/07/21 20:49:35, Tim Song wrote: > nit: no need for std::move also legacy code. Done. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:471: PA_LOG(INFO) << "Writing characteristic..."; On 2016/07/21 20:49:35, Tim Song wrote: > nit: it would be nice to log the number of bytes written. Done. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:310: std::map<int, std::unique_ptr<WireMessage>> map_to_message_; On 2016/07/21 20:49:35, Tim Song wrote: > Do you actually need to store a map of WireMessages that were sent? It seems you > are inserting and then erasing each written message. Wouldn't it suffice just to > store the last written WireMessage? I think the problem there is no guarantee when the write remote characteristic success callback is called. It could have been 2 outstanding writes and it doesn't finish the order you expect them. Unless it's guarantee FIFO and I can change it.
https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:115: if (remote_device) { On 2016/07/21 21:30:34, jingxuy wrote: > On 2016/07/21 20:49:35, Tim Song wrote: > > If remote_device == null, we should call DestroyConnection() > > DestroyConnection destroys a gatt connection. so if it's never established, > there is nothing to destroy. It also calls SetStatus(DISCONNECTED), which would inform the observers registered with the connection. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:310: std::map<int, std::unique_ptr<WireMessage>> map_to_message_; On 2016/07/21 21:30:34, jingxuy wrote: > On 2016/07/21 20:49:35, Tim Song wrote: > > Do you actually need to store a map of WireMessages that were sent? It seems > you > > are inserting and then erasing each written message. Wouldn't it suffice just > to > > store the last written WireMessage? > > I think the problem there is no guarantee when the write remote characteristic > success callback is called. It could have been 2 outstanding writes and it > doesn't finish the order you expect them. Unless it's guarantee FIFO and I can > change it. |characteristic->WriteRemoteCharacteristic()| is only called when the last request completes (whether success or error), so it seems to me that there is no way to have 2 outstanding requests at the same time.
https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:115: if (remote_device) { On 2016/07/22 00:52:50, Tim Song wrote: > On 2016/07/21 21:30:34, jingxuy wrote: > > On 2016/07/21 20:49:35, Tim Song wrote: > > > If remote_device == null, we should call DestroyConnection() > > > > DestroyConnection destroys a gatt connection. so if it's never established, > > there is nothing to destroy. > > It also calls SetStatus(DISCONNECTED), which would inform the observers > registered with the connection. Done. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:425: notify_session_ = std::move(notify_session); On 2016/07/21 21:30:34, jingxuy wrote: > On 2016/07/21 20:49:35, Tim Song wrote: > > nit: no need for std::move > > also legacy code. Done. Also wasn't thinking clearly yesterday. On second thought upon compilation failures. I think you need the std::move. Check out examples in: http://en.cppreference.com/w/cpp/memory/unique_ptr/operator%3D https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:310: std::map<int, std::unique_ptr<WireMessage>> map_to_message_; On 2016/07/22 00:52:50, Tim Song wrote: > On 2016/07/21 21:30:34, jingxuy wrote: > > On 2016/07/21 20:49:35, Tim Song wrote: > > > Do you actually need to store a map of WireMessages that were sent? It seems > > you > > > are inserting and then erasing each written message. Wouldn't it suffice > just > > to > > > store the last written WireMessage? > > > > I think the problem there is no guarantee when the write remote characteristic > > success callback is called. It could have been 2 outstanding writes and it > > doesn't finish the order you expect them. Unless it's guarantee FIFO and I can > > change it. > > |characteristic->WriteRemoteCharacteristic()| is only called when the last > request completes (whether success or error), so it seems to me that there is no > way to have 2 outstanding requests at the same time. I wasn't thinking clearly yesterday. But the reason that you need the map is that sendMessaeImpl can be called many times with different message before any one message finish sending. So at any given time, there could be more than 1 outstanding message and the callback needs to know which is which.
https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:115: if (remote_device) { On 2016/07/22 00:52:50, Tim Song wrote: > On 2016/07/21 21:30:34, jingxuy wrote: > > On 2016/07/21 20:49:35, Tim Song wrote: > > > If remote_device == null, we should call DestroyConnection() > > > > DestroyConnection destroys a gatt connection. so if it's never established, > > there is nothing to destroy. > > It also calls SetStatus(DISCONNECTED), which would inform the observers > registered with the connection. Done. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:425: notify_session_ = std::move(notify_session); On 2016/07/21 21:30:34, jingxuy wrote: > On 2016/07/21 20:49:35, Tim Song wrote: > > nit: no need for std::move > > also legacy code. Done. Also wasn't thinking clearly yesterday. On second thought upon compilation failures. I think you need the std::move. Check out examples in: http://en.cppreference.com/w/cpp/memory/unique_ptr/operator%3D https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:310: std::map<int, std::unique_ptr<WireMessage>> map_to_message_; On 2016/07/22 00:52:50, Tim Song wrote: > On 2016/07/21 21:30:34, jingxuy wrote: > > On 2016/07/21 20:49:35, Tim Song wrote: > > > Do you actually need to store a map of WireMessages that were sent? It seems > > you > > > are inserting and then erasing each written message. Wouldn't it suffice > just > > to > > > store the last written WireMessage? > > > > I think the problem there is no guarantee when the write remote characteristic > > success callback is called. It could have been 2 outstanding writes and it > > doesn't finish the order you expect them. Unless it's guarantee FIFO and I can > > change it. > > |characteristic->WriteRemoteCharacteristic()| is only called when the last > request completes (whether success or error), so it seems to me that there is no > way to have 2 outstanding requests at the same time. I wasn't thinking clearly yesterday. But the reason that you need the map is that sendMessaeImpl can be called many times with different message before any one message finish sending. So at any given time, there could be more than 1 outstanding message and the callback needs to know which is which.
https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc (right): https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.cc:425: notify_session_ = std::move(notify_session); On 2016/07/22 17:14:59, jingxuy wrote: > On 2016/07/21 21:30:34, jingxuy wrote: > > On 2016/07/21 20:49:35, Tim Song wrote: > > > nit: no need for std::move > > > > also legacy code. Done. > > Also wasn't thinking clearly yesterday. On second thought upon compilation > failures. I think you need the std::move. > > Check out examples in: > http://en.cppreference.com/w/cpp/memory/unique_ptr/operator%3D Ah I see, I was thinking of scoped_ptr, which has these semantics. https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:310: std::map<int, std::unique_ptr<WireMessage>> map_to_message_; On 2016/07/22 17:14:59, jingxuy wrote: > On 2016/07/22 00:52:50, Tim Song wrote: > > On 2016/07/21 21:30:34, jingxuy wrote: > > > On 2016/07/21 20:49:35, Tim Song wrote: > > > > Do you actually need to store a map of WireMessages that were sent? It > seems > > > you > > > > are inserting and then erasing each written message. Wouldn't it suffice > > just > > > to > > > > store the last written WireMessage? > > > > > > I think the problem there is no guarantee when the write remote > characteristic > > > success callback is called. It could have been 2 outstanding writes and it > > > doesn't finish the order you expect them. Unless it's guarantee FIFO and I > can > > > change it. > > > > |characteristic->WriteRemoteCharacteristic()| is only called when the last > > request completes (whether success or error), so it seems to me that there is > no > > way to have 2 outstanding requests at the same time. > > I wasn't thinking clearly yesterday. But the reason that you need the map is > that sendMessaeImpl can be called many times with different message before any > one message finish sending. So at any given time, there could be more than 1 > outstanding message and the callback needs to know which is which. SendMessageImpl() creates a new WriteRequest and puts it in the |write_requests_queue_|, and processes one request at a time, so it already does the queuing.
https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/490001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:310: std::map<int, std::unique_ptr<WireMessage>> map_to_message_; On 2016/07/22 18:41:34, Tim Song wrote: > On 2016/07/22 17:14:59, jingxuy wrote: > > On 2016/07/22 00:52:50, Tim Song wrote: > > > On 2016/07/21 21:30:34, jingxuy wrote: > > > > On 2016/07/21 20:49:35, Tim Song wrote: > > > > > Do you actually need to store a map of WireMessages that were sent? It > > seems > > > > you > > > > > are inserting and then erasing each written message. Wouldn't it suffice > > > just > > > > to > > > > > store the last written WireMessage? > > > > > > > > I think the problem there is no guarantee when the write remote > > characteristic > > > > success callback is called. It could have been 2 outstanding writes and it > > > > doesn't finish the order you expect them. Unless it's guarantee FIFO and I > > can > > > > change it. > > > > > > |characteristic->WriteRemoteCharacteristic()| is only called when the last > > > request completes (whether success or error), so it seems to me that there > is > > no > > > way to have 2 outstanding requests at the same time. > > > > I wasn't thinking clearly yesterday. But the reason that you need the map is > > that sendMessaeImpl can be called many times with different message before any > > one message finish sending. So at any given time, there could be more than 1 > > outstanding message and the callback needs to know which is which. > > SendMessageImpl() creates a new WriteRequest and puts it in the > |write_requests_queue_|, and processes one request at a time, so it already does > the queuing. Done.
lgtm https://codereview.chromium.org/2075313002/diff/510001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/510001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:1: no new line.
https://codereview.chromium.org/2075313002/diff/510001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h (right): https://codereview.chromium.org/2075313002/diff/510001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection.h:1: On 2016/07/22 23:21:40, Tim Song wrote: > no new line. Done.
The CQ bit was checked by jingxuy@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from khorimoto@chromium.org, sacomoto@chromium.org, tengs@chromium.org Link to the patchset: https://codereview.chromium.org/2075313002/#ps570001 (title: "delete a space")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by jingxuy@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from khorimoto@chromium.org, tengs@chromium.org, sacomoto@chromium.org Link to the patchset: https://codereview.chromium.org/2075313002/#ps590001 (title: "fix compile error on android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by jingxuy@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from khorimoto@chromium.org, tengs@chromium.org, sacomoto@chromium.org Link to the patchset: https://codereview.chromium.org/2075313002/#ps610001 (title: "fixed typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by jingxuy@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from khorimoto@chromium.org, tengs@chromium.org, sacomoto@chromium.org Link to the patchset: https://codereview.chromium.org/2075313002/#ps630001 (title: "expect true/false instead of equality")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jingxuy@google.com
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 ========== Using the uWeave packet generator and receiver to re-implement the BluetoothLowEnergyConnection class Major changes: SendMesssageImpl which instead of using the old protocol, it will just generate the message with the WeavePacketGenerator. GattCharacteristicValueChanged instead of parsing messages received with the old protocol, will now use the WeavePacketReceiver and just switch on the state of the receiver and take appropriate action. Design Doc for ProximityAuth uWeave Migration: https://docs.google.com/a/google.com/document/d/1HJ1fipxsNQwIxmKs-5_6TjroCAcs... BUG=617238 ========== to ========== Using the uWeave packet generator and receiver to re-implement the BluetoothLowEnergyConnection class Major changes: SendMesssageImpl which instead of using the old protocol, it will just generate the message with the WeavePacketGenerator. GattCharacteristicValueChanged instead of parsing messages received with the old protocol, will now use the WeavePacketReceiver and just switch on the state of the receiver and take appropriate action. Design Doc for ProximityAuth uWeave Migration: https://docs.google.com/a/google.com/document/d/1HJ1fipxsNQwIxmKs-5_6TjroCAcs... BUG=617238 ==========
Message was sent while issue was closed.
Committed patchset #33 (id:630001)
Message was sent while issue was closed.
Description was changed from ========== Using the uWeave packet generator and receiver to re-implement the BluetoothLowEnergyConnection class Major changes: SendMesssageImpl which instead of using the old protocol, it will just generate the message with the WeavePacketGenerator. GattCharacteristicValueChanged instead of parsing messages received with the old protocol, will now use the WeavePacketReceiver and just switch on the state of the receiver and take appropriate action. Design Doc for ProximityAuth uWeave Migration: https://docs.google.com/a/google.com/document/d/1HJ1fipxsNQwIxmKs-5_6TjroCAcs... BUG=617238 ========== to ========== Using the uWeave packet generator and receiver to re-implement the BluetoothLowEnergyConnection class Major changes: SendMesssageImpl which instead of using the old protocol, it will just generate the message with the WeavePacketGenerator. GattCharacteristicValueChanged instead of parsing messages received with the old protocol, will now use the WeavePacketReceiver and just switch on the state of the receiver and take appropriate action. Design Doc for ProximityAuth uWeave Migration: https://docs.google.com/a/google.com/document/d/1HJ1fipxsNQwIxmKs-5_6TjroCAcs... BUG=617238 Committed: https://crrev.com/f4f0a5cde8eff464de5359281560444ff442f920 Cr-Commit-Position: refs/heads/master@{#407655} ==========
Message was sent while issue was closed.
Patchset 33 (id:??) landed as https://crrev.com/f4f0a5cde8eff464de5359281560444ff442f920 Cr-Commit-Position: refs/heads/master@{#407655}
Message was sent while issue was closed.
A revert of this CL (patchset #33 id:630001) has been created in https://codereview.chromium.org/2189913002/ by benwells@chromium.org. The reason for reverting is: This CL has a memory problem in the unit tests, as reported by DrMemory. First build showing the error: https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%2... Note, that build doesn't include this change as there was a problem introduced earlier that caused the whole build to fail. Error text: UNADDRESSABLE ACCESS of freed memory: reading 0x069fa848-0x069fa84c 4 byte(s) # 0 proximity_auth::weave::BluetoothLowEnergyWeavePacketGenerator::Factory::NewInstance [components\proximity_auth\ble\bluetooth_low_energy_weave_packet_generator.cc:33] # 1 proximity_auth::weave::ProximityAuthBluetoothLowEnergyWeavePacketGeneratorTest_CreateConnectionRequestTest_Test::TestBody [components\proximity_auth\ble\bluetooth_low_energy_weave_packet_generator_unittest.cc:54] # 2 testing::internal::HandleExceptionsInMethodIfSupported<> [testing\gtest\src\gtest.cc:2458] Note: @0:05:46.506 in thread 13092 Note: 0x069fa848-0x069fa84c overlaps memory 0x069fa5d8-0x069fa8c0 that was freed here: Note: # 0 replace_operator_delete_nothrow [d:\drmemory_package\common\alloc_replace.c:2974] Note: # 1 proximity_auth::weave::ProximityAuthBluetoothLowEnergyWeaveClientConnectionTest_ConnectAfterADelayWhenThrottled_Test::`scalar deleting destructor' Note: # 2 testing::Test::DeleteSelf_ [testing\gtest\include\gtest\gtest.h:453] Note: # 3 testing::TestInfo::Run [testing\gtest\src\gtest.cc:2661] Note: instruction: mov (%eax) -> %edx Suppression (error hash=#E743E83C1B0FB1CF#): For more info on using suppressions see http://dev.chromium.org/developers/how-tos/using-drmemory#TOC-Suppressing-err... { UNADDRESSABLE ACCESS name=<insert_a_suppression_name_here> *!proximity_auth::weave::BluetoothLowEnergyWeavePacketGenerator::Factory::NewInstance *!proximity_auth::weave::ProximityAuthBluetoothLowEnergyWeavePacketGeneratorTest_CreateConnectionRequestTest_Test::TestBody *!testing::internal::HandleExceptionsInMethodIfSupported<> } The problem is real, to fix this SetInstanceForTesting should keep the previous factory around somewhere, and there should be a method to clear the testing instance. Or something like that.. |
