Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2183523006:
Chrome OS uWeave Characteristics Server (Closed)
DescriptionChrome OS uWeave Characteristics Server
NOTE: This CL is for review of design purpose. It will be split up into 3 smaller CLs.
The Server serves RX and TX characteristic as specified by uWeave protocol.
For every device that tries to connect to the Server, the Server spins off a ServerConnection to communicate with that device.
The Server advertise itself through an AdvertisementRotator which rotate both EID seed and EID and devices.
TODO(jingxuy): attach updated public design doc here.
BUG=631853
Patch Set 1 #
Total comments: 36
Patch Set 2 : Added comprehensive comments for classes and methods #Patch Set 3 : addressed a TODO so now server connection actually receives packets #Patch Set 4 : change GetEid to take RemoteDevice #
Total comments: 37
Patch Set 5 : fixed bugs in server connection #
Total comments: 2
Patch Set 6 : most recent version of the server #Patch Set 7 : added channel #Depends on Patchset: Messages
Total messages: 19 (5 generated)
Description was changed from ========== Server BUG= ========== to ========== Chrome OS uWeave Characteristics Server NOTE: This CL is for review of design purpose. It will be split up into 3 smaller CLs. The Server serves RX and TX characteristic as specified by uWeave protocol. For every device that tries to connect to the Server, the Server spins off a ServerConnection to communicate with that device. The Server advertise itself through an AdvertisementRotator which rotate both EID seed and EID and devices. BUG=631853 ==========
jingxuy@google.com changed reviewers: + khorimoto@chromium.org, sacomoto@chromium.org, tengs@chromium.org
jingxuy@google.com changed reviewers: + rkc@chromium.org
Please review the general design and make suggestions. I will break it down to smaller CL's with unit test when the general design is okayed. Thanks!
Do split these into smaller CLs, trying to get one large logical piece of work together in one CL. For example, implement the rotator in one, then in a dependent patch, whatever uses that, etc. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.cc (right): https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.cc:11: typedef BluetoothLowEnergyAdvertisementRotator::Advertisement Advertisement; So we're typedefing another typedef? This seems, odd. Isn't there a simpler way for this? https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h (right): https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h:22: const device::BluetoothAdvertisement::AdvertisementType kPeripheralAdType = Why is this in an anonymous namespace in a header file? https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h:28: class BluetoothLowEnergyAdvertisementRotator { Class comment? What does this class do? https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h:36: static void SetInstanceForTesting(Factory* factory); Is there a better way to do this. We've moved away from the entire "SetXXXForTesting" paradigm. Usually code should be designed in a way that we can create stubs instead. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h:51: // Return whether the rotator is empty. Unnecessary comment. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h:79: BluetoothLowEnergyAdvertisementRotator(std::string service_uuid); Why is this protected? Why inherits this class? https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h:89: std::vector<int> waiting_queue_; What do these variables hold? https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h (right): https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:18: // Represent a EID generator for a single device. The code client has the What's an EID? https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:24: // TODO(jingxuy): this is just stop compiler from complaining. Complaining about? Generally when a compiler complains, there is often a good reason. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:43: typedef uint8_t EidSeed[32]; Why 32? Also, make this a constexpr. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:49: uint16_t GetEid() { return 42; } Again, why 42? https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:52: BluetoothLowEnergyEidGenerator(const RemoteDevice& device) Why is this protected? https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_weave_server.h (right): https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:52: class BluetoothLowEnergyWeaveServer Class comment? Method comments?
Description was changed from ========== Chrome OS uWeave Characteristics Server NOTE: This CL is for review of design purpose. It will be split up into 3 smaller CLs. The Server serves RX and TX characteristic as specified by uWeave protocol. For every device that tries to connect to the Server, the Server spins off a ServerConnection to communicate with that device. The Server advertise itself through an AdvertisementRotator which rotate both EID seed and EID and devices. BUG=631853 ========== to ========== Chrome OS uWeave Characteristics Server NOTE: This CL is for review of design purpose. It will be split up into 3 smaller CLs. The Server serves RX and TX characteristic as specified by uWeave protocol. For every device that tries to connect to the Server, the Server spins off a ServerConnection to communicate with that device. The Server advertise itself through an AdvertisementRotator which rotate both EID seed and EID and devices. TODO(jingxuy): attach updated public design doc here. BUG=631853 ==========
Jing, I'll look over this CL tomorrow toward the end of the day. I'll be pretty busy until then, sorry! https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h (right): https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h:36: static void SetInstanceForTesting(Factory* factory); On 2016/07/27 00:56:34, rkc wrote: > Is there a better way to do this. We've moved away from the entire > "SetXXXForTesting" paradigm. Usually code should be designed in a way that we > can create stubs instead. Rahul, I actually suggested this approach to Jing for some other classes. However, I don't think that a Factory is needed at all for this class. It can just be injected into the constructor and does not need to be created via a Factory at all.
https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h (right): https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h:28: class BluetoothLowEnergyAdvertisementRotator { There should just be one EidGenerator used to generate all EIDs, but this implementation stores a map of string to EidGenerator. This class should just return the device that should be advertised to next instead of attempting to generate an EID for that device. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h (right): https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:22: class Factory { Is there a reason that a Factory is needed for this class? https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:43: typedef uint8_t EidSeed[32]; On 2016/07/27 00:56:34, rkc wrote: > Why 32? Also, make this a constexpr. +1, I'm not sure that seeds are 32 bytes, but even if they are, the seeds will be abstracted into a BeaconSeed class, so this typedef should not be necessary. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:49: uint16_t GetEid() { return 42; } On 2016/07/27 00:56:34, rkc wrote: > Again, why 42? I think she's just returning a fake value and will implement this later. Jing, please write TODO: implement in this case. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:49: uint16_t GetEid() { return 42; } This function should take a list of BeaconSeeds and return a current EID as well as an adjacent EID if possible.
https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.cc (right): https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.cc:11: typedef BluetoothLowEnergyAdvertisementRotator::Advertisement Advertisement; On 2016/07/27 00:56:33, rkc wrote: > So we're typedefing another typedef? This seems, odd. Isn't there a simpler way > for this? > Done. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h (right): https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h:22: const device::BluetoothAdvertisement::AdvertisementType kPeripheralAdType = On 2016/07/27 00:56:33, rkc wrote: > Why is this in an anonymous namespace in a header file? Done. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h:28: class BluetoothLowEnergyAdvertisementRotator { On 2016/07/27 00:56:34, rkc wrote: > Class comment? What does this class do? Done. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h:28: class BluetoothLowEnergyAdvertisementRotator { On 2016/07/27 17:58:48, Kyle Horimoto wrote: > There should just be one EidGenerator used to generate all EIDs, but this > implementation stores a map of string to EidGenerator. > > This class should just return the device that should be advertised to next > instead of attempting to generate an EID for that device. If there is going to be different seed for different devices, why should there just be one eid generator. We had a discussion about the DeviceRotator when you left that meeting. And Gustavo doesn't consider a queue itself is worthy of a separate class. He'd agree to a separate class if it also rotate EID on top just device and suggested naming the class AdvertisementRotator. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h:36: static void SetInstanceForTesting(Factory* factory); On 2016/07/27 06:34:53, Kyle Horimoto wrote: > On 2016/07/27 00:56:34, rkc wrote: > > Is there a better way to do this. We've moved away from the entire > > "SetXXXForTesting" paradigm. Usually code should be designed in a way that we > > can create stubs instead. > > Rahul, I actually suggested this approach to Jing for some other classes. > However, I don't think that a Factory is needed at all for this class. It can > just be injected into the constructor and does not need to be created via a > Factory at all. I would be open to other ways of doing this because I do find factories to be awkward to use. Are we all agreeing that passing the rotator as constructor parameter into the server class is the approach to solve this problem? https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h:51: // Return whether the rotator is empty. On 2016/07/27 00:56:34, rkc wrote: > Unnecessary comment. Done. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h:79: BluetoothLowEnergyAdvertisementRotator(std::string service_uuid); On 2016/07/27 00:56:33, rkc wrote: > Why is this protected? Why inherits this class? This the part of the implementation for the factory thing. I'll move it when we reach the consensus on how to solve the testing problem. But I guess it can be private either way. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_advertisement_rotator.h:89: std::vector<int> waiting_queue_; On 2016/07/27 00:56:34, rkc wrote: > What do these variables hold? Done. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h (right): https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:18: // Represent a EID generator for a single device. The code client has the On 2016/07/27 00:56:34, rkc wrote: > What's an EID? Done. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:22: class Factory { On 2016/07/27 17:58:48, Kyle Horimoto wrote: > Is there a reason that a Factory is needed for this class? I was under the impression that you want to mock out classes in testing process. Unless we are moving toward a different testing approach for which we can talk about. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:24: // TODO(jingxuy): this is just stop compiler from complaining. On 2016/07/27 00:56:34, rkc wrote: > Complaining about? > Generally when a compiler complains, there is often a good reason. I didn't have a .cc file since I intend this class to be not implemented by me. So it wouldn't let the ad_rotator construct this class. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:43: typedef uint8_t EidSeed[32]; On 2016/07/27 17:58:48, Kyle Horimoto wrote: > On 2016/07/27 00:56:34, rkc wrote: > > Why 32? Also, make this a constexpr. > > +1, I'm not sure that seeds are 32 bytes, but even if they are, the seeds will > be abstracted into a BeaconSeed class, so this typedef should not be necessary. According to Jeremy it is 256 bits. This typedef is just a place holder. I do not intend to implement the EID generator at all. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:49: uint16_t GetEid() { return 42; } On 2016/07/27 17:58:48, Kyle Horimoto wrote: > On 2016/07/27 00:56:34, rkc wrote: > > Again, why 42? > > I think she's just returning a fake value and will implement this later. Jing, > please write TODO: implement in this case. It has a TODO above the function explaining exactly what there is to do. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:49: uint16_t GetEid() { return 42; } On 2016/07/27 17:58:48, Kyle Horimoto wrote: > This function should take a list of BeaconSeeds and return a current EID as well > as an adjacent EID if possible. I think our design for the EID generator is different. My design owns a remote_device. So it should be able to pull the BeasonSeeds out of the remote_device. One of the main reasons for this difference is that I do not intend to implement the EID seed generator. Hence I have to abstract away any EID seed related things including BeaconSeed definition. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:52: BluetoothLowEnergyEidGenerator(const RemoteDevice& device) On 2016/07/27 00:56:34, rkc wrote: > Why is this protected? Same as the comment in the rotator. It comes with factory implementation. Let me know what testing injection we are going to use. https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_weave_server.h (right): https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:52: class BluetoothLowEnergyWeaveServer On 2016/07/27 00:56:34, rkc wrote: > Class comment? Method comments? Done.
https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... File components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h (right): https://codereview.chromium.org/2183523006/diff/1/components/proximity_auth/b... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:49: uint16_t GetEid() { return 42; } On 2016/07/27 23:31:48, jingxuy wrote: > On 2016/07/27 17:58:48, Kyle Horimoto wrote: > > This function should take a list of BeaconSeeds and return a current EID as > well > > as an adjacent EID if possible. > > I think our design for the EID generator is different. My design owns a > remote_device. So it should be able to pull the BeasonSeeds out of the > remote_device. One of the main reasons for this difference is that I do not > intend to implement the EID seed generator. Hence I have to abstract away any > EID seed related things including BeaconSeed definition. Resolved on offline discussion. Currently will take RemoteDevice. This is an incomplete interface available for extensions.
https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:50: uint16_t GetEid(const RemoteDevice& device) { return 42; } 1) Your function should take a public key as a parameter. 2) Your function should return a current and, if possible, an adjacent object containing a data array, a start timestamp, and a stop timestamp. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_server.cc (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.cc:264: void BluetoothLowEnergyWeaveServer::RefreshAdvertisement() { We can't change advertisements if connections are in progress but not yet authenticated. Your design needs to take this into account. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_server.h (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:131: bool HasDevice(std::string device_address); rkc@: Is this a reasonable function to use? I thought BLE MAC addresses were ephemeral - how does this affect the Chromium implementation? Same with GetRemoteDevice() and the address-based maps below. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:136: void EnableAdvertising(const RemoteDevice& remote_device); This class should rotate advertisements between the registered devices internally; this should not be part of the public API. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:153: void UpdateEidSeeds(); We should try to rotate which devices we advertise to every 5 seconds, and we rely on the EidGenerator to rotate the actual EID. This class shouldn't be in charge of that. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:157: void UpdateEids(); Same. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_server_connection.cc (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server_connection.cc:100: rx_characteristic_->NotifyValueChanged( You need to do the same sort of handling as you did in the client class here. Also, please create a named function for this. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server_connection.cc:134: rx_characteristic_->NotifyValueChanged(bluetooth_device_, packet, false); You don't call the OnDidSendMessage() callback.
https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:50: uint16_t GetEid(const RemoteDevice& device) { return 42; } On 2016/07/29 03:15:39, Kyle Horimoto wrote: > 1) Your function should take a public key as a parameter. > 2) Your function should return a current and, if possible, an adjacent object > containing a data array, a start timestamp, and a stop timestamp. I thought we agreed on it take RemoteDevice? But I don't think getting the exact parameter and return type is high priority now. Since this is a place holder, you guys are free to change it to anything. Only one function calls this function anyway and that function is mostly left as TODO. I'm thinking about changing the return type since it's more structurally important. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_server.cc (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.cc:264: void BluetoothLowEnergyWeaveServer::RefreshAdvertisement() { On 2016/07/29 03:15:39, Kyle Horimoto wrote: > We can't change advertisements if connections are in progress but not yet > authenticated. Your design needs to take this into account. the advertisement will be taken down when a connection is in progress. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_server.h (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:131: bool HasDevice(std::string device_address); On 2016/07/29 03:15:39, Kyle Horimoto wrote: > rkc@: Is this a reasonable function to use? I thought BLE MAC addresses were > ephemeral - how does this affect the Chromium implementation? Same with > GetRemoteDevice() and the address-based maps below. I had the same question sort of. But from the extend of my knowledge, RemoteDevice is what is synced down from the server and is not mutable. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:136: void EnableAdvertising(const RemoteDevice& remote_device); On 2016/07/29 03:15:39, Kyle Horimoto wrote: > This class should rotate advertisements between the registered devices > internally; this should not be part of the public API. I put it here so that the serverConnection can start and pause it's advertisement. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:153: void UpdateEidSeeds(); On 2016/07/29 03:15:39, Kyle Horimoto wrote: > We should try to rotate which devices we advertise to every 5 seconds, and we > rely on the EidGenerator to rotate the actual EID. This class shouldn't be in > charge of that. You misunderstood. This function is for refreshing advertisement already registered and crossing the 14 days border. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:157: void UpdateEids(); On 2016/07/29 03:15:39, Kyle Horimoto wrote: > Same. This function is for refreshing advertisement already registered and crossing the 8 hour border. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:168: void RotateAdvertisement(); This function is for actually rotating 5 seconds. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_server_connection.cc (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server_connection.cc:100: rx_characteristic_->NotifyValueChanged( On 2016/07/29 03:15:39, Kyle Horimoto wrote: > You need to do the same sort of handling as you did in the client class here. > > Also, please create a named function for this. The client class is different because the sending is not immediate and require a complicated process. The server class is straight forward and simple. Just send connection close immediately. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server_connection.cc:134: rx_characteristic_->NotifyValueChanged(bluetooth_device_, packet, false); On 2016/07/29 03:15:39, Kyle Horimoto wrote: > You don't call the OnDidSendMessage() callback. Done.
https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:50: uint16_t GetEid(const RemoteDevice& device) { return 42; } On 2016/07/29 18:45:43, jingxuy wrote: > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > 1) Your function should take a public key as a parameter. > > 2) Your function should return a current and, if possible, an adjacent object > > containing a data array, a start timestamp, and a stop timestamp. > > I thought we agreed on it take RemoteDevice? But I don't think getting the exact > parameter and return type is high priority now. Since this is a place holder, > you guys are free to change it to anything. Only one function calls this > function anyway and that function is mostly left as TODO. I'm thinking about > changing the return type since it's more structurally important. The public key of the local device as well as metadata about the remote device are both needed to compute the EID. If you want, you can just pass a null public key for the stub implementation. Either way, you certainly do need to return a valid return value. A short is not a valid EID. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_server.cc (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.cc:264: void BluetoothLowEnergyWeaveServer::RefreshAdvertisement() { On 2016/07/29 18:45:43, jingxuy wrote: > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > We can't change advertisements if connections are in progress but not yet > > authenticated. Your design needs to take this into account. > > the advertisement will be taken down when a connection is in progress. Yeah, that is what is wrong about this implementation. The connection needs to be authenticated before advertising is stopped, not just connected. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_server.h (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:131: bool HasDevice(std::string device_address); On 2016/07/29 18:45:43, jingxuy wrote: > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > rkc@: Is this a reasonable function to use? I thought BLE MAC addresses were > > ephemeral - how does this affect the Chromium implementation? Same with > > GetRemoteDevice() and the address-based maps below. > > I had the same question sort of. But from the extend of my knowledge, > RemoteDevice is what is synced down from the server and is not mutable. In that case, it is the Bluetooth MAC address from the old Smart Lock code which used Bluetooth classic to connect. We cannot rely on that address being non-null as it is only sent to the server in the opt-in flow for Smart Lock. You should key off of the device ID instead of the device address. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:136: void EnableAdvertising(const RemoteDevice& remote_device); On 2016/07/29 18:45:43, jingxuy wrote: > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > This class should rotate advertisements between the registered devices > > internally; this should not be part of the public API. > > I put it here so that the serverConnection can start and pause it's > advertisement. The server connection should not know anything about this class. This class should stop advertising by itself once the connection has been authenticated. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:153: void UpdateEidSeeds(); On 2016/07/29 18:45:43, jingxuy wrote: > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > We should try to rotate which devices we advertise to every 5 seconds, and we > > rely on the EidGenerator to rotate the actual EID. This class shouldn't be in > > charge of that. > > You misunderstood. This function is for refreshing advertisement already > registered and crossing the 14 days border. EidGenerator does this for you already; this class should not encapsulate that complexity. At the end of each 5-second interval, just ask EidGenerator what should be advertised and do what it says. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:157: void UpdateEids(); On 2016/07/29 18:45:43, jingxuy wrote: > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > Same. > > This function is for refreshing advertisement already registered and crossing > the 8 hour border. Same - EidGenerator does this for you already.
https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:50: uint16_t GetEid(const RemoteDevice& device) { return 42; } On 2016/07/29 19:04:18, Kyle Horimoto wrote: > On 2016/07/29 18:45:43, jingxuy wrote: > > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > > 1) Your function should take a public key as a parameter. > > > 2) Your function should return a current and, if possible, an adjacent > object > > > containing a data array, a start timestamp, and a stop timestamp. > > > > I thought we agreed on it take RemoteDevice? But I don't think getting the > exact > > parameter and return type is high priority now. Since this is a place holder, > > you guys are free to change it to anything. Only one function calls this > > function anyway and that function is mostly left as TODO. I'm thinking about > > changing the return type since it's more structurally important. > > The public key of the local device as well as metadata about the remote device > are both needed to compute the EID. If you want, you can just pass a null public > key for the stub implementation. Either way, you certainly do need to return a > valid return value. A short is not a valid EID. what's the type of public key? Are you talking about the string contained in RemoteDevice? https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_server.cc (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.cc:264: void BluetoothLowEnergyWeaveServer::RefreshAdvertisement() { On 2016/07/29 19:04:18, Kyle Horimoto wrote: > On 2016/07/29 18:45:43, jingxuy wrote: > > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > > We can't change advertisements if connections are in progress but not yet > > > authenticated. Your design needs to take this into account. > > > > the advertisement will be taken down when a connection is in progress. > > Yeah, that is what is wrong about this implementation. The connection needs to > be authenticated before advertising is stopped, not just connected. oh, okay. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_server.h (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:131: bool HasDevice(std::string device_address); On 2016/07/29 19:04:18, Kyle Horimoto wrote: > On 2016/07/29 18:45:43, jingxuy wrote: > > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > > rkc@: Is this a reasonable function to use? I thought BLE MAC addresses were > > > ephemeral - how does this affect the Chromium implementation? Same with > > > GetRemoteDevice() and the address-based maps below. > > > > I had the same question sort of. But from the extend of my knowledge, > > RemoteDevice is what is synced down from the server and is not mutable. > > In that case, it is the Bluetooth MAC address from the old Smart Lock code which > used Bluetooth classic to connect. We cannot rely on that address being non-null > as it is only sent to the server in the opt-in flow for Smart Lock. You should > key off of the device ID instead of the device address. hmmmm, I think this actually post a pretty large problem. What do you mean by the device id? https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:136: void EnableAdvertising(const RemoteDevice& remote_device); On 2016/07/29 19:04:18, Kyle Horimoto wrote: > On 2016/07/29 18:45:43, jingxuy wrote: > > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > > This class should rotate advertisements between the registered devices > > > internally; this should not be part of the public API. > > > > I put it here so that the serverConnection can start and pause it's > > advertisement. > > The server connection should not know anything about this class. This class > should stop advertising by itself once the connection has been authenticated. so you mean this class should be an observer of the serverConnection? https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:153: void UpdateEidSeeds(); On 2016/07/29 19:04:18, Kyle Horimoto wrote: > On 2016/07/29 18:45:43, jingxuy wrote: > > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > > We should try to rotate which devices we advertise to every 5 seconds, and > we > > > rely on the EidGenerator to rotate the actual EID. This class shouldn't be > in > > > charge of that. > > > > You misunderstood. This function is for refreshing advertisement already > > registered and crossing the 14 days border. > > EidGenerator does this for you already; this class should not encapsulate that > complexity. At the end of each 5-second interval, just ask EidGenerator what > should be advertised and do what it says. This does not do the refreshing. It calls EIDGenerator for a refreshed version. Read the implementation. This is when advertisements are registered but it's content has been obsolete. So unregister all current advertisements and refresh and re-register them. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:157: void UpdateEids(); On 2016/07/29 19:04:18, Kyle Horimoto wrote: > On 2016/07/29 18:45:43, jingxuy wrote: > > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > > Same. > > > > This function is for refreshing advertisement already registered and crossing > > the 8 hour border. > > Same - EidGenerator does this for you already. same as above. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:168: void RotateAdvertisement(); On 2016/07/29 18:45:43, jingxuy wrote: > This function is for actually rotating 5 seconds. @Kyle
https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_eid_generator.h:50: uint16_t GetEid(const RemoteDevice& device) { return 42; } On 2016/07/29 20:26:01, jingxuy wrote: > On 2016/07/29 19:04:18, Kyle Horimoto wrote: > > On 2016/07/29 18:45:43, jingxuy wrote: > > > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > > > 1) Your function should take a public key as a parameter. > > > > 2) Your function should return a current and, if possible, an adjacent > > object > > > > containing a data array, a start timestamp, and a stop timestamp. > > > > > > I thought we agreed on it take RemoteDevice? But I don't think getting the > > exact > > > parameter and return type is high priority now. Since this is a place > holder, > > > you guys are free to change it to anything. Only one function calls this > > > function anyway and that function is mostly left as TODO. I'm thinking about > > > changing the return type since it's more structurally important. > > > > The public key of the local device as well as metadata about the remote device > > are both needed to compute the EID. If you want, you can just pass a null > public > > key for the stub implementation. Either way, you certainly do need to return a > > valid return value. A short is not a valid EID. > > what's the type of public key? Are you talking about the string contained in > RemoteDevice? Yep, though it needs to be converted into a data array first. Ask tengs@ how to do this conversion - I think its just c_str(), but I'm not positive. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_server.h (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:131: bool HasDevice(std::string device_address); On 2016/07/29 20:26:01, jingxuy wrote: > On 2016/07/29 19:04:18, Kyle Horimoto wrote: > > On 2016/07/29 18:45:43, jingxuy wrote: > > > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > > > rkc@: Is this a reasonable function to use? I thought BLE MAC addresses > were > > > > ephemeral - how does this affect the Chromium implementation? Same with > > > > GetRemoteDevice() and the address-based maps below. > > > > > > I had the same question sort of. But from the extend of my knowledge, > > > RemoteDevice is what is synced down from the server and is not mutable. > > > > In that case, it is the Bluetooth MAC address from the old Smart Lock code > which > > used Bluetooth classic to connect. We cannot rely on that address being > non-null > > as it is only sent to the server in the opt-in flow for Smart Lock. You should > > key off of the device ID instead of the device address. > > hmmmm, I think this actually post a pretty large problem. What do you mean by > the device id? Interesting - I thought that the RemoteDevice object had a device ID, but it is the user ID. tengs@, is there a reason devices don't have IDs? Can we generate them? https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:136: void EnableAdvertising(const RemoteDevice& remote_device); On 2016/07/29 20:26:01, jingxuy wrote: > On 2016/07/29 19:04:18, Kyle Horimoto wrote: > > On 2016/07/29 18:45:43, jingxuy wrote: > > > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > > > This class should rotate advertisements between the registered devices > > > > internally; this should not be part of the public API. > > > > > > I put it here so that the serverConnection can start and pause it's > > > advertisement. > > > > The server connection should not know anything about this class. This class > > should stop advertising by itself once the connection has been authenticated. > > so you mean this class should be an observer of the serverConnection? Yeah, but I think it would be better to create a wrapper class which observes each individual connection and routes events. Then, this class would only need to listen to one class, which would result in much less complex code (the complexity would be pushed to the wrapper class). https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:153: void UpdateEidSeeds(); On 2016/07/29 20:26:01, jingxuy wrote: > On 2016/07/29 19:04:18, Kyle Horimoto wrote: > > On 2016/07/29 18:45:43, jingxuy wrote: > > > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > > > We should try to rotate which devices we advertise to every 5 seconds, and > > we > > > > rely on the EidGenerator to rotate the actual EID. This class shouldn't be > > in > > > > charge of that. > > > > > > You misunderstood. This function is for refreshing advertisement already > > > registered and crossing the 14 days border. > > > > EidGenerator does this for you already; this class should not encapsulate that > > complexity. At the end of each 5-second interval, just ask EidGenerator what > > should be advertised and do what it says. > > This does not do the refreshing. It calls EIDGenerator for a refreshed version. > Read the implementation. This is when advertisements are registered but it's > content has been obsolete. So unregister all current advertisements and refresh > and re-register them. Yeah, but this should not happen every 14 days. Each time the 5-second interval restarts, we should grab a new EID from EidGenerator; we do not need to run a recurring method every 14 days. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:157: void UpdateEids(); On 2016/07/29 20:26:01, jingxuy wrote: > On 2016/07/29 19:04:18, Kyle Horimoto wrote: > > On 2016/07/29 18:45:43, jingxuy wrote: > > > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > > > Same. > > > > > > This function is for refreshing advertisement already registered and > crossing > > > the 8 hour border. > > > > Same - EidGenerator does this for you already. > > same as above. Same as above. https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:168: void RotateAdvertisement(); On 2016/07/29 20:26:01, jingxuy wrote: > On 2016/07/29 18:45:43, jingxuy wrote: > > This function is for actually rotating 5 seconds. > > @Kyle Yep - is there a question I'm supposed to answer?
https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_server.h (right): https://codereview.chromium.org/2183523006/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.h:131: bool HasDevice(std::string device_address); On 2016/07/29 21:07:46, Kyle Horimoto wrote: > On 2016/07/29 20:26:01, jingxuy wrote: > > On 2016/07/29 19:04:18, Kyle Horimoto wrote: > > > On 2016/07/29 18:45:43, jingxuy wrote: > > > > On 2016/07/29 03:15:39, Kyle Horimoto wrote: > > > > > rkc@: Is this a reasonable function to use? I thought BLE MAC addresses > > were > > > > > ephemeral - how does this affect the Chromium implementation? Same with > > > > > GetRemoteDevice() and the address-based maps below. > > > > > > > > I had the same question sort of. But from the extend of my knowledge, > > > > RemoteDevice is what is synced down from the server and is not mutable. > > > > > > In that case, it is the Bluetooth MAC address from the old Smart Lock code > > which > > > used Bluetooth classic to connect. We cannot rely on that address being > > non-null > > > as it is only sent to the server in the opt-in flow for Smart Lock. You > should > > > key off of the device ID instead of the device address. > > > > hmmmm, I think this actually post a pretty large problem. What do you mean by > > the device id? > > Interesting - I thought that the RemoteDevice object had a device ID, but it is > the user ID. tengs@, is there a reason devices don't have IDs? Can we generate > them? An approach we've used on other platforms is just that the device ID would be the base64-encoded version of the public key.
Sorry I haven't been able to get to this till now. I'm only reviewing the BT usage - defering the main review to the other reviewers. https://codereview.chromium.org/2183523006/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_server.cc (right): https://codereview.chromium.org/2183523006/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.cc:295: adapter_->RegisterAdvertisement( This won't work. You can only advertise one advertisement at a time on Chrome OS. You will need to unregister the previous advertisement and then re-register it. This also means that some concept of timing will be involved. If you just register and unregister, the adapter may not have advertised yet. The adapter advertises once approximately every 1.2s. There 'currently' is no way to change this, but there will be at some point relatively soon.
https://codereview.chromium.org/2183523006/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_server.cc (right): https://codereview.chromium.org/2183523006/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_server.cc:295: adapter_->RegisterAdvertisement( On 2016/08/02 01:09:21, rkc wrote: > This won't work. You can only advertise one advertisement at a time on Chrome > OS. > You will need to unregister the previous advertisement and then re-register it. > > This also means that some concept of timing will be involved. If you just > register and unregister, the adapter may not have advertised yet. > > The adapter advertises once approximately every 1.2s. There 'currently' is no > way to change this, but there will be at some point relatively soon. Just to clarify, after some digging around in the kernel code, it does seem like we can do multiple advertisements. We can do a max of 5 at a time, and they will run rotated by the kernel with a 1.28s interval between each.
rkc@chromium.org changed reviewers: - rkc@chromium.org |
