This is a list of all comments for HD-DEN-14001-1.

Review Summary: No summary


----------------------------------------
File: firmware/App/Drivers/Battery.c
Revision Comment by wbracken on 28 September 2022, 11:18 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14023

  doxygen tags

  Reply by Dara Navaei on 18 October 2022, 16:54
  > Done.

    Reply by wbracken on 18 October 2022, 17:13
    > RESOLVED IN CODE WALKTHROUGH

Revision Comment by Sean Nash on 30 September 2022, 16:17 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14077

  I think the enum from commit fbb6603 was fine as it was.  I
  still think we should be doing only 1 read per 250ms interval
  and I think the one function with the large switch statement
  based on these enums was the way to achieve that - with the
  multiple status1..5 and remaining capacity1..5 enums spaced out
  to get them read more frequently.
  And I think the individual cases for the higher frequency
  registers could just call the high frequency broadcast function
  whenever a new value is read for those registers.  And so the
  slower broadcast could stay at end of list in that switch
  statement.

  Reply by Sean Nash on 13 October 2022, 23:04
  > RESOLVED in CODE WALKTHROUGH.

Revision Comment by wbracken on 28 September 2022, 11:19 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14024

  doxygen tags.

  Reply by Sean Nash on 17 October 2022, 18:55
  > Added.

    Reply by wbracken on 18 October 2022, 17:15
    > RESOLVED IN CODE WALKTHROUGH

Revision Comment by Sean Nash on 17 October 2022, 17:38 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14458

  Remove extra space after static.

  Reply by Sean Nash on 17 October 2022, 18:55
  > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 30 September 2022, 16:25 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14082

  I think we should just be calling one function to do a single
  read at a time.  If you try to read multiple times
  back-to-back, the bus reports busy and the read fails.  I would
  keep call to getBatteryManagementData() function with the large
  switch statement as it was with everything there and remove
  getBatteryStatusData() function altogether.

  Reply by Steve Jarpe on 03 October 2022, 10:21
  > OK, I see what you wanted. I have a couple of questions:
  > Is logging of the 3 values (remaining charge, battery status,
  > charger status) in a separate message every 750 ms correct?
  > If so, could that be done by calling the
  > publishBatteryStatusData function when the 3rd value (charger
  > status) is read?

    Reply by Sean Nash on 03 October 2022, 16:08
    > I think battery pack status, battery pack remaining
    > capacity and charger status would be in a separate message
    > sent by a separate "publish" function, but the timing would
    > just be whenever any of the 3 registers (or the 3rd of the
    > 3 if they are always next to each other in enum in same
    > order) has been read (call publish function from the cases
    > where we read one of the 3 registers).  If you publish
    > after reading any of the 3, you could sometimes get
    > broadcast 250ms apart.  If you publish after reading the
    > last of the 3 in a group (i.e. the status_1,
    > remaining_capacity_1, charger_status_1), the spacing of the
    > broadcast will depend on how many enums are between them.

      Reply by Sean Nash on 13 October 2022, 23:01
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by wbracken on 04 October 2022, 11:53 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14105

  Does didTimeout support a 1ms timeout?

  Reply by Sean Nash on 04 October 2022, 17:57
  > Sort of.  You will get something between the ms specified and
  > 1 ms less because you don't know where you are in a given ms
  > when you get your start time.
  > Recommend changing this to 2 ms timeout.

    Reply by wbracken on 18 October 2022, 11:19
    > RESOLVED IN CODE WALKTHROUGH

Revision Comment by Sean Nash on 30 September 2022, 16:22 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14078

  Need to save data to dataPtr so calling function gets the data.

  Reply by Sean Nash on 30 September 2022, 16:22
  > Dong fixed this.

    Reply by Sean Nash on 13 October 2022, 22:55
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 04 October 2022, 17:59 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14126

  Add blank line after declaration.

  Reply by Sean Nash on 13 October 2022, 23:03
  > RESOLVED in CODE WALKTHROUGH.

