This is a list of all comments for HD-DEN-14344-5.

Review Summary: No summary


----------------------------------------
File: firmware/App/Controllers/DialInFlow.c
Revision Comment by Sean Nash on 02 December 2022, 16:14 
https://devapps.diality.us/cru/HD-DEN-14344-5#c15097

  Not clear on what this refactoring of condition changed - looks
  the same.

  Reply by Michael Garthwaite on 02 December 2022, 17:03
  > originally wanted to change setDialInPumpTargetFlowRate() to
  > have the same conditions as testSetDialInPWM but we can
  > integrate it later pending Darren's results.

    Reply by Sean Nash on 05 December 2022, 09:48
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by jtaylor on 05 December 2022, 10:03 
https://devapps.diality.us/cru/HD-DEN-14344-5#c15132

  This might be better, with the complex conditional replaced
  with a temporary boolean (8.11.24)

  Reply by jtaylor on 05 December 2022, 13:39
  > RESOLVED in CODE WALKTHROUGH, no change

Revision Comment by Sean Nash on 02 December 2022, 16:15 
https://devapps.diality.us/cru/HD-DEN-14344-5#c15098

  Keep a blank line between declaration and code.

  Reply by Michael Garthwaite on 02 December 2022, 17:02
  > Fixed. Thanks!

    Reply by Sean Nash on 05 December 2022, 09:48
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 02 December 2022, 16:22 
https://devapps.diality.us/cru/HD-DEN-14344-5#c15103

  remove "," after pwm.

  Reply by Michael Garthwaite on 02 December 2022, 17:01
  > Fixed. Thanks!

    Reply by Sean Nash on 05 December 2022, 09:49
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 02 December 2022, 16:18 
https://devapps.diality.us/cru/HD-DEN-14344-5#c15099

  First part of condition should have () around it too.
  Do we want to set direction here even if we fail to satisfy
  condition below?

  Reply by Michael Garthwaite on 02 December 2022, 16:33
  > Fixed. This condition requires that the Pump is flagged off.
  > that same condition is also used below. we will always be
  > true in the first condition below if we satisfy this
  > condition. If we try to change direction when the pump is on,
  > both this condition and the one below will fail.

    Reply by Sean Nash on 05 December 2022, 09:51
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by jtaylor on 05 December 2022, 10:06 
https://devapps.diality.us/cru/HD-DEN-14344-5#c15133

  This might be better, with the complex conditional replaced
  with a temporary boolean (8.11.24)

  Reply by jtaylor on 05 December 2022, 13:39
  > RESOLVED in CODE WALKTHROUGH, no change

Revision Comment by wbracken on 02 December 2022, 17:43 
https://devapps.diality.us/cru/HD-DEN-14344-5#c15117

  Remove blank line.

  Reply by Sean Nash on 05 December 2022, 13:57
  > Removed.

    Reply by wbracken on 05 December 2022, 14:06
    > RESOLVED IN CODE WALKTHROUGH

Revision Comment by Sean Nash on 02 December 2022, 16:19 
https://devapps.diality.us/cru/HD-DEN-14344-5#c15100

  Remove blank line.

  Reply by Michael Garthwaite on 02 December 2022, 17:02
  > Fixed. Thanks!

    Reply by Sean Nash on 05 December 2022, 09:51
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 02 December 2022, 16:21 
https://devapps.diality.us/cru/HD-DEN-14344-5#c15101

  Should this be set to true even if result below is false?
  Maybe set to true in testSetDialInPumpWithPWM() function after
  all of the conditions are found to be met?

  Reply by Michael Garthwaite on 02 December 2022, 17:02
  > Fixed. moved to testSetDialInPumpWithPWM(). Thanks!

    Reply by Sean Nash on 05 December 2022, 09:52
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 02 December 2022, 16:22 
https://devapps.diality.us/cru/HD-DEN-14344-5#c15102

  Remove blank line.

  Reply by Michael Garthwaite on 02 December 2022, 17:02
  > Fixed. Thanks!

    Reply by Sean Nash on 05 December 2022, 09:52
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 05 December 2022, 09:53 
https://devapps.diality.us/cru/HD-DEN-14344-5#c15131

  Restore this blank line.

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

----------------------------------------
File: firmware/App/Modes/ModeTreatmentParams.c
Revision Comment by wbracken on 02 December 2022, 11:49 
https://devapps.diality.us/cru/HD-DEN-14344-5#c15096

  parameter0?

  Reply by Michael Garthwaite on 02 December 2022, 17:02
  > Fixed. Thanks!

    Reply by wbracken on 02 December 2022, 17:44
    > FIXED IN CODE WALKTHROUGH

      Reply by Dara Navaei on 19 October 2023, 09:11
      > RESOLVED in CODE WALKTHROUGH

----------------------------------------
File: firmware/App/Services/SystemComm.c
Revision Comment by Sean Nash on 02 December 2022, 16:23 
https://devapps.diality.us/cru/HD-DEN-14344-5#c15104

  Where did this enum name change take place?  There is no common
  in this code review.

  Reply by Michael Garthwaite on 02 December 2022, 16:28
  > i think there's a conflict between staging and develop. fixed
  > for staging merge. thanks!

    Reply by Sean Nash on 05 December 2022, 09:46
    > RESOLVED in CODE WALKTHROUGH.


---

ID: HD-DEN-14344-5 https://devapps.diality.us/cru/HD-DEN-14344-5

Title: HD-DEN-14344_SW S85 MG Dpi Pwmflow Fix
Statement of Objectives:


State: Closed

Summary: 

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