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

Review Summary: No summary


----------------------------------------
File: firmware/App/Modes/SampleWater.c
Revision Comment by Sean Nash on 22 March 2021, 09:57 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8427

  Consider renaming this to sampleWaterResultEntered to clarify
  its purpose.

  Reply by qnguyen on 22 March 2021, 10:29
  > Done.

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

Revision Comment by Sean Nash on 22 March 2021, 10:07 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8431

  Why is bad sample result handled here and good sample result
  handled in pre-treatment mode?

  Reply by qnguyen on 22 March 2021, 10:29
  > Moved handlers to pre-treatment mode.

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

Revision Comment by Sean Nash on 22 March 2021, 09:58 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8428

  Add fault.

  Reply by qnguyen on 22 March 2021, 10:29
  > Moved handle to pre-treatment mode. Go to standby mode when
  > sample water test failed.

    Reply by Sean Nash on 22 March 2021, 10:33
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Modes/ModePreTreat.c
Revision Comment by Sean Nash on 22 March 2021, 10:05 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8430

  How will DG standby get back to idle state from flush idle
  state?  Where does that happen?

  Reply by qnguyen on 22 March 2021, 10:08
  > When the sample water result entered by the user, HD will
  > command DG to end water sampling mode and go back to standby
  > idle state.

    Reply by Sean Nash on 22 March 2021, 10:33
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 10:04 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8429

  Use set dialysate temp from treatment parameters for trimmer
  heater target temp.  Add 2 degrees for primary heater target
  temp.

  Reply by qnguyen on 22 March 2021, 10:30
  > Done.

    Reply by Sean Nash on 22 March 2021, 10:33
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Modes/SampleWater.h
Revision Comment by Sean Nash on 22 March 2021, 10:37 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8449

  If returning a result, should return type be something other
  than boolean?  Maybe SELF_TEST_STATUS_T from Common.h?

  Reply by qnguyen on 22 March 2021, 11:40
  > Done.

    Reply by Sean Nash on 23 March 2021, 11:17
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Modes/SelfTests.c
Revision Comment by Sean Nash on 22 March 2021, 15:21 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8491

  Pressure should be float.  Add .0 to end of value.

  Reply by qnguyen on 22 March 2021, 15:36
  > Fixed.

    Reply by Sean Nash on 23 March 2021, 11:11
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 15:20 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8490

  These volumes (4 of them) should be floats so we should add .0
  to end of set value.

  Reply by qnguyen on 22 March 2021, 15:36
  > Fixed.

    Reply by Sean Nash on 23 March 2021, 11:11
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 11:09 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8458

  Should we have a specific fault for each no cartridge check
  instead of a single fault for all checks?  
  If we just have one fault, we are limited in what we can
  provide for data - you are giving the state that failure
  occurred.  
  If we had specific faults, we could provide more details (e.g.
  if board temp check fails, we could provide the measured board
  temp with fault).

  Reply by qnguyen on 22 March 2021, 20:41
  > Added specific fault for each check.

    Reply by Sean Nash on 23 March 2021, 11:16
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 11:02 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8456

  Rename to "havePumpsStarted".  
  Set to FALSE in handleNoCartSelfTestStoppedState() so pumps
  will be restarted on resume.

  Reply by qnguyen on 22 March 2021, 11:39
  > Done.

    Reply by Sean Nash on 23 March 2021, 11:16
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 11:06 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8457

  Is the blood leak self test fast enough to be done in less than
  50 ms?

  Reply by qnguyen on 22 March 2021, 11:24
  > Yes, I think so. The blood leak self test once set, the blood
  > leak detector status will be high right away.

    Reply by Sean Nash on 23 March 2021, 11:16
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 11:15 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8459

  What do you have in mind for door switch self-test?  I would
  think that if we are confirming door close/open at various
  states, that might be good enough.  Not sure what else we can
  do.

  Reply by qnguyen on 22 March 2021, 11:58
  > Removed. It seems nothing much we can do. The door switch is
  > simple and just a GPIO read from FPGA.

    Reply by Sean Nash on 23 March 2021, 11:16
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 13:52 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8472

  Shouldn't this be a bubble detector fault?

  Reply by qnguyen on 22 March 2021, 14:41
  > This test is more about the cartridge should be dry.
  > Sensors' faults should be part of POST or no cartridge
  > self-tests.

    Reply by Sean Nash on 22 March 2021, 15:33
    > Then I would rename the function/state to something less
    > bubble detector and more used cartridge check 1.

      Reply by qnguyen on 22 March 2021, 16:41
      > Combined air trap and bubble detectors into one used
      > cartridge check.

        Reply by Sean Nash on 23 March 2021, 11:15
        > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 13:53 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8473

  Shouldn't this be an air trap level sensor fault?

  Reply by qnguyen on 22 March 2021, 14:42
  > This test is more about the cartridge should be dry.
  > Sensors' faults should be part of POST or no cartridge
  > self-tests.

    Reply by Sean Nash on 22 March 2021, 15:35
    > Then I would rename the function/state to something less
    > bubble detector and more used cartridge check 2.

      Reply by qnguyen on 22 March 2021, 16:41
      > Combined air trap and bubble detectors into one used
      > cartridge check.

        Reply by Sean Nash on 23 March 2021, 11:15
        > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 13:54 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8474

  Remove blank lin.

  Reply by qnguyen on 22 March 2021, 16:41
  > Done.

    Reply by Sean Nash on 23 March 2021, 11:15
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 13:54 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8475

  Add blank line here.  
  Also add comment for these if statements to describe the
  conditions.  Looks like first if for when to end the test and
  second if is pass/fail criteria.

  Reply by qnguyen on 22 March 2021, 15:44
  > Fixed.

    Reply by Sean Nash on 23 March 2021, 11:14
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 14:02 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8476

  Air trap was already closed - why closing it again?

  Reply by qnguyen on 22 March 2021, 15:44
  > Wrong valve state. Fixed.

    Reply by Sean Nash on 23 March 2021, 11:14
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 14:04 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8477

  Is use of fabs() here appropriate?  Do we really not care which
  pressure is larger?  It looks like previous readings are with
  VBA/VBV closed and BP off while current readings are with BP on
  and valves open.

  Reply by qnguyen on 22 March 2021, 15:42
  > The valves were not correct. It should be back to the
  > previous state when the BP is off.
  > The pressure should be back with in 5 mmHg with the first
  > pressure measurements before we run BP.
  > Added 2 seconds time out for pressure to be normalized.

    Reply by Sean Nash on 23 March 2021, 11:14
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 14:16 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8478

  Shouldn't we turn BP off when we're done using it for test?

  Reply by qnguyen on 22 March 2021, 15:44
  > Fixed. Now the BP pump should be off when the test is done.

    Reply by Sean Nash on 23 March 2021, 11:13
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 15:04 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8485

  Valve positions same so pull out of if-else.

  Reply by qnguyen on 23 March 2021, 11:12
  > Done.

    Reply by Sean Nash on 23 March 2021, 11:13
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 13:49 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8471

  For bubble detectors, leak detectors, door, etc... we will want
  to use drivers when available.  Put a TODO on these so we don't
  forget.

  Reply by qnguyen on 23 March 2021, 11:11
  > Done.

    Reply by Sean Nash on 23 March 2021, 11:15
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 15:13 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8488

  Should be 0.0.

  Reply by qnguyen on 22 March 2021, 15:41
  > Fixed.

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

