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

Review Summary: No summary


----------------------------------------
File: firmware/App/Controllers/Heaters.c
Revision Comment by Sean Nash on 12 August 2021, 11:41 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10638

  Is there a problem with this alarm?  Need a TODO?

  Reply by qnguyen on 18 August 2021, 00:59
  > Renamed to match new alarm ID.

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

----------------------------------------
File: firmware/App/Modes/ModeFill.c
Revision Comment by Dara Navaei on 21 July 2021, 21:28 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10391

  Add the doxygen comment.

  Reply by qnguyen on 06 August 2021, 09:59
  > Done.

    Reply by Dara Navaei on 13 August 2021, 10:59
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 20 July 2021, 13:25 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10374

  I think this is pretty clear, but for consistency, add a
  comment for this condition.

  Reply by qnguyen on 06 August 2021, 09:59
  > Done.

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

Revision Comment by Sean Nash on 19 August 2021, 09:23 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10786

  I know bicarb pump "on" request above does not immediately turn
  the pump on, but I still think we should set target speed
  before we request pump on.

  Reply by qnguyen on 19 August 2021, 10:10
  > Done.

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

Revision Comment by Sean Nash on 29 April 2021, 09:03 
https://devapps.diality.us/cru/DG-DEN-7802-1#c9642

  Replace "good" with "in range".  Is this function checking temp
  range for alarm or for acceptable for dialysate to go to
  reservoir?  If latter, shouldn't the range be target +/- 2°C?

  Reply by qnguyen on 29 April 2021, 10:59
  > Fixed. Compared to target temperature set by HD.

    Reply by Sean Nash on 03 May 2021, 10:32
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 19 August 2021, 09:20 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10785

  I know acid pump "on" request above does not immediately turn
  the pump on, but I still think we should set target speed
  before we request pump on.

  Reply by qnguyen on 19 August 2021, 10:10
  > Done.

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

Revision Comment by Dara Navaei on 21 July 2021, 21:36 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10392

  Why is there a magic number here?

  Reply by qnguyen on 06 August 2021, 09:58
  > Added a define.

    Reply by Dara Navaei on 13 August 2021, 10:59
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 29 April 2021, 09:30 
https://devapps.diality.us/cru/DG-DEN-7802-1#c9644

  Add blank line between declarations and code.

  Reply by qnguyen on 29 April 2021, 10:59
  > Done.

    Reply by Sean Nash on 03 May 2021, 10:32
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 27 August 2021, 10:38 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10828

  The max part of these do not look right (backward).

  Reply by qnguyen on 27 August 2021, 15:24
  > Nice catch. Fixed.

    Reply by Sean Nash on 31 August 2021, 16:20
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 June 2021, 09:30 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10228

  Call checkDialysateConductivity() once at top of function.
  Then use return value for the two if conditions.
  Also, this second if should be an else if to ensure you don't
  do both.

  Reply by qnguyen on 26 June 2021, 01:14
  > Fixed.

    Reply by Sean Nash on 23 July 2021, 10:32
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 29 April 2021, 09:31 
https://devapps.diality.us/cru/DG-DEN-7802-1#c9646

  Generally, when writing conditions involving a BOOL, try to
  form condition to use TRUE or FALSE depending on whichever will
  favor the safer path (e.g. if a bug or stack/array overflow
  corrupted the BOOL to a value like 0x12345678, this condition
  would still pass).
  So in this condition, we are deciding whether dialysate can be
  delivered to the reservoir.  We would like to err on the side
  of not delivering to the reservoir (safer state).  So we should
  form the condition in a way that conductivity and temperature
  ranges being good must be TRUE (1) in order to deliver
  dialysate to the reservoir.  Any other value would result in
  coming out of dialysate delivery state which is the safer thing
  to do.

  Reply by qnguyen on 29 April 2021, 11:00
  > Fixed.

    Reply by Sean Nash on 03 May 2021, 10:31
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 10 June 2021, 09:34 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10229

  I think we should not stop RO pump or heater.  Just go to drain
  like production state (but no mixing).

  Reply by qnguyen on 26 June 2021, 01:14
  > Fixed.

    Reply by Sean Nash on 23 July 2021, 10:32
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 19 August 2021, 09:25 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10787

  Start comments w/ capital letter.

  Reply by qnguyen on 19 August 2021, 10:39
  > Done.

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

