This is a list of all comments for DG-DEN-3421-1.

Review Summary: No summary


----------------------------------------
File: firmware/App/Modes/ModeHeatDisinfect.c
Revision Comment by qnguyen on 10 August 2020, 21:32 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3017

  Suggest define number 86400 as seconds per day.

  Reply by Sean Nash on 11 August 2020, 09:22
  > And put it in common.h in fwcommon.

    Reply by Dara Navaei on 12 August 2020, 10:54
    > Done. Put it in common.h in fwcommon.

      Reply by qnguyen on 13 August 2020, 11:01
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:30 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3104

  Align Doxygen comments.

  Reply by Dara Navaei on 12 August 2020, 14:11
  > I am still waiting to finalize the #defines. Once they are
  > all done, I usually align all of them to the longest #define

    Reply by pmontazemi on 20 October 2020, 10:40
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by qnguyen on 10 August 2020, 21:37 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3018

  Rename TPi to TDi.

  Reply by Dara Navaei on 12 August 2020, 10:52
  > Done

    Reply by qnguyen on 13 August 2020, 11:01
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:30 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3105

  Align Doxygen comments.

  Reply by Dara Navaei on 12 August 2020, 14:11
  > I am still waiting to finalize the #defines. Once they are
  > all done, I usually align all of them to the longest #define

    Reply by pmontazemi on 20 October 2020, 10:40
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:30 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3106

  Align Doxygen comments.

  Reply by Dara Navaei on 12 August 2020, 14:10
  > I am still waiting to finalize the #defines. Once they are
  > all done, I usually align all of them to the longest #define

    Reply by pmontazemi on 20 October 2020, 10:40
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:34 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3107

  I would call Reservoir/RSVR/R all the same as R1 and R2
  everywhere.

  Reply by Dara Navaei on 12 August 2020, 14:10
  > Done

    Reply by pmontazemi on 19 October 2020, 10:38
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:34 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3108

  I would call Reservoir/RSVR/R all the same as R1 and R2
  everywhere.

  Reply by Dara Navaei on 12 August 2020, 14:09
  > Done

    Reply by pmontazemi on 19 October 2020, 10:38
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 10:39 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3014

  Fill out inputs/Outputs on all function headers.

  Reply by Dara Navaei on 12 August 2020, 10:56
  > Done

    Reply by Sean Nash on 13 August 2020, 10:44
    > Do not leave them as none.

      Reply by Dara Navaei on 19 October 2020, 15:35
      > Once I am close to finishing this story, I will go over
      > the doxygen documentation to make sure it is up to date.
      > Please resolve this comment. I will address all the
      > doxygen comment in the next Heat Disinfection code
      > review.

        Reply by Sean Nash on 26 October 2020, 10:47
        > RESOLVED in CODE WALKTHROUGH.