Revision Comment by Sean Nash on 22 March 2021, 15:16 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8489

  Why using fabs() here?  Reservoir 1 should have gone down and
  reservoir 2 should have gone up.  If opposite happened, we
  should not pass the test.

  Reply by qnguyen on 22 March 2021, 15:32
  > Fixed.

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

Revision Comment by Sean Nash on 22 March 2021, 15:31 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8495

  Probably should be fabs() here.  and change 1 to 1.0.

  Reply by qnguyen on 22 March 2021, 15:34
  > Fixed.

    Reply by Sean Nash on 23 March 2021, 11:09
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 15:23 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8492

  Should set settleStartTime here.

  Reply by qnguyen on 22 March 2021, 15:32
  > The reservoir should be settled for the first verify already.
  > There is no need to wait for another settle to start second
  > displacement.
  > Removed the check for settle time in second displacement
  > setup.

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

Revision Comment by Sean Nash on 22 March 2021, 15:30 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8493

  Why fabs() here?

  Reply by qnguyen on 22 March 2021, 15:35
  > Fixed.

    Reply by Sean Nash on 23 March 2021, 11:11
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 15:30 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8494

  Probably should be fabs() here.  And change 1 to 1.0.

  Reply by qnguyen on 22 March 2021, 15:35
  > Fixed.

    Reply by Sean Nash on 23 March 2021, 11:11
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Modes/Prime.h
Revision Comment by Sean Nash on 14 March 2021, 19:19 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8356

  Where is this used?  Does it need to be public?

  Reply by qnguyen on 15 March 2021, 09:28
  > It was used to calculated how much to fill the first
  > reservoir in mode pretreatment module, but not anymore.
  > Removed.

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