Revision Comment by Sean Nash on 19 August 2021, 09:26 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10789

  I would prefer we return a TRUE or FALSE explicitly (like
  function below).

  Reply by qnguyen on 19 August 2021, 10:13
  > Done.

    Reply by Sean Nash on 19 August 2021, 10:37
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Controllers/ConcentratePumps.c
Revision Comment by Sean Nash on 14 May 2021, 13:43 
https://devapps.diality.us/cru/DG-DEN-7802-1#c9863

  Why cast here?

  Reply by qnguyen on 14 May 2021, 15:20
  > Removed casting.

    Reply by Sean Nash on 10 June 2021, 10:34
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 14 May 2021, 13:43 
https://devapps.diality.us/cru/DG-DEN-7802-1#c9864

  Should maybe cast here though.

  Reply by qnguyen on 14 May 2021, 15:18
  > The macro already cast the data to U32.

    Reply by Sean Nash on 10 June 2021, 10:34
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 19 August 2021, 09:20 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10784

  Changes "Pumps" to "Pump".  Request is for single pump.

  Reply by qnguyen on 19 August 2021, 10:08
  > Done.

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

Revision Comment by pmontazemi on 28 April 2021, 10:47 
https://devapps.diality.us/cru/DG-DEN-7802-1#c9638

  The if should be < and the else if should be >= otherwise there
  is double-dipping in case the value equals.

  Reply by qnguyen on 28 April 2021, 20:17
  > Fixed.

    Reply by pmontazemi on 29 April 2021, 10:31
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Modes/ModeRecirculate.c
Revision Comment by Dara Navaei on 21 July 2021, 21:21 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10388

  If HD is not communicating, should we got transition to standby
  solo mode?

  Reply by qnguyen on 21 July 2021, 22:29
  > In DG standby mode, DG will transition to solo mode.

    Reply by Dara Navaei on 23 July 2021, 10:35
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Dara Navaei on 21 July 2021, 21:24 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10389

  fushLinesVolumeL is an input.

  Reply by qnguyen on 22 July 2021, 09:18
  > Done.

    Reply by Dara Navaei on 23 July 2021, 10:34
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Modes/ModeStandby.c
Revision Comment by Dara Navaei on 21 July 2021, 21:04 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10383

  Should we turn on VSP?

  Reply by qnguyen on 21 July 2021, 22:32
  > VSP should only be on when user requests to sample water.

    Reply by Dara Navaei on 23 July 2021, 10:38
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Dara Navaei on 21 July 2021, 21:02 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10382

  Are we supposed to switch to water sample state? This does not
  go to water sample state.

  Reply by qnguyen on 21 July 2021, 22:32
  > We have to flush the filter flush before user can request to
  > sample water.

    Reply by Dara Navaei on 23 July 2021, 10:39
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Dara Navaei on 21 July 2021, 21:08 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10384

  Why there is a endSampleWaterRequest in this state? We should
  never get here if sample water request is in place.
  Also, if this an endSampleWaterRequest here, VSP should be
  closed.

  Reply by qnguyen on 21 July 2021, 22:33
  > VSP is never open until this state is done.
  > The endSampleWaterRequest is for the user presses the stop
  > button during the filter flush and end treatment, allowing DG
  > to go back to standby idle state.

    Reply by Dara Navaei on 23 July 2021, 10:36
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Dara Navaei on 21 July 2021, 21:11 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10385

  This should be flush filter idle state.

  Reply by qnguyen on 22 July 2021, 09:20
  > Done.

    Reply by Dara Navaei on 23 July 2021, 10:36
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Dara Navaei on 21 July 2021, 21:13 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10387

  Input: startSampleWaterRequest, endSampleWaterRequest.

  Reply by qnguyen on 22 July 2021, 09:20
  > Done.

    Reply by Dara Navaei on 23 July 2021, 10:35
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Dara Navaei on 21 July 2021, 21:12 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10386

  Output: startSampleWaterRequest, endSampleWaterRequest,
  waterSampleStartTime.

  Reply by qnguyen on 22 July 2021, 09:20
  > Done.

    Reply by Dara Navaei on 23 July 2021, 10:36
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Services/Reservoirs.c
Revision Comment by Sean Nash on 12 August 2021, 12:53 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10649

  Change to #ifdef?

  Reply by qnguyen on 12 August 2021, 16:41
  > Done. Should we remove this build flag all together since no
  > V2 system available anymore?

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