Revision Comment by qnguyen on 10 August 2020, 21:41 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3019

  Remove extra space.

  Reply by Dara Navaei on 12 August 2020, 10:51
  > Done

    Reply by qnguyen on 13 August 2020, 11:00
    > Remove extra space before the first void.

      Reply by qnguyen on 19 October 2020, 10:48
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 11 August 2020, 09:29 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3023

  I assume this function is called by HD command handler.  I
  think HD command to start a heat disinfection should probably
  be handled by Standby Mode and not here.  Command is only valid
  in standby mode so it should take the command and decide if
  it's valid and request heat disinfect mode when it's ready
  (e.g. if it's performing a water sample it may defer starting
  heat disinfect until that's done).

  Reply by Dara Navaei on 12 August 2020, 14:18
  > Agreed. I will change the location of this function once I
  > started working on this story.

    Reply by Dara Navaei on 22 October 2020, 09:38
    > Changed the location of the start command to ModeStandby.

      Reply by Sean Nash on 26 October 2020, 10:46
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 11 August 2020, 09:28 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3022

  Call to init here is not necessary.  Op Modes will call this
  when transitioning to this mode.

  Reply by Dara Navaei on 12 August 2020, 10:50
  > Done

    Reply by Sean Nash on 13 August 2020, 10:42
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:35 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3109

  Replace For now with TODO

  Reply by Dara Navaei on 12 August 2020, 14:06
  > Done

    Reply by pmontazemi on 19 October 2020, 10:37
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 11 August 2020, 09:34 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3025

  Remove commented line.

  Reply by Dara Navaei on 12 August 2020, 10:49
  > Done

    Reply by Sean Nash on 13 August 2020, 10:42
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 10:40 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3015

  Add @param to function headers where appropriate.

  Reply by Dara Navaei on 11 August 2020, 09:32
  > All the headers have @param if appropriate.

    Reply by Sean Nash on 13 August 2020, 10:42
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 11 August 2020, 10:00 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3041

  This looks more like a heatDisinfectStartTime.  Elapsed time is
  calculated later.

  Reply by Dara Navaei on 12 August 2020, 10:13
  > Done

    Reply by Sean Nash on 13 August 2020, 10:38
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 11 August 2020, 09:44 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3029

  What is the purpose of this condition?  I would think it would
  be when water is at target 85 deg, but that's not what it looks
  like.

  Reply by Dara Navaei on 12 August 2020, 10:43
  > Fixed the logic

    Reply by Sean Nash on 13 August 2020, 10:41
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 11 August 2020, 09:47 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3031

  Aren't these pumps already set at this point?

  Reply by Dara Navaei on 12 August 2020, 14:46
  > I have not tested many parts of this code yet. I will
  > investigate and change the code if needed.

    Reply by Dara Navaei on 26 October 2020, 08:55
    > That is correct they are already set. I removed the code.

      Reply by Sean Nash on 26 October 2020, 10:46
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 11 August 2020, 09:50 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3033

  Aren't these pumps already set?

  Reply by Dara Navaei on 12 August 2020, 14:46
  > I have not tested many parts of this code yet. I will
  > investigate and change the code if needed.

    Reply by Dara Navaei on 26 October 2020, 08:54
    > That is right, they are already set. I removed the pump.

      Reply by Sean Nash on 26 October 2020, 10:45
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 11 August 2020, 09:49 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3032

  Should this be >= ?

  Reply by Dara Navaei on 12 August 2020, 10:26
  > The counter was supposed to start from 0 but I changed it to
  > 1 and added an = to it too.

    Reply by Sean Nash on 12 August 2020, 15:59
    > If it starts at 0, >= is appropriate.  If it starts at 1, >
    > is appropriate.

      Reply by Dara Navaei on 12 August 2020, 17:21
      > Changed it to >. It starts from 1.

        Reply by Sean Nash on 13 August 2020, 10:40
        > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 11 August 2020, 09:50 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3034

  Isn't RO pump already set?  If not, what about drain pump?

  Reply by Dara Navaei on 12 August 2020, 14:46
  > I have not tested many parts of this code yet. I will
  > investigate and change the code if needed.

    Reply by Dara Navaei on 26 October 2020, 08:53
    > That is right. The pump is already is set. I removed the
    > code.

      Reply by Sean Nash on 26 October 2020, 10:44
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 11 August 2020, 09:53 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3035

  Should primary heater be turned off here?

  Reply by Dara Navaei on 12 August 2020, 10:23
  > Yes, sure. I added the stop command in the internal heat
  > disinfect state.

    Reply by Sean Nash on 12 August 2020, 16:01
    > Isn't the internal FSM used in 2 or more places?  Is
    > turning off the heater appropriate in all cases?  If not,
    > move outside internal FSM to somewhere specific.

      Reply by Dara Navaei on 12 August 2020, 17:22
      > Agreed. Please let me test the code.

        Reply by Dara Navaei on 19 October 2020, 15:29
        > Added stop primary and trimmer heaters.

          Reply by Sean Nash on 20 October 2020, 10:42
          > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 11 August 2020, 09:54 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3037

  Should we fault here?

  Reply by Dara Navaei on 12 August 2020, 10:19
  > I added a TODO to add a software fault in default mode. I
  > have not added the faults yet.

    Reply by pmontazemi on 12 August 2020, 10:36
    > Where is the TODO?

      Reply by Sean Nash on 12 August 2020, 16:02
      > I see it above.

        Reply by Sean Nash on 13 August 2020, 10:39
        > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:37 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3114

  There are no spaces between function name and first
  parenthesis, please fix throughout code.

  Reply by Dara Navaei on 12 August 2020, 13:58
  > Will fix this throughout code

    Reply by Dara Navaei on 19 October 2020, 15:25
    > Fixed them throughout the entire code.

      Reply by pmontazemi on 20 October 2020, 10:39
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:37 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3113

  There are no spaces between function name and first
  parenthesis, please fix throughout code.

  Reply by Dara Navaei on 12 August 2020, 14:04
  > Will fix this throughout code

    Reply by Dara Navaei on 19 October 2020, 15:25
    > Fixed them throughout the entire code.

      Reply by pmontazemi on 20 October 2020, 10:39
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:37 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3112

  There are no spaces between function name and first
  parenthesis, please fix throughout code.

  Reply by Dara Navaei on 12 August 2020, 14:05
  > Will fix this throughout code

    Reply by Dara Navaei on 19 October 2020, 15:25
    > Fixed them throughout the entire code.

      Reply by pmontazemi on 20 October 2020, 10:40
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:37 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3111

  There are no spaces between function name and first
  parenthesis, please fix throughout code.

  Reply by Dara Navaei on 12 August 2020, 14:05
  > Will fix this throughout code

    Reply by Dara Navaei on 19 October 2020, 15:26
    > Fixed them throughout the entire code.

      Reply by pmontazemi on 20 October 2020, 10:40
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:38 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3115

  There are no spaces between function name and first
  parenthesis, please fix throughout code.

  Reply by Dara Navaei on 12 August 2020, 13:58
  > Will fix this throughout code

    Reply by Dara Navaei on 19 October 2020, 15:24
    > Fixed them throughout the entire code.

      Reply by pmontazemi on 20 October 2020, 10:39
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:39 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3116

  There are no spaces between function name and first
  parenthesis, please fix throughout code.

  Reply by Dara Navaei on 12 August 2020, 13:57
  > Will fix this in the code

    Reply by Dara Navaei on 19 October 2020, 15:21
    > Done

      Reply by pmontazemi on 20 October 2020, 10:39
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:39 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3117

  There are no spaces between function name and first
  parenthesis, please fix throughout code.

  Reply by Dara Navaei on 12 August 2020, 13:55
  > Will fix this in the code

    Reply by pmontazemi on 13 August 2020, 11:28
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 11 August 2020, 09:57 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3039

  Where is target temp in this?

  Reply by Dara Navaei on 12 August 2020, 13:56
  > Fixed it

    Reply by Sean Nash on 13 August 2020, 10:38
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:39 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3118

  There are no spaces between function name and first
  parenthesis, please fix throughout code.

  Reply by Dara Navaei on 12 August 2020, 13:51
  > Will address this issue in the code

    Reply by pmontazemi on 13 August 2020, 11:28
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:39 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3119

  Dangerous way of putting parentheses, are you calling function
  on the numerator only?

  Reply by Dara Navaei on 12 August 2020, 13:49
  > Yes. I am calculating the heat disinfection time since the
  > beginning of the mode in ms and then divide it to 60000 to
  > convert it to minutes for publication.

    Reply by pmontazemi on 13 August 2020, 11:26
    > Shouldn't this be ms to min?

      Reply by Dara Navaei on 19 October 2020, 15:17
      > I am dividing the elapsed time in ms to
      > MINUTE_TO_MS_CONVERSION = 60000 to convert the value back
      > to minutes to be reported out. This #define is also used
      > to convert minutes to ms by adding the value to this
      > #define.

        Reply by pmontazemi on 20 October 2020, 10:37
        > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:40 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3121

  Remove extra space on both lines before the "=".

  Reply by Dara Navaei on 12 August 2020, 13:47
  > Done

    Reply by pmontazemi on 13 August 2020, 11:26
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:40 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3122

  Remove extra space on all three lines before the "=".

  Reply by Dara Navaei on 12 August 2020, 13:47
  > Done

    Reply by pmontazemi on 13 August 2020, 11:26
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:40 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3123

  Remove extra space on both lines before the "=".

  Reply by Dara Navaei on 12 August 2020, 13:47
  > Done

    Reply by pmontazemi on 13 August 2020, 11:26
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:41 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3124

  Remove extra space on all three lines before the "=".

  Reply by Dara Navaei on 12 August 2020, 13:46
  > Done

    Reply by pmontazemi on 13 August 2020, 11:25
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:41 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3125

  Remove extra space on both lines before the "=".

  Reply by Dara Navaei on 12 August 2020, 13:45
  > Done.

    Reply by pmontazemi on 13 August 2020, 11:25
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Services/SystemComm.c
Revision Comment by qnguyen on 10 August 2020, 21:15 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3016

  Remove extra blank lines.

  Reply by Dara Navaei on 12 August 2020, 10:55
  > Done

    Reply by qnguyen on 13 August 2020, 11:01
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:44 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3131

  Where is the Doxygen /**@}*/ ending of file?

  Reply by Dara Navaei on 12 August 2020, 13:40
  > All modules will have doxygen hooks eventually

    Reply by pmontazemi on 13 August 2020, 11:23
    > Planned for DEN S26.

      Reply by pmontazemi on 13 August 2020, 11:23
      > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Services/SystemCommMessages.c
Revision Comment by pmontazemi on 12 August 2020, 10:45 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3133

  Comments are placed a space apart from the longest line (remove
  extra spaces for all and align).

  Reply by Dara Navaei on 12 August 2020, 13:37
  > Could you please explain more?

    Reply by pmontazemi on 13 August 2020, 11:22
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by qnguyen on 13 August 2020, 11:23 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3424

  Missing one / for doxygen comment.

  Reply by Dara Navaei on 19 October 2023, 08:34
  > RESOLVED in CODE WALKTHROUGH

Revision Comment by pmontazemi on 12 August 2020, 10:45 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3134

  Dogyxen comments missing for enum.

  Reply by Dara Navaei on 12 August 2020, 13:35
  > Done

    Reply by pmontazemi on 13 August 2020, 11:20
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:47 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3135

  Remove commented line (if still needed, add a TODO with
  explanation why it is there)

  Reply by Dara Navaei on 12 August 2020, 13:21
  > [~snash] Could you please respond?

    Reply by Dara Navaei on 19 October 2020, 17:30
    > Spoke with Sean, uncommented the alarm.

      Reply by pmontazemi on 20 October 2020, 10:38
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:48 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3136

  Is the "\" necessary in C for the Compiler we use?

  Reply by Dara Navaei on 12 August 2020, 13:20
  > Yes, I have seen issues if I don't put it.

    Reply by pmontazemi on 13 August 2020, 11:15
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:48 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3137

  Is the "\" necessary in C for the Compiler we use?

  Reply by Dara Navaei on 12 August 2020, 13:20
  > Yes, I have seen issues if I don't put it.

    Reply by pmontazemi on 13 August 2020, 11:15
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:48 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3138

  Is the "\" necessary in C for the Compiler we use?

  Reply by Dara Navaei on 12 August 2020, 13:20
  > Yes, I have seen issues if I don't put it.

    Reply by pmontazemi on 13 August 2020, 11:15
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:48 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3139

  Is the "\" necessary in C for the Compiler we use?

  Reply by Dara Navaei on 12 August 2020, 13:20
  > Yes, I have seen issues if I don't put it.

    Reply by pmontazemi on 13 August 2020, 11:15
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:48 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3140

  Is the "\" necessary in C for the Compiler we use?

  Reply by Dara Navaei on 12 August 2020, 13:20
  > Yes, I have seen issues if I don't put it.

    Reply by pmontazemi on 13 August 2020, 11:15
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:48 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3141

  Is the "\" necessary in C for the Compiler we use?

  Reply by Dara Navaei on 12 August 2020, 13:19
  > Yes, I have seen issues if I don't put it.

    Reply by pmontazemi on 13 August 2020, 11:15
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:48 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3142

  Is the "\" necessary in C for the Compiler we use?

  Reply by Dara Navaei on 12 August 2020, 13:19
  > Yes, I have seen issues if I don't put it.

    Reply by pmontazemi on 13 August 2020, 11:15
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:48 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3143

  Is the "\" necessary in C for the Compiler we use?

  Reply by Dara Navaei on 12 August 2020, 13:19
  > Yes, I have seen issues if I don't put it.

    Reply by pmontazemi on 13 August 2020, 11:15
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:48 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3144

  Is the "\" necessary in C for the Compiler we use?

  Reply by Dara Navaei on 12 August 2020, 13:19
  > Yes, I have seen issues if I don't put it.

    Reply by pmontazemi on 13 August 2020, 11:14
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:48 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3146

  Is the "\" necessary in C for the Compiler we use?

  Reply by Dara Navaei on 12 August 2020, 13:19
  > Yes, I have seen issues if I don't put it.

    Reply by pmontazemi on 13 August 2020, 11:14
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:49 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3147

  Is the "\" necessary in C for the Compiler we use?

  Reply by Dara Navaei on 12 August 2020, 11:11
  > Yes, I have seen issues if I don't put it.

    Reply by pmontazemi on 13 August 2020, 11:14
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:49 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3148

  Is the "\" necessary in C for the Compiler we use?

  Reply by Dara Navaei on 12 August 2020, 11:11
  > Yes, I have seen issues if I don't put it.

    Reply by Sean Nash on 12 August 2020, 15:38
    > I think it's required for macros.  I don't believe it's
    > required in normal code like this.

      Reply by Dara Navaei on 12 August 2020, 17:24
      > Yes, it is for the macros. The normal code fine

        Reply by pmontazemi on 13 August 2020, 11:14
        > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:49 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3150

  Where is the /**@}*/ Doxygen comment at eof?

  Reply by Dara Navaei on 12 August 2020, 14:48
  > All the modules will have doxygen hooks eventually

    Reply by pmontazemi on 13 August 2020, 11:13
    > Planned to catch up in DEN S26.

      Reply by pmontazemi on 13 August 2020, 11:14
      > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Services/SystemCommMessages.h
Revision Comment by pmontazemi on 12 August 2020, 10:49 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3151

  Where is the /*@}/ Doxygen comment at eof?

  Reply by Dara Navaei on 12 August 2020, 14:48
  > All the modules will have doxygen hooks eventually

    Reply by pmontazemi on 13 August 2020, 11:07
    > DECISION: This will be done in DEN S26.

      Reply by pmontazemi on 13 August 2020, 11:07
      > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Controllers/ROPump.c
Revision Comment by qnguyen on 11 August 2020, 09:41 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3028

  Align doxygen comment

  Reply by Dara Navaei on 12 August 2020, 10:44
  > Done

    Reply by qnguyen on 13 August 2020, 10:58
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 07:59 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3064

  Align Doxygen comment

  Reply by Dara Navaei on 12 August 2020, 08:59
  > Done

    Reply by pmontazemi on 20 October 2020, 10:41
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 08:00 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3065

  Define few seconds, too vague.

  Reply by Dara Navaei on 12 August 2020, 08:56
  > I changed the comment to mention a set period of time.

    Reply by pmontazemi on 19 October 2020, 10:43
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 10:05 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3006

  Same comment as Drain pump.  Keep the "Set" variabl.
  roPumpControlMode would be set by the "Set" function.
  roPumpControlModeSet would be set by the state machine when the
  requested mode is actually set.

  Reply by Dara Navaei on 12 August 2020, 14:30
  > Agreed will bring back the set variable once I started
  > working on the story

    Reply by Dara Navaei on 19 October 2020, 15:38
    > Brought the variable back.

      Reply by Sean Nash on 20 October 2020, 10:45
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 10:07 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3007

  Should we still have this?  See comment in header file.

  Reply by Dara Navaei on 12 August 2020, 14:28
  > Agreed. Will bring back the pressure override capability

    Reply by Dara Navaei on 19 October 2020, 15:37
    > I brought back the functionality.

      Reply by Sean Nash on 20 October 2020, 10:45
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 08:00 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3066

  Remove "_" after LPM

  Reply by Dara Navaei on 12 August 2020, 08:54
  > Done

    Reply by pmontazemi on 19 October 2020, 10:43
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 08:01 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3067

  Make this entire assignment one line (does not exceed 150
  characters)

  Reply by Dara Navaei on 12 August 2020, 08:53
  > This is not the end of the line, there is another line after
  > this that defines the data publish interval. Also, I wanted
  > to make sure the doxygen comments are aligned.

    Reply by pmontazemi on 19 October 2020, 10:42
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 09:38 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3073

  OpenLoopTarget with capital "T", change across entire DG FW
  code base.

  Reply by Dara Navaei on 12 August 2020, 10:05
  > Done

    Reply by pmontazemi on 19 October 2020, 10:42
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 09:39 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3074

  Align Doxygen comments

  Reply by Dara Navaei on 12 August 2020, 10:04
  > Done

    Reply by pmontazemi on 19 October 2020, 10:42
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 09:39 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3075

  No space after inputs, outputs, just ":" then space to be
  consistent everywhere.

  Reply by Dara Navaei on 12 August 2020, 10:03
  > Done

    Reply by pmontazemi on 19 October 2020, 10:41
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 09:40 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3076

  No space after inputs, outputs, just ":" then space to be
  consistent everywhere.

  Reply by Dara Navaei on 12 August 2020, 10:03
  > Done

    Reply by pmontazemi on 19 October 2020, 10:41
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 09:40 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3077

  No space after inputs, outputs, just ":" then space to be
  consistent everywhere.

  Reply by Dara Navaei on 12 August 2020, 10:02
  > Done

    Reply by pmontazemi on 19 October 2020, 10:41
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 09:40 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3078

  No space after inputs, outputs, just ":" then space to be
  consistent everywhere.

  Reply by Dara Navaei on 12 August 2020, 10:00
  > Done

    Reply by pmontazemi on 19 October 2020, 10:40
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 10:12 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3010

  PWM should not be set to flow rate.  Use your macro to convert
  flow to PWM.

  Reply by Dara Navaei on 12 August 2020, 10:59
  > Done

    Reply by Sean Nash on 13 August 2020, 10:50
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by qnguyen on 11 August 2020, 09:47 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3030

  why alarm is commented out?

  Reply by Dara Navaei on 12 August 2020, 10:28
  > [~snash] Could you please respond?

    Reply by Sean Nash on 12 August 2020, 15:52
    > I don't have this code in my branch so I didn't comment it
    > out.  Comment above should read flow rate out of range?

      Reply by Dara Navaei on 19 October 2020, 15:33
      > Uncommented the alarm.

        Reply by qnguyen on 20 October 2020, 10:34
        > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 10:09 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3008

  Keep this.

  Reply by Dara Navaei on 12 August 2020, 14:24
  > Done

    Reply by Sean Nash on 13 August 2020, 10:51
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 10:21 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3012

  I think I still need this build flag condition to prevent
  changing of PWM.

  Reply by Sean Nash on 13 August 2020, 10:48
  > @Sean - review and direct Dara how to resolve.

    Reply by Dara Navaei on 19 October 2020, 15:36
    > Could you please elaborate more?

      Reply by Dara Navaei on 22 October 2020, 09:38
      > Brought back the build switch.

        Reply by Sean Nash on 26 October 2020, 10:48
        > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 09:41 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3079

  Make it a one liner

  Reply by Dara Navaei on 12 August 2020, 09:53
  > Done

    Reply by pmontazemi on 19 October 2020, 10:40
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 10:12 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3011

  This should be done in "Set" function above.  And assigned to
  the non "Set" variable.  Then, here we would set
  roPumpPWMDutyCyclePctSet = roPumpPWMDutyCyclePct;

  Reply by Dara Navaei on 12 August 2020, 14:27
  > Done

    Reply by Sean Nash on 13 August 2020, 10:49
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 09:51 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3084

  One liner

  Reply by Dara Navaei on 12 August 2020, 10:02
  > Done

    Reply by pmontazemi on 19 October 2020, 10:40
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 09:52 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3085

  Target with capital "T"

  Reply by Dara Navaei on 12 August 2020, 10:02
  > Done

    Reply by pmontazemi on 19 October 2020, 10:40
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by qnguyen on 11 August 2020, 09:53 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3036

  Replace with function getTargetROPumpFlowRate().

  Reply by Dara Navaei on 12 August 2020, 10:20
  > Done

    Reply by qnguyen on 17 August 2020, 15:48
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by qnguyen on 11 August 2020, 09:56 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3038

  Should this be an else instead of else if?

  Reply by Dara Navaei on 12 August 2020, 10:17
  > It should logically be an else but I wanted to make sure we
  > don't go to the verify flow state due to any other unexpected
  > conditions.

    Reply by pmontazemi on 12 August 2020, 10:27
    > OK but don't you want to also add an else with an throw
    > error as catch all as last condition check?

      Reply by Sean Nash on 12 August 2020, 15:55
      > I don't understand Dara's response.  It should be an else
      > - making it an else if doesn't really have any effect
      > except that now there is an implied else after the else
      > if that you will not be able to get coverage for as it's
      > unreachable.  I think Quang is correct and it should be
      > an else.

        Reply by Dara Navaei on 12 August 2020, 17:17
        > Done

          Reply by qnguyen on 17 August 2020, 15:49
          > RESOLVED in CODE WALKTHROUGH.

Revision Comment by qnguyen on 11 August 2020, 09:59 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3040

  Replace with function getTargetROPumpFlowRate().

  Reply by Dara Navaei on 12 August 2020, 10:16
  > Done

    Reply by qnguyen on 13 August 2020, 10:56
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 26 October 2020, 08:42 
https://devapps.diality.us/cru/DG-DEN-3421-1#c5697

  Why is this called "valveData"?

  Reply by Dara Navaei on 26 October 2020, 08:49
  > Done

    Reply by Sean Nash on 26 October 2020, 10:43
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Controllers/DrainPump.c
Revision Comment by Sean Nash on 10 August 2020, 09:44 
https://devapps.diality.us/cru/DG-DEN-3421-1#c2999

  You removed the "Set" control mode.  I use these to distinguish
  actual mode from the mode being requested by a call to a Set
  function.  Set function sets drainPumpControlMode and state
  machine (off state) sets drainPumpControlModeSet if/when it
  executes the request and actually starts the pump.

  Reply by Dara Navaei on 12 August 2020, 14:37
  > Agreed. Will bring it back when I started working on this
  > story

    Reply by Dara Navaei on 19 October 2020, 15:43
    > Bought back the variable.

      Reply by Sean Nash on 20 October 2020, 10:48
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by qnguyen on 11 August 2020, 09:36 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3026

  Align doxygen comment

  Reply by Dara Navaei on 12 August 2020, 10:48
  > Done

    Reply by qnguyen on 13 August 2020, 10:58
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 09:38 
https://devapps.diality.us/cru/DG-DEN-3421-1#c2996

  RPM is not the same as DAC.  You are initializing the DAC to a
  minimum RPM (300 I think).  You are also setting your minimum
  and maximum DAC values to RPMs.

  Reply by Dara Navaei on 12 August 2020, 14:39
  > Agreed. Will fix this once I started working on the story

    Reply by Dara Navaei on 22 October 2020, 10:34
    > Created #defines to convert min and maximum RPMs to DACs.

      Reply by Sean Nash on 26 October 2020, 10:49
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by qnguyen on 12 October 2020, 09:47 
https://devapps.diality.us/cru/DG-DEN-3421-1#c5436

  Remove extra colon : between param and rpm and new.

  Reply by Dara Navaei on 19 October 2020, 15:06
  > Done. I will go over the entire doxygen documentation and
  > update it towards the end of this story.

    Reply by qnguyen on 20 October 2020, 10:32
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 09:33 
https://devapps.diality.us/cru/DG-DEN-3421-1#c2994

  Should also set drainPumpDAC in this function.

  Reply by Dara Navaei on 12 August 2020, 14:40
  > Agreed. Will fix this once I started working on the story

    Reply by Dara Navaei on 22 October 2020, 17:27
    > No. This function gets the deltaP, sets the pump control to
    > closed loop and set the closed loop flag to TRUE. In the
    > off state, the minimum DAC is set to 300 RPM and from there
    > the PI controller will take over.

      Reply by Sean Nash on 26 October 2020, 08:53
      > There are two DAC variables: drainPumpDAC and
      > drainPumpDACSet.  The first should be set here (as in RPM
      > set function above).  The second should be set from the
      > off state as you've stated.

        Reply by Dara Navaei on 26 October 2020, 08:59
        > Ok sure. I set drainPumpDAC here.

          Reply by Sean Nash on 26 October 2020, 10:43
          > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 11 August 2020, 12:20 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3045

  Isn't this relationship already known?

  Reply by Sean Nash on 11 August 2020, 15:16
  > Spec'd in HDD.  Just needs to be done and added to Drain Pump
  > broadcast data.

    Reply by pmontazemi on 19 October 2020, 10:46
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 09:27 
https://devapps.diality.us/cru/DG-DEN-3421-1#c2993

  Why is this commented out?  It will prevent SW2 from turning on
  drain pump.

  Reply by Dara Navaei on 12 August 2020, 14:42
  > I don't think I commented it. I uncommented it

    Reply by Sean Nash on 13 August 2020, 10:55
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 11 August 2020, 13:01 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3047

  Make these two lines a one-liner.

  Reply by Dara Navaei on 12 August 2020, 09:47
  > Done

    Reply by pmontazemi on 19 October 2020, 10:46
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 09:42 
https://devapps.diality.us/cru/DG-DEN-3421-1#c2998

  Should be more than just closed loop here.  Open loop included
  check for non-zero target speed.  Can't do that here since zero
  is a valid target for delta pressure.  Maybe define a NOT_SET
  value for delta pressure and check here to see if it's set to
  something other than NOT_SET.

  Reply by Dara Navaei on 12 August 2020, 14:38
  > Agreed. Will work on this once I started working on the story

    Reply by Dara Navaei on 22 October 2020, 16:47
    > Added a flag that is set to TRUE once closed loop control
    > is requested.

      Reply by Sean Nash on 26 October 2020, 10:49
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 09:40 
https://devapps.diality.us/cru/DG-DEN-3421-1#c2997

  Controller should be initialized to a DAC value (not an RPM).

  Reply by Dara Navaei on 12 August 2020, 14:39
  > Agreed. Will fix this once I started working on the story

    Reply by Dara Navaei on 22 October 2020, 10:34
    > Done

      Reply by Sean Nash on 26 October 2020, 10:49
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 09:34 
https://devapps.diality.us/cru/DG-DEN-3421-1#c2995

  Where is this target speed being set?  Do you even have a
  target speed in closed loop delta pressure mode?  drainPumpDAC
  value should be set in the set delta pressure function.  Only
  drainPumpDACSet should be set here.

  Reply by Dara Navaei on 12 August 2020, 14:40
  > Agreed. Will work on this once I started working on the story

    Reply by Dara Navaei on 22 October 2020, 17:18
    > I changed the code so the minimum RPM DAC is set until the
    > PI controller sets the proper DAC value.

      Reply by Sean Nash on 26 October 2020, 10:50
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 26 October 2020, 09:01 
https://devapps.diality.us/cru/DG-DEN-3421-1#c5706

  Set drainPumpDAC in set function - not here.

  Reply by Dara Navaei on 26 October 2020, 09:08
  > Done

    Reply by Sean Nash on 26 October 2020, 09:12
    > Don't see any change here.

      Reply by Dara Navaei on 26 October 2020, 09:31
      > Done

        Reply by Sean Nash on 26 October 2020, 10:43
        > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 09:47 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3000

  I don't think you want to use absolute value here.  We need to
  know positive vs. negative.  And we probably should swap order
  (should be out - in).  e.g. if target delta pressure is zero, a
  negative delta (out - in) would tell us that the RO pump is
  pulling water faster than the drain pump is pushing (creating a
  vacuum between them) and we would want the drain pump to speed
  up.  And if delta is positive, that means the drain pump is
  pushing water faster than the RO pump (creating a pressure
  build up between them) and we would want the drain pump to slow
  down.

  Reply by Dara Navaei on 12 August 2020, 14:37
  > Agreed. Removed fabs

    Reply by Sean Nash on 13 August 2020, 10:53
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 09:55 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3002

  Use drainPumpDACSet here as it is literally being set in next
  line of code.

  Reply by Dara Navaei on 12 August 2020, 14:36
  > Done

    Reply by Sean Nash on 13 August 2020, 10:53
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 09:56 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3003

  No absolute value.  I don't think we want measured delta here
  anyway (we can get that from the published pressure sensors
  from the Pressures module.  I think we want target delta
  pressure.

  Reply by Dara Navaei on 12 August 2020, 14:32
  > Agreed. Will work on it once I started working on the story

    Reply by Dara Navaei on 19 October 2020, 15:43
    > Removed the fabs.

      Reply by Sean Nash on 20 October 2020, 10:47
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 10:00 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3004

  F32?

  Reply by Dara Navaei on 12 August 2020, 14:31
  > Done

    Reply by Sean Nash on 13 August 2020, 10:52
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 07:37 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3060

  Remove extra line and don't forget what Doxygen needs as last
  line in the file.

  Reply by Dara Navaei on 12 August 2020, 09:46
  > Done

    Reply by pmontazemi on 19 October 2020, 10:45
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Controllers/DrainPump.h
Revision Comment by Sean Nash on 10 August 2020, 09:21 
https://devapps.diality.us/cru/DG-DEN-3421-1#c2992

  Should param be F32?

  Reply by Dara Navaei on 12 August 2020, 14:42
  > Done

    Reply by Sean Nash on 13 August 2020, 10:55
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 07:39 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3063

  Does the /**@}*/ come before or after the endif at eof?

  Reply by Dara Navaei on 12 August 2020, 09:35
  > Yes, they are all before the EOF and #endif

    Reply by pmontazemi on 19 October 2020, 10:44
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Controllers/ROPump.h
Revision Comment by Sean Nash on 26 October 2020, 08:40 
https://devapps.diality.us/cru/DG-DEN-3421-1#c5696

  Packing not needed if al fields are 32-bit.

  Reply by Dara Navaei on 26 October 2020, 08:48
  > Done

    Reply by Sean Nash on 26 October 2020, 10:44
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 August 2020, 10:03 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3005

  There is still a target pressure that gets set after target
  flow is achieved.  We may still want to be able to override it.

  Reply by Dara Navaei on 12 August 2020, 14:30
  > Agreed. Will bring back the pressure override capability once
  > I started working on the story

    Reply by Dara Navaei on 19 October 2020, 15:41
    > Brought back the capability.

      Reply by Sean Nash on 20 October 2020, 10:45
      > RESOLVED in CODE WALKTHROUGH.

Revision Comment by qnguyen on 12 October 2020, 09:44 
https://devapps.diality.us/cru/DG-DEN-3421-1#c5435

  Reverse the change. The macro is obsolete and should not be
  used anymore.

  Reply by Dara Navaei on 19 October 2020, 17:29
  > Done

    Reply by qnguyen on 20 October 2020, 10:32
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Services/PIControllers.h
Revision Comment by Sean Nash on 10 August 2020, 10:23 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3013

  I don't know if this will cause any Vectorcast problems.  But
  generally, I thought we agreed to add new enums to bottom of
  list.

  Reply by Dara Navaei on 12 August 2020, 10:57
  > Done

    Reply by Sean Nash on 13 August 2020, 10:44
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by qnguyen on 12 October 2020, 09:41 
https://devapps.diality.us/cru/DG-DEN-3421-1#c5433

  should there be a P in front of I_CONTROLLER?

  Reply by Dara Navaei on 19 October 2020, 15:10
  > The ramp up stage is only an I controller. The final stage is
  > a PI controller.

    Reply by qnguyen on 20 October 2020, 10:34
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Services/SystemComm.h
Revision Comment by pmontazemi on 12 August 2020, 10:44 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3132

  Where is the Doxygen /**@}*/ ending of file?

  Reply by Dara Navaei on 12 August 2020, 13:40
  > All modules will have doxygen hooks eventually

    Reply by pmontazemi on 13 August 2020, 11:22
    > Planned for DEN S26.

      Reply by pmontazemi on 13 August 2020, 11:22
      > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Services/CommBuffers.c
Revision Comment by pmontazemi on 12 August 2020, 10:42 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3126

  Add space between , and i

  Reply by Dara Navaei on 12 August 2020, 13:43
  > Done

    Reply by pmontazemi on 13 August 2020, 11:24
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:42 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3127

  One liner

  Reply by Dara Navaei on 12 August 2020, 13:43
  > Done

    Reply by pmontazemi on 13 August 2020, 11:24
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Services/CommBuffers.h
Revision Comment by pmontazemi on 12 August 2020, 10:43 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3129

  Where is the Doxygen /**@}*/ ending of file?

  Reply by Dara Navaei on 12 August 2020, 13:41
  > All modules will have doxygen hooks eventually

    Reply by pmontazemi on 13 August 2020, 11:24
    > Planned for DEN S26.

      Reply by pmontazemi on 13 August 2020, 11:24
      > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: Common.h
Revision Comment by pmontazemi on 12 August 2020, 10:50 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3153

  We either add a "." or not at the end of each comment, but we
  need to be consistent in the way we do it.

  Reply by Dara Navaei on 12 August 2020, 14:49
  > I prefer not to put ".", but let's make a decision in code
  > walkthrough

    Reply by pmontazemi on 12 August 2020, 14:55
    > Then remove them all.

      Reply by Dara Navaei on 12 August 2020, 17:24
      > The #define comments should have "." I will add them in
      > my code. The enums should not have "." in the end.

        Reply by pmontazemi on 13 August 2020, 11:04
        > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 10:50 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3154

  Where is the /*@}/ Doxygen comment at eof?

  Reply by Dara Navaei on 12 August 2020, 11:09
  > [~snash] Could you please respond?

    Reply by Sean Nash on 12 August 2020, 15:36
    > It's above the test stuff.  I think it should be moved to
    > bottom here.

      Reply by Dara Navaei on 12 August 2020, 17:28
      > Done

        Reply by pmontazemi on 13 August 2020, 11:03
        > RESOLVED in CODE WALKTHROUGH.

        Reply by Sean Nash on 13 August 2020, 11:03
        > I put it a little bit above bottom and it looks like
        > Dara added at bottom.  There should only be one of
        > these.  Dara, keep mine and remove this one at very
        > bottom.

----------------------------------------
File: HDDefs.h
Revision Comment by pmontazemi on 12 August 2020, 07:38 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3061

  Doxygen comment missing.

  Reply by Dara Navaei on 12 August 2020, 09:45
  > Done

    Reply by pmontazemi on 19 October 2020, 10:44
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 12 August 2020, 07:38 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3062

  Does the /**@}*/ come before or after the endif at eof?

  Reply by Dara Navaei on 12 August 2020, 09:42
  > Yes, they are all before the EOF and #endif

    Reply by pmontazemi on 19 October 2020, 10:44
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Services/AlarmMgmt.h
Revision Comment by pmontazemi on 20 August 2020, 11:18 
https://devapps.diality.us/cru/DG-DEN-3421-1#c3607

  50 what? A bit more verbose?

  Reply by Dara Navaei on 19 October 2020, 15:11
  > This is the integer value of the that enum. The values of
  > displayed as a comment every 5 enums.

    Reply by pmontazemi on 20 October 2020, 10:36
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Controllers/TemperatureSensors.c
Revision Comment by qnguyen on 26 October 2020, 09:39 
https://devapps.diality.us/cru/DG-DEN-3421-1#c5719

  Merge issue. Reverse the change.

  Reply by Dara Navaei on 26 October 2020, 09:55
  > This is not a merge issue. We deliberately changed it to 5
  > for testing.

    Reply by qnguyen on 26 October 2020, 10:38
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/DGCommon.h
Revision Comment by Sean Nash on 26 October 2020, 09:08 
https://devapps.diality.us/cru/DG-DEN-3421-1#c5708

  This looks like a merge issue.  I see duplicate build switches.

  Reply by Dara Navaei on 26 October 2020, 09:14
  > Removed the double switches.

    Reply by Sean Nash on 26 October 2020, 10:42
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Modes/ModeInitPOST.c
Revision Comment by pmontazemi on 19 October 2020, 09:51 
https://devapps.diality.us/cru/DG-DEN-3421-1#c5570

  Add TODO

  Reply by Dara Navaei on 19 October 2020, 15:03
  > Done

    Reply by pmontazemi on 20 October 2020, 10:36
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by pmontazemi on 19 October 2020, 09:51 
https://devapps.diality.us/cru/DG-DEN-3421-1#c5571

  Add TODO

  Reply by Dara Navaei on 19 October 2020, 15:03
  > Done

    Reply by pmontazemi on 20 October 2020, 10:36
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Modes/ModeRecirculate.c
Revision Comment by qnguyen on 12 October 2020, 09:42 
https://devapps.diality.us/cru/DG-DEN-3421-1#c5434

  what is 0.8? Should we define it to make it more clear?

  Reply by Dara Navaei on 19 October 2020, 15:07
  > This is for testing only since the way we control the pump
  > has changed. Added a TODO to update this value in a #define
  > later.

    Reply by qnguyen on 20 October 2020, 10:33
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Modes/ModeStandby.c
Revision Comment by Sean Nash on 26 October 2020, 09:04 
https://devapps.diality.us/cru/DG-DEN-3421-1#c5707

  Need to do the TODO here.  Must be in standby mode and in its
  idle state.

  Reply by Dara Navaei on 26 October 2020, 09:10
  > I added the checks.

    Reply by Sean Nash on 26 October 2020, 10:42
    > RESOLVED in CODE WALKTHROUGH.


---

ID: DG-DEN-3421-1 https://devapps.diality.us/cru/DG-DEN-3421-1

Title: DG-DEN-3421_DG Heat Disinfection
Statement of Objectives:


State: Closed

Summary: 

Author: Dara Navaei
Moderator: Dara Navaei
Reviewers: (3 active, 0 completed*)
     qnguyen
     Sean Nash
     pmontazemi