Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1524873002:
Creates a BattOrConnection for communicating with the BattOr (Closed)
|
Created:
5 years ago by charliea (OOO until 10-5) Modified:
5 years ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreates a BattOrConnection for communicating with the BattOr
I first tried to implement the protocol with this logic rolled into
BattOrAgent. However, BattOrAgent needs to act as a state machine
stepping through the BattOr protocol and, as the state machine grew,
it became obvious that we should move as much unrelated logic as
possible out of the class. The byte-level protocol now seen in
battor_connection.cc was an obvious choice to be moved elsewhere.
For a look at how these communication primitives will be used to
implement the protocol, see http://bit.ly/1UpLv3p.
BUG=542837
Committed: https://crrev.com/07027e6fbaa31604a908f7d1d2b42e789e93add8
Cr-Commit-Position: refs/heads/master@{#365606}
Patch Set 1 : #
Total comments: 71
Patch Set 2 : Code review #
Total comments: 16
Patch Set 3 : Code review #Patch Set 4 : Code review #
Total comments: 2
Patch Set 5 : Fixed gyp build file #Patch Set 6 : Added missing dep #
Messages
Total messages: 35 (12 generated)
Patchset #1 (id:1) has been deleted
charliea@chromium.org changed reviewers: + nednguyen@google.com, primiano@chromium.org, zhenw@chromium.org
https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_agent.gyp (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent.gyp:42: 'target_name': 'sql_unittests', Where does "sql" come from? https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_connection.cc (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:37: // - The number of escape bytes contained in the message Can you make the argument match the explanation? For example: - message: ... - message_content: ... ... https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:136: BattOrMessageType type, wrong indent https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:138: uint16_t bytes_to_send) { DCHECK the length of bytes == bytes_to_send. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:160: data_vector.push_back(BATTOR_SPECIAL_BYTE_ESCAPE); Add {...} when if clause takes more than 1 line. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:173: uint16_t bytes_to_read) { wrong indent https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:237: // how many escape bytes we've already read. Move the comment to above ParseMessage. Some more thoughts: It looks like we only need escape_byte_count here. And we only need some other info in the below ParseMessage. Those two have different purposes. Can we make two functions, each handle a different task? Those two functions can share the same parsing logic. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_connection.h (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:27: // protocol. Is there a description of BattOr protocol and how it works? Or is there a link to such thing (e.g., a github link)? https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_connection_unittest.cc (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection_unittest.cc:100: bool connect_success; I think the practice is to make variables private and have methods to check their values? https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_protocol_types.h (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_protocol_types.h:17: BATTOR_SPECIAL_BYTE_END, I think explicitly writing the values here would make it easier to understand the protocol, especially when BattOrMessageType follows BattOrControlByte values.
Ok, generally makes sense. I think my major comment would be to use a Listener inner class in the Connection. THis would remove dozens of lines for the callbacks and the base::bind and would make the code easier to read IMHO. The rest is just nits & co, I can have a final pass once this gets to its final shape. Hint: you might want to run git cl format before sending out patchsets ;-) https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/BUIL... File tools/battor_agent/BUILD.gn (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/BUIL... tools/battor_agent/BUILD.gn:19: static_library("battor_agent_lib") { I missed this in a previous review. I think this should be a source_set instad of static_library to avoid headaches if one day you'll want to componentize this. See https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/cookboo... https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_agent.cc (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent.cc:69: connection_ = nullptr; typically I see this as connection_.reset() (doesn't change anything in practice, I am surprised that this version even compiles) https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_agent.gyp (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent.gyp:42: 'target_name': 'sql_unittests', On 2015/12/14 23:39:44, Zhen Wang wrote: > Where does "sql" come from? Yeah I guess you meant battor_agent_unittests. Now we all know where you did copy this gyp snippet from :P https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_agent.h (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent.h:72: scoped_refptr<base::SingleThreadTaskRunner> file_thread_task_runner_; You don't seem to need these task runner here anymore. What I would do here is construct the connection_ in the BattOrAgent ctor, so that you don't have to keep the task runners (only the connection would do) https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_agent_bin.cc (left): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:63: PLOG(FATAL); out of cusiosity why are you removing the P here? https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_connection.cc (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:20: const uint32 kBattOrBitrate = 2000000; nit s/uint32/uint32_t/ :the non stdint versions are deprecated https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:28: const uint32 kMaxMessageSize = 50000; ditto uint32_t https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:38: void ParseMessage(const vector<char>* message, ordering of arguments: input arguments should come first and be of the form "const...&" unless there is a reason not to. output arguments should come last and be *. Maybe s/message_content/parsed_content/ ? https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:63: *type = static_cast<BattOrMessageType>((*message)[1]); shouldn't you CHECK that *type isn't > MESSAGE_TYPE_MAX (or similar) https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:138: uint16_t bytes_to_send) { On 2015/12/14 23:39:46, Zhen Wang wrote: > DCHECK the length of bytes == bytes_to_send. well, how if they are not a string (read: can genenuinely contain non-terminating 0s)? https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:139: vector<char> data_vector; just "data" should be enough. It's clear its a vector :) https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:194: scoped_ptr<vector<char>> bytes_already_read) { I wonder if this bytes_alread_read is really an argument here or is part of the state of your connection class ? you keep hopping this back and forth https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:195: auto io_buffer = make_scoped_refptr(new net::IOBuffer((size_t)bytes_to_read)); no c-style casts, use static_cast? (actually why don't you use size_t everywhere instead of int16_t?) https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_connection.h (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:28: class BattOrConnection : public base::SupportsWeakPtr<BattOrConnection> { Mostly a design thing here: If you used the same Listener pattern here (have an inner Listener class, which is imnplemented by BattOrAgent) you could avoid having an extra WeakPtr to care about here. The way it would work would be: this class would keep a WeakPtr<Listener> listener_ here in its private section. If you destroy the BattOrAgent (which owns the connection) what would happen is that the outstanding callbacks (from Connection -> Agent) would be automatically canceled, because the weakptr has been invalidated. Also, that would avoid you to bind() everything when you call methods, making the code a bit more readable and more performant. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:30: typedef base::Callback<void(bool success)> SendCallback; in C++ using "using" is the preferred way for this (it's just a bit more readable) i.e. using SendCallback = base::Callback<void(bool success)> same below https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:42: const base::Callback<void(bool success)> callback); Ironically this has the same signature of SendCallback, but here you take the full signature (by copy) and below (in SendBytes) you use the typedef and make it const& (which makes sense). Be consistent (I personally tend to like the "using" + short version here, otherwise when you add const ... & the full signature would become unreadable) https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:47: void SendBytes(const SendCallback& callback, Small nit: I'd move the callback to be the last argument (if you decide to keep the callbacks instead of using the listener), as it is kind of a return argument if you think about that. It's just more readable and intuitive if you say: "this is what I want to write, these are the bytes, this is callback where you tell me when you are done." as opposite to: "this is the callback to invoke when you are done, ah by the way these are the bytes I want to send" https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:49: const char* bytes, typically I see this as "const void*". It typically makes the casting easier on the caller side. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:57: void ReadBytes(const ReadCallback& callback, uint16_t bytes_to_read); ditto here about ordering https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:62: protected: why protected? Is there anything extending this class? https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_protocol_types.h (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_protocol_types.h:11: enum BattOrControlByte : uint8_t { I wonder if all these should be c++11 enum classes, i.e. enum class BattOrControlByte : uint8_t { START = 0, END = 1 ... MAX = 2 } this would also avoid to repeat the BATTOR_...TYPE prefix, as the scoped would be enclosed in BattorControlByte (e.g., BattorControlByte::START) https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_protocol_types.h:17: BATTOR_SPECIAL_BYTE_END, On 2015/12/14 23:39:47, Zhen Wang wrote: > I think explicitly writing the values here would make it easier to understand > the protocol, especially when BattOrMessageType follows BattOrControlByte > values. +1 https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_protocol_types.h:60: struct BattOrControlMessage { Not really a huge fan of packed structs, make the code generally harder to read an debug. Since you are using this only in one place, maybe you want to just pack it explicitly (by writing the individual bytes?) https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_protocol_types.h:67: struct BattOrControlMessageAck { nothing seems to use this
https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/BUIL... File tools/battor_agent/BUILD.gn (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/BUIL... tools/battor_agent/BUILD.gn:19: static_library("battor_agent_lib") { On 2015/12/15 11:07:19, Primiano Tucci wrote: > I missed this in a previous review. > I think this should be a source_set instad of static_library to avoid headaches > if one day you'll want to componentize this. > > See > https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/cookboo... Done. It looks like no parallel option is available for GYP? https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_agent.cc (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent.cc:69: connection_ = nullptr; On 2015/12/15 11:07:19, Primiano Tucci wrote: > typically I see this as connection_.reset() (doesn't change anything in > practice, I am surprised that this version even compiles) Done. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_agent.gyp (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent.gyp:42: 'target_name': 'sql_unittests', On 2015/12/15 11:07:19, Primiano Tucci wrote: > On 2015/12/14 23:39:44, Zhen Wang wrote: > > Where does "sql" come from? > > Yeah I guess you meant battor_agent_unittests. > Now we all know where you did copy this gyp snippet from :P Welp, my cover's blown. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_agent.h (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent.h:72: scoped_refptr<base::SingleThreadTaskRunner> file_thread_task_runner_; On 2015/12/15 11:07:19, Primiano Tucci wrote: > You don't seem to need these task runner here anymore. > What I would do here is construct the connection_ in the BattOrAgent ctor, so > that you don't have to keep the task runners (only the connection would do) Done. In an earlier code review, Zhen and I decided that it made sense to release the connection after StopTracing() was called. Before, the way that I was planning on doing this was to just delete the BattOrConnection in StopTracing() and create a new one if someone then called StartTracing(). This would release any serial handles that we had. However, if we don't keep the task runners around (which I don't think we should), we need some other method on BattOrConnection to disconnect. Given that, I added BattOrConnection::Close() and BattOrConnection::IsOpen() methods to help us manage the connection state without having to constantly recreate the object and keep the task runners around. I also went ahead and renamed BattOrConnection::Connect() to BattOrConnection::Open() (I figured we don't "connect" connections, we "open" them.) https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_agent_bin.cc (left): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:63: PLOG(FATAL); On 2015/12/15 11:07:19, Primiano Tucci wrote: > out of cusiosity why are you removing the P here? PLOG just appends the last system error to whatever message I specify. In our case, a system error wasn't the reason for exiting the program, and may have been completely unrelated to our program running. For example, when I did: $ cd /this/directory/doesnt/exist bash: cd: /this/directory/doesnt/exist: No such file or directory and then: $ out/Default/battor_agent StartTracing /dev/nonexistent I got: Fatal error when communicating with the BattOr: cd: /this/directory/doesnt/exist: No such file or directory Given that we weren't necessarily exiting due to a system error, I think LOG(FATAL) is probably the way to go https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_connection.cc (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:20: const uint32 kBattOrBitrate = 2000000; On 2015/12/15 11:07:20, Primiano Tucci wrote: > nit s/uint32/uint32_t/ :the non stdint versions are deprecated Done. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:28: const uint32 kMaxMessageSize = 50000; On 2015/12/15 11:07:20, Primiano Tucci wrote: > ditto uint32_t Done. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:37: // - The number of escape bytes contained in the message On 2015/12/14 23:39:46, Zhen Wang wrote: > Can you make the argument match the explanation? For example: > > - message: ... > - message_content: ... > ... Done. I also consolidated is_complete and is_invalid_so_far into a single concept (an enum, MessageHealth). https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:38: void ParseMessage(const vector<char>* message, On 2015/12/15 11:07:20, Primiano Tucci wrote: > ordering of arguments: > input arguments should come first and be of the form "const...&" unless there is > a reason not to. > output arguments should come last and be *. > > Maybe s/message_content/parsed_content/ ? Done. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:63: *type = static_cast<BattOrMessageType>((*message)[1]); On 2015/12/15 11:07:19, Primiano Tucci wrote: > shouldn't you CHECK that *type isn't > MESSAGE_TYPE_MAX (or similar) Done. I didn't CHECK though, because it's possible that the bytes got garbled going over the wire - I signaled an invalid state instead. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:136: BattOrMessageType type, On 2015/12/14 23:39:46, Zhen Wang wrote: > wrong indent Doh. Looks like I forgot to rerun git cl format after running git add battor_communicator* https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:138: uint16_t bytes_to_send) { On 2015/12/15 11:07:19, Primiano Tucci wrote: > On 2015/12/14 23:39:46, Zhen Wang wrote: > > DCHECK the length of bytes == bytes_to_send. > > well, how if they are not a string (read: can genenuinely contain > non-terminating 0s)? Yea, I don't think that it's possible to determine the size of bytes (otherwise, I'd remove the bytes_to_send parameter altogether). https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:139: vector<char> data_vector; On 2015/12/15 11:07:19, Primiano Tucci wrote: > just "data" should be enough. It's clear its a vector :) Ah, sorry about that. At some point, the incoming parameter was named "data", so I named the vector "data_vector" to differentiate. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:160: data_vector.push_back(BATTOR_SPECIAL_BYTE_ESCAPE); On 2015/12/14 23:39:46, Zhen Wang wrote: > Add {...} when if clause takes more than 1 line. Done. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:173: uint16_t bytes_to_read) { On 2015/12/14 23:39:44, Zhen Wang wrote: > wrong indent Doh. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:194: scoped_ptr<vector<char>> bytes_already_read) { On 2015/12/15 11:07:19, Primiano Tucci wrote: > I wonder if this bytes_alread_read is really an argument here or is part of the > state of your connection class ? > you keep hopping this back and forth Good point. I added a new private variable named pending_read_buffer_ and this seemed to clean things up a lot. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:195: auto io_buffer = make_scoped_refptr(new net::IOBuffer((size_t)bytes_to_read)); On 2015/12/15 11:07:19, Primiano Tucci wrote: > no c-style casts, use static_cast? > (actually why don't you use size_t everywhere instead of int16_t?) Done, although this did involve some casting in order to match a few of the callbacks that had interfaces dictated by device/serial. Also, there are a few places I would've liked to check for overflow before casting, but it turns out that problem is surprisingly difficult https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:237: // how many escape bytes we've already read. On 2015/12/14 23:39:46, Zhen Wang wrote: > Move the comment to above ParseMessage. > > Some more thoughts: > It looks like we only need escape_byte_count here. And we only need some other > info in the below ParseMessage. Those two have different purposes. Can we make > two functions, each handle a different task? Those two functions can share the > same parsing logic. I had thought about that, but I was trying to avoid another reparsing the of the message. I think that I've gotten around this by having another temporary variable (alongside the newly-created pending_read_buffer_). Let me know what you think. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_connection.h (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:27: // protocol. On 2015/12/14 23:39:46, Zhen Wang wrote: > Is there a description of BattOr protocol and how it works? Or is there a link > to such thing (e.g., a github link)? Done. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:28: class BattOrConnection : public base::SupportsWeakPtr<BattOrConnection> { On 2015/12/15 11:07:20, Primiano Tucci wrote: > Mostly a design thing here: > If you used the same Listener pattern here (have an inner Listener class, which > is imnplemented by BattOrAgent) you could avoid having an extra WeakPtr to care > about here. > > The way it would work would be: > this class would keep a WeakPtr<Listener> listener_ here in its private section. > If you destroy the BattOrAgent (which owns the connection) what would happen is > that the outstanding callbacks (from Connection -> Agent) would be automatically > canceled, because the weakptr has been invalidated. > > Also, that would avoid you to bind() everything when you call methods, making > the code a bit more readable and more performant. I thought about this for a while (obviously, that's the system we're using for the BattOrAgent), and couldn't decide whether it was overkill or not. I'll go ahead and implement it. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:30: typedef base::Callback<void(bool success)> SendCallback; On 2015/12/15 11:07:20, Primiano Tucci wrote: > in C++ using "using" is the preferred way for this (it's just a bit more > readable) > i.e. using SendCallback = base::Callback<void(bool success)> > > same below Ack. This actually went away after implementing the Listener interface. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:42: const base::Callback<void(bool success)> callback); On 2015/12/15 11:07:20, Primiano Tucci wrote: > Ironically this has the same signature of SendCallback, but here you take the > full signature (by copy) and below (in SendBytes) you use the typedef and make > it const& (which makes sense). > Be consistent (I personally tend to like the "using" + short version here, > otherwise when you add const ... & the full signature would become unreadable) Acknowledged. Not completely sure what my reasoning was here... I remember thinking about this, but it doesn't look like I actually landed on a coherent plan. I ended up going with the listener interface, anyhow. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:47: void SendBytes(const SendCallback& callback, On 2015/12/15 11:07:20, Primiano Tucci wrote: > Small nit: I'd move the callback to be the last argument (if you decide to keep > the callbacks instead of using the listener), as it is kind of a return argument > if you think about that. > It's just more readable and intuitive if you say: > "this is what I want to write, these are the bytes, this is callback where you > tell me when you are done." > as opposite to: > "this is the callback to invoke when you are done, ah by the way these are the > bytes I want to send" Ack. I ended up going with the Listener interface, but it's a fair point nonetheless. I definitely agree that I prefer the out parameters to go at the end of the argument list. The problem that I ran into was that, lots of times, I'd want to use partial application to bind the callback as a first parameter in a method that needed to fit a certain interface. Here's an example: void Connection::Send(const std::string& message, const base::Callback<void(bool success)>& callback) { io_handler_->Send( message, base::Bind(&Connection::InternalSendCallback, AsWeakPtr(), callback)); } void Connection::InternalSendCallback( const base::Callback<void(bool success)>& callback, device::serial::SendError error) { // Do some transformation on the raw return value - here, we just convert from // device::serial::SendError to a success bool. callback.Run(error == SEND_ERROR_NONE); } Because I can only do partial application to parameters at the beginning of the parameter list, it forces me to put the callback there. Then, for consistency, I don't want the methods that need to be partially applied to be the only ones with first-parameter callbacks, so I just moved all of the callbacks to the first method. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:49: const char* bytes, On 2015/12/15 11:07:20, Primiano Tucci wrote: > typically I see this as "const void*". > It typically makes the casting easier on the caller side. Done. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:57: void ReadBytes(const ReadCallback& callback, uint16_t bytes_to_read); On 2015/12/15 11:07:20, Primiano Tucci wrote: > ditto here about ordering (see above) https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:62: protected: On 2015/12/15 11:07:20, Primiano Tucci wrote: > why protected? Is there anything extending this class? In battor_connection_unittest, we override CreateIoHandler() to use a fake serial connection. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_connection_unittest.cc (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection_unittest.cc:100: bool connect_success; On 2015/12/14 23:39:47, Zhen Wang wrote: > I think the practice is to make variables private and have methods to check > their values? Done. I wasn't sure if this was the case in unit tests, given that something like BattOrConnectionTest_InitSecondsCorrectBytes is a subclass of BattOrConnectionTest. I figured it might be okay to just use protected variables, but I definitely defer to you here. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_protocol_types.h (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_protocol_types.h:11: enum BattOrControlByte : uint8_t { On 2015/12/15 11:07:20, Primiano Tucci wrote: > I wonder if all these should be c++11 enum classes, i.e. > enum class BattOrControlByte : uint8_t { > START = 0, > END = 1 > ... > MAX = 2 > } > > this would also avoid to repeat the BATTOR_...TYPE prefix, as the scoped would > be enclosed in BattorControlByte (e.g., BattorControlByte::START) Definitely not an expert in this area, but it seems like this might be one of not-very-many cases where we actually want loosely typed (non-class) enums because we want it to be really easy to use the enums as their hex values. The strongly typed class enums seem to excel at being hard to use as integers, which is exactly what we want to do. For example, with loosely typed enums, we can do: if ((*message)[0] != BATTOR_CONTROL_BYTE_START) { whereas, with strongly typed enums, we'd have to do: if ((*message[0] != static_cast<char>(BattOrControlByte::Start))) { at least for me, the version w/o the cast seems less ugly. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_protocol_types.h:17: BATTOR_SPECIAL_BYTE_END, On 2015/12/15 11:07:20, Primiano Tucci wrote: > On 2015/12/14 23:39:47, Zhen Wang wrote: > > I think explicitly writing the values here would make it easier to understand > > the protocol, especially when BattOrMessageType follows BattOrControlByte > > values. > +1 Done. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_protocol_types.h:60: struct BattOrControlMessage { On 2015/12/15 11:07:20, Primiano Tucci wrote: > Not really a huge fan of packed structs, make the code generally harder to read > an debug. > Since you are using this only in one place, maybe you want to just pack it > explicitly (by writing the individual bytes?) In our case, the BattOr firmware expects the struct to be packed, which means that we need to do it in order to ensure that what we send over the wire is compatible. It seems like manually packing the struct doesn't really get us much besides added code complexity. I added a comment above stating why the packing is necessary - do you think that's good enough? https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_protocol_types.h:67: struct BattOrControlMessageAck { On 2015/12/15 11:07:20, Primiano Tucci wrote: > nothing seems to use this Ah, sorry. I'll add this back in when the next CL uses it.
Ran a bit short of time so couldn't look at the test, but the rest looks LGTM with some minor comments. Maybe wait for zhen/ned to give a second pass. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_connection.h (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:62: protected: On 2015/12/15 23:50:04, charliea wrote: > On 2015/12/15 11:07:20, Primiano Tucci wrote: > > why protected? Is there anything extending this class? > > In battor_connection_unittest, we override CreateIoHandler() to use a fake > serial connection. ah got it, thanks Maybe add a one line comment saying: overridden by test mockup, or similar. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_protocol_types.h (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_protocol_types.h:11: enum BattOrControlByte : uint8_t { On 2015/12/15 23:50:04, charliea wrote: > On 2015/12/15 11:07:20, Primiano Tucci wrote: > > I wonder if all these should be c++11 enum classes, i.e. > > enum class BattOrControlByte : uint8_t { > > START = 0, > > END = 1 > > ... > > MAX = 2 > > } > > > > this would also avoid to repeat the BATTOR_...TYPE prefix, as the scoped would > > be enclosed in BattorControlByte (e.g., BattorControlByte::START) > > Definitely not an expert in this area, but it seems like this might be one of > not-very-many cases where we actually want loosely typed (non-class) enums > because we want it to be really easy to use the enums as their hex values. The > strongly typed class enums seem to excel at being hard to use as integers, which > is exactly what we want to do. > > For example, with loosely typed enums, we can do: > > if ((*message)[0] != BATTOR_CONTROL_BYTE_START) { > > whereas, with strongly typed enums, we'd have to do: > > if ((*message[0] != static_cast<char>(BattOrControlByte::Start))) { > > at least for me, the version w/o the cast seems less ugly. ah ok got it, makes sense https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_protocol_types.h:60: struct BattOrControlMessage { On 2015/12/15 23:50:04, charliea wrote: > On 2015/12/15 11:07:20, Primiano Tucci wrote: > > Not really a huge fan of packed structs, make the code generally harder to > read > > an debug. > > Since you are using this only in one place, maybe you want to just pack it > > explicitly (by writing the individual bytes?) > > In our case, the BattOr firmware expects the struct to be packed, which means > that we need to do it in order to ensure that what we send over the wire is > compatible. It seems like manually packing the struct doesn't really get us much > besides added code complexity. > > I added a comment above stating why the packing is necessary - do you think > that's good enough? ok makes sense https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_protocol_types.h:67: struct BattOrControlMessageAck { On 2015/12/15 23:50:04, charliea wrote: > On 2015/12/15 11:07:20, Primiano Tucci wrote: > > nothing seems to use this > > Ah, sorry. I'll add this back in when the next CL uses it. ah nevermind if you are going to use it. I though was a leftover https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_agent.gyp (right): https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_agent.gyp:47: '../base/base.gyp:test_support_base', nit: alpha orderding of these https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_agent.h (right): https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_agent.h:55: // Callback for when the connection is opened. I'd probably just say // BattOrConnection::Listener implementation. What the callbacks do is pretty clear from the name :) https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_connection.cc (right): https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:236: void BattOrConnection::OnBytesRead(int bytes_read, nit: you probably want to reorder this so that OnBytesRead is just after ReadMoreBytes, so the code is easier to follow https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:281: listener_->OnBytesRead(true, type, parsed_content.Pass()); One general comment about callbacks: think about re-entrancy. In general you have two options here when invoking the listener callbacks: - invoking them inline (As you are doing) - re-posting them (you should have the right task runner already). So the deal is: are you ever going to call back into the battor agent from these callbacks? If so, your code might end up being re-entrant and difficult to debug / reason about. The easy way to break re-entrancy is Posting those callbacks. It shouldn't be a problem for the callback that are the last statement (the one up) but could make the situation funky if you: - Call OnBytesRead if health != COMPOLETE above - The OnBytesRead invokes back something on the COnnection that changes the state of the Connection object - You then call the 2nd OnBytesRead callback. Up to you really, just keep in mind that re-entrancy might bite you when you have callback (with or without listener) https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_connection.h (right): https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:45: // Callback for when the connection is opened. same thing here: probably these are self describing :) Up to you really https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:57: base::WeakPtr<Listener> listener, Maybe you want to add a comment to say why the listener needs to be weakptr. If I understood everything correctly, the problem is that by using IOHandler you can end up having callbacks even after you destroy that. So you can have BattOrAgent (destroyes)-> BattorConnection (destroyes) -> SerialIOOHandler but after these destructions the Serial IO thingy might have queued up tasks that would hit a nullptr, hence you need a weakptr. Is that correct? If so add a comment
lgtm https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_connection.cc (right): https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:39: // nit: remove the empty line. https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:275: if (health != MessageHealth::COMPLETE) nit: I think (health == MessageHealth::INCOMPLETE) is more intuitive. Logically, this is also correct if you are going to add more types of message health.
charliea@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke for net/base DEP https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_connection.h (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:62: protected: On 2015/12/16 16:47:43, Primiano Tucci wrote: > On 2015/12/15 23:50:04, charliea wrote: > > On 2015/12/15 11:07:20, Primiano Tucci wrote: > > > why protected? Is there anything extending this class? > > > > In battor_connection_unittest, we override CreateIoHandler() to use a fake > > serial connection. > > ah got it, thanks > Maybe add a one line comment saying: overridden by test mockup, or similar. Done. https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_protocol_types.h (right): https://codereview.chromium.org/1524873002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_protocol_types.h:67: struct BattOrControlMessageAck { On 2015/12/16 16:47:43, Primiano Tucci wrote: > On 2015/12/15 23:50:04, charliea wrote: > > On 2015/12/15 11:07:20, Primiano Tucci wrote: > > > nothing seems to use this > > > > Ah, sorry. I'll add this back in when the next CL uses it. > > ah nevermind if you are going to use it. > I though was a leftover SG. In that case, I went ahead and added it back in. https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_agent.h (right): https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_agent.h:55: // Callback for when the connection is opened. On 2015/12/16 16:47:43, Primiano Tucci wrote: > I'd probably just say > // BattOrConnection::Listener implementation. > What the callbacks do is pretty clear from the name :) Hah, true :) Changed. https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_connection.cc (right): https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:39: // On 2015/12/16 18:01:41, Zhen Wang wrote: > nit: remove the empty line. Done. https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:236: void BattOrConnection::OnBytesRead(int bytes_read, On 2015/12/16 16:47:43, Primiano Tucci wrote: > nit: you probably want to reorder this so that OnBytesRead is just after > ReadMoreBytes, so the code is easier to follow Done (here and in the header file). https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:275: if (health != MessageHealth::COMPLETE) On 2015/12/16 18:01:41, Zhen Wang wrote: > nit: I think (health == MessageHealth::INCOMPLETE) is more intuitive. Logically, > this is also correct if you are going to add more types of message health. Ah, yea, I agree. Done. https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_connection.cc:281: listener_->OnBytesRead(true, type, parsed_content.Pass()); On 2015/12/16 16:47:43, Primiano Tucci wrote: > One general comment about callbacks: think about re-entrancy. > In general you have two options here when invoking the listener callbacks: > - invoking them inline (As you are doing) > - re-posting them (you should have the right task runner already). > > So the deal is: are you ever going to call back into the battor agent from these > callbacks? If so, your code might end up being re-entrant and difficult to debug > / reason about. > The easy way to break re-entrancy is Posting those callbacks. > It shouldn't be a problem for the callback that are the last statement (the one > up) but could make the situation funky if you: > > - Call OnBytesRead if health != COMPOLETE above > - The OnBytesRead invokes back something on the COnnection that changes the > state of the Connection object > - You then call the 2nd OnBytesRead callback. > > Up to you really, just keep in mind that re-entrancy might bite you when you > have callback (with or without listener) Ah, interesting! Thanks for the explanation. I'd seen the idiom of posting to the current task runner before, and I didn't understand why that was done rather than just calling the function directly. Your explanation definitely made it make more sense. I think in this case, it's probably okay to just call the listeners directly, at least for now. The fact that this class is accessing a serial connection which is itself stateful pretty much guarantees that this code won't be reentrant, and that the caller has to take measures herself to guarantee that it's never called twice at the same time. It seems like, if in the future we end up needing to post the execution of these callbacks instead, it shouldn't be too difficult to change. https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_connection.h (right): https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:45: // Callback for when the connection is opened. On 2015/12/16 16:47:43, Primiano Tucci wrote: > same thing here: probably these are self describing :) > Up to you really Done. https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:57: base::WeakPtr<Listener> listener, On 2015/12/16 16:47:43, Primiano Tucci wrote: > Maybe you want to add a comment to say why the listener needs to be weakptr. > If I understood everything correctly, the problem is that by using IOHandler you > can end up having callbacks even after you destroy that. > So you can have > BattOrAgent (destroyes)-> BattorConnection (destroyes) -> SerialIOOHandler > but after these destructions the Serial IO thingy might have queued up tasks > that would hit a nullptr, hence you need a weakptr. > Is that correct? If so add a comment Hmmm... I don't think that's quite right, but you bring up a really good point. I don't actually think that the WeakPtr is necessary here, and we can instead just use a Listener*. It is true that the SerialIoHandler can call whatever callback we pass it after we've been destroyed and released our handle to it. However, all of the callbacks that we pass it our on BattOrConnection, so that just means that it's important the BattOrConnection passes WeakPtrs to the SerialIoHandler. If the BattOrConnection callback is called, then the BattOrAgent (the Listener) is guaranteed to still be alive, because it owns and outlives the BattOrConnection.
You're just using net for IOBuffer, right? LGTM.
lgtm overall with a question https://codereview.chromium.org/1524873002/diff/80001/tools/battor_agent/batt... File tools/battor_agent/battor_agent.h (right): https://codereview.chromium.org/1524873002/diff/80001/tools/battor_agent/batt... tools/battor_agent/battor_agent.h:53: // BattOrConnection::Listener implementation. Should these method be private and BattorConnection be a friend class with BattorConnection?
https://codereview.chromium.org/1524873002/diff/80001/tools/battor_agent/batt... File tools/battor_agent/battor_agent.h (right): https://codereview.chromium.org/1524873002/diff/80001/tools/battor_agent/batt... tools/battor_agent/battor_agent.h:53: // BattOrConnection::Listener implementation. On 2015/12/16 18:37:24, nednguyen wrote: > Should these method be private and BattorConnection be a friend class with > BattorConnection? Err, I mean BattOrConnection is friend with BattorAgent
On 2015/12/16 18:32:10, mmenke wrote: > You're just using net for IOBuffer, right? LGTM. Yep!
On 2015/12/16 18:37:24, nednguyen wrote: > lgtm overall with a question > > https://codereview.chromium.org/1524873002/diff/80001/tools/battor_agent/batt... > File tools/battor_agent/battor_agent.h (right): > > https://codereview.chromium.org/1524873002/diff/80001/tools/battor_agent/batt... > tools/battor_agent/battor_agent.h:53: // BattOrConnection::Listener > implementation. > Should these method be private and BattorConnection be a friend class with > BattorConnection? Yea, that's a fair question. A quick search within the Chromium codebase seems to suggest that it's typical to keep all of the interface methods public: https://code.google.com/p/chromium/codesearch#search/&q=class.*listener&p=2&s...
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zhenw@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1524873002/#ps80001 (title: "Code review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1524873002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1524873002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zhenw@chromium.org, nednguyen@google.com, primiano@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1524873002/#ps100001 (title: "Fixed gyp build file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1524873002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1524873002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zhenw@chromium.org, nednguyen@google.com, primiano@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1524873002/#ps120001 (title: "Added missing dep")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1524873002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1524873002/120001
https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_connection.h (right): https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_connection.h:57: base::WeakPtr<Listener> listener, On 2015/12/16 18:27:55, charliea wrote: > On 2015/12/16 16:47:43, Primiano Tucci wrote: > > Maybe you want to add a comment to say why the listener needs to be weakptr. > > If I understood everything correctly, the problem is that by using IOHandler > you > > can end up having callbacks even after you destroy that. > > So you can have > > BattOrAgent (destroyes)-> BattorConnection (destroyes) -> SerialIOOHandler > > but after these destructions the Serial IO thingy might have queued up tasks > > that would hit a nullptr, hence you need a weakptr. > > Is that correct? If so add a comment > > Hmmm... I don't think that's quite right, but you bring up a really good point. > I don't actually think that the WeakPtr is necessary here, and we can instead > just use a Listener*. It is true that the SerialIoHandler can call whatever > callback we pass it after we've been destroyed and released our handle to it. > However, all of the callbacks that we pass it our on BattOrConnection, so that > just means that it's important the BattOrConnection passes WeakPtrs to the > SerialIoHandler. If the BattOrConnection callback is called, then the > BattOrAgent (the Listener) is guaranteed to still be alive, because it owns and > outlives the BattOrConnection. Ok yeah let's clarify a bit this, let me see if I am getting everything. - The Agent outlives the Connection (connection is a scoped_ptr in agent, right?) - You can never have an Connection alive without an Agent (the listener). - As long as you invoke the callbacks Connection -> Agent synchronously (Without posting) Listener* is fine. It you reached a Connection method, the agent must be alive. - You will need a weakptr back if you start PostTask-ing the callbacks back to the Agent. In that case you cannot say PostTask(base::Unretained(listener)) because the listener might go away before the task gets posted. - I agree that you still need the Connection to be a WeakPtr-able, as you are not guaranteed about the SerialIOHandler -> Connection callbacks vs. lifetime. Does it make sense?
On 2015/12/16 19:41:51, Primiano Tucci wrote: > https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... > File tools/battor_agent/battor_connection.h (right): > > https://codereview.chromium.org/1524873002/diff/40001/tools/battor_agent/batt... > tools/battor_agent/battor_connection.h:57: base::WeakPtr<Listener> listener, > On 2015/12/16 18:27:55, charliea wrote: > > On 2015/12/16 16:47:43, Primiano Tucci wrote: > > > Maybe you want to add a comment to say why the listener needs to be weakptr. > > > If I understood everything correctly, the problem is that by using IOHandler > > you > > > can end up having callbacks even after you destroy that. > > > So you can have > > > BattOrAgent (destroyes)-> BattorConnection (destroyes) -> SerialIOOHandler > > > but after these destructions the Serial IO thingy might have queued up tasks > > > that would hit a nullptr, hence you need a weakptr. > > > Is that correct? If so add a comment > > > > Hmmm... I don't think that's quite right, but you bring up a really good > point. > > I don't actually think that the WeakPtr is necessary here, and we can instead > > just use a Listener*. It is true that the SerialIoHandler can call whatever > > callback we pass it after we've been destroyed and released our handle to it. > > However, all of the callbacks that we pass it our on BattOrConnection, so that > > just means that it's important the BattOrConnection passes WeakPtrs to the > > SerialIoHandler. If the BattOrConnection callback is called, then the > > BattOrAgent (the Listener) is guaranteed to still be alive, because it owns > and > > outlives the BattOrConnection. > > Ok yeah let's clarify a bit this, let me see if I am getting everything. > - The Agent outlives the Connection (connection is a scoped_ptr in agent, > right?) > - You can never have an Connection alive without an Agent (the listener). > - As long as you invoke the callbacks Connection -> Agent synchronously (Without > posting) Listener* is fine. It you reached a Connection method, the agent must > be alive. > - You will need a weakptr back if you start PostTask-ing the callbacks back to > the Agent. In that case you cannot say PostTask(base::Unretained(listener)) > because the listener might go away before the task gets posted. > - I agree that you still need the Connection to be a WeakPtr-able, as you are > not guaranteed about the SerialIOHandler -> Connection callbacks vs. lifetime. > > Does it make sense? Yep! That looks right on all accounts to me.
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Creates a BattOrConnection for communicating with the BattOr I first tried to implement the protocol with this logic rolled into BattOrAgent. However, BattOrAgent needs to act as a state machine stepping through the BattOr protocol and, as the state machine grew, it became obvious that we should move as much unrelated logic as possible out of the class. The byte-level protocol now seen in battor_connection.cc was an obvious choice to be moved elsewhere. For a look at how these communication primitives will be used to implement the protocol, see http://bit.ly/1UpLv3p. BUG=542837 ========== to ========== Creates a BattOrConnection for communicating with the BattOr I first tried to implement the protocol with this logic rolled into BattOrAgent. However, BattOrAgent needs to act as a state machine stepping through the BattOr protocol and, as the state machine grew, it became obvious that we should move as much unrelated logic as possible out of the class. The byte-level protocol now seen in battor_connection.cc was an obvious choice to be moved elsewhere. For a look at how these communication primitives will be used to implement the protocol, see http://bit.ly/1UpLv3p. BUG=542837 Committed: https://crrev.com/07027e6fbaa31604a908f7d1d2b42e789e93add8 Cr-Commit-Position: refs/heads/master@{#365606} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/07027e6fbaa31604a908f7d1d2b42e789e93add8 Cr-Commit-Position: refs/heads/master@{#365606}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:120001) has been created in https://codereview.chromium.org/1533643002/ by danakj@chromium.org. The reason for reverting is: Build failures on windows? Not sure why this made it through the CQ then. Please follow up with a bug @ infra team when that is sorted out. https://build.chromium.org/p/chromium/builders/Win/builds/38410/steps/compile... FAILED: ninja -t msvc -e environment.x86 -- C:\b\build\goma/gomacc "C:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo /showIncludes /FC @obj\tools\battor_agent\battor_agent_lib.battor_agent.obj.rsp /c ..\..\tools\battor_agent\battor_agent.cc /Foobj\tools\battor_agent\battor_agent_lib.battor_agent.obj /Fdobj\tools\battor_agent\battor_agent_lib.cc.pdb c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(67) : error C2065: 'packed' : undeclared identifier c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(73) : error C2065: 'packed' : undeclared identifier c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(73) : error C2371: 'battor::__attribute__' : redefinition; different basic types c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(67) : see declaration of 'battor::__attribute__' FAILED: ninja -t msvc -e environment.x86 -- C:\b\build\goma/gomacc "C:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo /showIncludes /FC @obj\tools\battor_agent\battor_agent_lib.battor_connection.obj.rsp /c ..\..\tools\battor_agent\battor_connection.cc /Foobj\tools\battor_agent\battor_agent_lib.battor_connection.obj /Fdobj\tools\battor_agent\battor_agent_lib.cc.pdb c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(67) : error C2065: 'packed' : undeclared identifier c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(73) : error C2065: 'packed' : undeclared identifier c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(73) : error C2371: 'battor::__attribute__' : redefinition; different basic types c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(67) : see declaration of 'battor::__attribute__' FAILED: ninja -t msvc -e environment.x86 -- C:\b\build\goma/gomacc "C:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo /showIncludes /FC @obj\tools\battor_agent\battor_agent.battor_agent_bin.obj.rsp /c ..\..\tools\battor_agent\battor_agent_bin.cc /Foobj\tools\battor_agent\battor_agent.battor_agent_bin.obj /Fdobj\tools\battor_agent\battor_agent.cc.pdb c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(67) : error C2065: 'packed' : undeclared identifier c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(73) : error C2065: 'packed' : undeclared identifier c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(73) : error C2371: 'battor::__attribute__' : redefinition; different basic types c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(67) : see declaration of 'battor::__attribute__' ninja: build stopped: subcommand failed..
Message was sent while issue was closed.
On 2015/12/16 21:23:09, danakj (behind on reviews) wrote: > A revert of this CL (patchset #6 id:120001) has been created in > https://codereview.chromium.org/1533643002/ by mailto:danakj@chromium.org. > > The reason for reverting is: Build failures on windows? Not sure why this made > it through the CQ then. Please follow up with a bug @ infra team when that is > sorted out. > https://build.chromium.org/p/chromium/builders/Win/builds/38410/steps/compile... > > FAILED: ninja -t msvc -e environment.x86 -- C:\b\build\goma/gomacc > "C:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo > /showIncludes /FC @obj\tools\battor_agent\battor_agent_lib.battor_agent.obj.rsp > /c ..\..\tools\battor_agent\battor_agent.cc > /Foobj\tools\battor_agent\battor_agent_lib.battor_agent.obj > /Fdobj\tools\battor_agent\battor_agent_lib.cc.pdb > c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(67) : > error C2065: 'packed' : undeclared identifier > c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(73) : > error C2065: 'packed' : undeclared identifier > c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(73) : > error C2371: 'battor::__attribute__' : redefinition; different basic types > > c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(67) : > see declaration of 'battor::__attribute__' > FAILED: ninja -t msvc -e environment.x86 -- C:\b\build\goma/gomacc > "C:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo > /showIncludes /FC > @obj\tools\battor_agent\battor_agent_lib.battor_connection.obj.rsp /c > ..\..\tools\battor_agent\battor_connection.cc > /Foobj\tools\battor_agent\battor_agent_lib.battor_connection.obj > /Fdobj\tools\battor_agent\battor_agent_lib.cc.pdb > c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(67) : > error C2065: 'packed' : undeclared identifier > c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(73) : > error C2065: 'packed' : undeclared identifier > c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(73) : > error C2371: 'battor::__attribute__' : redefinition; different basic types > > c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(67) : > see declaration of 'battor::__attribute__' > FAILED: ninja -t msvc -e environment.x86 -- C:\b\build\goma/gomacc > "C:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo > /showIncludes /FC @obj\tools\battor_agent\battor_agent.battor_agent_bin.obj.rsp > /c ..\..\tools\battor_agent\battor_agent_bin.cc > /Foobj\tools\battor_agent\battor_agent.battor_agent_bin.obj > /Fdobj\tools\battor_agent\battor_agent.cc.pdb > c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(67) : > error C2065: 'packed' : undeclared identifier > c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(73) : > error C2065: 'packed' : undeclared identifier > c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(73) : > error C2371: 'battor::__attribute__' : redefinition; different basic types > > c:\b\build\slave\win\build\src\tools\battor_agent\battor_protocol_types.h(67) : > see declaration of 'battor::__attribute__' > ninja: build stopped: subcommand failed.. Sorry about that - I'll look into fixing this. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