Revision Comment by Sean Nash on 12 August 2021, 12:57 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10651

  Change large to small in brief.  
  Why is this changing?  Who calls this function?

  Reply by qnguyen on 12 August 2021, 16:42
  > The mode fill is calling this function to get the base weight
  > before a fill (for load cell vs integrated flow check).
  > The large filter takes too long and mode fill sometimes get
  > higher value if drain and fill happens too fast.

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

----------------------------------------
File: firmware/App/Services/Reservoirs.h
Revision Comment by Sean Nash on 01 July 2021, 11:27 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10344

  I believe this will eventually be a constant in the
  non-volatile calibration record.  So will have a getter
  function in NV-Data module for this value.

  Reply by Dara Navaei on 21 July 2021, 20:26
  > The getter from non-volatile module is in my branch.

    Reply by Sean Nash on 23 July 2021, 10:31
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Controllers/Pressures.c
Revision Comment by Sean Nash on 12 August 2021, 11:43 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10639

  Is ADC range check necessary?

  Reply by qnguyen on 18 August 2021, 00:59
  > Removed as it is not needed anymore.

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

----------------------------------------
File: firmware/App/Modes/ModeDrain.c
Revision Comment by Sean Nash on 12 August 2021, 11:44 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10640

  Why needed?

  Reply by qnguyen on 12 August 2021, 22:57
  > To check for HD communication.

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

----------------------------------------
File: firmware/App/Services/AlarmMgmt.c
Revision Comment by Sean Nash on 12 August 2021, 12:49 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10644

  Why needed?

  Reply by qnguyen on 12 August 2021, 16:40
  > AlarmMgmt checks for HD communication (in SystemComm) before
  > broadcast trigger message (prevent NACK issue).

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

----------------------------------------
File: firmware/App/Services/SystemComm.c
Revision Comment by Sean Nash on 20 July 2021, 13:32 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10375

  Why not alarm?

  Reply by qnguyen on 22 July 2021, 10:56
  > Added back alarm.

    Reply by Sean Nash on 23 July 2021, 10:30
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Services/Interrupts.c
Revision Comment by Sean Nash on 12 August 2021, 12:50 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10645

  Why commented out?

  Reply by qnguyen on 12 August 2021, 16:47
  > Removed. This windowed count is removed in branch DEN-8886.
  > There is no definition for it anymore.

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

Revision Comment by Sean Nash on 12 August 2021, 12:50 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10646

  Is there a problem with this alarm?  Add TODO?

  Reply by qnguyen on 12 August 2021, 16:46
  > Removed. This alarm is removed in branch DEN-8886.

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

----------------------------------------
File: firmware/App/Controllers/Voltages.h
Revision Comment by Sean Nash on 19 August 2021, 09:15 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10783

  So we are not monitoring these, but we are publishing them?

  Reply by qnguyen on 19 August 2021, 09:26
  > Removed.

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

----------------------------------------
File: firmware/App/Modes/ModeGenIdle.c
Revision Comment by Sean Nash on 12 August 2021, 12:48 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10642

  Do we need this state?  What's difference between the 2 states
  now that VDr is never recirculating?

  Reply by qnguyen on 12 August 2021, 16:38
  > HD normally looks at this state to command fill or drain. We
  > would want to wait until the flush lines to be done before
  > drain or fill.

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

----------------------------------------
File: firmware/App/Modes/OperationModes.c
Revision Comment by Sean Nash on 12 August 2021, 12:48 
https://devapps.diality.us/cru/DG-DEN-7802-1#c10643

  Change /* CIRC to /* GENE

  Reply by qnguyen on 13 August 2021, 10:53
  > Done.

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


---

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

Title: DG-DEN-7802_DG-DEV Dialysate Generation
Statement of Objectives:


State: Closed

Summary: 

Author: qnguyen
Reviewers: (1 active, 2 completed*)
     Sean Nash   (*)
     Dara Navaei   (*)
     pmontazemi