----------------------------------------
File: firmware/App/Controllers/DGInterface.c
Revision Comment by Sean Nash on 28 February 2021, 16:21 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8246

  Where does this get set to TRUE?  Who uses this flag?

  Reply by qnguyen on 28 February 2021, 17:09
  > No one is using this flag. This flag gets set to TRUE
  > previously when the HD starts to command DG to sample water.
  > The DG sample water command consists of start, stop, end
  > sample water. So this flag does not make sense anymore.

    Reply by Sean Nash on 01 March 2021, 10:35
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 14 March 2021, 19:09 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8353

  From where is this function called?

  Reply by qnguyen on 15 March 2021, 09:20
  > It is called from task general.

    Reply by Sean Nash on 19 March 2021, 09:15
    > There is a DG monitor function near top of this module.  I
    > recommend calling it from there.  TaskGeneral should only
    > be calling very high level functions.

      Reply by qnguyen on 19 March 2021, 10:41
      > Make it private function to this module and call from DG
      > monitor function.

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

Revision Comment by Sean Nash on 14 March 2021, 18:58 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8352

  First condition should be in parenthesis too.

  Reply by qnguyen on 15 March 2021, 09:22
  > Fixed.

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

----------------------------------------
File: firmware/App/Tasks/TaskGeneral.c
Revision Comment by Sean Nash on 28 February 2021, 16:23 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8247

  Why added?

  Reply by qnguyen on 28 February 2021, 17:12
  > Removed. It was leftover when battery exec function was
  > called from general task.

    Reply by Sean Nash on 01 March 2021, 10:35
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/include/i2c.h
Revision Comment by pmontazemi on 28 February 2021, 22:22 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8251

  Remove extra line.

  Reply by qnguyen on 01 March 2021, 08:51
  > This file is generated by halcogen and should not be
  > modified.

    Reply by pmontazemi on 01 March 2021, 10:37
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Modes/ConsumableSelfTest.c
Revision Comment by Sean Nash on 14 March 2021, 19:16 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8354

  Does this flag indicate install requested or install confirmed?

  Reply by qnguyen on 15 March 2021, 09:24
  > Renamed to consumableInstallConfirmed for less confusing.

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

Revision Comment by Sean Nash on 22 March 2021, 10:53 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8453

  Can DG check if DG door closed?  If so, should we confirm that
  the door is closed?

  Reply by qnguyen on 22 March 2021, 11:59
  > The DG does not expose motors to user. So we do not need to
  > check for DG's closed door.

    Reply by Sean Nash on 22 March 2021, 13:44
    > I was thinking more about whether concentrate pumps will
    > work properly with door open or not.  Is closing the door
    > required for flow path or any other reason?

      Reply by qnguyen on 22 March 2021, 14:19
      > Straw door needs to be open for concentrate jugs'
      > connection. Other than that, I am not aware of any flow
      > path that requires closed door.
      > Added TODO.

        Reply by Sean Nash on 23 March 2021, 11:16
        > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 22 March 2021, 10:11 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8433

  When using build switches, convention is to put the normal code
  first.  So use #ifndef and swap the statements below.

  Reply by qnguyen on 22 March 2021, 10:30
  > Done.

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

Revision Comment by Sean Nash on 14 March 2021, 19:17 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8355

  Does HD have to follow these next 3 states of DG?  Or could it
  just wait for DG to get to the dialysate productions state?

  Reply by qnguyen on 15 March 2021, 09:25
  > Since the UI follows HD states and not DG, HD has to follow
  > these next 3 states of DG. This allows UI to tick off the
  > checklist.

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

Revision Comment by Sean Nash on 22 March 2021, 10:14 
https://devapps.diality.us/cru/HD-DEN-7091-1#c8434

  If DG somehow gets ahead of HD here, I worry we will get stuck.
  Consider second condition with "getDGSubMode() >=
  DG_FILL_MODE_STATE_BICARB_PUMP_CHECK".  Same for states below.

  Reply by qnguyen on 22 March 2021, 10:30
  > Done.

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

----------------------------------------
File: firmware/App/Controllers/SyringePump.c
Revision Comment by pmontazemi on 06 April 2021, 09:07 
https://devapps.diality.us/cru/HD-DEN-7091-1#c9018

  In comments, replace ten and five with 10 and 5.

  Reply by qnguyen on 06 April 2021, 09:34
  > Done. Updated syringe pump driver to match HD-DEN-7117-1.

    Reply by pmontazemi on 06 April 2021, 10:27
    > RESOLVED in CODE WALKTHROUGH.


---

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

Title: HD-DEN-7091_Pre Treatment Mode
Statement of Objectives:


State: Closed

Summary: 

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