Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2053013002:
Weave Packet Receiver (Closed)
DescriptionWeave Packet Generator
This CL adds a packet receiver that respects the uWeave.BLE
protocol.
The packet receiver expects this chain of events:
1 control_request/response -> 0+ data packets -> 0 or 1 control_close.
The packet receiver keeps track of the receiving state and will jump to ERROR state when anything unexpected happens. (receiving packets out of order, invalid packet content, etc)
The expected user of packet receiver should feed a packet into receiver, check the state, and take appropriate action. If actions are not taken in the appropriate state, the receiver will crash. Data message should be retrieved promptly when the state reaches DATA_READY or else the data will be lost when the next packet arrives.
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/688334563b18b7d80b3207da6c0d30d17323b351
Cr-Commit-Position: refs/heads/master@{#402884}
Patch Set 1 : code complete built #
Total comments: 38
Patch Set 2 : fixed state transition obvious bugs #Patch Set 3 : fixed typo in comment #
Total comments: 17
Patch Set 4 : fixed a slight typo in the .h file #
Total comments: 10
Patch Set 5 : finished minor todo and addressed some comments #Patch Set 6 : disallowed multiple request/response, added some error log #Patch Set 7 : added state transtion out of connecting #
Total comments: 24
Patch Set 8 : Don't review yet! fix state problems and added more error assistant and added unittest #Patch Set 9 : Added unit test and fix some state machine problems #Patch Set 10 : wrong first bit is now packet_out_of_sequence error instead of unknown, switch log from base to PA #
Total comments: 2
Patch Set 11 : now accepts connection close without a reason #Patch Set 12 : fixed typo build problem #
Total comments: 64
Patch Set 13 : commented test and h file. Fixed memory leak in test. style modification in cc file #
Total comments: 33
Patch Set 14 : divided tests into little chunks #
Total comments: 6
Patch Set 15 : added receiver error code and some test along the waywq #
Total comments: 21
Patch Set 16 : addressed comments in cc file #Patch Set 17 : ifdef guard around htons includeswq #Patch Set 18 : BUILD.gn merge #Patch Set 19 : fix macro problem and change functions to virtual #Patch Set 20 : fixed narrowing conversion from uint32_t to uint8-t #Messages
Total messages: 58 (19 generated)
Description was changed from ========== uWeave Receiver BUG= ========== to ========== uWeave Receiver BUG=617238 ==========
jingxuy@google.com changed reviewers: + khorimoto@chromium.org, msarda@chromium.org, sacomoto@chromium.org, tengs@chromium.org
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:1) has been deleted
https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:7: #include "base/logging.h" Please use "components/proximity_auth/logging.h" https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:66: return std::string(data_message_.begin(), data_message_.end()); Check if state_ == DATA_READY? https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:85: switch (GetControlCommand(packet)) { Please refactor this switch to its own function. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:87: if (state_ == State::RECEIVING_DATA) { Shouldn't this be checking that state_ != EXCHANGING_CONTROLS? You should also do a state transition here. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:93: // TODO(jingxuy): what do I do with the 13 bytes of data? I don't think you'll get any extra data. The packet that you receive doesn't have to be of the maximum packet size. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:96: if (state_ == State::RECEIVING_DATA) { shouldn't this be checking that state_ != EXCHANGING_CONTROLS? You should also do a state transition here. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:102: // TODO(jingxuy): what do I do with the 15 bytes of data? same here. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:104: case ControlPacketCommand::CONNECTION_CLOSE: check that the current state is valid. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:114: case PacketType::DATA: Also refactor this block into its own function. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:116: if (state_ != State::EXCHANGING_CONTROLS) { Shouldn't this state you check here be DATA_READY? https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:146: DCHECK_LT(index, packet.size()); I don't think you should DCHECK this condition, as we are getting these packets from the remote device. We should deal with invalid packets (state_ = ERROR?) rather than crashing. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:179: // Bit determining whether the packet is first pacet is bit 3. nit: s/pacet/packet/ https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit:2016 https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:38: EXCHANGING_CONTROLS = 0x00, A better name for this state might be CONNECTING. I would also add a CONNECTED state to distinguish from the DATA_READY state. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:62: void ReceivePacket(const Packet& packet); Perhaps you should return the current state as well, so the caller can do something like: if (receiver_->ReceivePacket(packet) == State::DATA_READY) { ProcessMessage(receiver_->GetDataMessage()); }
https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:7: #include "base/logging.h" On 2016/06/09 22:08:34, Tim Song wrote: > Please use "components/proximity_auth/logging.h" base/logging is where DCHECK is defined, should I actually switch this include? https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:66: return std::string(data_message_.begin(), data_message_.end()); On 2016/06/09 22:08:35, Tim Song wrote: > Check if state_ == DATA_READY? This entire section has been rewritten. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:85: switch (GetControlCommand(packet)) { On 2016/06/09 22:08:34, Tim Song wrote: > Please refactor this switch to its own function. Done. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:87: if (state_ == State::RECEIVING_DATA) { On 2016/06/09 22:08:35, Tim Song wrote: > Shouldn't this be checking that state_ != EXCHANGING_CONTROLS? You should also > do a state transition here. Done. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:93: // TODO(jingxuy): what do I do with the 13 bytes of data? On 2016/06/09 22:08:34, Tim Song wrote: > I don't think you'll get any extra data. The packet that you receive doesn't > have to be of the maximum packet size. This is some uWeave stuff that all control packets are 20 bytes and that extra data could be tagged at the end. I was more asking Gustavo of this https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:96: if (state_ == State::RECEIVING_DATA) { On 2016/06/09 22:08:35, Tim Song wrote: > shouldn't this be checking that state_ != EXCHANGING_CONTROLS? You should also > do a state transition here. Done. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:102: // TODO(jingxuy): what do I do with the 15 bytes of data? On 2016/06/09 22:08:34, Tim Song wrote: > same here. Done. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:104: case ControlPacketCommand::CONNECTION_CLOSE: On 2016/06/09 22:08:35, Tim Song wrote: > check that the current state is valid. Done. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:114: case PacketType::DATA: On 2016/06/09 22:08:34, Tim Song wrote: > Also refactor this block into its own function. Done. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:116: if (state_ != State::EXCHANGING_CONTROLS) { On 2016/06/09 22:08:34, Tim Song wrote: > Shouldn't this state you check here be DATA_READY? Done. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:146: DCHECK_LT(index, packet.size()); On 2016/06/09 22:08:34, Tim Song wrote: > I don't think you should DCHECK this condition, as we are getting these packets > from the remote device. We should deal with invalid packets (state_ = ERROR?) > rather than crashing. This is an internal function. I'm just dchecking to make sure that user didn't call it with anything invalid. In the new version of code, control won't even reach here if the state is ERROR. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:179: // Bit determining whether the packet is first pacet is bit 3. On 2016/06/09 22:08:35, Tim Song wrote: > nit: s/pacet/packet/ Done. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/06/09 22:08:35, Tim Song wrote: > nit:2016 Done. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:38: EXCHANGING_CONTROLS = 0x00, On 2016/06/09 22:08:35, Tim Song wrote: > A better name for this state might be CONNECTING. I would also add a CONNECTED > state to distinguish from the DATA_READY state. Currently connecting and connected are the same state. I can change the name though. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:62: void ReceivePacket(const Packet& packet); On 2016/06/09 22:08:35, Tim Song wrote: > Perhaps you should return the current state as well, so the caller can do > something like: > > if (receiver_->ReceivePacket(packet) == State::DATA_READY) { > ProcessMessage(receiver_->GetDataMessage()); > } Done.
https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:38: EXCHANGING_CONTROLS = 0x00, On 2016/06/09 23:29:04, jingxuy wrote: > On 2016/06/09 22:08:35, Tim Song wrote: > > A better name for this state might be CONNECTING. I would also add a CONNECTED > > state to distinguish from the DATA_READY state. > > Currently connecting and connected are the same state. I can change the name > though. Shouldn't the "connected" state equivalent to receiving data? https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:12: namespace { The anonymous namespace should be above the proximity_auth one, not nested inside of it. https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:79: BluetoothLowEnergyWeavePacketReceiver::ReceivePacket(const Packet& packet) { General comment: You don't preserve messages if they aren't immediately received. For example, say you receive 2 data messages back-to-back that are 2 packets each. 1) Receive packet 1 of message 1. State = RECEIVING_DATA. 2) Receive packet 2 of message 1. State = DATA_READY. 3) No one has called GetDataMessage() yet. 4) Receive packet 1 of message 2. State = RECEIVING DATA. 5) First message is lost and was never read. You need to make sure that this class keeps track of previous messages which have not yet been retrieved. https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:126: AppendData(packet); You also need to check that it has data at all (packet.size() > 1). Same everywhere else you call AppendData(). https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:140: case PacketType::CONTROL: If we're receiving partial data, we shouldn't even receive a CONNECTION_CLOSE. Think about it: say we are in the middle of a data transfer with counters 0, 1, 2, and 3. But, midway through, we get a connection close packet with counter 4. That packet counter would be out of order, so an error would occur and this code would never be executed. https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:144: // Shouldn't be receiving request/response in the middle of a data DLOG something here and everywhere else where there's an unexpected error. https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:174: // TODO(jingxuy): what do I do with the 13 bytes of data? You should AppendData() and set state to DATA_READY. Same below. https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:193: reason_for_close_ = static_cast<ReasonForClose>(GetShortField(packet, 1)); What happens if the value passed isn't a ReasonForClose? Remember that you're receiving this from another (possibly buggy) client, so you can't assume that the data you're receiving is always correct. https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:49: void ResetReceiver(); Why is this function necessary? We should be creating a new receiver for each Connection. https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:67: BluetoothLowEnergyWeavePacketReceiver(); What do you think about passing an enum here about whether we are the GATT server or client? For example, we shouldn't be accepting a connection request if we are a client and we shouldn't be accepting a connection response if we are a server.
Also, same thing as your other CL: please improve the CL description.
https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:7: #include "base/logging.h" On 2016/06/09 23:29:03, jingxuy wrote: > On 2016/06/09 22:08:34, Tim Song wrote: > > Please use "components/proximity_auth/logging.h" > > base/logging is where DCHECK is defined, should I actually switch this include? Okay, it seems like you don't do any logging, so this should be fine. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:93: // TODO(jingxuy): what do I do with the 13 bytes of data? On 2016/06/09 23:29:03, jingxuy wrote: > On 2016/06/09 22:08:34, Tim Song wrote: > > I don't think you'll get any extra data. The packet that you receive doesn't > > have to be of the maximum packet size. > > This is some uWeave stuff that all control packets are 20 bytes and that extra > data could be tagged at the end. I was more asking Gustavo of this The Android and Mac implementations do not append anything after the control signal. I don't think you should worry about it for now. https://codereview.chromium.org/2053013002/diff/160001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/160001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:122: case PacketType::DATA: I would move this case to a new function (or merge it with ReceiveWithPartialData), because it is not correct to receive DATA packets when you are not connected, and likewise, it is not correct to receive CONNECTION_REQUEST and CONNECTION_RESPONSE messages when you expect data. https://codereview.chromium.org/2053013002/diff/160001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:173: packet_size_ = GetShortField(packet, 5); You will also want to transition to the DATA_READY state, or better yet a new CONNECTED state. https://codereview.chromium.org/2053013002/diff/160001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:185: packet_size_ = GetShortField(packet, 3); You will also want to transition to the DATA_READY state, or better yet a new CONNECTED state. https://codereview.chromium.org/2053013002/diff/160001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:231: uint8_t count = (packet[0] >> 4) & 7; Also check that packet.size() >= 1 before reading this value. https://codereview.chromium.org/2053013002/diff/160001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/160001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:49: void ResetReceiver(); It's usually simpler and less prone to error to create a new instance of this object, especially when we already have a factory.
https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:12: namespace { On 2016/06/09 23:59:46, Kyle Horimoto wrote: > The anonymous namespace should be above the proximity_auth one, not nested > inside of it. Done. https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:79: BluetoothLowEnergyWeavePacketReceiver::ReceivePacket(const Packet& packet) { On 2016/06/09 23:59:46, Kyle Horimoto wrote: > General comment: You don't preserve messages if they aren't immediately > received. For example, say you receive 2 data messages back-to-back that are 2 > packets each. > > 1) Receive packet 1 of message 1. State = RECEIVING_DATA. > 2) Receive packet 2 of message 1. State = DATA_READY. > 3) No one has called GetDataMessage() yet. > 4) Receive packet 1 of message 2. State = RECEIVING DATA. > 5) First message is lost and was never read. > > You need to make sure that this class keeps track of previous messages which > have not yet been retrieved. Acknowledged. https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:126: AppendData(packet); On 2016/06/09 23:59:46, Kyle Horimoto wrote: > You also need to check that it has data at all (packet.size() > 1). Same > everywhere else you call AppendData(). Acknowledged. https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:140: case PacketType::CONTROL: On 2016/06/09 23:59:46, Kyle Horimoto wrote: > If we're receiving partial data, we shouldn't even receive a CONNECTION_CLOSE. > Think about it: say we are in the middle of a data transfer with counters 0, 1, > 2, and 3. But, midway through, we get a connection close packet with counter 4. > That packet counter would be out of order, so an error would occur and this code > would never be executed. Pending response from Gustavo https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:174: // TODO(jingxuy): what do I do with the 13 bytes of data? On 2016/06/09 23:59:45, Kyle Horimoto wrote: > You should AppendData() and set state to DATA_READY. > > Same below. Pending response from Gustavo https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:193: reason_for_close_ = static_cast<ReasonForClose>(GetShortField(packet, 1)); On 2016/06/09 23:59:46, Kyle Horimoto wrote: > What happens if the value passed isn't a ReasonForClose? Remember that you're > receiving this from another (possibly buggy) client, so you can't assume that > the data you're receiving is always correct. Done. https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:49: void ResetReceiver(); On 2016/06/09 23:59:46, Kyle Horimoto wrote: > Why is this function necessary? We should be creating a new receiver for each > Connection. Done. https://codereview.chromium.org/2053013002/diff/140001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:67: BluetoothLowEnergyWeavePacketReceiver(); On 2016/06/09 23:59:46, Kyle Horimoto wrote: > What do you think about passing an enum here about whether we are the GATT > server or client? For example, we shouldn't be accepting a connection request if > we are a client and we shouldn't be accepting a connection response if we are a > server. I think I always frown upon passing in a boolean configuration parameter. I think if you want a separate server/client receiver, it would be better if the two classes inherit from the same base class but for the receiving control request/response.
https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:7: #include "base/logging.h" On 2016/06/10 21:25:21, Tim Song wrote: > On 2016/06/09 23:29:03, jingxuy wrote: > > On 2016/06/09 22:08:34, Tim Song wrote: > > > Please use "components/proximity_auth/logging.h" > > > > base/logging is where DCHECK is defined, should I actually switch this > include? > > Okay, it seems like you don't do any logging, so this should be fine. oh actually I need to do some logging and should I use the DLOG from base/logging or does proximity auth logging do something special https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:93: // TODO(jingxuy): what do I do with the 13 bytes of data? On 2016/06/10 21:25:21, Tim Song wrote: > On 2016/06/09 23:29:03, jingxuy wrote: > > On 2016/06/09 22:08:34, Tim Song wrote: > > > I don't think you'll get any extra data. The packet that you receive doesn't > > > have to be of the maximum packet size. > > > > This is some uWeave stuff that all control packets are 20 bytes and that extra > > data could be tagged at the end. I was more asking Gustavo of this > > The Android and Mac implementations do not append anything after the control > signal. I don't think you should worry about it for now. yah, I think worrying about it actually creates more problem that I might need another state. But I think the receiver should be more generalized to be able to deal with that data since the protocol allows it. https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:38: EXCHANGING_CONTROLS = 0x00, On 2016/06/09 23:59:45, Kyle Horimoto wrote: > On 2016/06/09 23:29:04, jingxuy wrote: > > On 2016/06/09 22:08:35, Tim Song wrote: > > > A better name for this state might be CONNECTING. I would also add a > CONNECTED > > > state to distinguish from the DATA_READY state. > > > > Currently connecting and connected are the same state. I can change the name > > though. > > Shouldn't the "connected" state equivalent to receiving data? I think what Tim has as connected is the equivalent of receiving data https://codereview.chromium.org/2053013002/diff/160001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/160001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:122: case PacketType::DATA: On 2016/06/10 21:25:21, Tim Song wrote: > I would move this case to a new function (or merge it with > ReceiveWithPartialData), because it is not correct to receive DATA packets when > you are not connected, and likewise, it is not correct to receive > CONNECTION_REQUEST and CONNECTION_RESPONSE messages when you expect data. Done. https://codereview.chromium.org/2053013002/diff/160001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:173: packet_size_ = GetShortField(packet, 5); On 2016/06/10 21:25:21, Tim Song wrote: > You will also want to transition to the DATA_READY state, or better yet a new > CONNECTED state. Done. https://codereview.chromium.org/2053013002/diff/160001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:185: packet_size_ = GetShortField(packet, 3); On 2016/06/10 21:25:21, Tim Song wrote: > You will also want to transition to the DATA_READY state, or better yet a new > CONNECTED state. Done. https://codereview.chromium.org/2053013002/diff/160001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:231: uint8_t count = (packet[0] >> 4) & 7; On 2016/06/10 21:25:21, Tim Song wrote: > Also check that packet.size() >= 1 before reading this value. Done. https://codereview.chromium.org/2053013002/diff/160001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/160001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:49: void ResetReceiver(); On 2016/06/10 21:25:21, Tim Song wrote: > It's usually simpler and less prone to error to create a new instance of this > object, especially when we already have a factory. Done.
Description was changed from ========== uWeave Receiver BUG=617238 ========== to ========== Weave Packet Generator This CL adds a packet receiver that respects the Weave.BLE protocol. BUG=617238 ==========
https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:7: #include "base/logging.h" On 2016/06/11 00:10:46, jingxuy wrote: > On 2016/06/10 21:25:21, Tim Song wrote: > > On 2016/06/09 23:29:03, jingxuy wrote: > > > On 2016/06/09 22:08:34, Tim Song wrote: > > > > Please use "components/proximity_auth/logging.h" > > > > > > base/logging is where DCHECK is defined, should I actually switch this > > include? > > > > Okay, it seems like you don't do any logging, so this should be fine. > > oh actually I need to do some logging and should I use the DLOG from > base/logging or does proximity auth logging do something special The proximity_auth specific logger PA_LOG is a wrapper over the regular logging library, so we can distinguish which logs are from our component. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:102: void BluetoothLowEnergyWeavePacketReceiver::ReceiveControlPacket( This should probably be called ReceiveConnectionPacket(). It shouldn't be valid to receive a CONNECTION_CLOSE control in this state. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:133: DCHECK(state_ == State::RECEIVING_DATA); Also check if state_ == State::DATA_READY https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:171: uint16_t max_version = GetShortField(packet, 3); You should validate that the packet is sufficiently long. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:190: packet_size_ = GetShortField(packet, 3); You should validate that the packet is sufficiently long.
First pass. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:171: uint16_t max_version = GetShortField(packet, 3); On 2016/06/11 00:39:46, Tim Song wrote: > You should validate that the packet is sufficiently long. +1. And you should move the error state if that's not the case. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:172: if (!(min_version == 1 && max_version == 1)) { This is not correct. As long as, min_version <= 1 <= max_version, you are fine. The version should only contain your supported version. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:177: // TODO(jingxuy): what do I do with the 13 bytes of data? If there is any data in the connection request payload, you should store it |data_message_| and move to State::DATA_READY, the owner of this class should then report that as the first received message. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:190: packet_size_ = GetShortField(packet, 3); On 2016/06/11 00:39:46, Tim Song wrote: > You should validate that the packet is sufficiently long. +1. And you should move the error state if that's not the case. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:192: // TODO(jingxuy): what do I do with the 15 bytes of data? If there is any data in the connection response payload, you should store it |data_message_| and move to State::DATA_READY, the owner of this class should then report that as the first received message. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:200: uint16_t reason = GetShortField(packet, 1); Check if the packet is large enough. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:208: reason_for_close_ = static_cast<ReasonForClose>(GetShortField(packet, 1)); Check if the packet is large enough. But you should also accept the cases where the connection close packet contains no payload, as this the case for the current implementations. Consider and UNKNOWN_ERROR in this case. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:44: }; When this class detect an error, e.g. wrong packet counter or version not supported, you'll have to send a connection close packet to the other peer containing the reason for close. So, when you are in the Error state, you will need an appropriated reason for close. You should probably add a new |GetReasonForClose()| method, and change the name of method below to |GetReceivedReasonForClose()|. Also, add the corresponding |reason_for_close_| and |received_reason_for_close_|.
Can you please add a unit test? Also, can you please improve the CL title/description? Thanks!
Description was changed from ========== Weave Packet Generator This CL adds a packet receiver that respects the Weave.BLE protocol. BUG=617238 ========== to ========== Weave Packet Generator This CL adds a packet receiver that respects the uWeave.BLE protocol. The packet receiver expects this chain of events: 1 control_request/response -> 0+ data packets -> 0 or 1 control_close. The packet receiver keeps track of the receiving state and will jump to ERROR state when anything unexpected happens. (receiving packets out of order, invalid packet content, etc) The expected user of packet receiver should feed a packet into receiver, check the state, and take appropriate action. If actions are not taken in the appropriate state, the receiver will crash. Data message should be retrieved promptly when the state reaches DATA_READY or else the data will be lost when the next packet arrives. Design Doc for ProximityAuth uWeave Migration: https://docs.google.com/a/google.com/document/d/1HJ1fipxsNQwIxmKs-5_6TjroCAcs... BUG=617238 ==========
https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:102: void BluetoothLowEnergyWeavePacketReceiver::ReceiveControlPacket( On 2016/06/11 00:39:46, Tim Song wrote: > This should probably be called ReceiveConnectionPacket(). It shouldn't be valid > to receive a CONNECTION_CLOSE control in this state. Done. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:133: DCHECK(state_ == State::RECEIVING_DATA); On 2016/06/11 00:39:46, Tim Song wrote: > Also check if state_ == State::DATA_READY Done. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:171: uint16_t max_version = GetShortField(packet, 3); On 2016/06/13 17:16:40, sacomoto wrote: > On 2016/06/11 00:39:46, Tim Song wrote: > > You should validate that the packet is sufficiently long. > > +1. > > And you should move the error state if that's not the case. Done. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:172: if (!(min_version == 1 && max_version == 1)) { On 2016/06/13 17:16:40, sacomoto wrote: > This is not correct. > > As long as, min_version <= 1 <= max_version, you are fine. The version should > only contain your supported version. Done. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:177: // TODO(jingxuy): what do I do with the 13 bytes of data? On 2016/06/13 17:16:40, sacomoto wrote: > If there is any data in the connection request payload, you should store it > |data_message_| and move to State::DATA_READY, the owner of this class should > then report that as the first received message. Done. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:190: packet_size_ = GetShortField(packet, 3); On 2016/06/13 17:16:40, sacomoto wrote: > On 2016/06/11 00:39:46, Tim Song wrote: > > You should validate that the packet is sufficiently long. > > +1. > > And you should move the error state if that's not the case. Done. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:192: // TODO(jingxuy): what do I do with the 15 bytes of data? On 2016/06/13 17:16:40, sacomoto wrote: > If there is any data in the connection response payload, you should store it > |data_message_| and move to State::DATA_READY, the owner of this class should > then report that as the first received message. Done. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:200: uint16_t reason = GetShortField(packet, 1); On 2016/06/13 17:16:40, sacomoto wrote: > Check if the packet is large enough. Done. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:208: reason_for_close_ = static_cast<ReasonForClose>(GetShortField(packet, 1)); On 2016/06/13 17:16:40, sacomoto wrote: > Check if the packet is large enough. > > But you should also accept the cases where the connection close packet contains > no payload, as this the case for the current implementations. Consider and > UNKNOWN_ERROR in this case. I don't really understand the comment. Should I not jump to State::ERROR upon receiving a short close packet? Should I invent the error as UNKNOWN_ERROR? https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:44: }; On 2016/06/13 17:16:40, sacomoto wrote: > When this class detect an error, e.g. wrong packet counter or version not > supported, you'll have to send a connection close packet to the other peer > containing the reason for close. > > So, when you are in the Error state, you will need an appropriated reason for > close. > > You should probably add a new |GetReasonForClose()| method, and change the name > of method below to |GetReceivedReasonForClose()|. Also, add the corresponding > |reason_for_close_| and |received_reason_for_close_|. Done.
https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:208: reason_for_close_ = static_cast<ReasonForClose>(GetShortField(packet, 1)); On 2016/06/16 08:42:28, jingxuy wrote: > On 2016/06/13 17:16:40, sacomoto wrote: > > Check if the packet is large enough. > > > > But you should also accept the cases where the connection close packet > contains > > no payload, as this the case for the current implementations. Consider and > > UNKNOWN_ERROR in this case. > > I don't really understand the comment. Should I not jump to State::ERROR upon > receiving a short close packet? Should I invent the error as UNKNOWN_ERROR? I was referring to the current iOS and Android implementations that do not include the reason for close in the payload. The connection close in those cases will only contain a single byte. https://codereview.chromium.org/2053013002/diff/280001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/280001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:291: if (packet.size() < kMinConnectionCloseSize || The current iOS and Android implementations do not include the reason for close in the payload. So, you should probably add if (packet.size() == kLegacyConnectionCloseSize) { state_ = State::CONNECTION_CLOSED; reason_for_close_ = ReasonForClose::UNKNOWN_ERROR; return; } before this if.
https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:7: #include "base/logging.h" On 2016/06/11 00:39:45, Tim Song wrote: > On 2016/06/11 00:10:46, jingxuy wrote: > > On 2016/06/10 21:25:21, Tim Song wrote: > > > On 2016/06/09 23:29:03, jingxuy wrote: > > > > On 2016/06/09 22:08:34, Tim Song wrote: > > > > > Please use "components/proximity_auth/logging.h" > > > > > > > > base/logging is where DCHECK is defined, should I actually switch this > > > include? > > > > > > Okay, it seems like you don't do any logging, so this should be fine. > > > > oh actually I need to do some logging and should I use the DLOG from > > base/logging or does proximity auth logging do something special > > The proximity_auth specific logger PA_LOG is a wrapper over the regular logging > library, so we can distinguish which logs are from our component. Done. https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/220001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:208: reason_for_close_ = static_cast<ReasonForClose>(GetShortField(packet, 1)); On 2016/06/17 15:31:07, sacomoto wrote: > On 2016/06/16 08:42:28, jingxuy wrote: > > On 2016/06/13 17:16:40, sacomoto wrote: > > > Check if the packet is large enough. > > > > > > But you should also accept the cases where the connection close packet > > contains > > > no payload, as this the case for the current implementations. Consider and > > > UNKNOWN_ERROR in this case. > > > > I don't really understand the comment. Should I not jump to State::ERROR upon > > receiving a short close packet? Should I invent the error as UNKNOWN_ERROR? > > I was referring to the current iOS and Android implementations that do not > include the reason for close in the payload. The connection close in those cases > will only contain a single byte. Done. https://codereview.chromium.org/2053013002/diff/280001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/280001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:291: if (packet.size() < kMinConnectionCloseSize || On 2016/06/17 15:31:07, sacomoto wrote: > The current iOS and Android implementations do not include the reason for close > in the payload. > > So, you should probably add > > if (packet.size() == kLegacyConnectionCloseSize) { > state_ = State::CONNECTION_CLOSED; > reason_for_close_ = ReasonForClose::UNKNOWN_ERROR; > return; > } > > before this if. Done.
Please update your tests with better documentation. It's too hard for me to review your test without comments letting me know what the hex values, etc. mean. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:18: const uint16_t kMinPacketSize = 20; nit: Please word this constant differently. It's not the minimum packet size - it's the minimum value for the maximum packet size. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:71: // max_packet_size_ is well defined in every state. It doesn't seem like this is true. max_packet_size_ isn't initialized to any value. I think you should initialize it to 0. However, please make a new constant for that value instead of using 0 directly. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:93: Error("Empty packet is not a valid uWeave packet.", For these error messages, please make them declarative about what happened. You can add a secondary sentence if you need more context. For example, this could be "Received an empty packet, which is not a valid in the uWeave protocol." Another example: "Server couldn't process connection response." ==> "Received a connection response packet; this is invalid when in server mode." https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:99: VerifyPacketCounter(packet); If the packet counter is not verified correctly, you shouldn't continue. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:123: // Receiving an message in connection close or error state is not valid. This should just be NOTREACHED(); We shouldn't ever get to this point. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:174: bool expect_first_packet) { There should be no need to pass this parameter. We can already derive this value based on state_. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:193: } else if (expect_first_packet ^ IsFirstDataPacket(packet)) { Why xor? If we expect that this is the first packet and it is the first packet, then we would be XORing true and true, which would produce false. You should use != instead. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:226: // Packet size of 0 means the server can observe the ATT_MTU and selecte an nit: s/selecte/select/ https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:226: // Packet size of 0 means the server can observe the ATT_MTU and selecte an Please use a constant instead of using 0 directly. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:266: Error("Server must support at least 20 bytes per packet.", nit: Don't hard-code 20 in your error message. Use kMinPacketSize. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:334: // packet[byte_offset + 1] is the upper byte and packet[byte_offset] is the As I mentioned in https://codereview.chromium.org/2031953003/, please use ntohs() and/or htons(). We can't assume endianness. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:43: CONNECTING = 0x00, Please add descriptions to each state which make it clear what each one means. For example, if someone who has never seen this file before looks at this enum, will he/she be able to understand what "waiting" means? Will he/she be able to understand what the difference between "receiving data" and "data ready" mean? Please make it very clear exactly what circumstances apply to each state. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:58: // Return the packet size that the receiver parsed out of request/response. Please list the conditions under which this can be called. For example, you couldn't call this before the connection has been established. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:62: BluetoothLowEnergyWeavePacketGenerator::ReasonForClose GetReasonForClose(); Please add documentation to the GetReason{For|To}Close() functions. It is unclear to a reader what the difference between these two is, and it is also unclear what exactly is returned by these. For example, you could say something like: Returns the reason that the connection was closed by the remote device. The reason is provided as part of a "connection close" control packet received via ReceivePacket(). This function should only be called when the state is CONNECTION_CLOSED, which occurs after the "connection close" packet is received. If this function is called when the state is not CONNECTION_CLOSED, an error is thrown. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:71: std::string GetDataMessage(); Likewise, please document this function. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:94: bool IsLowerTwoBitsCleared(const Packet& packet); nit: s/Is/Are/ https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:96: void Error( nit: s/Error/TransitionToErrorState/ https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:112: uint32_t packet_number_; nit: How about next_packet_number_ instead? Otherwise, it could be confusing whether this was the last value sent or the next value to use. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:43: // TODO(jingxuy): put comments on hex values Please also describe what the other values are. For example, below you have {0x80, 1, 0, 1, 0, 0, 0}, but it is unclear what any of these values mean. People who read your code should be able to tell what this means without cross-referencing the spec. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:48: std::vector<uint8_t> p1{0x18, 'a', 'b'}; If a packet is NOT the last packet in a message, it should be full (i.e., its length should be the maximum allowed length). While it is also good to handle this situation gracefully, this is more of an edge case than the base case which should be tested. You should write a small test which ensures that these types of packets are handled correctly, but the majority of your tests should test what we actually expect to happen in practice.
https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:18: const uint16_t kMinPacketSize = 20; On 2016/06/20 23:25:40, Kyle Horimoto wrote: > nit: Please word this constant differently. It's not the minimum packet size - > it's the minimum value for the maximum packet size. Done. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:71: // max_packet_size_ is well defined in every state. On 2016/06/20 23:25:40, Kyle Horimoto wrote: > It doesn't seem like this is true. max_packet_size_ isn't initialized to any > value. I think you should initialize it to 0. However, please make a new > constant for that value instead of using 0 directly. It's default to 20 in the SetMaxPacketSize in the constructor https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:93: Error("Empty packet is not a valid uWeave packet.", On 2016/06/20 23:25:39, Kyle Horimoto wrote: > For these error messages, please make them declarative about what happened. You > can add a secondary sentence if you need more context. > > For example, this could be "Received an empty packet, which is not a valid in > the uWeave protocol." > > Another example: "Server couldn't process connection response." ==> "Received a > connection response packet; this is invalid when in server mode." Done. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:99: VerifyPacketCounter(packet); On 2016/06/20 23:25:40, Kyle Horimoto wrote: > If the packet counter is not verified correctly, you shouldn't continue. this is caught on the case ERROR: https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:123: // Receiving an message in connection close or error state is not valid. On 2016/06/20 23:25:39, Kyle Horimoto wrote: > This should just be NOTREACHED(); > > We shouldn't ever get to this point. Done. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:174: bool expect_first_packet) { On 2016/06/20 23:25:39, Kyle Horimoto wrote: > There should be no need to pass this parameter. We can already derive this value > based on state_. I was trying to avoid switching on the state twice. I think the switch(state) should have divided the action by state. I didn't want the helper function to also switch on state again. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:193: } else if (expect_first_packet ^ IsFirstDataPacket(packet)) { On 2016/06/20 23:25:39, Kyle Horimoto wrote: > Why xor? If we expect that this is the first packet and it is the first packet, > then we would be XORing true and true, which would produce false. You should use > != instead. I think true and true are suppose to be false for that statement since it's not an errorneous state because expect and isfirst are the same. but oxr is actually equivalent to one != other, so I'll change it. it was oxr because it made more sense in code I modified. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:226: // Packet size of 0 means the server can observe the ATT_MTU and selecte an On 2016/06/20 23:25:39, Kyle Horimoto wrote: > nit: s/selecte/select/ Done. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:266: Error("Server must support at least 20 bytes per packet.", On 2016/06/20 23:25:39, Kyle Horimoto wrote: > nit: Don't hard-code 20 in your error message. Use kMinPacketSize. Done. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:334: // packet[byte_offset + 1] is the upper byte and packet[byte_offset] is the On 2016/06/20 23:25:39, Kyle Horimoto wrote: > As I mentioned in https://codereview.chromium.org/2031953003/, please use > ntohs() and/or htons(). We can't assume endianness. I think Gustavo already replied and resolved this. The endianness of the packet is guaranteed to be network endiannness and I am returning an uint16 by shifting and not writing to memory directly. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:43: CONNECTING = 0x00, On 2016/06/20 23:25:40, Kyle Horimoto wrote: > Please add descriptions to each state which make it clear what each one means. > For example, if someone who has never seen this file before looks at this enum, > will he/she be able to understand what "waiting" means? Will he/she be able to > understand what the difference between "receiving data" and "data ready" mean? > Please make it very clear exactly what circumstances apply to each state. It was a TODO that I didn't get to. Done. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:58: // Return the packet size that the receiver parsed out of request/response. On 2016/06/20 23:25:40, Kyle Horimoto wrote: > Please list the conditions under which this can be called. For example, you > couldn't call this before the connection has been established. This is always valid, see comment in cc file https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:62: BluetoothLowEnergyWeavePacketGenerator::ReasonForClose GetReasonForClose(); On 2016/06/20 23:25:40, Kyle Horimoto wrote: > Please add documentation to the GetReason{For|To}Close() functions. It is > unclear to a reader what the difference between these two is, and it is also > unclear what exactly is returned by these. > > For example, you could say something like: > > Returns the reason that the connection was closed by the remote device. The > reason is provided as part of a "connection close" control packet received via > ReceivePacket(). This function should only be called when the state is > CONNECTION_CLOSED, which occurs after the "connection close" packet is received. > If this function is called when the state is not CONNECTION_CLOSED, an error is > thrown. Done. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:94: bool IsLowerTwoBitsCleared(const Packet& packet); On 2016/06/20 23:25:40, Kyle Horimoto wrote: > nit: s/Is/Are/ Done. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:96: void Error( On 2016/06/20 23:25:40, Kyle Horimoto wrote: > nit: s/Error/TransitionToErrorState/ I changed it to ThrowError https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:112: uint32_t packet_number_; On 2016/06/20 23:25:40, Kyle Horimoto wrote: > nit: How about next_packet_number_ instead? Otherwise, it could be confusing > whether this was the last value sent or the next value to use. Done. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:43: // TODO(jingxuy): put comments on hex values On 2016/06/20 23:25:40, Kyle Horimoto wrote: > Please also describe what the other values are. For example, below you have > {0x80, 1, 0, 1, 0, 0, 0}, but it is unclear what any of these values mean. > People who read your code should be able to tell what this means without > cross-referencing the spec. Done. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:48: std::vector<uint8_t> p1{0x18, 'a', 'b'}; On 2016/06/20 23:25:40, Kyle Horimoto wrote: > If a packet is NOT the last packet in a message, it should be full (i.e., its > length should be the maximum allowed length). > > While it is also good to handle this situation gracefully, this is more of an > edge case than the base case which should be tested. You should write a small > test which ensures that these types of packets are handled correctly, but the > majority of your tests should test what we actually expect to happen in > practice. Done.
I will finish reviewing the tests tomorrow. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:18: const uint16_t kMinPacketSize = 20; On 2016/06/21 01:19:14, jingxuy wrote: > On 2016/06/20 23:25:40, Kyle Horimoto wrote: > > nit: Please word this constant differently. It's not the minimum packet size - > > it's the minimum value for the maximum packet size. > > Done. Haha, MinMax is also very confusing. Can you think of another name? https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:99: VerifyPacketCounter(packet); On 2016/06/21 01:19:14, jingxuy wrote: > On 2016/06/20 23:25:40, Kyle Horimoto wrote: > > If the packet counter is not verified correctly, you shouldn't continue. > > this is caught on the case ERROR: I realize that, but that case will cause "Received message in ERROR state." to be printed. You never received a message in ERROR state, so this log will be incorrect and could cause confusion to others while they are debugging. You should probably have VerifyPacketCounter() return a boolean of whether the verification succeeded or not and use that. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:174: bool expect_first_packet) { On 2016/06/21 01:19:14, jingxuy wrote: > On 2016/06/20 23:25:39, Kyle Horimoto wrote: > > There should be no need to pass this parameter. We can already derive this > value > > based on state_. > > I was trying to avoid switching on the state twice. I think the switch(state) > should have divided the action by state. I didn't want the helper function to > also switch on state again. I'm not sure what you mean. You can just use: bool expect_first_packet = state_ == State::RECEIVING_DATA; Make sure that you use that after the DCHECK() lines to ensure that you have eliminated other possible error conditions by that point. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:266: Error("Server must support at least 20 bytes per packet.", On 2016/06/21 01:19:14, jingxuy wrote: > On 2016/06/20 23:25:39, Kyle Horimoto wrote: > > nit: Don't hard-code 20 in your error message. Use kMinPacketSize. > > Done. Use a std::stringstream instead of this. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:334: // packet[byte_offset + 1] is the upper byte and packet[byte_offset] is the On 2016/06/21 01:19:14, jingxuy wrote: > On 2016/06/20 23:25:39, Kyle Horimoto wrote: > > As I mentioned in https://codereview.chromium.org/2031953003/, please use > > ntohs() and/or htons(). We can't assume endianness. > > I think Gustavo already replied and resolved this. The endianness of the packet > is guaranteed to be network endiannness and I am returning an uint16 by shifting > and not writing to memory directly. You are correct that the packet is guaranteed to be in network endianness, but you cannot assume that your host endianness is the same as the network endianness. If the Chromebook running this code has the opposite endianness, the wrong value will be produced. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:96: void Error( On 2016/06/21 01:19:14, jingxuy wrote: > On 2016/06/20 23:25:40, Kyle Horimoto wrote: > > nit: s/Error/TransitionToErrorState/ > > I changed it to ThrowError This function doesn't throw an error, though. Please use another name. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:43: // TODO(jingxuy): put comments on hex values On 2016/06/21 01:19:15, jingxuy wrote: > On 2016/06/20 23:25:40, Kyle Horimoto wrote: > > Please also describe what the other values are. For example, below you have > > {0x80, 1, 0, 1, 0, 0, 0}, but it is unclear what any of these values mean. > > People who read your code should be able to tell what this means without > > cross-referencing the spec. > > Done. You still need to use constants for the last two 0's. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:63: // The connection hasn't been estabalish. Accept a CONNECTION_REQUEST if the s/estabalish/established/ https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:66: // The state will transition to WAITING after a request/respone if they s/respone/response/ https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:68: // request/resposne have extra data since the extra data is treated as a s/resposne/response/ https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:72: // to arrive. Will accept all but request/response packets. The first data connection request/response https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc (right): https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:60: kControlRequestHeader, kWeaveVersion, 0, kWeaveVersion, 0, 0, 0}; Isn't network order big endian? Shouldn't it be 0 first, then kWeaveVersion?
https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:174: bool expect_first_packet) { On 2016/06/21 02:10:26, Kyle Horimoto wrote: > On 2016/06/21 01:19:14, jingxuy wrote: > > On 2016/06/20 23:25:39, Kyle Horimoto wrote: > > > There should be no need to pass this parameter. We can already derive this > > value > > > based on state_. > > > > I was trying to avoid switching on the state twice. I think the switch(state) > > should have divided the action by state. I didn't want the helper function to > > also switch on state again. > > I'm not sure what you mean. You can just use: > > bool expect_first_packet = state_ == State::RECEIVING_DATA; > > Make sure that you use that after the DCHECK() lines to ensure that you have > eliminated other possible error conditions by that point. It's more of a code style issue. I was trying to test all the state problems within the switch(state) in ReceivePacket. This function is intended to be not state cognisant. Instead of checking the states here, the state is checked in the switch statement and passed in here. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:334: // packet[byte_offset + 1] is the upper byte and packet[byte_offset] is the On 2016/06/21 02:10:26, Kyle Horimoto wrote: > On 2016/06/21 01:19:14, jingxuy wrote: > > On 2016/06/20 23:25:39, Kyle Horimoto wrote: > > > As I mentioned in https://codereview.chromium.org/2031953003/, please use > > > ntohs() and/or htons(). We can't assume endianness. > > > > I think Gustavo already replied and resolved this. The endianness of the > packet > > is guaranteed to be network endiannness and I am returning an uint16 by > shifting > > and not writing to memory directly. > > You are correct that the packet is guaranteed to be in network endianness, but > you cannot assume that your host endianness is the same as the network > endianness. If the Chromebook running this code has the opposite endianness, the > wrong value will be produced. I was referring to Gustavo's comment on writing or not writing to memory directly. The endianness only matter if you are addressing memory directly. For instance you have uint16_t val and have uint8_t* val_ptr = (uint8_t *)(&uint16_t); then you write to val_ptr[0] and val_ptr[1]. This is accessing bytes in memory. However if you have uint16_t val = upper << 8 | lower, this is not writing to memory, it's performing a mathematical calculation which would be upper * 2^8 + lower, which is guaranteed to be correct or otherwise a computer breaks down.
https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:18: const uint16_t kMinPacketSize = 20; On 2016/06/21 02:10:26, Kyle Horimoto wrote: > On 2016/06/21 01:19:14, jingxuy wrote: > > On 2016/06/20 23:25:40, Kyle Horimoto wrote: > > > nit: Please word this constant differently. It's not the minimum packet size > - > > > it's the minimum value for the maximum packet size. > > > > Done. > > Haha, MinMax is also very confusing. Can you think of another name? Ping. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:99: VerifyPacketCounter(packet); On 2016/06/21 02:10:26, Kyle Horimoto wrote: > On 2016/06/21 01:19:14, jingxuy wrote: > > On 2016/06/20 23:25:40, Kyle Horimoto wrote: > > > If the packet counter is not verified correctly, you shouldn't continue. > > > > this is caught on the case ERROR: > > I realize that, but that case will cause "Received message in ERROR state." to > be printed. You never received a message in ERROR state, so this log will be > incorrect and could cause confusion to others while they are debugging. You > should probably have VerifyPacketCounter() return a boolean of whether the > verification succeeded or not and use that. Ping. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:174: bool expect_first_packet) { On 2016/06/21 06:19:26, jingxuy wrote: > On 2016/06/21 02:10:26, Kyle Horimoto wrote: > > On 2016/06/21 01:19:14, jingxuy wrote: > > > On 2016/06/20 23:25:39, Kyle Horimoto wrote: > > > > There should be no need to pass this parameter. We can already derive this > > > value > > > > based on state_. > > > > > > I was trying to avoid switching on the state twice. I think the > switch(state) > > > should have divided the action by state. I didn't want the helper function > to > > > also switch on state again. > > > > I'm not sure what you mean. You can just use: > > > > bool expect_first_packet = state_ == State::RECEIVING_DATA; > > > > Make sure that you use that after the DCHECK() lines to ensure that you have > > eliminated other possible error conditions by that point. > > It's more of a code style issue. I was trying to test all the state problems > within the switch(state) in ReceivePacket. This function is intended to be not > state cognisant. Instead of checking the states here, the state is checked in > the switch statement and passed in here. In general, you should strive to make functions encapsulate as much logic as you can so that clients of those functions do not have to know about the internals of those functions in order to call them. Of course, this isn't always possible, but it definitely is here. Regarding your comment about this function not being "state cognizant," it IS state cognizant: 1) The DCHECK() at the top checks that you are in the right state. 2) You call ReceiveConnectionClose(), which requires that you are in a specific state to call it. 3) You call AppendData(), which requires that you are in a specific state to call it. 4) You modify state_ internally. So, since your DCHECK() essentially just checks that the expect_first_packet value you passed in was correct, I think it makes more sense to just compute it internally. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:266: Error("Server must support at least 20 bytes per packet.", On 2016/06/21 02:10:26, Kyle Horimoto wrote: > On 2016/06/21 01:19:14, jingxuy wrote: > > On 2016/06/20 23:25:39, Kyle Horimoto wrote: > > > nit: Don't hard-code 20 in your error message. Use kMinPacketSize. > > > > Done. > > Use a std::stringstream instead of this. Ping. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:334: // packet[byte_offset + 1] is the upper byte and packet[byte_offset] is the On 2016/06/21 06:19:26, jingxuy wrote: > On 2016/06/21 02:10:26, Kyle Horimoto wrote: > > On 2016/06/21 01:19:14, jingxuy wrote: > > > On 2016/06/20 23:25:39, Kyle Horimoto wrote: > > > > As I mentioned in https://codereview.chromium.org/2031953003/, please use > > > > ntohs() and/or htons(). We can't assume endianness. > > > > > > I think Gustavo already replied and resolved this. The endianness of the > > packet > > > is guaranteed to be network endiannness and I am returning an uint16 by > > shifting > > > and not writing to memory directly. > > > > You are correct that the packet is guaranteed to be in network endianness, but > > you cannot assume that your host endianness is the same as the network > > endianness. If the Chromebook running this code has the opposite endianness, > the > > wrong value will be produced. > > I was referring to Gustavo's comment on writing or not writing to memory > directly. > The endianness only matter if you are addressing memory directly. > For instance you have uint16_t val and have uint8_t* val_ptr = (uint8_t > *)(&uint16_t); > then you write to val_ptr[0] and val_ptr[1]. This is accessing bytes in memory. > However if you have uint16_t val = upper << 8 | lower, this is not writing to > memory, > it's performing a mathematical calculation which would be upper * 2^8 + lower, > which > is guaranteed to be correct or otherwise a computer breaks down. I think the misunderstanding is that the direct 2-byte value that we receive is not necessarily encoded in the same way that your machine encodes things. It might be helpful to use an example. Pretend instead that instead of endianness, we are dealing with encrypting/decrypting a value. A 2-byte value is encrypted in the network, and we have received it. Now, we need to decrypt it into a value that our computer understands. To do so, we would have to run some special decrypt() function that takes in the network-encrypted 2-byte value and returns the decrypted 2-byte value in order for your machine to use it the right way. Now, suppose that the encryption technique used is that you just swap the two bytes, so encrypt(0xff00) returns 0x00ff, and decrypt(0x00ff) returns 0xff00 again. This is exactly what happens in this endianness situation. However, you don't necessarily know if the endianness on the network is the same as on the machine, so on machines with the same endianness as the network htons() and ntohs() just return the same value that they were passed. That's why you can't assume endianness and must use these functions. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:58: // Return the packet size that the receiver parsed out of request/response. On 2016/06/21 01:19:15, jingxuy wrote: > On 2016/06/20 23:25:40, Kyle Horimoto wrote: > > Please list the conditions under which this can be called. For example, you > > couldn't call this before the connection has been established. > > This is always valid, see comment in cc file Readers of this class should only need to look at the .h file to glean this information. Please place it in the .h file. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:96: void Error( On 2016/06/21 02:10:26, Kyle Horimoto wrote: > On 2016/06/21 01:19:14, jingxuy wrote: > > On 2016/06/20 23:25:40, Kyle Horimoto wrote: > > > nit: s/Error/TransitionToErrorState/ > > > > I changed it to ThrowError > > This function doesn't throw an error, though. Please use another name. Ping. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:43: // TODO(jingxuy): put comments on hex values On 2016/06/21 02:10:27, Kyle Horimoto wrote: > On 2016/06/21 01:19:15, jingxuy wrote: > > On 2016/06/20 23:25:40, Kyle Horimoto wrote: > > > Please also describe what the other values are. For example, below you have > > > {0x80, 1, 0, 1, 0, 0, 0}, but it is unclear what any of these values mean. > > > People who read your code should be able to tell what this means without > > > cross-referencing the spec. > > > > Done. > > You still need to use constants for the last two 0's. Ping. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:63: // The connection hasn't been estabalish. Accept a CONNECTION_REQUEST if the On 2016/06/21 02:10:27, Kyle Horimoto wrote: > s/estabalish/established/ Ping. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:66: // The state will transition to WAITING after a request/respone if they On 2016/06/21 02:10:27, Kyle Horimoto wrote: > s/respone/response/ Ping. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:68: // request/resposne have extra data since the extra data is treated as a On 2016/06/21 02:10:27, Kyle Horimoto wrote: > s/resposne/response/ Ping. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:72: // to arrive. Will accept all but request/response packets. The first data On 2016/06/21 02:10:27, Kyle Horimoto wrote: > connection request/response Ping. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc (right): https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:60: kControlRequestHeader, kWeaveVersion, 0, kWeaveVersion, 0, 0, 0}; On 2016/06/21 02:10:27, Kyle Horimoto wrote: > Isn't network order big endian? Shouldn't it be 0 first, then kWeaveVersion? Ping. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:113: WellBehavingServerPacketsWithControlDataTest) { Also test: *when less than the full 13 bytes of control data are sent *the error condition where >13 bytes are sent. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:186: EXPECT_EQ((uint32_t)30, receiver->GetMaxPacketSize()); For this test, can you actually try passing a packet which is the max size? Thanks! https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:212: WellBehavingClientPacketsWithControlDataTest) { Also test: *when the full 15 bytes of control data are sent *the error condition where >15 bytes are sent. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:305: TEST_F(ProximityAuthBluetoothLowEnergyWeavePacketReceiverTest, nit: For this test and the ones below, please just test one thing per test. Break each of these sub-tests out into its own top-level test. Thanks! https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:306: InappropriateConnectingTypeTest) { Also test that if a packet is received after we've reached an error state that we remain in an error state. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:410: ; nit: Remove ; https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:412: // First bit is not set when it's supposed to be with last bit not set. I'm not sure I understand this - can you explain? https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:506: EXPECT_EQ(ReasonForClose::UNKNOWN_ERROR, receiver->GetReasonToClose()); Let's make error values for these cases - it seems a lot of them have an "unknown error" value. I realize that you're just reusing the error values for the connection close packet, but since this is a class that encapsulates this logic, we can create our own values.
https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:18: const uint16_t kMinPacketSize = 20; On 2016/06/21 18:13:37, Kyle Horimoto wrote: > On 2016/06/21 02:10:26, Kyle Horimoto wrote: > > On 2016/06/21 01:19:14, jingxuy wrote: > > > On 2016/06/20 23:25:40, Kyle Horimoto wrote: > > > > nit: Please word this constant differently. It's not the minimum packet > size > > - > > > > it's the minimum value for the maximum packet size. > > > > > > Done. > > > > Haha, MinMax is also very confusing. Can you think of another name? > > Ping. Done. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:99: VerifyPacketCounter(packet); On 2016/06/21 02:10:26, Kyle Horimoto wrote: > On 2016/06/21 01:19:14, jingxuy wrote: > > On 2016/06/20 23:25:40, Kyle Horimoto wrote: > > > If the packet counter is not verified correctly, you shouldn't continue. > > > > this is caught on the case ERROR: > > I realize that, but that case will cause "Received message in ERROR state." to > be printed. You never received a message in ERROR state, so this log will be > incorrect and could cause confusion to others while they are debugging. You > should probably have VerifyPacketCounter() return a boolean of whether the > verification succeeded or not and use that. Done. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:174: bool expect_first_packet) { On 2016/06/21 18:13:37, Kyle Horimoto wrote: > On 2016/06/21 06:19:26, jingxuy wrote: > > On 2016/06/21 02:10:26, Kyle Horimoto wrote: > > > On 2016/06/21 01:19:14, jingxuy wrote: > > > > On 2016/06/20 23:25:39, Kyle Horimoto wrote: > > > > > There should be no need to pass this parameter. We can already derive > this > > > > value > > > > > based on state_. > > > > > > > > I was trying to avoid switching on the state twice. I think the > > switch(state) > > > > should have divided the action by state. I didn't want the helper function > > to > > > > also switch on state again. > > > > > > I'm not sure what you mean. You can just use: > > > > > > bool expect_first_packet = state_ == State::RECEIVING_DATA; > > > > > > Make sure that you use that after the DCHECK() lines to ensure that you have > > > eliminated other possible error conditions by that point. > > > > It's more of a code style issue. I was trying to test all the state problems > > within the switch(state) in ReceivePacket. This function is intended to be not > > state cognisant. Instead of checking the states here, the state is checked in > > the switch statement and passed in here. > > In general, you should strive to make functions encapsulate as much logic as you > can so that clients of those functions do not have to know about the internals > of those functions in order to call them. Of course, this isn't always possible, > but it definitely is here. > > Regarding your comment about this function not being "state cognizant," it IS > state cognizant: > 1) The DCHECK() at the top checks that you are in the right state. > 2) You call ReceiveConnectionClose(), which requires that you are in a specific > state to call it. > 3) You call AppendData(), which requires that you are in a specific state to > call it. > 4) You modify state_ internally. > > So, since your DCHECK() essentially just checks that the expect_first_packet > value you passed in was correct, I think it makes more sense to just compute it > internally. I don't really think DCHECK count as part of the logic really. And I don't really think it's good practice to switch on state twice in a state machine. But I will change the implementation. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:266: Error("Server must support at least 20 bytes per packet.", On 2016/06/21 18:13:37, Kyle Horimoto wrote: > On 2016/06/21 02:10:26, Kyle Horimoto wrote: > > On 2016/06/21 01:19:14, jingxuy wrote: > > > On 2016/06/20 23:25:39, Kyle Horimoto wrote: > > > > nit: Don't hard-code 20 in your error message. Use kMinPacketSize. > > > > > > Done. > > > > Use a std::stringstream instead of this. > > Ping. Done. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:334: // packet[byte_offset + 1] is the upper byte and packet[byte_offset] is the On 2016/06/21 18:13:37, Kyle Horimoto wrote: > On 2016/06/21 06:19:26, jingxuy wrote: > > On 2016/06/21 02:10:26, Kyle Horimoto wrote: > > > On 2016/06/21 01:19:14, jingxuy wrote: > > > > On 2016/06/20 23:25:39, Kyle Horimoto wrote: > > > > > As I mentioned in https://codereview.chromium.org/2031953003/, please > use > > > > > ntohs() and/or htons(). We can't assume endianness. > > > > > > > > I think Gustavo already replied and resolved this. The endianness of the > > > packet > > > > is guaranteed to be network endiannness and I am returning an uint16 by > > > shifting > > > > and not writing to memory directly. > > > > > > You are correct that the packet is guaranteed to be in network endianness, > but > > > you cannot assume that your host endianness is the same as the network > > > endianness. If the Chromebook running this code has the opposite endianness, > > the > > > wrong value will be produced. > > > > I was referring to Gustavo's comment on writing or not writing to memory > > directly. > > The endianness only matter if you are addressing memory directly. > > For instance you have uint16_t val and have uint8_t* val_ptr = (uint8_t > > *)(&uint16_t); > > then you write to val_ptr[0] and val_ptr[1]. This is accessing bytes in > memory. > > However if you have uint16_t val = upper << 8 | lower, this is not writing to > > memory, > > it's performing a mathematical calculation which would be upper * 2^8 + lower, > > which > > is guaranteed to be correct or otherwise a computer breaks down. > > I think the misunderstanding is that the direct 2-byte value that we receive is > not necessarily encoded in the same way that your machine encodes things. > > It might be helpful to use an example. Pretend instead that instead of > endianness, we are dealing with encrypting/decrypting a value. A 2-byte value is > encrypted in the network, and we have received it. Now, we need to decrypt it > into a value that our computer understands. To do so, we would have to run some > special decrypt() function that takes in the network-encrypted 2-byte value and > returns the decrypted 2-byte value in order for your machine to use it the right > way. > > Now, suppose that the encryption technique used is that you just swap the two > bytes, so encrypt(0xff00) returns 0x00ff, and decrypt(0x00ff) returns 0xff00 > again. This is exactly what happens in this endianness situation. However, you > don't necessarily know if the endianness on the network is the same as on the > machine, so on machines with the same endianness as the network htons() and > ntohs() just return the same value that they were passed. That's why you can't > assume endianness and must use these functions. This is resolved in the generator as a correct implementation but I'll implement the ntohs version. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:58: // Return the packet size that the receiver parsed out of request/response. On 2016/06/21 18:13:37, Kyle Horimoto wrote: > On 2016/06/21 01:19:15, jingxuy wrote: > > On 2016/06/20 23:25:40, Kyle Horimoto wrote: > > > Please list the conditions under which this can be called. For example, you > > > couldn't call this before the connection has been established. > > > > This is always valid, see comment in cc file > > Readers of this class should only need to look at the .h file to glean this > information. Please place it in the .h file. I meant refer to my reply comment to you in the cc file explaining this function is always defined. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:71: std::string GetDataMessage(); On 2016/06/20 23:25:40, Kyle Horimoto wrote: > Likewise, please document this function. Done. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:96: void Error( On 2016/06/21 02:10:26, Kyle Horimoto wrote: > On 2016/06/21 01:19:14, jingxuy wrote: > > On 2016/06/20 23:25:40, Kyle Horimoto wrote: > > > nit: s/Error/TransitionToErrorState/ > > > > I changed it to ThrowError > > This function doesn't throw an error, though. Please use another name. Done. https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc (right): https://codereview.chromium.org/2053013002/diff/320001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:43: // TODO(jingxuy): put comments on hex values On 2016/06/21 18:13:38, Kyle Horimoto wrote: > On 2016/06/21 02:10:27, Kyle Horimoto wrote: > > On 2016/06/21 01:19:15, jingxuy wrote: > > > On 2016/06/20 23:25:40, Kyle Horimoto wrote: > > > > Please also describe what the other values are. For example, below you > have > > > > {0x80, 1, 0, 1, 0, 0, 0}, but it is unclear what any of these values mean. > > > > People who read your code should be able to tell what this means without > > > > cross-referencing the spec. > > > > > > Done. > > > > You still need to use constants for the last two 0's. > > Ping. Done. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:63: // The connection hasn't been estabalish. Accept a CONNECTION_REQUEST if the On 2016/06/21 18:13:38, Kyle Horimoto wrote: > On 2016/06/21 02:10:27, Kyle Horimoto wrote: > > s/estabalish/established/ > > Ping. Done. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:66: // The state will transition to WAITING after a request/respone if they On 2016/06/21 18:13:38, Kyle Horimoto wrote: > On 2016/06/21 02:10:27, Kyle Horimoto wrote: > > s/respone/response/ > > Ping. Done. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:68: // request/resposne have extra data since the extra data is treated as a On 2016/06/21 18:13:38, Kyle Horimoto wrote: > On 2016/06/21 02:10:27, Kyle Horimoto wrote: > > s/resposne/response/ > > Ping. Done. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:72: // to arrive. Will accept all but request/response packets. The first data On 2016/06/21 02:10:27, Kyle Horimoto wrote: > connection request/response Done. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc (right): https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:60: kControlRequestHeader, kWeaveVersion, 0, kWeaveVersion, 0, 0, 0}; On 2016/06/21 18:13:38, Kyle Horimoto wrote: > On 2016/06/21 02:10:27, Kyle Horimoto wrote: > > Isn't network order big endian? Shouldn't it be 0 first, then kWeaveVersion? > > Ping. Done. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:113: WellBehavingServerPacketsWithControlDataTest) { On 2016/06/21 18:13:38, Kyle Horimoto wrote: > Also test: > > *when less than the full 13 bytes of control data are sent > *the error condition where >13 bytes are sent. greater than 13 bytes is tested in controlpacketsizetest https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:186: EXPECT_EQ((uint32_t)30, receiver->GetMaxPacketSize()); On 2016/06/21 18:13:38, Kyle Horimoto wrote: > For this test, can you actually try passing a packet which is the max size? > Thanks! Done. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:212: WellBehavingClientPacketsWithControlDataTest) { On 2016/06/21 18:13:38, Kyle Horimoto wrote: > Also test: > > *when the full 15 bytes of control data are sent > *the error condition where >15 bytes are sent. Both the full packet and Error condition > 15 is tested in controlpacketsizetest. But I created a more through max size test. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:305: TEST_F(ProximityAuthBluetoothLowEnergyWeavePacketReceiverTest, On 2016/06/21 18:13:38, Kyle Horimoto wrote: > nit: For this test and the ones below, please just test one thing per test. > Break each of these sub-tests out into its own top-level test. Thanks! Done. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:306: InappropriateConnectingTypeTest) { On 2016/06/21 18:13:38, Kyle Horimoto wrote: > Also test that if a packet is received after we've reached an error state that > we remain in an error state. Done. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:410: ; On 2016/06/21 18:13:38, Kyle Horimoto wrote: > nit: Remove ; Done. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:412: // First bit is not set when it's supposed to be with last bit not set. On 2016/06/21 18:13:39, Kyle Horimoto wrote: > I'm not sure I understand this - can you explain? You receive the first packet and first bit and last bit is not set. The test below is you receive first packet and first bit is not set but the last bit is set. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:506: EXPECT_EQ(ReasonForClose::UNKNOWN_ERROR, receiver->GetReasonToClose()); On 2016/06/21 18:13:38, Kyle Horimoto wrote: > Let's make error values for these cases - it seems a lot of them have an > "unknown error" value. I realize that you're just reusing the error values for > the connection close packet, but since this is a class that encapsulates this > logic, we can create our own values. GetReasonToClose has to return ReasonForClose because that would be what's receiver's user sent over a connectionClose packet. We can get our own error code in addition to ReasonToClose. Will be a TODO
LGTM after you address my comment. https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc (right): https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:506: EXPECT_EQ(ReasonForClose::UNKNOWN_ERROR, receiver->GetReasonToClose()); On 2016/06/22 01:42:23, jingxuy wrote: > On 2016/06/21 18:13:38, Kyle Horimoto wrote: > > Let's make error values for these cases - it seems a lot of them have an > > "unknown error" value. I realize that you're just reusing the error values for > > the connection close packet, but since this is a class that encapsulates this > > logic, we can create our own values. > > GetReasonToClose has to return ReasonForClose because that would be what's > receiver's user sent over a connectionClose packet. We can get our own error > code in addition to ReasonToClose. > Will be a TODO Oops, my mistake. Sounds good. https://codereview.chromium.org/2053013002/diff/360001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/360001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:107: if (state_ == State::ERROR) { You should only return early if the state change was caused by VerifyPacketCounter(). Otherwise, the ERROR case below is dead code and is never hit. I think you should return a boolean from VerifyPacketCounter() to do this.
https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc (right): https://codereview.chromium.org/2053013002/diff/340001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver_unittest.cc:506: EXPECT_EQ(ReasonForClose::UNKNOWN_ERROR, receiver->GetReasonToClose()); On 2016/06/22 01:56:19, Kyle Horimoto wrote: > On 2016/06/22 01:42:23, jingxuy wrote: > > On 2016/06/21 18:13:38, Kyle Horimoto wrote: > > > Let's make error values for these cases - it seems a lot of them have an > > > "unknown error" value. I realize that you're just reusing the error values > for > > > the connection close packet, but since this is a class that encapsulates > this > > > logic, we can create our own values. > > > > GetReasonToClose has to return ReasonForClose because that would be what's > > receiver's user sent over a connectionClose packet. We can get our own error > > code in addition to ReasonToClose. > > Will be a TODO > > Oops, my mistake. Sounds good. Done. https://codereview.chromium.org/2053013002/diff/360001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/360001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:107: if (state_ == State::ERROR) { On 2016/06/22 01:56:20, Kyle Horimoto wrote: > You should only return early if the state change was caused by > VerifyPacketCounter(). Otherwise, the ERROR case below is dead code and is never > hit. I think you should return a boolean from VerifyPacketCounter() to do this. Done.
https://codereview.chromium.org/2053013002/diff/360001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/360001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:107: if (state_ == State::ERROR) { On 2016/06/22 22:37:43, jingxuy wrote: > On 2016/06/22 01:56:20, Kyle Horimoto wrote: > > You should only return early if the state change was caused by > > VerifyPacketCounter(). Otherwise, the ERROR case below is dead code and is > never > > hit. I think you should return a boolean from VerifyPacketCounter() to do > this. > > Done. We should still print that we received a message in the ERROR state if we did receive one. https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:148: BluetoothLowEnergyWeavePacketGenerator::ReasonForClose GetReasonToClose(); Is this function necessary anymore?
https://codereview.chromium.org/2053013002/diff/360001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/360001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:107: if (state_ == State::ERROR) { On 2016/06/22 22:44:06, Kyle Horimoto wrote: > On 2016/06/22 22:37:43, jingxuy wrote: > > On 2016/06/22 01:56:20, Kyle Horimoto wrote: > > > You should only return early if the state change was caused by > > > VerifyPacketCounter(). Otherwise, the ERROR case below is dead code and is > > never > > > hit. I think you should return a boolean from VerifyPacketCounter() to do > > this. > > > > Done. > > We should still print that we received a message in the ERROR state if we did > receive one. Wait, this is the not updated version https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:148: BluetoothLowEnergyWeavePacketGenerator::ReasonForClose GetReasonToClose(); On 2016/06/22 22:44:06, Kyle Horimoto wrote: > Is this function necessary anymore? yah, it's what Gustavo wanted. Like a reason to send over connection_close for the receiver's user
https://codereview.chromium.org/2053013002/diff/360001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/360001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:107: if (state_ == State::ERROR) { On 2016/06/22 22:55:53, jingxuy wrote: > On 2016/06/22 22:44:06, Kyle Horimoto wrote: > > On 2016/06/22 22:37:43, jingxuy wrote: > > > On 2016/06/22 01:56:20, Kyle Horimoto wrote: > > > > You should only return early if the state change was caused by > > > > VerifyPacketCounter(). Otherwise, the ERROR case below is dead code and is > > > never > > > > hit. I think you should return a boolean from VerifyPacketCounter() to do > > > this. > > > > > > Done. > > > > We should still print that we received a message in the ERROR state if we did > > receive one. > > Wait, this is the not updated version Yes, I know. In your current version, you do not print that you received an message in the ERROR state if you did receive one. You removed that line entirely.
Still LGTM. https://codereview.chromium.org/2053013002/diff/360001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/360001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:107: if (state_ == State::ERROR) { On 2016/06/22 22:59:10, Kyle Horimoto wrote: > On 2016/06/22 22:55:53, jingxuy wrote: > > On 2016/06/22 22:44:06, Kyle Horimoto wrote: > > > On 2016/06/22 22:37:43, jingxuy wrote: > > > > On 2016/06/22 01:56:20, Kyle Horimoto wrote: > > > > > You should only return early if the state change was caused by > > > > > VerifyPacketCounter(). Otherwise, the ERROR case below is dead code and > is > > > > never > > > > > hit. I think you should return a boolean from VerifyPacketCounter() to > do > > > > this. > > > > > > > > Done. > > > > > > We should still print that we received a message in the ERROR state if we > did > > > receive one. > > > > Wait, this is the not updated version > > Yes, I know. In your current version, you do not print that you received an > message in the ERROR state if you did receive one. You removed that line > entirely. Nevermind - I see what you did. Looks good.
https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:95: BluetoothLowEnergyWeavePacketReceiver::GetReceiverError() { DCHECK for State::ERROR? https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:106: } else if (state_ == State::ERROR) { This should be the first condition before doing anything with the packet. https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:108: MoveToErrorState(ReasonForClose::UNKNOWN_ERROR, If we're in the error state, we shouldn't override the existing error reason. https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:120: case State::RECEIVING_DATA: You can merge this case with the WAITING case. https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:340: } else if (packet.size() < kMaxConnectionCloseSize) { You can merge this range check case with the one above. https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:379: uint8_t* received_ptr = (uint8_t*)(&received); Use a reinterp_cast rather than a C cast https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:455: return 20; return kMaxPacketSizeLowerBound instead?
LGTM after addressing Tim's comments. Thanks, Jing! https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:95: BluetoothLowEnergyWeavePacketReceiver::GetReceiverError() { On 2016/06/23 01:19:47, Tim Song wrote: > DCHECK for State::ERROR? The |ReceiverError| enum has a |NO_ERROR| value, so I'm assuming this method can be called at any moment, no only in the State::ERROR. https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:69: // CONNECTICNG and unlimited number of times from RECEIVING_DATA. nit: s/CONNECTICNG/CONNECTING/. https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:148: BluetoothLowEnergyWeavePacketGenerator::ReasonForClose GetReasonToClose(); On 2016/06/22 22:55:53, jingxuy wrote: > On 2016/06/22 22:44:06, Kyle Horimoto wrote: > > Is this function necessary anymore? > > yah, it's what Gustavo wanted. Like a reason to send over connection_close for > the receiver's user Jing is right. You'll need a reason to send in the connection close packet to the other side if something goes wrong with the received packet (e.g. no common version supported). You could rely only on the |GetReceiverError()| method but then the user of this class will have to map the |ReceiverError| enum to the |BluetoothLowEnergyWeavePacketGenerator::ReasonForClose| enum.
https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc (right): https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:95: BluetoothLowEnergyWeavePacketReceiver::GetReceiverError() { On 2016/06/23 16:09:19, sacomoto wrote: > On 2016/06/23 01:19:47, Tim Song wrote: > > DCHECK for State::ERROR? > > The |ReceiverError| enum has a |NO_ERROR| value, so I'm assuming this method can > be called at any moment, no only in the State::ERROR. Yes, I added a comment in the .h file to say this can be called any time. https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:106: } else if (state_ == State::ERROR) { On 2016/06/23 01:19:47, Tim Song wrote: > This should be the first condition before doing anything with the packet. Done. https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:108: MoveToErrorState(ReasonForClose::UNKNOWN_ERROR, On 2016/06/23 01:19:46, Tim Song wrote: > If we're in the error state, we shouldn't override the existing error reason. Done. https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:120: case State::RECEIVING_DATA: On 2016/06/23 01:19:47, Tim Song wrote: > You can merge this case with the WAITING case. Done. https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:340: } else if (packet.size() < kMaxConnectionCloseSize) { On 2016/06/23 01:19:46, Tim Song wrote: > You can merge this range check case with the one above. They are different cases. The greater than connectionclosesize is not valid, but smaller than is valid because of the legacy implementation. https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:379: uint8_t* received_ptr = (uint8_t*)(&received); On 2016/06/23 01:19:46, Tim Song wrote: > Use a reinterp_cast rather than a C cast Done. https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc:455: return 20; On 2016/06/23 01:19:47, Tim Song wrote: > return kMaxPacketSizeLowerBound instead? Done. https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h (right): https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:69: // CONNECTICNG and unlimited number of times from RECEIVING_DATA. On 2016/06/23 16:09:19, sacomoto wrote: > nit: s/CONNECTICNG/CONNECTING/. Done. https://codereview.chromium.org/2053013002/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h:148: BluetoothLowEnergyWeavePacketGenerator::ReasonForClose GetReasonToClose(); On 2016/06/23 16:09:19, sacomoto wrote: > On 2016/06/22 22:55:53, jingxuy wrote: > > On 2016/06/22 22:44:06, Kyle Horimoto wrote: > > > Is this function necessary anymore? > > > > yah, it's what Gustavo wanted. Like a reason to send over connection_close for > > the receiver's user > > Jing is right. You'll need a reason to send in the connection close packet to > the other side if something goes wrong with the received packet (e.g. no common > version supported). > > You could rely only on the |GetReceiverError()| method but then the user of this > class will have to map the |ReceiverError| enum to the > |BluetoothLowEnergyWeavePacketGenerator::ReasonForClose| enum. Done.
lgtm
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/2053013002/#ps440001 (title: "BUILD.gn merge")
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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Before submitting this CL, please move the constants shared between both this class and the generator CL into a common location and have both classes reference them.
On 2016/06/27 16:52:41, Kyle Horimoto wrote: > Before submitting this CL, please move the constants shared between both this > class and the generator CL into a common location and have both classes > reference them. 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, tengs@chromium.org, sacomoto@chromium.org Link to the patchset: https://codereview.chromium.org/2053013002/#ps460001 (title: "fix macro problem and change functions to virtual")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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/2053013002/#ps480001 (title: "fixed narrowing conversion from uint32_t to uint8-t")
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 ========== Weave Packet Generator This CL adds a packet receiver that respects the uWeave.BLE protocol. The packet receiver expects this chain of events: 1 control_request/response -> 0+ data packets -> 0 or 1 control_close. The packet receiver keeps track of the receiving state and will jump to ERROR state when anything unexpected happens. (receiving packets out of order, invalid packet content, etc) The expected user of packet receiver should feed a packet into receiver, check the state, and take appropriate action. If actions are not taken in the appropriate state, the receiver will crash. Data message should be retrieved promptly when the state reaches DATA_READY or else the data will be lost when the next packet arrives. Design Doc for ProximityAuth uWeave Migration: https://docs.google.com/a/google.com/document/d/1HJ1fipxsNQwIxmKs-5_6TjroCAcs... BUG=617238 ========== to ========== Weave Packet Generator This CL adds a packet receiver that respects the uWeave.BLE protocol. The packet receiver expects this chain of events: 1 control_request/response -> 0+ data packets -> 0 or 1 control_close. The packet receiver keeps track of the receiving state and will jump to ERROR state when anything unexpected happens. (receiving packets out of order, invalid packet content, etc) The expected user of packet receiver should feed a packet into receiver, check the state, and take appropriate action. If actions are not taken in the appropriate state, the receiver will crash. Data message should be retrieved promptly when the state reaches DATA_READY or else the data will be lost when the next packet arrives. 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 #20 (id:480001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Weave Packet Generator This CL adds a packet receiver that respects the uWeave.BLE protocol. The packet receiver expects this chain of events: 1 control_request/response -> 0+ data packets -> 0 or 1 control_close. The packet receiver keeps track of the receiving state and will jump to ERROR state when anything unexpected happens. (receiving packets out of order, invalid packet content, etc) The expected user of packet receiver should feed a packet into receiver, check the state, and take appropriate action. If actions are not taken in the appropriate state, the receiver will crash. Data message should be retrieved promptly when the state reaches DATA_READY or else the data will be lost when the next packet arrives. Design Doc for ProximityAuth uWeave Migration: https://docs.google.com/a/google.com/document/d/1HJ1fipxsNQwIxmKs-5_6TjroCAcs... BUG=617238 ========== to ========== Weave Packet Generator This CL adds a packet receiver that respects the uWeave.BLE protocol. The packet receiver expects this chain of events: 1 control_request/response -> 0+ data packets -> 0 or 1 control_close. The packet receiver keeps track of the receiving state and will jump to ERROR state when anything unexpected happens. (receiving packets out of order, invalid packet content, etc) The expected user of packet receiver should feed a packet into receiver, check the state, and take appropriate action. If actions are not taken in the appropriate state, the receiver will crash. Data message should be retrieved promptly when the state reaches DATA_READY or else the data will be lost when the next packet arrives. Design Doc for ProximityAuth uWeave Migration: https://docs.google.com/a/google.com/document/d/1HJ1fipxsNQwIxmKs-5_6TjroCAcs... BUG=617238 Committed: https://crrev.com/688334563b18b7d80b3207da6c0d30d17323b351 Cr-Commit-Position: refs/heads/master@{#402884} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/688334563b18b7d80b3207da6c0d30d17323b351 Cr-Commit-Position: refs/heads/master@{#402884} |
