This is a list of all comments for HD-DEN-12609-2.

Review Summary: No summary


----------------------------------------
File: firmware/App/Modes/Prime.c
Revision Comment by Darren Cox on 24 August 2022, 15:31 
https://devapps.diality.us/cru/HD-DEN-12609-2#c13551

  Extra space before comment.

  Reply by Michael Garthwaite on 24 August 2022, 16:47
  > Fixed. Thanks!

    Reply by Darren Cox on 26 August 2022, 14:14
    > RESOLVED in CODE WALKTHROUGH

----------------------------------------
File: firmware/App/Controllers/PresOccl.c
Revision Comment by Sean Nash on 14 June 2022, 09:15 
https://devapps.diality.us/cru/HD-DEN-12609-2#c13138

  Add space after ifs.

  Reply by Michael Garthwaite on 24 August 2022, 10:42
  > Fixed. Thanks!

    Reply by Sean Nash on 26 August 2022, 14:10
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 25 August 2022, 16:35 
https://devapps.diality.us/cru/HD-DEN-12609-2#c13556

  Remove all of these new blank lines.

  Reply by Michael Garthwaite on 26 August 2022, 10:14
  > Fixed. Thanks!

    Reply by Sean Nash on 26 August 2022, 14:06
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 14 June 2022, 09:19 
https://devapps.diality.us/cru/HD-DEN-12609-2#c13139

  Not sure if this is an issue - but looks like this clear alarm
  condition check can happen even when occlusion level after
  install hasn't been set yet.

  Reply by Michael Garthwaite on 24 August 2022, 10:36
  > Can you clarify what case you're thinking of?

    Reply by Sean Nash on 24 August 2022, 15:58
    > Can we get to this if condition before we've even set
    > bloodPumpOcclusionAfterCartridgeInstall ?  And, if so, is
    > that a problem?

      Reply by Michael Garthwaite on 26 August 2022, 10:15
      > We can get to this but
      > bloodPumpOcclusionAfterCartridgeInstall will be set to 0.
      > Therefore, bpOccl will need to be <= 5500 for the alarm
      > to clear. Im unsure if it would be a problem. We use
      > isAlarmTriggered() to clear it only if has been raised
      > previously. 
      > 
      > Open to discussion in code walkthrough.

        Reply by Sean Nash on 26 August 2022, 14:12
        > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Darren Cox on 24 August 2022, 15:29 
https://devapps.diality.us/cru/HD-DEN-12609-2#c13549

  Comment says PreTreat, If checks for Treatment.  Also space
  needed after if.

  Reply by Michael Garthwaite on 24 August 2022, 16:47
  > Fixed. Thanks!

    Reply by Sean Nash on 25 August 2022, 16:37
    > Still see lines of code like this one that require a space
    > after if (i.e. "if(..." should be "if (...").

      Reply by Darren Cox on 26 August 2022, 14:13
      > RESOLVED in CODE WALKTHROUGH

Revision Comment by Darren Cox on 24 August 2022, 15:30 
https://devapps.diality.us/cru/HD-DEN-12609-2#c13550

  Blank lines before and after if.

  Reply by Michael Garthwaite on 24 August 2022, 16:47
  > Fixed. Thanks!

    Reply by Sean Nash on 25 August 2022, 16:40
    > I think Darren is referring to a needed space after if
    > (before opening parenthesis)?  And not adding blank lines
    > before if statements.

      Reply by Michael Garthwaite on 26 August 2022, 10:13
      > Understood. Fixed. Thanks!

        Reply by Darren Cox on 26 August 2022, 14:13
        > RESOLVED in CODE WALKTHROUGH

----------------------------------------
File: firmware/App/Modes/ModePreTreat.c
Revision Comment by Sean Nash on 14 June 2022, 09:21 
https://devapps.diality.us/cru/HD-DEN-12609-2#c13140

  Add blank line between functions.

  Reply by Michael Garthwaite on 24 August 2022, 10:42
  > Fixed. Thanks!

    Reply by Sean Nash on 26 August 2022, 14:08
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Services/Reservoirs.c
Revision Comment by Sean Nash on 14 June 2022, 09:24 
https://devapps.diality.us/cru/HD-DEN-12609-2#c13141

  These are redundant with above.  Keep above definitions as they
  have the 'F' suffix that we want.  Remove these.

  Reply by Michael Garthwaite on 24 August 2022, 10:42
  > Fixed. Thanks!

    Reply by Sean Nash on 26 August 2022, 14:07
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 14 June 2022, 09:26 
https://devapps.diality.us/cru/HD-DEN-12609-2#c13142

  This appears to be a timer counter (task intervals, not ms).

  Reply by Sean Nash on 26 August 2022, 14:07
  > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 14 June 2022, 09:27 
https://devapps.diality.us/cru/HD-DEN-12609-2#c13143

  Should probably log ms - so need to multiply by task interval.

  Reply by Sean Nash on 26 August 2022, 14:07
  > RESOLVED in CODE WALKTHROUGH.


---

ID: HD-DEN-12609-2 https://devapps.diality.us/cru/HD-DEN-12609-2

Title: HD-DEN-12609_SW Dev Sprint 69 MG
Statement of Objectives:


State: Closed

Summary: 

Author: Michael Garthwaite
Moderator: Michael Garthwaite
Reviewers: (1 active, 2 completed*)
     Sean Nash   (*)
     Dara Navaei   (*)
     Darren Cox