Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2390893002:
[BattOr] Make BattOr able to return firmware version. (Closed)
|
Created:
4 years, 2 months ago by rnephew (Reviews Here) Modified:
4 years, 1 month ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[BattOr] Make BattOr able to return firmware version.
BUG=652380
Committed: https://crrev.com/b0964536bbda88e09d28e2ea84cb12e8978da4e0
Cr-Commit-Position: refs/heads/master@{#428014}
Patch Set 1 #
Total comments: 2
Patch Set 2 : [BattOr] Make BattOr able to return firmware version. #
Total comments: 1
Patch Set 3 : Start Work On Getting Git Hash #
Total comments: 3
Patch Set 4 : add control enums #
Total comments: 3
Patch Set 5 : First functional version #
Total comments: 11
Patch Set 6 : [BattOr] Make BattOr able to return firmware version. #
Total comments: 10
Patch Set 7 : [BattOr] Make BattOr able to return firmware version. #Patch Set 8 : fix compiling error #
Messages
Total messages: 38 (12 generated)
charliea@chromium.org changed reviewers: + charliea@chromium.org
https://codereview.chromium.org/2390893002/diff/1/tools/battor_agent/battor_a... File tools/battor_agent/battor_agent.cc (right): https://codereview.chromium.org/2390893002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent.cc:163: DCHECK(thread_checker_.CalledOnValidThread()); So after you make this look like: void BattOrAgent::GetVersion() { ... } I think the key thing that's not right here yet is that we need to actually *get* the EEPROM from the BattOr before we're able to look up the version number in it. The EEPROM is basically just a struct that we request from the BattOr and the BattOr sends back. However, in order to even perform that request, we need to initialize the connection with the BattOr. You can copy how to do this from StartTracing, StopTracing, and RecordClockSyncMarker. After the connection is initialized, OnConnectionOpened() will be called. You'll probably want to add an additional case in the switch statement there along the lines of: case Command::GET_VERSION: PerformAction(Action::SEND_EEPROM_REQUEST); return; Then, after you receive the EEPROM (in the switch() statement of BattOrAgent::OnBytesRead, under Action::READ_EEPROM), you'll want to add some logic along the lines of if (command_ == Command::GET_VERSION) { CompleteCommand(BATTOR_ERROR_NONE); } else if (command_ == Command::STOP_TRACING) { // Existing logic... } else { // Throw some error } Finally, you'll want to add your battor_eeprom_->version lookup logic that you have right below into CompleteCommand under a new GET_VERSION section. CompleteCommand is basically called to say "we have all the information that we need from the BattOr stored in member variables of the BattOrAgent: go ahead and call the callback with the right info". https://codereview.chromium.org/2390893002/diff/1/tools/battor_agent/battor_a... File tools/battor_agent/battor_agent.h (right): https://codereview.chromium.org/2390893002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent.h:59: Rather than `int Version()` below, I think you want `void GetVersion()` here, with the assumption that `void OnGetVersionComplete(bool success, int version)` will be called when the command finishes. Basically, you want to change this from being a function that returns its value via a return value to a function that returns its value via a callback. The reason for this is that you actually need to do some communication with the BattOr in order to get that version (over serial), and that communication takes time. There's also an assumption that the thread that these functions (StartTracing, StopTracing, etc.) are run on won't block. These two facts together make it impossible to return our version via return value. The reason that SupportsExplicitClockSync *isn't* like this is because we know the answer (true) without having to communicate with the BattOr.
With your last comments I was able to get it mostly working (still not getting the real version number, but that shouldn't be too hard). There is an issue I cant seem to solve though where if you try to get the Version twice in a row it all falls apart. rnephew@rnephew0:~/chromium/linux/src/out/Default$ ./battor_agent --battor-path=/dev/ttyUSB0 Version 1 Version [1004/122505:FATAL:battor_agent_bin.cc(88)] Fatal error when communicating with the BattOr: UNEXPECTED MESSAGE #0 0x7f7b67927d2e base::debug::StackTrace::StackTrace() #1 0x7f7b67995b8f logging::LogMessage::~LogMessage() #2 0x00000040e2f9 battor::(anonymous namespace)::HandleError() #3 0x00000040ef43 battor::BattOrAgentBin::OnGetVersionComplete() #4 0x000000427da9 _ZN4base8internal13FunctorTraitsIMN6battor11BattOrAgent8ListenerEFviNS2_11BattOrErrorEEvE6InvokeIPS4_JRKiRKS5_EEEvS7_OT_DpOT0_ #5 0x000000427cab _ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKMN6battor11BattOrAgent8ListenerEFviNS4_11BattOrErrorEEJPS6_RKiRKS7_EEEvOT_DpOT0_ #6 0x000000427c24 _ZN4base8internal7InvokerINS0_9BindStateIMN6battor11BattOrAgent8ListenerEFviNS3_11BattOrErrorEEJNS0_17UnretainedWrapperIS5_EEiS6_EEEFvvEE7RunImplIRKS8_RKSt5tupleIJSA_iS6_EEJLm0ELm1ELm2EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE #7 0x000000427afc _ZN4base8internal7InvokerINS0_9BindStateIMN6battor11BattOrAgent8ListenerEFviNS3_11BattOrErrorEEJNS0_17UnretainedWrapperIS5_EEiS6_EEEFvvEE3RunEPNS0_13BindStateBaseE #8 0x7f7b678f817b base::internal::RunMixin<>::Run() #9 0x7f7b6792d691 base::debug::TaskAnnotator::RunTask() #10 0x7f7b679bd6ef base::MessageLoop::RunTask() #11 0x7f7b679bd934 base::MessageLoop::DeferOrRunPendingTask() #12 0x7f7b679bdbfe base::MessageLoop::DoWork() #13 0x7f7b679d7aae base::MessagePumpLibevent::Run() #14 0x7f7b679bd2a6 base::MessageLoop::RunHandler() #15 0x7f7b67a62454 base::RunLoop::Run() #16 0x7f7b67b09408 base::Thread::Run() #17 0x7f7b67b09caa base::Thread::ThreadMain() #18 0x7f7b67af0c9a base::(anonymous namespace)::ThreadFunc() #19 0x7f7b67d60184 start_thread #20 0x7f7b65eb537d clone Aborted (core dumped)
I also noticed a problem where now if I do version it wont exit if I type Exit: rnephew@rnephew0:~/chromium/linux/src/out/Default$ ./battor_agent --battor-path=/dev/ttyUSB0 Version 1 Exit Exit (Second Exit actually makes it exit) rnephew@rnephew0:~/chromium/linux/src/out/Default$ ./battor_agent --battor-path=/dev/ttyUSB0 Version StartTracing StopTracing // doesn't exit after completing as expected <press enter> Help message displays then exits. rnephew@rnephew0:~/chromium/linux/src/out/Default$ ./battor_agent --battor-path=/dev/ttyUSB0 Version StartTracing StopTracing // doesn't exit after completing as expected Exit // Exits after this
https://codereview.chromium.org/2390893002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_agent.cc (right): https://codereview.chromium.org/2390893002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent.cc:258: CompleteCommand(BATTOR_ERROR_NONE); This should be the error case, not the happy case. Also, after any time you call CompleteCommand(), you also need to call return to exit the function.
https://codereview.chromium.org/2390893002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_agent.cc (right): https://codereview.chromium.org/2390893002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_agent.cc:407: case Action::READ_GIT_HASH: Note to self: This isn't being triggered ever. Why?
On 2016/10/05 17:14:21, rnephew (Reviews Here) wrote: > https://codereview.chromium.org/2390893002/diff/40001/tools/battor_agent/batt... > File tools/battor_agent/battor_agent.cc (right): > > https://codereview.chromium.org/2390893002/diff/40001/tools/battor_agent/batt... > tools/battor_agent/battor_agent.cc:407: case Action::READ_GIT_HASH: > Note to self: This isn't being triggered ever. Why? Serial log from trying to run Version on patch set 3: Opening serial connection. Serial connection open finished with success: 1. Bytes sent: 0x00 0x03 0x09 0x02 0x00 0x02 0x00 0x02 0x00 0x02 0x00 0x01. Read requested. Before doing a serial read, checking to see if we already have a complete message in the 'already read' buffer. No complete message found in the 'already read' buffer. Starting read of up to 191 bytes.
https://codereview.chromium.org/2390893002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_protocol_types.h (right): https://codereview.chromium.org/2390893002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_protocol_types.h:62: // Tells the BattOr to send back the git hash of the firmware. I'd go ahead and add the other CONTROL_TYPE enums here, also: https://github.com/aschulm/battor/blob/master/fw/control.h
https://codereview.chromium.org/2390893002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_protocol_types.h (right): https://codereview.chromium.org/2390893002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_protocol_types.h:62: // Tells the BattOr to send back the git hash of the firmware. On 2016/10/05 20:35:16, charliea wrote: > I'd go ahead and add the other CONTROL_TYPE enums here, also: > https://github.com/aschulm/battor/blob/master/fw/control.h Done.
Patchset #5 (id:80001) has been deleted
Should be fully functional now. PTAL. https://codereview.chromium.org/2390893002/diff/60001/tools/battor_agent/batt... File tools/battor_agent/battor_agent_bin.cc (right): https://codereview.chromium.org/2390893002/diff/60001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:77: " Version\n" Since its a git hash and not version, should I change it to GitHash?
https://codereview.chromium.org/2390893002/diff/60001/tools/battor_agent/batt... File tools/battor_agent/battor_agent_bin.cc (right): https://codereview.chromium.org/2390893002/diff/60001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:77: " Version\n" On 2016/10/18 22:55:24, rnephew (Reviews Here) wrote: > Since its a git hash and not version, should I change it to GitHash? I think "GetFirmwareGitHash" might be better, actually. "GitHash" probably doesn't mean much to people that don't know about this binary already, whereas "GetFirmwareGitHash" pretty accurately conveys what we're doing. https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... File tools/battor_agent/battor_agent.cc (right): https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... tools/battor_agent/battor_agent.cc:110: bool ParseGitHashFrame(BattOrMessageType type, I think that, if you use an std::string, this entire translation can be written as: return std::string(msg.begin(), msg.end()); My reasoning is that strings have a constructor that accepts a begin and end iterator, and a vector provides that. https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... tools/battor_agent/battor_agent.cc:110: bool ParseGitHashFrame(BattOrMessageType type, I think it's probably okay to do this inline in OnMessageRead() rather than in a separate function. The only reason that I ended up creating a separate function for ParseSampleFrame is because the logic is so extensive for that. https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... tools/battor_agent/battor_agent.cc:414: if (bytes->size() <= 0 ){ Also don't think this should be necessary once we use the iterator-based constructor. https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... File tools/battor_agent/battor_agent.h (right): https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... tools/battor_agent/battor_agent.h:46: virtual void OnGetVersionComplete(const char* version, Probably should use a const std::string& here? https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... tools/battor_agent/battor_agent.h:119: // Actions required for checking firmware version. Maybe "returning the firmware version" instead of "checking firmware version"? "checking" might indicate that this command compares it against some known good version or something, whereas this command explicitly just returns what's already on there. https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... File tools/battor_agent/battor_protocol_types.h (right): https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... tools/battor_agent/battor_protocol_types.h:64: //Read if the BattOr is portable or not. nit (here and below): kill space before comment
Also, please add a unittest for this in battor_agent_unittest.cc
https://codereview.chromium.org/2390893002/diff/60001/tools/battor_agent/batt... File tools/battor_agent/battor_agent_bin.cc (right): https://codereview.chromium.org/2390893002/diff/60001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:77: " Version\n" On 2016/10/18 23:46:29, charliea wrote: > On 2016/10/18 22:55:24, rnephew (Reviews Here) wrote: > > Since its a git hash and not version, should I change it to GitHash? > > I think "GetFirmwareGitHash" might be better, actually. "GitHash" probably > doesn't mean much to people that don't know about this binary already, whereas > "GetFirmwareGitHash" pretty accurately conveys what we're doing. Done. https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... File tools/battor_agent/battor_agent.cc (right): https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... tools/battor_agent/battor_agent.cc:110: bool ParseGitHashFrame(BattOrMessageType type, On 2016/10/18 23:46:29, charliea wrote: > I think that, if you use an std::string, this entire translation can be written > as: > > return std::string(msg.begin(), msg.end()); > > My reasoning is that strings have a constructor that accepts a begin and end > iterator, and a vector provides that. Done. https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... tools/battor_agent/battor_agent.cc:414: if (bytes->size() <= 0 ){ On 2016/10/18 23:46:29, charliea wrote: > Also don't think this should be necessary once we use the iterator-based > constructor. Done. https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... File tools/battor_agent/battor_agent.h (right): https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... tools/battor_agent/battor_agent.h:46: virtual void OnGetVersionComplete(const char* version, On 2016/10/18 23:46:29, charliea wrote: > Probably should use a const std::string& here? Done. https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... tools/battor_agent/battor_agent.h:119: // Actions required for checking firmware version. On 2016/10/18 23:46:29, charliea wrote: > Maybe "returning the firmware version" instead of "checking firmware version"? > > "checking" might indicate that this command compares it against some known good > version or something, whereas this command explicitly just returns what's > already on there. Done. https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... File tools/battor_agent/battor_protocol_types.h (right): https://codereview.chromium.org/2390893002/diff/100001/tools/battor_agent/bat... tools/battor_agent/battor_protocol_types.h:64: //Read if the BattOr is portable or not. On 2016/10/18 23:46:29, charliea wrote: > nit (here and below): kill space before comment Done.
https://codereview.chromium.org/2390893002/diff/120001/tools/battor_agent/bat... File tools/battor_agent/battor_agent.h (right): https://codereview.chromium.org/2390893002/diff/120001/tools/battor_agent/bat... tools/battor_agent/battor_agent.h:91: GET_GIT_HASH, (here and elsewhere): probably want "firmware git hash" rather than just "git hash" https://codereview.chromium.org/2390893002/diff/120001/tools/battor_agent/bat... tools/battor_agent/battor_agent.h:119: // Actions required for checking firmware version. same as before: "Actions required for returning the git hash of the firmware version." https://codereview.chromium.org/2390893002/diff/120001/tools/battor_agent/bat... File tools/battor_agent/battor_agent_unittest.cc (right): https://codereview.chromium.org/2390893002/diff/120001/tools/battor_agent/bat... tools/battor_agent/battor_agent_unittest.cc:4: #include <memory> I think there should probably be one empty line after the header https://codereview.chromium.org/2390893002/diff/120001/tools/battor_agent/bat... tools/battor_agent/battor_agent_unittest.cc:172: READ_GIT_HASH_REQUEST_SENT, I think probably GIT_FIRMWARE_HASH_REQUEST_[SENT,RECEIVED] is more consistent with the names of the other enums https://codereview.chromium.org/2390893002/diff/120001/tools/battor_agent/bat... tools/battor_agent/battor_agent_unittest.cc:263: // Runs BattOrAgent::GetFirmwareGitHash until it reaches the specified I think it probably makes sense to move this below RunRecordClockSyncMarkerTo... in order to have a consistent ordering about the commands in these files. Right now, StartTracing appears first, then StopTracing, then RecordClockSyncMarker, so it probably makes sense to put all GitHash functions under RecordClockSyncMarker functions
Patchset #7 (id:140001) has been deleted
https://codereview.chromium.org/2390893002/diff/120001/tools/battor_agent/bat... File tools/battor_agent/battor_agent.h (right): https://codereview.chromium.org/2390893002/diff/120001/tools/battor_agent/bat... tools/battor_agent/battor_agent.h:91: GET_GIT_HASH, On 2016/10/20 12:23:28, charliea wrote: > (here and elsewhere): probably want "firmware git hash" rather than just "git > hash" Done. https://codereview.chromium.org/2390893002/diff/120001/tools/battor_agent/bat... tools/battor_agent/battor_agent.h:119: // Actions required for checking firmware version. On 2016/10/20 12:23:28, charliea wrote: > same as before: "Actions required for returning the git hash of the firmware > version." I remember changing this... I swear. https://codereview.chromium.org/2390893002/diff/120001/tools/battor_agent/bat... File tools/battor_agent/battor_agent_unittest.cc (right): https://codereview.chromium.org/2390893002/diff/120001/tools/battor_agent/bat... tools/battor_agent/battor_agent_unittest.cc:4: #include <memory> On 2016/10/20 12:23:28, charliea wrote: > I think there should probably be one empty line after the header Done. https://codereview.chromium.org/2390893002/diff/120001/tools/battor_agent/bat... tools/battor_agent/battor_agent_unittest.cc:172: READ_GIT_HASH_REQUEST_SENT, On 2016/10/20 12:23:28, charliea wrote: > I think probably GIT_FIRMWARE_HASH_REQUEST_[SENT,RECEIVED] is more consistent > with the names of the other enums Done. https://codereview.chromium.org/2390893002/diff/120001/tools/battor_agent/bat... tools/battor_agent/battor_agent_unittest.cc:263: // Runs BattOrAgent::GetFirmwareGitHash until it reaches the specified On 2016/10/20 12:23:28, charliea wrote: > I think it probably makes sense to move this below RunRecordClockSyncMarkerTo... > in order to have a consistent ordering about the commands in these files. Right > now, StartTracing appears first, then StopTracing, then RecordClockSyncMarker, > so it probably makes sense to put all GitHash functions under > RecordClockSyncMarker functions Done.
Ping
lgtm Great work Randy!
The CQ bit was checked by rnephew@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2390893002/#ps180001 (title: "fix compiling error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
rnephew@chromium.org changed reviewers: + nduca@chromium.org, oysteine@chormium.org
Adding owners for content/browser/tracing/*
rnephew@chromium.org changed reviewers: + primiano@chromium.org
Ping + Adding another owner. The rest of the owners I haven't added yet are marked as OOO.
On 2016/10/26 21:14:59, rnephew (Reviews Here) wrote: content/browser/tracing LGTM
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [BattOr] Make BattOr able to return firmware version. BUG=652380 ========== to ========== [BattOr] Make BattOr able to return firmware version. BUG=652380 Committed: https://crrev.com/b0964536bbda88e09d28e2ea84cb12e8978da4e0 Cr-Commit-Position: refs/heads/master@{#428014} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b0964536bbda88e09d28e2ea84cb12e8978da4e0 Cr-Commit-Position: refs/heads/master@{#428014} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
