Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2031953003:
Weave Packet Generator (Closed)
DescriptionWeave Packet Generator
This CL adds a packet generator that respects the uWeave.BLE protocol.
The packet generator accepts raw bytes as messages to be sent and wraps the message with the uWeave protocol.
The uWeave packets include control packets of request/response/close of a connection and data packets that are the original message chopped up to fixed size packets with uWeave protocol headers.
Design Doc of ProximityAuth uWeave Migration:
https://docs.google.com/a/google.com/document/d/1HJ1fipxsNQwIxmKs-5_6TjroCAcs6qRhAYvOnIr6mQw/edit?usp=sharing
BUG=617238
Committed: https://crrev.com/6742a47fff173bfa1bf0a4ec3a4655436c9c03e4
Cr-Commit-Position: refs/heads/master@{#401993}
Patch Set 1 #Patch Set 2 : ProximityAuth uWeave Migration #Patch Set 3 : uWeave Packet Generator #
Total comments: 97
Patch Set 4 : changed magic numbers to enums #
Total comments: 6
Patch Set 5 : addressed .h comments and passed the tests with network endian #Patch Set 6 : changed argument type to some Set* function fromt ints to enum #Patch Set 7 : changed function name from cmd to command #
Total comments: 32
Patch Set 8 : changed name of ControlPacketCommand to ControlCommand #
Total comments: 18
Patch Set 9 : add back the test that's accidentally deleted #Patch Set 10 : addressed some stylistic comemnts in the .cc file #Patch Set 11 : addressed style comments #
Total comments: 19
Patch Set 12 : Don't review yet! commit before moving machine #Patch Set 13 : make tests more readible/comprehensive #Patch Set 14 : enforce the minimum packet size to be 2 so can contain header and 1 byte of data #
Total comments: 48
Patch Set 15 : outlaw empty message #Patch Set 16 : addressed comments in cc and test file #Patch Set 17 : wrap around test #Patch Set 18 : improved style of test code #
Total comments: 4
Patch Set 19 : flipped how shorts are ordered #
Total comments: 2
Patch Set 20 : changed implementation from shifting to htons for clearity #Patch Set 21 : increased the number of packets to test wrap arounds even better! #
Total comments: 14
Patch Set 22 : fixed double i counter issue #
Total comments: 2
Patch Set 23 : use reinterpet_cast #
Total comments: 4
Patch Set 24 : removed files that doesn't exist yet in the master in BUILD.gn #Patch Set 25 : allowed re-generation of request/response without incrementing the counter. accomodate actual conne… #Patch Set 26 : minor style update #
Total comments: 2
Patch Set 27 : final style change #Patch Set 28 : ifdef guard around htons include and changed some uint16 to uint8 #Patch Set 29 : updated gyp files #Patch Set 30 : include the header that actually enables platform dependent include of headers #Messages
Total messages: 75 (20 generated)
jingxuy@google.com changed reviewers: + khorimoto@chromium.org, sacomoto@chromium.org, tengs@chromium.org
msarda@chromium.org changed reviewers: + msarda@chromium.org
Drive-by: Please create a bug and attach all the CLs for this refactoring to that bug. That will allow us to track the work done and maybe have a better idea on what we want to achieve here. Ideally link the design doc in there as well (be careful to only include information that can be made public in the document). Please add a better CL description: Title: Weave Packet Generator Description: Weave Packet Generator This CL adds a packet generator that respects the Weave.BLE protocol.
+1 to msarda's comments - please improve your title. Someone who reads this years from now should understand what you're doing in this CL. Other reviewers: Jing has told me that she had a few design flaws in this CL and that we should hold off on reviewing it until she lets us know that it is ready. Sorry for the inconvenience if you have already started reviewing!
Description was changed from ========== completed packet generator along with unit test BUG= ========== to ========== completed packet generator along with unit test BUG= ==========
Description was changed from ========== completed packet generator along with unit test BUG= ========== to ========== Weave Packet Generator This CL adds a packet generator that respects the Weave.BLE protocol. BUG= ==========
Description was changed from ========== Weave Packet Generator This CL adds a packet generator that respects the Weave.BLE protocol. BUG= ========== to ========== Weave Packet Generator This CL adds a packet generator that respects the Weave.BLE protocol. BUG=617238 ==========
First pass on the bluetooth_low_energy_weave_packet_generator.cc. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:49: SetControlCmd(packet, 0, 0); Please create an enum for the command values: enum ControlCommand { CONNECTION_REQUEST = 0, ... } https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:50: // Min supported version is 1. Please use a constant for the min supported version. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:52: // Max supported version is 1. The same for the max supported version. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:54: // Max packet size is 0 which means defer the decision to server. The same for max packet size. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:62: Packet* packet = CreateControlPacket(); See the comments above. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:76: ReasonForClose reason_for_close) { See the comments above. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:144: In the uWeave protocol the integers sent in the control packet are encoded in network byte order (most significative byte first, big endian). So, before writing |val| into |packet| you should convert it to BigEndian if necessary. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:153: uint32_t index, Please remove the |index| parameter, as it's always 0. The packet type bit is always the first bit of the first byte. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:154: uint8_t val) { Please use |bool| instead of |uint8_t|. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:157: DCHECK(IsBit(val)); Please remove the last two DCHECK's, and the |IsBit()| helper method. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:162: uint32_t index, Please remove the |index| parameter, as it's always 0. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:163: uint8_t val) { Please pass the ControlCommand enum (see a few comments above). https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:172: uint32_t index) { Please remove the |index| parameter, as it's always 0. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:175: uint8_t counter = packet_number_ % 8; Please add a kMaxPacketCounter constant, and use it here. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:181: uint32_t index, Please remove the |index| parameter, as it's always 0. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:182: uint8_t val) { Please remove the |val| parameter, as this is always 1. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:191: uint8_t val) { See the comments above.
First pass on the bluetooth_low_energy_weave_packet_generator.cc.
First pass through. Just went through the implementation - didn't review the test yet. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:19: factory_instance_ = this; Remove this. See comment below. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:23: BluetoothLowEnergyWeavePacketGenerator::Factory::NewInstance() { This should be structured a bit differently. Do something like this instead (with proper syntax): if (factory_instance_ == nullptr) { factory_instance_ = new Factory(); } return std::unique_ptr<>(factory_instance_->BuildInstance()); https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:40: packet_size_ = 20; Please initialize these via an initializer list. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:40: packet_size_ = 20; Please use a constant instead of "20" directly. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:50: // Min supported version is 1. On 2016/06/06 15:25:20, sacomoto wrote: > Please use a constant for the min supported version. Just use the kWeaveVersion constant you already have. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:69: SetIntField(packet, 3, packet_size_); I'm confused by your use of packet_size_. It's used by data packets and connection responses? Can you please clarify this? https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:92: SetPacketTypeBit(packet, 0, 1); Use a constant for the value to pass (e.g., kControlPacketTypeValue). Same for data packets. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:98: bool BluetoothLowEnergyWeavePacketGenerator::IsBit(uint8_t val) { Why is this method used? I don't really see the value of it. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:108: uint32_t payload_size = packet_size_ - 1; Please comment about why this is the case. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:109: // (message.length() + 1) + (payload_size - 1) / (payload_size). Can you please explain this calculation? https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:117: Packet* packet = &weave_message->at(i); Instead of at(), just use the bracket notation shorthand: []. Same everywhere else at() is used - the bracket notation is very common in C++. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:131: // Since even empty string has \0, weave_message is guaranteed to be nonempty. weave_message isn't necessarily guaranteed to be nonempty because of that. It's only guaranteed to be nonempty in the case that num_packets > 0. Please DCHECK() that this is true at the top of the function. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:152: void BluetoothLowEnergyWeavePacketGenerator::SetPacketTypeBit(Packet* packet, For all these Send*() methods, please give a description of what is happening. For example, describe that the packet type bit is the first bit of the first byte of the packet and add comments in your implementation that make it clear what is going on. It should be easy to reason about your code without having to do complex bit manipulation in your head. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:154: uint8_t val) { On 2016/06/06 15:25:21, sacomoto wrote: > Please use |bool| instead of |uint8_t|. +1 https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:17: // Internally tracks the packet number. nit: This is an implementation detail of this class, so it doesn't belong in the class description. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:25: NewInstance(); nit: +4 spaces. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:35: static Factory* factory_instance_; You need to statically initialize this to nullptr. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:38: enum ReasonForClose { nit: Add a description over this enum which lets readers know how it will be used. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:43: APPLICATION_ERROR = 0x80 nit: Why does this jump from 0x03 to 0x80? https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:46: typedef std::vector<uint8_t> Packet; Hmm, are we sure this works correctly regarding endianness? Will we have to use htonl(), etc? sacomoto/msarda/tengs - have you dealt with this before? https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:61: static const int kWeaveVersion = 1; This and kControlPacketSize should be moved to the top of the .cc file in an anonymous namespace. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:78: uint32_t packet_size_; This is only for data packets, right? If so, please change its name accordingly.
addressed most comments. Currently failing tests because of the big/little endian situation, but otherwise good https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:19: factory_instance_ = this; On 2016/06/06 19:30:54, Kyle Horimoto wrote: > Remove this. See comment below. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:23: BluetoothLowEnergyWeavePacketGenerator::Factory::NewInstance() { On 2016/06/06 19:30:54, Kyle Horimoto wrote: > This should be structured a bit differently. Do something like this instead > (with proper syntax): > > if (factory_instance_ == nullptr) { > factory_instance_ = new Factory(); > } > > return std::unique_ptr<>(factory_instance_->BuildInstance()); Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:40: packet_size_ = 20; On 2016/06/06 19:30:54, Kyle Horimoto wrote: > Please use a constant instead of "20" directly. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:49: SetControlCmd(packet, 0, 0); On 2016/06/06 15:25:20, sacomoto wrote: > Please create an enum for the command values: > > enum ControlCommand { > CONNECTION_REQUEST = 0, > ... > } Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:50: // Min supported version is 1. On 2016/06/06 15:25:20, sacomoto wrote: > Please use a constant for the min supported version. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:52: // Max supported version is 1. On 2016/06/06 15:25:20, sacomoto wrote: > The same for the max supported version. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:54: // Max packet size is 0 which means defer the decision to server. On 2016/06/06 15:25:20, sacomoto wrote: > The same for max packet size. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:62: Packet* packet = CreateControlPacket(); On 2016/06/06 15:25:21, sacomoto wrote: > See the comments above. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:76: ReasonForClose reason_for_close) { On 2016/06/06 15:25:20, sacomoto wrote: > See the comments above. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:92: SetPacketTypeBit(packet, 0, 1); On 2016/06/06 19:30:54, Kyle Horimoto wrote: > Use a constant for the value to pass (e.g., kControlPacketTypeValue). Same for > data packets. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:98: bool BluetoothLowEnergyWeavePacketGenerator::IsBit(uint8_t val) { On 2016/06/06 19:30:54, Kyle Horimoto wrote: > Why is this method used? I don't really see the value of it. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:117: Packet* packet = &weave_message->at(i); On 2016/06/06 19:30:54, Kyle Horimoto wrote: > Instead of at(), just use the bracket notation shorthand: []. Same everywhere > else at() is used - the bracket notation is very common in C++. [] does not work with unique_ptr, hence the at() https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:131: // Since even empty string has \0, weave_message is guaranteed to be nonempty. On 2016/06/06 19:30:55, Kyle Horimoto wrote: > weave_message isn't necessarily guaranteed to be nonempty because of that. It's > only guaranteed to be nonempty in the case that num_packets > 0. Please DCHECK() > that this is true at the top of the function. the string that is passed in at least has to be an empty string which consist of \0 character. Hence weave_message is guaranteed to be non-empty https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:144: On 2016/06/06 15:25:20, sacomoto wrote: > In the uWeave protocol the integers sent in the control packet are encoded in > network byte order (most significative byte first, big endian). > > So, before writing |val| into |packet| you should convert it to BigEndian if > necessary. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:153: uint32_t index, On 2016/06/06 15:25:21, sacomoto wrote: > Please remove the |index| parameter, as it's always 0. The packet type bit is > always the first bit of the first byte. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:154: uint8_t val) { On 2016/06/06 15:25:21, sacomoto wrote: > Please use |bool| instead of |uint8_t|. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:157: DCHECK(IsBit(val)); On 2016/06/06 15:25:20, sacomoto wrote: > Please remove the last two DCHECK's, and the |IsBit()| helper method. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:162: uint32_t index, On 2016/06/06 15:25:21, sacomoto wrote: > Please remove the |index| parameter, as it's always 0. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:163: uint8_t val) { On 2016/06/06 15:25:20, sacomoto wrote: > Please pass the ControlCommand enum (see a few comments above). Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:172: uint32_t index) { On 2016/06/06 15:25:21, sacomoto wrote: > Please remove the |index| parameter, as it's always 0. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:175: uint8_t counter = packet_number_ % 8; On 2016/06/06 15:25:20, sacomoto wrote: > Please add a kMaxPacketCounter constant, and use it here. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:181: uint32_t index, On 2016/06/06 15:25:20, sacomoto wrote: > Please remove the |index| parameter, as it's always 0. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:182: uint8_t val) { On 2016/06/06 15:25:20, sacomoto wrote: > Please remove the |val| parameter, as this is always 1. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:191: uint8_t val) { On 2016/06/06 15:25:20, sacomoto wrote: > See the comments above. Done.
Hey Jing, I think you forgot to upload the latest patch.
On 2016/06/07 09:51:29, sacomoto wrote: > Hey Jing, I think you forgot to upload the latest patch. +1, I'm not seeing it either.
On 2016/06/07 16:55:38, Kyle Horimoto wrote: > On 2016/06/07 09:51:29, sacomoto wrote: > > Hey Jing, I think you forgot to upload the latest patch. > > +1, I'm not seeing it either. Hi, I think I uploaded it to the wrong branch...
On 2016/06/07 20:34:17, jingxuy wrote: > On 2016/06/07 16:55:38, Kyle Horimoto wrote: > > On 2016/06/07 09:51:29, sacomoto wrote: > > > Hey Jing, I think you forgot to upload the latest patch. > > > > +1, I'm not seeing it either. > > I think I uploaded it to the wrong branch... It is addressed now
https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:69: SetIntField(packet, 3, packet_size_); On 2016/06/06 19:30:54, Kyle Horimoto wrote: > I'm confused by your use of packet_size_. It's used by data packets and > connection responses? Can you please clarify this? Ping. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:108: uint32_t payload_size = packet_size_ - 1; On 2016/06/06 19:30:54, Kyle Horimoto wrote: > Please comment about why this is the case. Ping. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:109: // (message.length() + 1) + (payload_size - 1) / (payload_size). On 2016/06/06 19:30:54, Kyle Horimoto wrote: > Can you please explain this calculation? Ping. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:131: // Since even empty string has \0, weave_message is guaranteed to be nonempty. On 2016/06/07 01:19:05, jingxuy wrote: > On 2016/06/06 19:30:55, Kyle Horimoto wrote: > > weave_message isn't necessarily guaranteed to be nonempty because of that. > It's > > only guaranteed to be nonempty in the case that num_packets > 0. Please > DCHECK() > > that this is true at the top of the function. > > the string that is passed in at least has to be an empty string which consist of > \0 character. Hence weave_message is guaranteed to be non-empty That's not true. An empty string has a length of 0. '\0' is not included in a string length computation. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:144: On 2016/06/07 01:19:05, jingxuy wrote: > On 2016/06/06 15:25:20, sacomoto wrote: > > In the uWeave protocol the integers sent in the control packet are encoded in > > network byte order (most significative byte first, big endian). > > > > So, before writing |val| into |packet| you should convert it to BigEndian if > > necessary. > > Done. You can't assume the endianness of this machine. Please use htons(). https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:152: void BluetoothLowEnergyWeavePacketGenerator::SetPacketTypeBit(Packet* packet, On 2016/06/06 19:30:54, Kyle Horimoto wrote: > For all these Send*() methods, please give a description of what is happening. > For example, describe that the packet type bit is the first bit of the first > byte of the packet and add comments in your implementation that make it clear > what is going on. It should be easy to reason about your code without having to > do complex bit manipulation in your head. Ping. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:17: // Internally tracks the packet number. On 2016/06/06 19:30:55, Kyle Horimoto wrote: > nit: This is an implementation detail of this class, so it doesn't belong in the > class description. Ping. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:25: NewInstance(); On 2016/06/06 19:30:55, Kyle Horimoto wrote: > nit: +4 spaces. Ping. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:38: enum ReasonForClose { On 2016/06/06 19:30:55, Kyle Horimoto wrote: > nit: Add a description over this enum which lets readers know how it will be > used. Ping. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:43: APPLICATION_ERROR = 0x80 On 2016/06/06 19:30:55, Kyle Horimoto wrote: > nit: Why does this jump from 0x03 to 0x80? Nevermind - this is part of the spec. Disregard. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:61: static const int kWeaveVersion = 1; On 2016/06/06 19:30:55, Kyle Horimoto wrote: > This and kControlPacketSize should be moved to the top of the .cc file in an > anonymous namespace. Ping. Same with these other constants. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:78: uint32_t packet_size_; On 2016/06/06 19:30:55, Kyle Horimoto wrote: > This is only for data packets, right? If so, please change its name accordingly. Ping. https://codereview.chromium.org/2031953003/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:16: BluetoothLowEnergyWeavePacketGenerator::Factory::factory_instance_ = NULL; Initialize to nullptr. https://codereview.chromium.org/2031953003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:20: if (!factory_instance_) { Check for equality with nullptr. https://codereview.chromium.org/2031953003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:121: void BluetoothLowEnergyWeavePacketGenerator::SetIntField(Packet* packet, nit: Rename this to SetShortField(). https://codereview.chromium.org/2031953003/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:38: enum ControlPacketCommand { REQUEST = 0x00, RESPONSE = 0x01, CLOSE = 0x02 }; Prefix these with CONNECTION_.
https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:69: SetIntField(packet, 3, packet_size_); On 2016/06/06 19:30:54, Kyle Horimoto wrote: > I'm confused by your use of packet_size_. It's used by data packets and > connection responses? Can you please clarify this? Do you want me to put a comment above packet_size_? It is kind of confusing since this class is both used by the client and server. But the server should do some ATT_MTU detection and then call SetDataPacketSize to set packet_size_ according to the max packet size it observes. Then the server will send the Response message with the packet_size_ as the maximum packet size of the connection, which would be used to chop up data packets from either the server or the client's perspective On the client side, the receiver should parse out the packet_size_ from the server response and use it as maximum packet size to chop up and send data https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:108: uint32_t payload_size = packet_size_ - 1; On 2016/06/06 19:30:54, Kyle Horimoto wrote: > Please comment about why this is the case. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:109: // (message.length() + 1) + (payload_size - 1) / (payload_size). On 2016/06/06 19:30:54, Kyle Horimoto wrote: > Can you please explain this calculation? Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:131: // Since even empty string has \0, weave_message is guaranteed to be nonempty. On 2016/06/08 00:06:29, Kyle Horimoto wrote: > On 2016/06/07 01:19:05, jingxuy wrote: > > On 2016/06/06 19:30:55, Kyle Horimoto wrote: > > > weave_message isn't necessarily guaranteed to be nonempty because of that. > > It's > > > only guaranteed to be nonempty in the case that num_packets > 0. Please > > DCHECK() > > > that this is true at the top of the function. > > > > the string that is passed in at least has to be an empty string which consist > of > > \0 character. Hence weave_message is guaranteed to be non-empty > > That's not true. An empty string has a length of 0. '\0' is not included in a > string length computation. length used in this case is the number of bytes in the string which is string.length + 1 https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:144: On 2016/06/08 00:06:29, Kyle Horimoto wrote: > On 2016/06/07 01:19:05, jingxuy wrote: > > On 2016/06/06 15:25:20, sacomoto wrote: > > > In the uWeave protocol the integers sent in the control packet are encoded > in > > > network byte order (most significative byte first, big endian). > > > > > > So, before writing |val| into |packet| you should convert it to BigEndian if > > > necessary. > > > > Done. > > You can't assume the endianness of this machine. Please use htons(). what do you mean by endianness of this machine? I am fairly certain that in C the upper byte is the upper byte independent of the machine? I am using a direct uint16_t value not a byte stream. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:152: void BluetoothLowEnergyWeavePacketGenerator::SetPacketTypeBit(Packet* packet, On 2016/06/06 19:30:54, Kyle Horimoto wrote: > For all these Send*() methods, please give a description of what is happening. > For example, describe that the packet type bit is the first bit of the first > byte of the packet and add comments in your implementation that make it clear > what is going on. It should be easy to reason about your code without having to > do complex bit manipulation in your head. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:17: // Internally tracks the packet number. On 2016/06/06 19:30:55, Kyle Horimoto wrote: > nit: This is an implementation detail of this class, so it doesn't belong in the > class description. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:25: NewInstance(); On 2016/06/08 00:06:29, Kyle Horimoto wrote: > On 2016/06/06 19:30:55, Kyle Horimoto wrote: > > nit: +4 spaces. > > Ping. I had 4 spaces but git cl format deleted them https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:35: static Factory* factory_instance_; On 2016/06/06 19:30:55, Kyle Horimoto wrote: > You need to statically initialize this to nullptr. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:38: enum ReasonForClose { On 2016/06/08 00:06:29, Kyle Horimoto wrote: > On 2016/06/06 19:30:55, Kyle Horimoto wrote: > > nit: Add a description over this enum which lets readers know how it will be > > used. > > Ping. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:43: APPLICATION_ERROR = 0x80 On 2016/06/08 00:06:29, Kyle Horimoto wrote: > On 2016/06/06 19:30:55, Kyle Horimoto wrote: > > nit: Why does this jump from 0x03 to 0x80? > > Nevermind - this is part of the spec. Disregard. Done. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:46: typedef std::vector<uint8_t> Packet; On 2016/06/06 19:30:55, Kyle Horimoto wrote: > Hmm, are we sure this works correctly regarding endianness? Will we have to use > htonl(), etc? sacomoto/msarda/tengs - have you dealt with this before? You can check out the example from the legacy implementation. I'm pretty sure that the endianness only matters because of the protocol, like the protocol is saying that lower bytes of short comes before upper bytes. But bytes are bytes. They will sent and received in the same order. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:78: uint32_t packet_size_; On 2016/06/08 00:06:29, Kyle Horimoto wrote: > On 2016/06/06 19:30:55, Kyle Horimoto wrote: > > This is only for data packets, right? If so, please change its name > accordingly. > > Ping. They are already addressed. https://codereview.chromium.org/2031953003/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:16: BluetoothLowEnergyWeavePacketGenerator::Factory::factory_instance_ = NULL; On 2016/06/08 00:06:29, Kyle Horimoto wrote: > Initialize to nullptr. Done. https://codereview.chromium.org/2031953003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:20: if (!factory_instance_) { On 2016/06/08 00:06:30, Kyle Horimoto wrote: > Check for equality with nullptr. Done.
https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:61: static const int kWeaveVersion = 1; On 2016/06/08 00:06:29, Kyle Horimoto wrote: > On 2016/06/06 19:30:55, Kyle Horimoto wrote: > > This and kControlPacketSize should be moved to the top of the .cc file in an > > anonymous namespace. > > Ping. Same with these other constants. This is done but I think they need to be in the class (a different class) because eventually it has to be a shared location for the client and the server.
Patchset #8 (id:140001) has been deleted
Patchset #8 (id:160001) has been deleted
https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:16: nit: consistent spacing for this block. https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:29: BluetoothLowEnergyWeavePacketGenerator::Factory::factory_instance_ = add a // static. comment for static variables and functions. https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:29: BluetoothLowEnergyWeavePacketGenerator* BuildInstance(); This should be virtual if you are overriding it in the subclass. You should also return an std::unique_ptr rather than a raw pointer, as it isn't less efficient and it clarifies ownership. https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:64: std::unique_ptr<std::vector<Packet>> EncodeDataMessage(std::string message); Just return a std::vector<Packet>. The compiler will optimize the returned value to elide the copy operation: http://stackoverflow.com/questions/3134831/in-c-is-it-still-bad-practice-to-r... https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:70: Packet* CreateControlPacket(); Same here. I would return a unique_ptr even in private functions, as it's free and clarifies ownership. https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:82: // The default data packet length is 20 unless setDataPacketLength() is called nit: set should be capitalized.
https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:69: SetIntField(packet, 3, packet_size_); On 2016/06/08 21:30:35, jingxuy wrote: > On 2016/06/06 19:30:54, Kyle Horimoto wrote: > > I'm confused by your use of packet_size_. It's used by data packets and > > connection responses? Can you please clarify this? > > Do you want me to put a comment above packet_size_? > > It is kind of confusing since this class is both used by the client and server. > But the server should do some ATT_MTU detection and then call SetDataPacketSize > to set packet_size_ according to the max packet size it observes. Then the > server will send the Response message with the packet_size_ as the maximum > packet size of the connection, which would be used to chop up data packets from > either the server or the client's perspective > > On the client side, the receiver should parse out the packet_size_ from the > server response and use it as maximum packet size to chop up and send data How about this: just don't use packet_size_ for the connection response and use it only for data packets. Then, for this function, have this function take an optional parameter which defaults to kDefaultDataPacketSize. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:78: uint32_t packet_size_; On 2016/06/08 21:30:36, jingxuy wrote: > On 2016/06/08 00:06:29, Kyle Horimoto wrote: > > On 2016/06/06 19:30:55, Kyle Horimoto wrote: > > > This is only for data packets, right? If so, please change its name > > accordingly. > > > > Ping. > > They are already addressed. I addressed this in the .cc, but please make this data packets and change the name. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:107: uint32_t payload_size = packet_size_ - 1; s/payload_size/payload_size_per_packet/ https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:112: uint32_t num_packets = (message.length() + payload_size) / payload_size; nit: Please just use the computation you wrote above in the comment. It will make your code more readable. And don't worry, a compiler pass will optimize your code so it won't be any more inefficient. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:116: uint32_t last_byte = message.length() + 1; This name is confusing because it implies that this is the value of the last byte. How about num_bytes_in_message? https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:121: uint32_t begin = payload_size * i; nit: Move the begin/end calculations to right before you use them. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:142: uint32_t index, s/index/byte_offset https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:148: uint8_t upper = val >> 8 & 0xFF; nit: Use parentheses to make it clear which operation is performed first. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:35: std::vector<uint8_t> expected(20, 0); Use your Packet type here and below. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:35: std::vector<uint8_t> expected(20, 0); Please use constants for 20, 0x80, 1, 1. For all hex values, you'll want to make it a bitwise calculation to create the constant. Same for all other constants. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:54: expected[3] = 20; Write another test which tests alternate packet sizes. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:66: ReasonForClose::APPLICATION_ERROR); Test other reason values too. You can just make a helper function which just takes the reason as a parameter and runs this test. Then, make a bunch of tests that just call the helper function with different reason values. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:82: generator->SetDataPacketSize(4); Also test the default packet size. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:105: TEST_F(ProximityAuthBluetoothLowEnergyWeavePacketGeneratorTest, Please test the overflow from 7 back to 0. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:113: EXPECT_EQ(0x80, packet->at(0)); This is hard to read. Make a helper function getCounterFromPacket(Packet) which extracts the counter value and returns it.
Also, one more thing: Please make your CL description more precise. Use this as your guide: "If I looked at this CL in 3 or 4 years, would I know what I meant by it?" You don't have to go into a ton of detail about the background, but at least provide enough to give readers that may look at this CL in the future enough context to understand it. Thanks!
https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:69: SetIntField(packet, 3, packet_size_); On 2016/06/09 22:55:42, Kyle Horimoto wrote: > On 2016/06/08 21:30:35, jingxuy wrote: > > On 2016/06/06 19:30:54, Kyle Horimoto wrote: > > > I'm confused by your use of packet_size_. It's used by data packets and > > > connection responses? Can you please clarify this? > > > > Do you want me to put a comment above packet_size_? > > > > It is kind of confusing since this class is both used by the client and > server. > > But the server should do some ATT_MTU detection and then call > SetDataPacketSize > > to set packet_size_ according to the max packet size it observes. Then the > > server will send the Response message with the packet_size_ as the maximum > > packet size of the connection, which would be used to chop up data packets > from > > either the server or the client's perspective > > > > On the client side, the receiver should parse out the packet_size_ from the > > server response and use it as maximum packet size to chop up and send data > > How about this: just don't use packet_size_ for the connection response and use > it only for data packets. Then, for this function, have this function take an > optional parameter which defaults to kDefaultDataPacketSize. We can talk more about this when you get back. I don't really understand your comment. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:78: uint32_t packet_size_; On 2016/06/09 22:55:43, Kyle Horimoto wrote: > On 2016/06/08 21:30:36, jingxuy wrote: > > On 2016/06/08 00:06:29, Kyle Horimoto wrote: > > > On 2016/06/06 19:30:55, Kyle Horimoto wrote: > > > > This is only for data packets, right? If so, please change its name > > > accordingly. > > > > > > Ping. > > > > They are already addressed. > > I addressed this in the .cc, but please make this data packets and change the > name. Since we have this discussion elsewhere, will close this comment https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:107: uint32_t payload_size = packet_size_ - 1; On 2016/06/09 22:55:43, Kyle Horimoto wrote: > s/payload_size/payload_size_per_packet/ Done. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:112: uint32_t num_packets = (message.length() + payload_size) / payload_size; On 2016/06/09 22:55:43, Kyle Horimoto wrote: > nit: Please just use the computation you wrote above in the comment. It will > make your code more readable. And don't worry, a compiler pass will optimize > your code so it won't be any more inefficient. Done. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:116: uint32_t last_byte = message.length() + 1; On 2016/06/09 22:55:43, Kyle Horimoto wrote: > This name is confusing because it implies that this is the value of the last > byte. How about num_bytes_in_message? Done. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:121: uint32_t begin = payload_size * i; On 2016/06/09 22:55:43, Kyle Horimoto wrote: > nit: Move the begin/end calculations to right before you use them. I don't understand the comment. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:142: uint32_t index, On 2016/06/09 22:55:43, Kyle Horimoto wrote: > s/index/byte_offset Done. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:148: uint8_t upper = val >> 8 & 0xFF; On 2016/06/09 22:55:43, Kyle Horimoto wrote: > nit: Use parentheses to make it clear which operation is performed first. Done. https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:16: On 2016/06/09 22:45:35, Tim Song wrote: > nit: consistent spacing for this block. Done. https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:29: BluetoothLowEnergyWeavePacketGenerator::Factory::factory_instance_ = On 2016/06/09 22:45:35, Tim Song wrote: > add a // static. comment for static variables and functions. Done. https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:29: BluetoothLowEnergyWeavePacketGenerator* BuildInstance(); On 2016/06/09 22:45:35, Tim Song wrote: > This should be virtual if you are overriding it in the subclass. You should also > return an std::unique_ptr rather than a raw pointer, as it isn't less efficient > and it clarifies ownership. this function is protected, the function that calls it which is NewInstance returns unique_ptr https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:64: std::unique_ptr<std::vector<Packet>> EncodeDataMessage(std::string message); On 2016/06/09 22:45:35, Tim Song wrote: > Just return a std::vector<Packet>. The compiler will optimize the returned value > to elide the copy operation: > http://stackoverflow.com/questions/3134831/in-c-is-it-still-bad-practice-to-r... The reason that it's a unique_ptr right now is because Kyle thinks it's better to clarify ownership https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:70: Packet* CreateControlPacket(); On 2016/06/09 22:45:35, Tim Song wrote: > Same here. I would return a unique_ptr even in private functions, as it's free > and clarifies ownership. this is a private helper function, I think it's fine if it does not have very interfacing return type https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:82: // The default data packet length is 20 unless setDataPacketLength() is called On 2016/06/09 22:45:35, Tim Song wrote: > nit: set should be capitalized. Done.
Sorry for the delay. Let me know if skipped some comments that was directed to me. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:144: On 2016/06/08 21:30:36, jingxuy wrote: > On 2016/06/08 00:06:29, Kyle Horimoto wrote: > > On 2016/06/07 01:19:05, jingxuy wrote: > > > On 2016/06/06 15:25:20, sacomoto wrote: > > > > In the uWeave protocol the integers sent in the control packet are encoded > > in > > > > network byte order (most significative byte first, big endian). > > > > > > > > So, before writing |val| into |packet| you should convert it to BigEndian > if > > > > necessary. > > > > > > Done. > > > > You can't assume the endianness of this machine. Please use htons(). > > what do you mean by endianness of this machine? I am fairly certain that in C > the upper byte is the upper byte independent of the machine? I am using a direct > uint16_t value not a byte stream. You are both right. The byte ordering is machine dependent (although almost all machines are little endian), and this affects how multibyte types are stored in memory. But the endianess doesn't matter once the values are loaded in the CPU registers (i.e. one shift to the right is equivalent to a division by 2, no matter the endianess): http://stackoverflow.com/questions/7184789/does-bit-shift-depend-on-endianness So, yes, Jing solution is correct in this case. However, if you don't rely on bit shifts, and uses the value directly from memory (like the code below). Then, you should take the endianess into account. uint8_t *ptr; ptr = (uint8_t *)&val; packet->at(index) = ptr[0]; packet->at(index + 1) = ptr[1]; The solution above is incorrect for a little endian machine. You have to explicitly convert the host endianess to big endian. uint8_t *ptr; uint8_t big_endian_val = htons(val); ptr = (uint8_t *)&big_endian_val; packet->at(index) = ptr[0]; packet->at(index + 1) = ptr[1]; I would probably use this solution as it avoids bit shifts and does the conversion explicitly, but it's a personal preference. Feel free to keep your solution, Jing. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:46: typedef std::vector<uint8_t> Packet; On 2016/06/08 21:30:36, jingxuy wrote: > On 2016/06/06 19:30:55, Kyle Horimoto wrote: > > Hmm, are we sure this works correctly regarding endianness? Will we have to > use > > htonl(), etc? sacomoto/msarda/tengs - have you dealt with this before? > > You can check out the example from the legacy implementation. I'm pretty sure > that the endianness only matters because of the protocol, like the protocol is > saying that lower bytes of short comes before upper bytes. But bytes are bytes. > They will sent and received in the same order. Jing is correct. The endianess only matter for multibyte types that are sent inside the control packets. The actual bytes sent are going to be sent and received in the same order. https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:11: #include "base/logging.h" Is this header really necesary? https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:17: const uint16_t kServerWeaveVersion = 1; This constant should not be necessary. See my comment for the |CreateConnectionResponse()| int header file. https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:112: // The message's length is the length of the string plus '\0'. The uWeave protocol does not require the bytes sent in a data packet to be terminated by '\0'. The data packet contains raw bytes, not C strings. https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:120: const char* byte_message = message.c_str(); You should not append the '\0' at the end of the message. This is probably harmless, as the other implementations will most likely simply ignore it, but we should stick to the protocol definition. DCHECK(messsage.size() != 0) in the beginning of the method or return nil in that case. And, more importantly, do not append '\0' to end of the packet. https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:43: }; Why PacketType and ControlCommand are public? https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:58: std::unique_ptr<Packet> CreateConnectionResponse(); This method should receive the chosen protocol version as a parameter. Without parsing the connection request packet, I don't see how this class could decide the right version to use. The connection request packet contains a min and max protocol version supported by the client. After receiving that packet, the server should reply with a connection response packet containing the maximum version supported by _both_ the server and the client. https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:106: PacketCounterTest) { Please generate at least 8 packets in this test. We should test that the packet counter wraps around 7 correctly.
Please update your CL title/description. As I've mentioned previously, your CL should be easily understood by someone who reads it years from now. Include background information, links to design documents, etc. Make it clear what is going on. https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:69: SetIntField(packet, 3, packet_size_); On 2016/06/11 00:49:32, jingxuy wrote: > On 2016/06/09 22:55:42, Kyle Horimoto wrote: > > On 2016/06/08 21:30:35, jingxuy wrote: > > > On 2016/06/06 19:30:54, Kyle Horimoto wrote: > > > > I'm confused by your use of packet_size_. It's used by data packets and > > > > connection responses? Can you please clarify this? > > > > > > Do you want me to put a comment above packet_size_? > > > > > > It is kind of confusing since this class is both used by the client and > > server. > > > But the server should do some ATT_MTU detection and then call > > SetDataPacketSize > > > to set packet_size_ according to the max packet size it observes. Then the > > > server will send the Response message with the packet_size_ as the maximum > > > packet size of the connection, which would be used to chop up data packets > > from > > > either the server or the client's perspective > > > > > > On the client side, the receiver should parse out the packet_size_ from the > > > server response and use it as maximum packet size to chop up and send data > > > > How about this: just don't use packet_size_ for the connection response and > use > > it only for data packets. Then, for this function, have this function take an > > optional parameter which defaults to kDefaultDataPacketSize. > > We can talk more about this when you get back. I don't really understand your > comment. Let me know when you are ready to discuss this. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:35: std::vector<uint8_t> expected(20, 0); On 2016/06/09 22:55:43, Kyle Horimoto wrote: > Use your Packet type here and below. Ping. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:35: std::vector<uint8_t> expected(20, 0); On 2016/06/09 22:55:43, Kyle Horimoto wrote: > Please use constants for 20, 0x80, 1, 1. For all hex values, you'll want to make > it a bitwise calculation to create the constant. > > Same for all other constants. Ping. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:54: expected[3] = 20; On 2016/06/09 22:55:43, Kyle Horimoto wrote: > Write another test which tests alternate packet sizes. Ping. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:66: ReasonForClose::APPLICATION_ERROR); On 2016/06/09 22:55:43, Kyle Horimoto wrote: > Test other reason values too. You can just make a helper function which just > takes the reason as a parameter and runs this test. Then, make a bunch of tests > that just call the helper function with different reason values. Ping. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:82: generator->SetDataPacketSize(4); On 2016/06/09 22:55:43, Kyle Horimoto wrote: > Also test the default packet size. Ping. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:105: TEST_F(ProximityAuthBluetoothLowEnergyWeavePacketGeneratorTest, On 2016/06/09 22:55:43, Kyle Horimoto wrote: > Please test the overflow from 7 back to 0. Ping. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:113: EXPECT_EQ(0x80, packet->at(0)); On 2016/06/09 22:55:43, Kyle Horimoto wrote: > This is hard to read. Make a helper function getCounterFromPacket(Packet) which > extracts the counter value and returns it. Ping. https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:29: BluetoothLowEnergyWeavePacketGenerator* BuildInstance(); On 2016/06/11 00:49:32, jingxuy wrote: > On 2016/06/09 22:45:35, Tim Song wrote: > > This should be virtual if you are overriding it in the subclass. You should > also > > return an std::unique_ptr rather than a raw pointer, as it isn't less > efficient > > and it clarifies ownership. > > this function is protected, the function that calls it which is NewInstance > returns unique_ptr You can just have NewInstance() return the unique_ptr instead of creating one too. https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:64: std::unique_ptr<std::vector<Packet>> EncodeDataMessage(std::string message); On 2016/06/11 00:49:32, jingxuy wrote: > On 2016/06/09 22:45:35, Tim Song wrote: > > Just return a std::vector<Packet>. The compiler will optimize the returned > value > > to elide the copy operation: > > > http://stackoverflow.com/questions/3134831/in-c-is-it-still-bad-practice-to-r... > > The reason that it's a unique_ptr right now is because Kyle thinks it's better > to clarify ownership Tim is saying to return a full vector (by value) instead of a pointer (by reference). That idea SGTM. https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:70: Packet* CreateControlPacket(); On 2016/06/11 00:49:32, jingxuy wrote: > On 2016/06/09 22:45:35, Tim Song wrote: > > Same here. I would return a unique_ptr even in private functions, as it's free > > and clarifies ownership. > > this is a private helper function, I think it's fine if it does not have very > interfacing return type It's generally a best practice to use this everywhere where dynamically-allocated memory is returned so that ownership is always clear, even if it is for a private function. https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:17: const uint16_t kServerWeaveVersion = 1; On 2016/06/13 16:05:19, sacomoto wrote: > This constant should not be necessary. See my comment for the > |CreateConnectionResponse()| int header file. Regarding your other comment: you are certainly correct that we need to ensure that we support the min/max versions, but we also only implement v1. So, I think the right thing to do is check that v1 is >= the min version passed and <= the max version passed, then just use it.
Description was changed from ========== Weave Packet Generator This CL adds a packet generator that respects the Weave.BLE protocol. BUG=617238 ========== to ========== Weave Packet Generator This CL adds a packet generator that respects the uWeave.BLE protocol. The packet generator accepts raw bytes as messages to be sent and wraps the message with the uWeave protocol. The uWeave packets include control packets of request/response/close of a connection and data packets that are the original message chopped up to fixed size packets with uWeave protocol headers. Design Doc of ProximityAuth uWeave Migration: https://docs.google.com/a/google.com/document/d/1HJ1fipxsNQwIxmKs-5_6TjroCAcs... BUG=617238 ==========
https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/40001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:144: On 2016/06/13 16:05:18, sacomoto wrote: > On 2016/06/08 21:30:36, jingxuy wrote: > > On 2016/06/08 00:06:29, Kyle Horimoto wrote: > > > On 2016/06/07 01:19:05, jingxuy wrote: > > > > On 2016/06/06 15:25:20, sacomoto wrote: > > > > > In the uWeave protocol the integers sent in the control packet are > encoded > > > in > > > > > network byte order (most significative byte first, big endian). > > > > > > > > > > So, before writing |val| into |packet| you should convert it to > BigEndian > > if > > > > > necessary. > > > > > > > > Done. > > > > > > You can't assume the endianness of this machine. Please use htons(). > > > > what do you mean by endianness of this machine? I am fairly certain that in C > > the upper byte is the upper byte independent of the machine? I am using a > direct > > uint16_t value not a byte stream. > > You are both right. The byte ordering is machine dependent (although almost all > machines are little endian), and this affects how multibyte types are stored in > memory. But the endianess doesn't matter once the values are loaded in the CPU > registers (i.e. one shift to the right is equivalent to a division by 2, no > matter the endianess): > > http://stackoverflow.com/questions/7184789/does-bit-shift-depend-on-endianness > > So, yes, Jing solution is correct in this case. > > However, if you don't rely on bit shifts, and uses the value directly from > memory (like the code below). Then, you should take the endianess into account. > > uint8_t *ptr; > ptr = (uint8_t *)&val; > packet->at(index) = ptr[0]; > packet->at(index + 1) = ptr[1]; > > The solution above is incorrect for a little endian machine. You have to > explicitly convert the host endianess to big endian. > > uint8_t *ptr; > uint8_t big_endian_val = htons(val); > ptr = (uint8_t *)&big_endian_val; > packet->at(index) = ptr[0]; > packet->at(index + 1) = ptr[1]; > > I would probably use this solution as it avoids bit shifts and does the > conversion explicitly, but it's a personal preference. Feel free to keep your > solution, Jing. Acknowledged. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:35: std::vector<uint8_t> expected(20, 0); On 2016/06/13 17:55:53, Kyle Horimoto wrote: > On 2016/06/09 22:55:43, Kyle Horimoto wrote: > > Use your Packet type here and below. > > Ping. Done. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:54: expected[3] = 20; On 2016/06/13 17:55:53, Kyle Horimoto wrote: > On 2016/06/09 22:55:43, Kyle Horimoto wrote: > > Write another test which tests alternate packet sizes. > > Ping. Done. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:66: ReasonForClose::APPLICATION_ERROR); On 2016/06/09 22:55:43, Kyle Horimoto wrote: > Test other reason values too. You can just make a helper function which just > takes the reason as a parameter and runs this test. Then, make a bunch of tests > that just call the helper function with different reason values. Done. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:82: generator->SetDataPacketSize(4); On 2016/06/13 17:55:53, Kyle Horimoto wrote: > On 2016/06/09 22:55:43, Kyle Horimoto wrote: > > Also test the default packet size. > > Ping. Done. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:105: TEST_F(ProximityAuthBluetoothLowEnergyWeavePacketGeneratorTest, On 2016/06/13 17:55:53, Kyle Horimoto wrote: > On 2016/06/09 22:55:43, Kyle Horimoto wrote: > > Please test the overflow from 7 back to 0. > > Ping. Done. https://codereview.chromium.org/2031953003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:113: EXPECT_EQ(0x80, packet->at(0)); On 2016/06/09 22:55:43, Kyle Horimoto wrote: > This is hard to read. Make a helper function getCounterFromPacket(Packet) which > extracts the counter value and returns it. Done. https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:29: BluetoothLowEnergyWeavePacketGenerator* BuildInstance(); On 2016/06/13 17:55:53, Kyle Horimoto wrote: > On 2016/06/11 00:49:32, jingxuy wrote: > > On 2016/06/09 22:45:35, Tim Song wrote: > > > This should be virtual if you are overriding it in the subclass. You should > > also > > > return an std::unique_ptr rather than a raw pointer, as it isn't less > > efficient > > > and it clarifies ownership. > > > > this function is protected, the function that calls it which is NewInstance > > returns unique_ptr > > You can just have NewInstance() return the unique_ptr instead of creating one > too. Done. https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:64: std::unique_ptr<std::vector<Packet>> EncodeDataMessage(std::string message); On 2016/06/13 17:55:53, Kyle Horimoto wrote: > On 2016/06/11 00:49:32, jingxuy wrote: > > On 2016/06/09 22:45:35, Tim Song wrote: > > > Just return a std::vector<Packet>. The compiler will optimize the returned > > value > > > to elide the copy operation: > > > > > > http://stackoverflow.com/questions/3134831/in-c-is-it-still-bad-practice-to-r... > > > > The reason that it's a unique_ptr right now is because Kyle thinks it's better > > to clarify ownership > > Tim is saying to return a full vector (by value) instead of a pointer (by > reference). That idea SGTM. Hmmm, maybe I was unclear when I asked the question but I was asking why we needed the unique_ptr wrapper around just std::vector and I thought you wanted a conceptual ownership thing. Now it makes more sense. Done. https://codereview.chromium.org/2031953003/diff/180001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:70: Packet* CreateControlPacket(); On 2016/06/13 17:55:53, Kyle Horimoto wrote: > On 2016/06/11 00:49:32, jingxuy wrote: > > On 2016/06/09 22:45:35, Tim Song wrote: > > > Same here. I would return a unique_ptr even in private functions, as it's > free > > > and clarifies ownership. > > > > this is a private helper function, I think it's fine if it does not have very > > interfacing return type > > It's generally a best practice to use this everywhere where > dynamically-allocated memory is returned so that ownership is always clear, even > if it is for a private function. Done. https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:11: #include "base/logging.h" On 2016/06/13 16:05:19, sacomoto wrote: > Is this header really necesary? it is used for DCHECK https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:17: const uint16_t kServerWeaveVersion = 1; On 2016/06/13 17:55:53, Kyle Horimoto wrote: > On 2016/06/13 16:05:19, sacomoto wrote: > > This constant should not be necessary. See my comment for the > > |CreateConnectionResponse()| int header file. > > Regarding your other comment: you are certainly correct that we need to ensure > that we support the min/max versions, but we also only implement v1. So, I think > the right thing to do is check that v1 is >= the min version passed and <= the > max version passed, then just use it. see my comment in the header file. https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:112: // The message's length is the length of the string plus '\0'. On 2016/06/13 16:05:18, sacomoto wrote: > The uWeave protocol does not require the bytes sent in a data packet to be > terminated by '\0'. The data packet contains raw bytes, not C strings. I had a misconception thinking that we are sending the whole string. You are right that the string is a byte array without the '\0' https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:120: const char* byte_message = message.c_str(); On 2016/06/13 16:05:19, sacomoto wrote: > You should not append the '\0' at the end of the message. This is probably > harmless, as the other implementations will most likely simply ignore it, but we > should stick to the protocol definition. > > DCHECK(messsage.size() != 0) in the beginning of the method or return nil in > that case. And, more importantly, do not append '\0' to end of the packet. Done. https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:43: }; On 2016/06/13 16:05:19, sacomoto wrote: > Why PacketType and ControlCommand are public? It's part of a todo in the receiver. the commonalities b/t generator and receiver will be moved to a separate class https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:58: std::unique_ptr<Packet> CreateConnectionResponse(); On 2016/06/13 16:05:19, sacomoto wrote: > This method should receive the chosen protocol version as a parameter. Without > parsing the connection request packet, I don't see how this class could decide > the right version to use. > > The connection request packet contains a min and max protocol version supported > by the client. After receiving that packet, the server should reply with a > connection response packet containing the maximum version supported by _both_ > the server and the client. Initially the protocol version is a parameter, but Kyle suggested that since there is only 1 version right now, it's okay to leave it off. Since if there is another version, the class will undergo significant enough changes that supporting versions that it's not necessary right now to support versions that don't exist. https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:106: PacketCounterTest) { On 2016/06/13 16:05:19, sacomoto wrote: > Please generate at least 8 packets in this test. We should test that the packet > counter wraps around 7 correctly. Done.
https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:64: SetControlCommand(&packet, ControlCommand::CONNECTION_REQUEST); Instead of passing |&packet| everywhere, just make the Set*() functions take a |Packet&| as a parameter and pass |packet|. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:109: std::string message) { Should we throw an error here if message is empty? I don't think it makes sense to return a packet if there was no message. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:17: using proximity_auth::BluetoothLowEnergyWeavePacketGenerator; I don't think we should do this. Generally it's only advisable to use "using" if the namespace is really long and makes your code difficult to write on one <=80 character line. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:41: expected[1] = expected_reason_for_close; It looks like the connection close packet is 3 bytes (1-byte header and 2-byte reason). Shouldn't we be setting expected[2] in this case? https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:75: std::vector<uint8_t> packet = generator->CreateConnectionRequest(); Can we just use Packet instead of vector<>? Same elsewhere. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:307: for (uint8_t i = 0; i < kDataLength; ++i) { This doesn't actually test it wrapping around. You need to have at least 9 packets for it to wrap around.
https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:43: }; On 2016/06/17 00:56:45, jingxuy wrote: > On 2016/06/13 16:05:19, sacomoto wrote: > > Why PacketType and ControlCommand are public? > > It's part of a todo in the receiver. the commonalities b/t generator and > receiver will be moved to a separate class Ok, that makes sense. https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:58: std::unique_ptr<Packet> CreateConnectionResponse(); On 2016/06/17 00:56:45, jingxuy wrote: > On 2016/06/13 16:05:19, sacomoto wrote: > > This method should receive the chosen protocol version as a parameter. Without > > parsing the connection request packet, I don't see how this class could decide > > the right version to use. > > > > The connection request packet contains a min and max protocol version > supported > > by the client. After receiving that packet, the server should reply with a > > connection response packet containing the maximum version supported by _both_ > > the server and the client. > > Initially the protocol version is a parameter, but Kyle suggested that since > there is only 1 version right now, it's okay to leave it off. Since if there is > another version, the class will undergo significant enough changes that > supporting versions that it's not necessary right now to support versions that > don't exist. Ok, that makes sense. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:104: DCHECK(size >= 2); s/2/20. The BLE specs guarantee at least 20 for the MTU. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:109: std::string message) { On 2016/06/17 04:04:12, Kyle Horimoto wrote: > Should we throw an error here if message is empty? I don't think it makes sense > to return a packet if there was no message. +1 I suggested a DCHECK in an earlier comment, but an error is fine too.
https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:43: }; On 2016/06/17 15:38:04, sacomoto wrote: > On 2016/06/17 00:56:45, jingxuy wrote: > > On 2016/06/13 16:05:19, sacomoto wrote: > > > Why PacketType and ControlCommand are public? > > > > It's part of a todo in the receiver. the commonalities b/t generator and > > receiver will be moved to a separate class > > Ok, that makes sense. Done. https://codereview.chromium.org/2031953003/diff/240001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:58: std::unique_ptr<Packet> CreateConnectionResponse(); On 2016/06/17 15:38:04, sacomoto wrote: > On 2016/06/17 00:56:45, jingxuy wrote: > > On 2016/06/13 16:05:19, sacomoto wrote: > > > This method should receive the chosen protocol version as a parameter. > Without > > > parsing the connection request packet, I don't see how this class could > decide > > > the right version to use. > > > > > > The connection request packet contains a min and max protocol version > > supported > > > by the client. After receiving that packet, the server should reply with a > > > connection response packet containing the maximum version supported by > _both_ > > > the server and the client. > > > > Initially the protocol version is a parameter, but Kyle suggested that since > > there is only 1 version right now, it's okay to leave it off. Since if there > is > > another version, the class will undergo significant enough changes that > > supporting versions that it's not necessary right now to support versions that > > don't exist. > > Ok, that makes sense. Done. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:104: DCHECK(size >= 2); On 2016/06/17 15:38:04, sacomoto wrote: > s/2/20. > > The BLE specs guarantee at least 20 for the MTU. I think 20 is the guaranteed max size, the 2 is just a sanity check for the min size. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:109: std::string message) { On 2016/06/17 15:38:04, sacomoto wrote: > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > Should we throw an error here if message is empty? I don't think it makes > sense > > to return a packet if there was no message. > > +1 > > I suggested a DCHECK in an earlier comment, but an error is fine too. Done.
https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:64: SetControlCommand(&packet, ControlCommand::CONNECTION_REQUEST); On 2016/06/17 04:04:12, Kyle Horimoto wrote: > Instead of passing |&packet| everywhere, just make the Set*() functions take a > |Packet&| as a parameter and pass |packet|. Ping. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:104: DCHECK(size >= 2); On 2016/06/17 18:59:00, jingxuy wrote: > On 2016/06/17 15:38:04, sacomoto wrote: > > s/2/20. > > > > The BLE specs guarantee at least 20 for the MTU. > > I think 20 is the guaranteed max size, the 2 is just a sanity check for the min > size. Nope - 20 should be the minimum, not the maximum. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:17: using proximity_auth::BluetoothLowEnergyWeavePacketGenerator; On 2016/06/17 04:04:12, Kyle Horimoto wrote: > I don't think we should do this. Generally it's only advisable to use "using" if > the namespace is really long and makes your code difficult to write on one <=80 > character line. Ping. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:41: expected[1] = expected_reason_for_close; On 2016/06/17 04:04:12, Kyle Horimoto wrote: > It looks like the connection close packet is 3 bytes (1-byte header and 2-byte > reason). Shouldn't we be setting expected[2] in this case? Ping. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:75: std::vector<uint8_t> packet = generator->CreateConnectionRequest(); On 2016/06/17 04:04:12, Kyle Horimoto wrote: > Can we just use Packet instead of vector<>? Same elsewhere. Ping. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:307: for (uint8_t i = 0; i < kDataLength; ++i) { On 2016/06/17 04:04:12, Kyle Horimoto wrote: > This doesn't actually test it wrapping around. You need to have at least 9 > packets for it to wrap around. Ping.
https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:64: SetControlCommand(&packet, ControlCommand::CONNECTION_REQUEST); On 2016/06/20 18:01:23, Kyle Horimoto wrote: > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > Instead of passing |&packet| everywhere, just make the Set*() functions take a > > |Packet&| as a parameter and pass |packet|. > > Ping. Is it the convention that if the caller expects a parameter to be modified, it should be passed around as a pointer. Maybe instead I could do Packet* packet = &(CreaterControlPacket(kMinConnectionRequestSize); https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:104: DCHECK(size >= 2); On 2016/06/20 18:01:23, Kyle Horimoto wrote: > On 2016/06/17 18:59:00, jingxuy wrote: > > On 2016/06/17 15:38:04, sacomoto wrote: > > > s/2/20. > > > > > > The BLE specs guarantee at least 20 for the MTU. > > > > I think 20 is the guaranteed max size, the 2 is just a sanity check for the > min > > size. > > Nope - 20 should be the minimum, not the maximum. I think 20 is the minimum maximum. As the receiver must be able to receive packets up to 20 bytes. But I think creating packets of less than 20 bytes is legal. Though I'm not sure if it's necessary to allow that implementation in the generator. I left it there for the flexibility https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:17: using proximity_auth::BluetoothLowEnergyWeavePacketGenerator; On 2016/06/20 18:01:23, Kyle Horimoto wrote: > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > I don't think we should do this. Generally it's only advisable to use "using" > if > > the namespace is really long and makes your code difficult to write on one > <=80 > > character line. > > Ping. Because of the helper function, there will be a lot of proximity_auth::BluetoothLowEnergyWeavePacketGenerator repeated. So I thought a using is appropriate https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:41: expected[1] = expected_reason_for_close; On 2016/06/20 18:01:23, Kyle Horimoto wrote: > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > It looks like the connection close packet is 3 bytes (1-byte header and 2-byte > > reason). Shouldn't we be setting expected[2] in this case? > > Ping. it's the network endian https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:75: std::vector<uint8_t> packet = generator->CreateConnectionRequest(); On 2016/06/20 18:01:23, Kyle Horimoto wrote: > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > Can we just use Packet instead of vector<>? Same elsewhere. > > Ping. I think vector makes it more clear what the return type of the class actually is and what functions are defined on it. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:307: for (uint8_t i = 0; i < kDataLength; ++i) { On 2016/06/20 18:01:23, Kyle Horimoto wrote: > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > This doesn't actually test it wrapping around. You need to have at least 9 > > packets for it to wrap around. > > Ping. Done.
https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:64: SetControlCommand(&packet, ControlCommand::CONNECTION_REQUEST); On 2016/06/20 21:29:32, jingxuy wrote: > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > Instead of passing |&packet| everywhere, just make the Set*() functions take > a > > > |Packet&| as a parameter and pass |packet|. > > > > Ping. > > Is it the convention that if the caller expects a parameter to be modified, it > should be passed around as a pointer. > Maybe instead I could do Packet* packet = > &(CreaterControlPacket(kMinConnectionRequestSize); Ah, you're right. Nope, please leave it as-is. However, the convention is actually: FunctionName(<value parameters>, <input parameters>, <output parameters) So, you should always be passing the packet as the last parameter to these functions since it is an output parameter. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:104: DCHECK(size >= 2); On 2016/06/20 21:29:32, jingxuy wrote: > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > On 2016/06/17 18:59:00, jingxuy wrote: > > > On 2016/06/17 15:38:04, sacomoto wrote: > > > > s/2/20. > > > > > > > > The BLE specs guarantee at least 20 for the MTU. > > > > > > I think 20 is the guaranteed max size, the 2 is just a sanity check for the > > min > > > size. > > > > Nope - 20 should be the minimum, not the maximum. > > I think 20 is the minimum maximum. As the receiver must be able to receive > packets up to 20 bytes. But I think creating packets of less than 20 bytes is > legal. Though I'm not sure if it's necessary to allow that implementation in the > generator. I left it there for the flexibility I think there's a misunderstanding. We aren't saying that sending a data packet <20 bytes is illegal - it certainly is legal to do so. This SetDataPacketSize() function is setting the maximum packet size allowed. By default, this is set to 20 bytes, but this can be increased based on the "connection request" packet. It might be a good idea to rename this function so that it is clear that you are setting the size of a "full" packet and that it is still valid to send data packets which are shorter than that value. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:17: using proximity_auth::BluetoothLowEnergyWeavePacketGenerator; On 2016/06/20 21:29:32, jingxuy wrote: > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > I don't think we should do this. Generally it's only advisable to use > "using" > > if > > > the namespace is really long and makes your code difficult to write on one > > <=80 > > > character line. > > > > Ping. > > Because of the helper function, there will be a lot of > proximity_auth::BluetoothLowEnergyWeavePacketGenerator repeated. So I thought a > using is appropriate Repeating proximity_auth:: isn't a bad thing - it makes it clear what type you are referring to. In general, you should not do this, but if this results in cleaner looking code (since you won't have to use line wraps as often in the middle of a single line of code), then it's fine by me. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:41: expected[1] = expected_reason_for_close; On 2016/06/20 21:29:32, jingxuy wrote: > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > It looks like the connection close packet is 3 bytes (1-byte header and > 2-byte > > > reason). Shouldn't we be setting expected[2] in this case? > > > > Ping. > > it's the network endian In that case, please read both values into a uint16_t and use htons() and ntohs() to do this. Your code shouldn't make assumptions about what the host- or network-endianness are. Please also incorporate use of these functions into your implementation file. Thanks! https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:75: std::vector<uint8_t> packet = generator->CreateConnectionRequest(); On 2016/06/20 21:29:32, jingxuy wrote: > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > Can we just use Packet instead of vector<>? Same elsewhere. > > > > Ping. > > I think vector makes it more clear what the return type of the class actually is > and what functions are defined on it. If so, you should get rid of your Packet typedef entirely. If you make a type to be used, you should use it. Using it in some places and not using it in other places results in a lack of uniformity which makes your code harder to read. So, either remove it entirely or use it everywhere where it is applicable. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:307: for (uint8_t i = 0; i < kDataLength; ++i) { On 2016/06/20 21:29:32, jingxuy wrote: > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > This doesn't actually test it wrapping around. You need to have at least 9 > > > packets for it to wrap around. > > > > Ping. > > Done. I don't see a change here. Did you forget to upload it?
https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:17: using proximity_auth::BluetoothLowEnergyWeavePacketGenerator; On 2016/06/20 21:52:17, Kyle Horimoto wrote: > On 2016/06/20 21:29:32, jingxuy wrote: > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > I don't think we should do this. Generally it's only advisable to use > > "using" > > > if > > > > the namespace is really long and makes your code difficult to write on one > > > <=80 > > > > character line. > > > > > > Ping. > > > > Because of the helper function, there will be a lot of > > proximity_auth::BluetoothLowEnergyWeavePacketGenerator repeated. So I thought > a > > using is appropriate > > Repeating proximity_auth:: isn't a bad thing - it makes it clear what type you > are referring to. In general, you should not do this, but if this results in > cleaner looking code (since you won't have to use line wraps as often in the > middle of a single line of code), then it's fine by me. Done. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:41: expected[1] = expected_reason_for_close; On 2016/06/20 21:52:17, Kyle Horimoto wrote: > On 2016/06/20 21:29:32, jingxuy wrote: > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > It looks like the connection close packet is 3 bytes (1-byte header and > > 2-byte > > > > reason). Shouldn't we be setting expected[2] in this case? > > > > > > Ping. > > > > it's the network endian > > In that case, please read both values into a uint16_t and use htons() and > ntohs() to do this. Your code shouldn't make assumptions about what the host- or > network-endianness are. > > Please also incorporate use of these functions into your implementation file. > Thanks! what do you mean? This is specified by the weave spec. We had a discussion about this with Gustavo earlier in the generator. If you are talking about I couldn't assume network-endianness, I don't think it would ever be reasonable for network-endianness to change. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:75: std::vector<uint8_t> packet = generator->CreateConnectionRequest(); On 2016/06/20 21:52:17, Kyle Horimoto wrote: > On 2016/06/20 21:29:32, jingxuy wrote: > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > Can we just use Packet instead of vector<>? Same elsewhere. > > > > > > Ping. > > > > I think vector makes it more clear what the return type of the class actually > is > > and what functions are defined on it. > > If so, you should get rid of your Packet typedef entirely. If you make a type to > be used, you should use it. Using it in some places and not using it in other > places results in a lack of uniformity which makes your code harder to read. So, > either remove it entirely or use it everywhere where it is applicable. I was intending it to be used as a shorthand and conceptually clearer type . It should only be used inside the h and cc file and shouldn't be used by an external code. I will think about this... https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:307: for (uint8_t i = 0; i < kDataLength; ++i) { On 2016/06/20 21:52:17, Kyle Horimoto wrote: > On 2016/06/20 21:29:32, jingxuy wrote: > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > This doesn't actually test it wrapping around. You need to have at least 9 > > > > packets for it to wrap around. > > > > > > Ping. > > > > Done. > > I don't see a change here. Did you forget to upload it? Done.
https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:64: SetControlCommand(&packet, ControlCommand::CONNECTION_REQUEST); On 2016/06/20 21:52:17, Kyle Horimoto wrote: > On 2016/06/20 21:29:32, jingxuy wrote: > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > Instead of passing |&packet| everywhere, just make the Set*() functions > take > > a > > > > |Packet&| as a parameter and pass |packet|. > > > > > > Ping. > > > > Is it the convention that if the caller expects a parameter to be modified, it > > should be passed around as a pointer. > > Maybe instead I could do Packet* packet = > > &(CreaterControlPacket(kMinConnectionRequestSize); > > Ah, you're right. Nope, please leave it as-is. However, the convention is > actually: > > FunctionName(<value parameters>, <input parameters>, <output parameters) > > So, you should always be passing the packet as the last parameter to these > functions since it is an output parameter. Ping. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:104: DCHECK(size >= 2); On 2016/06/20 21:52:17, Kyle Horimoto wrote: > On 2016/06/20 21:29:32, jingxuy wrote: > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > On 2016/06/17 18:59:00, jingxuy wrote: > > > > On 2016/06/17 15:38:04, sacomoto wrote: > > > > > s/2/20. > > > > > > > > > > The BLE specs guarantee at least 20 for the MTU. > > > > > > > > I think 20 is the guaranteed max size, the 2 is just a sanity check for > the > > > min > > > > size. > > > > > > Nope - 20 should be the minimum, not the maximum. > > > > I think 20 is the minimum maximum. As the receiver must be able to receive > > packets up to 20 bytes. But I think creating packets of less than 20 bytes is > > legal. Though I'm not sure if it's necessary to allow that implementation in > the > > generator. I left it there for the flexibility > > I think there's a misunderstanding. We aren't saying that sending a data packet > <20 bytes is illegal - it certainly is legal to do so. This SetDataPacketSize() > function is setting the maximum packet size allowed. By default, this is set to > 20 bytes, but this can be increased based on the "connection request" packet. > > It might be a good idea to rename this function so that it is clear that you are > setting the size of a "full" packet and that it is still valid to send data > packets which are shorter than that value. Ping. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:41: expected[1] = expected_reason_for_close; On 2016/06/21 01:46:56, jingxuy wrote: > On 2016/06/20 21:52:17, Kyle Horimoto wrote: > > On 2016/06/20 21:29:32, jingxuy wrote: > > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > > It looks like the connection close packet is 3 bytes (1-byte header and > > > 2-byte > > > > > reason). Shouldn't we be setting expected[2] in this case? > > > > > > > > Ping. > > > > > > it's the network endian > > > > In that case, please read both values into a uint16_t and use htons() and > > ntohs() to do this. Your code shouldn't make assumptions about what the host- > or > > network-endianness are. > > > > Please also incorporate use of these functions into your implementation file. > > Thanks! > > what do you mean? This is specified by the weave spec. We had a discussion about > this with Gustavo earlier in the generator. If you are talking about I couldn't > assume network-endianness, I don't think it would ever be reasonable for > network-endianness to change. Network endianness is big-endian, though. Shouldn't they be in the opposite order? https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:307: for (uint8_t i = 0; i < kDataLength; ++i) { On 2016/06/21 01:46:56, jingxuy wrote: > On 2016/06/20 21:52:17, Kyle Horimoto wrote: > > On 2016/06/20 21:29:32, jingxuy wrote: > > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > > This doesn't actually test it wrapping around. You need to have at least > 9 > > > > > packets for it to wrap around. > > > > > > > > Ping. > > > > > > Done. > > > > I don't see a change here. Did you forget to upload it? > > Done. Your newest patchset that you just uploaded doesn't have the unit test anymore :/
https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:64: SetControlCommand(&packet, ControlCommand::CONNECTION_REQUEST); On 2016/06/21 02:15:29, Kyle Horimoto wrote: > On 2016/06/20 21:52:17, Kyle Horimoto wrote: > > On 2016/06/20 21:29:32, jingxuy wrote: > > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > > Instead of passing |&packet| everywhere, just make the Set*() functions > > take > > > a > > > > > |Packet&| as a parameter and pass |packet|. > > > > > > > > Ping. > > > > > > Is it the convention that if the caller expects a parameter to be modified, > it > > > should be passed around as a pointer. > > > Maybe instead I could do Packet* packet = > > > &(CreaterControlPacket(kMinConnectionRequestSize); > > > > Ah, you're right. Nope, please leave it as-is. However, the convention is > > actually: > > > > FunctionName(<value parameters>, <input parameters>, <output parameters) > > > > So, you should always be passing the packet as the last parameter to these > > functions since it is an output parameter. > > Ping. Done. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:104: DCHECK(size >= 2); On 2016/06/21 02:15:29, Kyle Horimoto wrote: > On 2016/06/20 21:52:17, Kyle Horimoto wrote: > > On 2016/06/20 21:29:32, jingxuy wrote: > > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > > On 2016/06/17 18:59:00, jingxuy wrote: > > > > > On 2016/06/17 15:38:04, sacomoto wrote: > > > > > > s/2/20. > > > > > > > > > > > > The BLE specs guarantee at least 20 for the MTU. > > > > > > > > > > I think 20 is the guaranteed max size, the 2 is just a sanity check for > > the > > > > min > > > > > size. > > > > > > > > Nope - 20 should be the minimum, not the maximum. > > > > > > I think 20 is the minimum maximum. As the receiver must be able to receive > > > packets up to 20 bytes. But I think creating packets of less than 20 bytes > is > > > legal. Though I'm not sure if it's necessary to allow that implementation in > > the > > > generator. I left it there for the flexibility > > > > I think there's a misunderstanding. We aren't saying that sending a data > packet > > <20 bytes is illegal - it certainly is legal to do so. This > SetDataPacketSize() > > function is setting the maximum packet size allowed. By default, this is set > to > > 20 bytes, but this can be increased based on the "connection request" packet. > > > > It might be a good idea to rename this function so that it is clear that you > are > > setting the size of a "full" packet and that it is still valid to send data > > packets which are shorter than that value. > > Ping. I knew what you guys meant. I just think we should restrict the max packet size sent. But now I think of it, the packet_size_ parameter is also used in control packet. So I suppose sending something less than 20 doesn't make any sense. So I will change the DCHECK to be 20 https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:41: expected[1] = expected_reason_for_close; On 2016/06/21 02:15:29, Kyle Horimoto wrote: > On 2016/06/21 01:46:56, jingxuy wrote: > > On 2016/06/20 21:52:17, Kyle Horimoto wrote: > > > On 2016/06/20 21:29:32, jingxuy wrote: > > > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > > > It looks like the connection close packet is 3 bytes (1-byte header > and > > > > 2-byte > > > > > > reason). Shouldn't we be setting expected[2] in this case? > > > > > > > > > > Ping. > > > > > > > > it's the network endian > > > > > > In that case, please read both values into a uint16_t and use htons() and > > > ntohs() to do this. Your code shouldn't make assumptions about what the > host- > > or > > > network-endianness are. > > > > > > Please also incorporate use of these functions into your implementation > file. > > > Thanks! > > > > what do you mean? This is specified by the weave spec. We had a discussion > about > > this with Gustavo earlier in the generator. If you are talking about I > couldn't > > assume network-endianness, I don't think it would ever be reasonable for > > network-endianness to change. > > Network endianness is big-endian, though. Shouldn't they be in the opposite > order? You can take a look at the cc file that Gustavo okayed. I am pretty sure the lower byte comes first in the packet. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:307: for (uint8_t i = 0; i < kDataLength; ++i) { On 2016/06/21 02:15:29, Kyle Horimoto wrote: > On 2016/06/21 01:46:56, jingxuy wrote: > > On 2016/06/20 21:52:17, Kyle Horimoto wrote: > > > On 2016/06/20 21:29:32, jingxuy wrote: > > > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > > > This doesn't actually test it wrapping around. You need to have at > least > > 9 > > > > > > packets for it to wrap around. > > > > > > > > > > Ping. > > > > > > > > Done. > > > > > > I don't see a change here. Did you forget to upload it? > > > > Done. > > Your newest patchset that you just uploaded doesn't have the unit test anymore > :/ Done.
https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:41: expected[1] = expected_reason_for_close; On 2016/06/21 06:03:11, jingxuy wrote: > On 2016/06/21 02:15:29, Kyle Horimoto wrote: > > On 2016/06/21 01:46:56, jingxuy wrote: > > > On 2016/06/20 21:52:17, Kyle Horimoto wrote: > > > > On 2016/06/20 21:29:32, jingxuy wrote: > > > > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > > > > It looks like the connection close packet is 3 bytes (1-byte header > > and > > > > > 2-byte > > > > > > > reason). Shouldn't we be setting expected[2] in this case? > > > > > > > > > > > > Ping. > > > > > > > > > > it's the network endian > > > > > > > > In that case, please read both values into a uint16_t and use htons() and > > > > ntohs() to do this. Your code shouldn't make assumptions about what the > > host- > > > or > > > > network-endianness are. > > > > > > > > Please also incorporate use of these functions into your implementation > > file. > > > > Thanks! > > > > > > what do you mean? This is specified by the weave spec. We had a discussion > > about > > > this with Gustavo earlier in the generator. If you are talking about I > > couldn't > > > assume network-endianness, I don't think it would ever be reasonable for > > > network-endianness to change. > > > > Network endianness is big-endian, though. Shouldn't they be in the opposite > > order? > > You can take a look at the cc file that Gustavo okayed. I am pretty sure the > lower byte comes first in the packet. I just chatted with Gustavo, and he confirmed that the byte next to the header should be 0. But my point is that we shouldn't even have to think about this - the whole point of ntohs() and htons() is that we just use those functions and let them decide how the bytes should be ordered. Please use those functions here so that we know we are doing the correct thing and so that someone reading this code at a later time does not have to think about this to decide whether the bytes are in the right order or not. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:75: std::vector<uint8_t> packet = generator->CreateConnectionRequest(); On 2016/06/21 01:46:56, jingxuy wrote: > On 2016/06/20 21:52:17, Kyle Horimoto wrote: > > On 2016/06/20 21:29:32, jingxuy wrote: > > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > > Can we just use Packet instead of vector<>? Same elsewhere. > > > > > > > > Ping. > > > > > > I think vector makes it more clear what the return type of the class > actually > > is > > > and what functions are defined on it. > > > > If so, you should get rid of your Packet typedef entirely. If you make a type > to > > be used, you should use it. Using it in some places and not using it in other > > places results in a lack of uniformity which makes your code harder to read. > So, > > either remove it entirely or use it everywhere where it is applicable. > > I was intending it to be used as a shorthand and conceptually clearer type . It > should only be used inside the h and cc file and shouldn't be used by an > external code. I will think about this... Ping. https://codereview.chromium.org/2031953003/diff/380001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:147: void BluetoothLowEnergyWeavePacketGenerator::SetShortField(uint32_t byte_offset, Please use htons() here as well, thanks! https://codereview.chromium.org/2031953003/diff/380001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:265: const uint8_t kNumPackets = 9; Can we just increase this to something much bigger...say, 100? Just to make sure that it loops through multiple times properly.
https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:41: expected[1] = expected_reason_for_close; On 2016/06/21 17:17:30, Kyle Horimoto wrote: > On 2016/06/21 06:03:11, jingxuy wrote: > > On 2016/06/21 02:15:29, Kyle Horimoto wrote: > > > On 2016/06/21 01:46:56, jingxuy wrote: > > > > On 2016/06/20 21:52:17, Kyle Horimoto wrote: > > > > > On 2016/06/20 21:29:32, jingxuy wrote: > > > > > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > > > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > > > > > It looks like the connection close packet is 3 bytes (1-byte > header > > > and > > > > > > 2-byte > > > > > > > > reason). Shouldn't we be setting expected[2] in this case? > > > > > > > > > > > > > > Ping. > > > > > > > > > > > > it's the network endian > > > > > > > > > > In that case, please read both values into a uint16_t and use htons() > and > > > > > ntohs() to do this. Your code shouldn't make assumptions about what the > > > host- > > > > or > > > > > network-endianness are. > > > > > > > > > > Please also incorporate use of these functions into your implementation > > > file. > > > > > Thanks! > > > > > > > > what do you mean? This is specified by the weave spec. We had a discussion > > > about > > > > this with Gustavo earlier in the generator. If you are talking about I > > > couldn't > > > > assume network-endianness, I don't think it would ever be reasonable for > > > > network-endianness to change. > > > > > > Network endianness is big-endian, though. Shouldn't they be in the opposite > > > order? > > > > You can take a look at the cc file that Gustavo okayed. I am pretty sure the > > lower byte comes first in the packet. > > I just chatted with Gustavo, and he confirmed that the byte next to the header > should be 0. > > But my point is that we shouldn't even have to think about this - the whole > point of ntohs() and htons() is that we just use those functions and let them > decide how the bytes should be ordered. Please use those functions here so that > we know we are doing the correct thing and so that someone reading this code at > a later time does not have to think about this to decide whether the bytes are > in the right order or not. The _upper_ byte of the short comes first. In your implementation of the BluetoothLowEnergyWeavePacketGenerator::SetIntField() you are also doing it: packet->at(index) = upper; packet->at(index + 1) = lower; As I said in the other comment, I agree with Kyle, I prefer a solution explicitly doing the conversions with htons().
https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:41: expected[1] = expected_reason_for_close; On 2016/06/21 17:39:22, sacomoto wrote: > On 2016/06/21 17:17:30, Kyle Horimoto wrote: > > On 2016/06/21 06:03:11, jingxuy wrote: > > > On 2016/06/21 02:15:29, Kyle Horimoto wrote: > > > > On 2016/06/21 01:46:56, jingxuy wrote: > > > > > On 2016/06/20 21:52:17, Kyle Horimoto wrote: > > > > > > On 2016/06/20 21:29:32, jingxuy wrote: > > > > > > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > > > > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > > > > > > It looks like the connection close packet is 3 bytes (1-byte > > header > > > > and > > > > > > > 2-byte > > > > > > > > > reason). Shouldn't we be setting expected[2] in this case? > > > > > > > > > > > > > > > > Ping. > > > > > > > > > > > > > > it's the network endian > > > > > > > > > > > > In that case, please read both values into a uint16_t and use htons() > > and > > > > > > ntohs() to do this. Your code shouldn't make assumptions about what > the > > > > host- > > > > > or > > > > > > network-endianness are. > > > > > > > > > > > > Please also incorporate use of these functions into your > implementation > > > > file. > > > > > > Thanks! > > > > > > > > > > what do you mean? This is specified by the weave spec. We had a > discussion > > > > about > > > > > this with Gustavo earlier in the generator. If you are talking about I > > > > couldn't > > > > > assume network-endianness, I don't think it would ever be reasonable for > > > > > network-endianness to change. > > > > > > > > Network endianness is big-endian, though. Shouldn't they be in the > opposite > > > > order? > > > > > > You can take a look at the cc file that Gustavo okayed. I am pretty sure the > > > lower byte comes first in the packet. > > > > I just chatted with Gustavo, and he confirmed that the byte next to the header > > should be 0. > > > > But my point is that we shouldn't even have to think about this - the whole > > point of ntohs() and htons() is that we just use those functions and let them > > decide how the bytes should be ordered. Please use those functions here so > that > > we know we are doing the correct thing and so that someone reading this code > at > > a later time does not have to think about this to decide whether the bytes are > > in the right order or not. > > The _upper_ byte of the short comes first. In your implementation of the > BluetoothLowEnergyWeavePacketGenerator::SetIntField() you are also doing it: > > packet->at(index) = upper; > packet->at(index + 1) = lower; > > As I said in the other comment, I agree with Kyle, I prefer a solution > explicitly doing the conversions with htons(). No that was my original implementation. Then Gustavo corrected me and now it is this packet->at(byte_offset) = lower; packet->at(byte_offset + 1) = upper;
https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:41: expected[1] = expected_reason_for_close; On 2016/06/21 17:47:31, jingxuy wrote: > On 2016/06/21 17:39:22, sacomoto wrote: > > On 2016/06/21 17:17:30, Kyle Horimoto wrote: > > > On 2016/06/21 06:03:11, jingxuy wrote: > > > > On 2016/06/21 02:15:29, Kyle Horimoto wrote: > > > > > On 2016/06/21 01:46:56, jingxuy wrote: > > > > > > On 2016/06/20 21:52:17, Kyle Horimoto wrote: > > > > > > > On 2016/06/20 21:29:32, jingxuy wrote: > > > > > > > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > > > > > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > > > > > > > It looks like the connection close packet is 3 bytes (1-byte > > > header > > > > > and > > > > > > > > 2-byte > > > > > > > > > > reason). Shouldn't we be setting expected[2] in this case? > > > > > > > > > > > > > > > > > > Ping. > > > > > > > > > > > > > > > > it's the network endian > > > > > > > > > > > > > > In that case, please read both values into a uint16_t and use > htons() > > > and > > > > > > > ntohs() to do this. Your code shouldn't make assumptions about what > > the > > > > > host- > > > > > > or > > > > > > > network-endianness are. > > > > > > > > > > > > > > Please also incorporate use of these functions into your > > implementation > > > > > file. > > > > > > > Thanks! > > > > > > > > > > > > what do you mean? This is specified by the weave spec. We had a > > discussion > > > > > about > > > > > > this with Gustavo earlier in the generator. If you are talking about I > > > > > couldn't > > > > > > assume network-endianness, I don't think it would ever be reasonable > for > > > > > > network-endianness to change. > > > > > > > > > > Network endianness is big-endian, though. Shouldn't they be in the > > opposite > > > > > order? > > > > > > > > You can take a look at the cc file that Gustavo okayed. I am pretty sure > the > > > > lower byte comes first in the packet. > > > > > > I just chatted with Gustavo, and he confirmed that the byte next to the > header > > > should be 0. > > > > > > But my point is that we shouldn't even have to think about this - the whole > > > point of ntohs() and htons() is that we just use those functions and let > them > > > decide how the bytes should be ordered. Please use those functions here so > > that > > > we know we are doing the correct thing and so that someone reading this code > > at > > > a later time does not have to think about this to decide whether the bytes > are > > > in the right order or not. > > > > The _upper_ byte of the short comes first. In your implementation of the > > BluetoothLowEnergyWeavePacketGenerator::SetIntField() you are also doing it: > > > > packet->at(index) = upper; > > packet->at(index + 1) = lower; > > > > As I said in the other comment, I agree with Kyle, I prefer a solution > > explicitly doing the conversions with htons(). > > No that was my original implementation. Then Gustavo corrected me and now it is > this > packet->at(byte_offset) = lower; > packet->at(byte_offset + 1) = upper; I'm under the impression that endianness overcomplicates a very simple operation. I didn't even think endianness is relevant until Gustavo brought it up. I think it's fairly straight forward how to convert int into bytes and the other way around without assuming anything about endianness of the machine. And the packets endianness is very intuitive. https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:75: std::vector<uint8_t> packet = generator->CreateConnectionRequest(); On 2016/06/21 17:17:30, Kyle Horimoto wrote: > On 2016/06/21 01:46:56, jingxuy wrote: > > On 2016/06/20 21:52:17, Kyle Horimoto wrote: > > > On 2016/06/20 21:29:32, jingxuy wrote: > > > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > > > Can we just use Packet instead of vector<>? Same elsewhere. > > > > > > > > > > Ping. > > > > > > > > I think vector makes it more clear what the return type of the class > > actually > > > is > > > > and what functions are defined on it. > > > > > > If so, you should get rid of your Packet typedef entirely. If you make a > type > > to > > > be used, you should use it. Using it in some places and not using it in > other > > > places results in a lack of uniformity which makes your code harder to read. > > So, > > > either remove it entirely or use it everywhere where it is applicable. > > > > I was intending it to be used as a shorthand and conceptually clearer type . > It > > should only be used inside the h and cc file and shouldn't be used by an > > external code. I will think about this... > > Ping. Done.
https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:41: expected[1] = expected_reason_for_close; On 2016/06/21 18:13:07, jingxuy wrote: > On 2016/06/21 17:47:31, jingxuy wrote: > > On 2016/06/21 17:39:22, sacomoto wrote: > > > On 2016/06/21 17:17:30, Kyle Horimoto wrote: > > > > On 2016/06/21 06:03:11, jingxuy wrote: > > > > > On 2016/06/21 02:15:29, Kyle Horimoto wrote: > > > > > > On 2016/06/21 01:46:56, jingxuy wrote: > > > > > > > On 2016/06/20 21:52:17, Kyle Horimoto wrote: > > > > > > > > On 2016/06/20 21:29:32, jingxuy wrote: > > > > > > > > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > > > > > > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > > > > > > > > It looks like the connection close packet is 3 bytes (1-byte > > > > header > > > > > > and > > > > > > > > > 2-byte > > > > > > > > > > > reason). Shouldn't we be setting expected[2] in this case? > > > > > > > > > > > > > > > > > > > > Ping. > > > > > > > > > > > > > > > > > > it's the network endian > > > > > > > > > > > > > > > > In that case, please read both values into a uint16_t and use > > htons() > > > > and > > > > > > > > ntohs() to do this. Your code shouldn't make assumptions about > what > > > the > > > > > > host- > > > > > > > or > > > > > > > > network-endianness are. > > > > > > > > > > > > > > > > Please also incorporate use of these functions into your > > > implementation > > > > > > file. > > > > > > > > Thanks! > > > > > > > > > > > > > > what do you mean? This is specified by the weave spec. We had a > > > discussion > > > > > > about > > > > > > > this with Gustavo earlier in the generator. If you are talking about > I > > > > > > couldn't > > > > > > > assume network-endianness, I don't think it would ever be reasonable > > for > > > > > > > network-endianness to change. > > > > > > > > > > > > Network endianness is big-endian, though. Shouldn't they be in the > > > opposite > > > > > > order? > > > > > > > > > > You can take a look at the cc file that Gustavo okayed. I am pretty sure > > the > > > > > lower byte comes first in the packet. > > > > > > > > I just chatted with Gustavo, and he confirmed that the byte next to the > > header > > > > should be 0. > > > > > > > > But my point is that we shouldn't even have to think about this - the > whole > > > > point of ntohs() and htons() is that we just use those functions and let > > them > > > > decide how the bytes should be ordered. Please use those functions here so > > > that > > > > we know we are doing the correct thing and so that someone reading this > code > > > at > > > > a later time does not have to think about this to decide whether the bytes > > are > > > > in the right order or not. > > > > > > The _upper_ byte of the short comes first. In your implementation of the > > > BluetoothLowEnergyWeavePacketGenerator::SetIntField() you are also doing it: > > > > > > packet->at(index) = upper; > > > packet->at(index + 1) = lower; > > > > > > As I said in the other comment, I agree with Kyle, I prefer a solution > > > explicitly doing the conversions with htons(). > > > > No that was my original implementation. Then Gustavo corrected me and now it > is > > this > > packet->at(byte_offset) = lower; > > packet->at(byte_offset + 1) = upper; > > I'm under the impression that endianness overcomplicates a very simple > operation. I didn't even think endianness is relevant until Gustavo brought it > up. I think it's fairly straight forward how to convert int into bytes and the > other way around without assuming anything about endianness of the machine. And > the packets endianness is very intuitive. It is necessary to consider the endianness of the message as it would result in incorrect values if we did not. Sorry, but you have to make sure the bytes are in the right order. https://codereview.chromium.org/2031953003/diff/400001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/400001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:157: packet->at(byte_offset) = upper; This still assumes something about the endianness of the machine vs. the endianness of the network. Specifically, it assumes that the machine and the network have the same endianness. Consider the following situation: the machine is little-endian, the network is big-endian, the other device we are talking to is big-endian, and the value we are trying to set is 100. In the little endian machine, the decimal value of 100 would be represented as 0x6400. Thus, upper would be 0x64 and lower would be 0x00. Then, we would place 0x64 at the byte offset and 0x00 at the byte offset + 1. Then, we would send this over the network, and the big-endian device on the other side receives it. The other machine would read 0x6400 as the value, which is 25600, not 100. You really need to use htons() to do this correctly.
https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/300001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:41: expected[1] = expected_reason_for_close; On 2016/06/21 18:39:12, Kyle Horimoto wrote: > On 2016/06/21 18:13:07, jingxuy wrote: > > On 2016/06/21 17:47:31, jingxuy wrote: > > > On 2016/06/21 17:39:22, sacomoto wrote: > > > > On 2016/06/21 17:17:30, Kyle Horimoto wrote: > > > > > On 2016/06/21 06:03:11, jingxuy wrote: > > > > > > On 2016/06/21 02:15:29, Kyle Horimoto wrote: > > > > > > > On 2016/06/21 01:46:56, jingxuy wrote: > > > > > > > > On 2016/06/20 21:52:17, Kyle Horimoto wrote: > > > > > > > > > On 2016/06/20 21:29:32, jingxuy wrote: > > > > > > > > > > On 2016/06/20 18:01:23, Kyle Horimoto wrote: > > > > > > > > > > > On 2016/06/17 04:04:12, Kyle Horimoto wrote: > > > > > > > > > > > > It looks like the connection close packet is 3 bytes > (1-byte > > > > > header > > > > > > > and > > > > > > > > > > 2-byte > > > > > > > > > > > > reason). Shouldn't we be setting expected[2] in this case? > > > > > > > > > > > > > > > > > > > > > > Ping. > > > > > > > > > > > > > > > > > > > > it's the network endian > > > > > > > > > > > > > > > > > > In that case, please read both values into a uint16_t and use > > > htons() > > > > > and > > > > > > > > > ntohs() to do this. Your code shouldn't make assumptions about > > what > > > > the > > > > > > > host- > > > > > > > > or > > > > > > > > > network-endianness are. > > > > > > > > > > > > > > > > > > Please also incorporate use of these functions into your > > > > implementation > > > > > > > file. > > > > > > > > > Thanks! > > > > > > > > > > > > > > > > what do you mean? This is specified by the weave spec. We had a > > > > discussion > > > > > > > about > > > > > > > > this with Gustavo earlier in the generator. If you are talking > about > > I > > > > > > > couldn't > > > > > > > > assume network-endianness, I don't think it would ever be > reasonable > > > for > > > > > > > > network-endianness to change. > > > > > > > > > > > > > > Network endianness is big-endian, though. Shouldn't they be in the > > > > opposite > > > > > > > order? > > > > > > > > > > > > You can take a look at the cc file that Gustavo okayed. I am pretty > sure > > > the > > > > > > lower byte comes first in the packet. > > > > > > > > > > I just chatted with Gustavo, and he confirmed that the byte next to the > > > header > > > > > should be 0. > > > > > > > > > > But my point is that we shouldn't even have to think about this - the > > whole > > > > > point of ntohs() and htons() is that we just use those functions and let > > > them > > > > > decide how the bytes should be ordered. Please use those functions here > so > > > > that > > > > > we know we are doing the correct thing and so that someone reading this > > code > > > > at > > > > > a later time does not have to think about this to decide whether the > bytes > > > are > > > > > in the right order or not. > > > > > > > > The _upper_ byte of the short comes first. In your implementation of the > > > > BluetoothLowEnergyWeavePacketGenerator::SetIntField() you are also doing > it: > > > > > > > > packet->at(index) = upper; > > > > packet->at(index + 1) = lower; > > > > > > > > As I said in the other comment, I agree with Kyle, I prefer a solution > > > > explicitly doing the conversions with htons(). > > > > > > No that was my original implementation. Then Gustavo corrected me and now it > > is > > > this > > > packet->at(byte_offset) = lower; > > > packet->at(byte_offset + 1) = upper; > > > > I'm under the impression that endianness overcomplicates a very simple > > operation. I didn't even think endianness is relevant until Gustavo brought it > > up. I think it's fairly straight forward how to convert int into bytes and the > > other way around without assuming anything about endianness of the machine. > And > > the packets endianness is very intuitive. > > It is necessary to consider the endianness of the message as it would result in > incorrect values if we did not. Sorry, but you have to make sure the bytes are > in the right order. This is resolved as the current implementation is okay but htons is more readible. https://codereview.chromium.org/2031953003/diff/400001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/400001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:157: packet->at(byte_offset) = upper; On 2016/06/21 18:39:12, Kyle Horimoto wrote: > This still assumes something about the endianness of the machine vs. the > endianness of the network. Specifically, it assumes that the machine and the > network have the same endianness. > > Consider the following situation: the machine is little-endian, the network is > big-endian, the other device we are talking to is big-endian, and the value we > are trying to set is 100. In the little endian machine, the decimal value of 100 > would be represented as 0x6400. Thus, upper would be 0x64 and lower would be > 0x00. Then, we would place 0x64 at the byte offset and 0x00 at the byte offset + > 1. > > Then, we would send this over the network, and the big-endian device on the > other side receives it. The other machine would read 0x6400 as the value, which > is 25600, not 100. > > You really need to use htons() to do this correctly. Resolved like the other comment.
LGTM after you address this last comment. Great job - thanks for sticking through the review! :) https://codereview.chromium.org/2031953003/diff/380001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:265: const uint8_t kNumPackets = 9; On 2016/06/21 17:17:31, Kyle Horimoto wrote: > Can we just increase this to something much bigger...say, 100? Just to make sure > that it loops through multiple times properly. Ping.
https://codereview.chromium.org/2031953003/diff/380001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/380001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:265: const uint8_t kNumPackets = 9; On 2016/06/21 22:45:31, Kyle Horimoto wrote: > On 2016/06/21 17:17:31, Kyle Horimoto wrote: > > Can we just increase this to something much bigger...say, 100? Just to make > sure > > that it loops through multiple times properly. > > Ping. Done.
https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:111: DCHECK(!message.empty()); Return an empty vector instead. https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:131: packet->push_back(0); Instead of calling push_back() on every single byte. You can just call resize(), and then index the vector. https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:136: for (uint32_t i = begin; i < end; ++i) { You're using i as the counter for this for-loop and the parent for-loop. https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:156: uint8_t* network_val_ptr = (uint8_t*)(&network_val); C-type casts are frowned upon. It's better to use a static_cast. https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:62: // Packet size must be greater than 2. Why have this limitation as long as its greater than 0? BTW, it should also say 20 instead of 2. https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:65: // Will crash if message is empty. Crashing isn't the right thing to do. You should return an empty vector instead.
https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:111: DCHECK(!message.empty()); On 2016/06/22 18:41:31, Tim Song wrote: > Return an empty vector instead. Pending discussion as replied in the h file https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:136: for (uint32_t i = begin; i < end; ++i) { On 2016/06/22 18:41:31, Tim Song wrote: > You're using i as the counter for this for-loop and the parent for-loop. Done. https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:156: uint8_t* network_val_ptr = (uint8_t*)(&network_val); On 2016/06/22 18:41:31, Tim Song wrote: > C-type casts are frowned upon. It's better to use a static_cast. static cast from uint16_t* to uint8_t* is not allowed. https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:62: // Packet size must be greater than 2. On 2016/06/22 18:41:31, Tim Song wrote: > Why have this limitation as long as its greater than 0? BTW, it should also say > 20 instead of 2. I originally set it to 2 because there need to be at least 1 byte for header and 1 byte for data. Need the header and then the data byte is making sure sending data packet doesn't spin forever because it can't send any byte. packet_size_ is actually also used in sending the original control message, which would have a field that says maximum packet size, and that has to be at least 20 or the request will be rejected. So Gustavo and Kyle suggested that you shouldn't be setting maximum packet size to anything less than 20. https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:65: // Will crash if message is empty. On 2016/06/22 18:41:32, Tim Song wrote: > Crashing isn't the right thing to do. You should return an empty vector instead. It originally returned an empty vector returned on an empty message received. But Gustavo and Kyle both considered empty message to be an error.
lgtm https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:156: uint8_t* network_val_ptr = (uint8_t*)(&network_val); On 2016/06/22 19:07:02, jingxuy wrote: > On 2016/06/22 18:41:31, Tim Song wrote: > > C-type casts are frowned upon. It's better to use a static_cast. > > static cast from uint16_t* to uint8_t* is not allowed. You can use a reinterp_cast or something like: packet->at(byte_offset) = static_cast<uint8_t>(network_val >> 8); packet->at(byte_offset + 1) = static_cast<uint8_t>(network_val & 0x00FF); https://codereview.chromium.org/2031953003/diff/460001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/460001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:62: // Packet size must be greater than 20. greater than or equal to
https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:131: packet->push_back(0); On 2016/06/22 18:41:31, Tim Song wrote: > Instead of calling push_back() on every single byte. You can just call resize(), > and then index the vector. Acknowledged. https://codereview.chromium.org/2031953003/diff/440001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:156: uint8_t* network_val_ptr = (uint8_t*)(&network_val); On 2016/06/22 19:32:10, Tim Song wrote: > On 2016/06/22 19:07:02, jingxuy wrote: > > On 2016/06/22 18:41:31, Tim Song wrote: > > > C-type casts are frowned upon. It's better to use a static_cast. > > > > static cast from uint16_t* to uint8_t* is not allowed. > > You can use a reinterp_cast or something like: > > packet->at(byte_offset) = static_cast<uint8_t>(network_val >> 8); > packet->at(byte_offset + 1) = static_cast<uint8_t>(network_val & 0x00FF); Done. https://codereview.chromium.org/2031953003/diff/460001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2031953003/diff/460001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:62: // Packet size must be greater than 20. On 2016/06/22 19:32:10, Tim Song wrote: > greater than or equal to Done.
LGTM. The code looks very good. Thanks, Jing! https://codereview.chromium.org/2031953003/diff/480001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/480001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:112: generator->SetDataPacketSize(kSelectedPacketSize); nit: Please create another instance of the |generator| and use it here. This is a bit misleading, as the connection request packet you are expecting below doesn't exist in the protocol. The counter of the connection response packet is always 0. https://codereview.chromium.org/2031953003/diff/480001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:246: EXPECT_EQ(1, GetCounterFromHeader(packet[0])); nit: I'd remove the generation of the connection response packet. As the a normal protocol flow would never create a connection request with counter 1.
https://codereview.chromium.org/2031953003/diff/480001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc (right): https://codereview.chromium.org/2031953003/diff/480001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:112: generator->SetDataPacketSize(kSelectedPacketSize); On 2016/06/23 14:53:15, sacomoto wrote: > nit: Please create another instance of the |generator| and use it here. This is > a bit misleading, as the connection request packet you are expecting below > doesn't exist in the protocol. The counter of the connection response packet is > always 0. Done. https://codereview.chromium.org/2031953003/diff/480001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc:246: EXPECT_EQ(1, GetCounterFromHeader(packet[0])); On 2016/06/23 14:53:15, sacomoto wrote: > nit: I'd remove the generation of the connection response packet. As the a > normal protocol flow would never create a connection request with counter 1. Done.
https://codereview.chromium.org/2031953003/diff/540001/components/proximity_a... File components/proximity_auth/ble/BUILD.gn (right): https://codereview.chromium.org/2031953003/diff/540001/components/proximity_a... components/proximity_auth/ble/BUILD.gn:17: "bluetooth_low_energy_weave_cnnnection.h", Why are all these files here? You should only have the generator files. Same below for unit tests.
https://codereview.chromium.org/2031953003/diff/540001/components/proximity_a... File components/proximity_auth/ble/BUILD.gn (right): https://codereview.chromium.org/2031953003/diff/540001/components/proximity_a... components/proximity_auth/ble/BUILD.gn:17: "bluetooth_low_energy_weave_cnnnection.h", On 2016/06/24 00:28:43, Kyle Horimoto wrote: > Why are all these files here? You should only have the generator files. Same > below for unit tests. 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/2031953003/#ps560001 (title: "final style change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031953003/560001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
I think you should also update the GYP files: https://cs.chromium.org/chromium/src/components/proximity_auth.gypi https://cs.chromium.org/chromium/src/components/components_tests.gyp tengs@, they are still being used, right? Or GYP is completely deprecated now?
GYP is deprecated, but I don't think it's ready to be completely removed yet, so we should still update those gypi files.
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/2031953003/#ps580001 (title: "ifdef guard around htons include and changed some uint16 to uint8")
The CQ bit was unchecked by jingxuy@google.com
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/2031953003/#ps600001 (title: "updated gyp files")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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/2031953003/#ps620001 (title: "include the header that actually enables platform dependent include of headers")
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 generator that respects the uWeave.BLE protocol. The packet generator accepts raw bytes as messages to be sent and wraps the message with the uWeave protocol. The uWeave packets include control packets of request/response/close of a connection and data packets that are the original message chopped up to fixed size packets with uWeave protocol headers. Design Doc of 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 generator that respects the uWeave.BLE protocol. The packet generator accepts raw bytes as messages to be sent and wraps the message with the uWeave protocol. The uWeave packets include control packets of request/response/close of a connection and data packets that are the original message chopped up to fixed size packets with uWeave protocol headers. Design Doc of 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 #30 (id:620001)
Message was sent while issue was closed.
Description was changed from ========== Weave Packet Generator This CL adds a packet generator that respects the uWeave.BLE protocol. The packet generator accepts raw bytes as messages to be sent and wraps the message with the uWeave protocol. The uWeave packets include control packets of request/response/close of a connection and data packets that are the original message chopped up to fixed size packets with uWeave protocol headers. Design Doc of 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 generator that respects the uWeave.BLE protocol. The packet generator accepts raw bytes as messages to be sent and wraps the message with the uWeave protocol. The uWeave packets include control packets of request/response/close of a connection and data packets that are the original message chopped up to fixed size packets with uWeave protocol headers. Design Doc of ProximityAuth uWeave Migration: https://docs.google.com/a/google.com/document/d/1HJ1fipxsNQwIxmKs-5_6TjroCAcs... BUG=617238 Committed: https://crrev.com/6742a47fff173bfa1bf0a4ec3a4655436c9c03e4 Cr-Commit-Position: refs/heads/master@{#401993} ==========
Message was sent while issue was closed.
Patchset 30 (id:??) landed as https://crrev.com/6742a47fff173bfa1bf0a4ec3a4655436c9c03e4 Cr-Commit-Position: refs/heads/master@{#401993} |