Revision Comment by wbracken on 05 October 2022, 18:48 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14131

  BatteryStatusData is also an input

  Reply by Dara Navaei on 18 October 2022, 15:25
  > Done.

    Reply by wbracken on 18 October 2022, 17:08
    > RESOLVED IN CODE WALKTHROUGH

Revision Comment by Sean Nash on 30 September 2022, 16:24 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14081

  Keep multiple reads of remaining capacity and status in the
  switch as before.

  Reply by Sean Nash on 13 October 2022, 23:00
  > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Darren Cox on 14 October 2022, 10:19 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14394

  Need one more indent for this IF statement.

  Reply by Dara Navaei on 18 October 2022, 15:20
  > Done.

    Reply by Sean Nash on 18 October 2022, 16:39
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by wbracken on 28 September 2022, 11:20 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14025

  Update function header.

  Reply by Dara Navaei on 18 October 2022, 15:25
  > Done.

    Reply by wbracken on 18 October 2022, 17:11
    > RESOLVED IN CODE WALKTHROUGH

Revision Comment by Sean Nash on 30 September 2022, 16:23 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14080

  Keep read for battery charger status here before end of list.

  Reply by Sean Nash on 13 October 2022, 22:57
  > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 04 October 2022, 18:01 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14128

  Cases should be pulled back one indent (4 spaces) to align with
  others.

  Reply by Sean Nash on 13 October 2022, 23:00
  > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 17 October 2022, 17:40 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14459

  This should be the first if and the one above should just be an
  else (no if).

  Reply by Sean Nash on 17 October 2022, 18:11
  > Fixed.

    Reply by Sean Nash on 17 October 2022, 18:12
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by wbracken on 03 October 2022, 10:58 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14087

  BatteryStatusData is an input.

  Reply by Dara Navaei on 18 October 2022, 15:26
  > Done.

    Reply by wbracken on 18 October 2022, 17:09
    > RESOLVED IN CODE WALKTHROUGH

Revision Comment by Sean Nash on 04 October 2022, 18:01 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14129

  Remove extra blank lines.  Should only be 1 here.

  Reply by Sean Nash on 13 October 2022, 23:02
  > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 04 October 2022, 18:02 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14130

  Add 2 blank lines before test support banner.

  Reply by Sean Nash on 13 October 2022, 22:56
  > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Drivers/Battery.h
Revision Comment by wbracken on 28 September 2022, 11:22 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14026

  Add doxygen

  Reply by Dara Navaei on 18 October 2022, 16:51
  > Done.

    Reply by wbracken on 18 October 2022, 17:09
    > RESOLVED IN CODE WALKTHROUGH

----------------------------------------
File: firmware/App/Controllers/DialInFlow.c
Revision Comment by Sean Nash on 30 September 2022, 16:38 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14083

  Restore alarm when done.

  Reply by Sean Nash on 13 October 2022, 23:03
  > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Tasks/TaskBG.c
Revision Comment by Sean Nash on 13 October 2022, 22:55 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14347

  Remove this blank line.

  Reply by Sean Nash on 13 October 2022, 23:04
  > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: AlarmDefs.h
Revision Comment by Darren Cox on 14 October 2022, 12:30 
https://devapps.diality.us/cru/HD-DEN-14001-1#c14413

  Did we lose 1 second of time or is this the 2nd alarm?  Comment
  is not clear.

  Reply by Sean Nash on 17 October 2022, 17:42
  > Agree.  Change "_SECOND" to "_IN_TREATMENT"

    Reply by Sean Nash on 17 October 2022, 18:57
    > Fixed.

      Reply by Sean Nash on 18 October 2022, 11:05
      > RESOLVED in CODE WALKTHROUGH.


---

ID: HD-DEN-14001-1 https://devapps.diality.us/cru/HD-DEN-14001-1

Title: HD-DEN-14001_HD Battery Driver Update
Statement of Objectives:


State: Closed

Summary: 

Author: Steve Jarpe
Moderator: Steve Jarpe
Reviewers: (2 active, 2 completed*)
     Sean Nash   (*)
     wbracken   (*)
     Dara Navaei
     Darren Cox