This is a list of all comments for DG-DEN-5855-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/DrainPump.c Revision Comment by qnguyen on 30 November 2020, 10:46 https://devapps.diality.us/cru/DG-DEN-5855-1#c6325 Persistent Alarm module now uses time interval rather than count. Change this to time period. Reply by Dara Navaei on 03 December 2020, 10:33 > Done Reply by qnguyen on 07 December 2020, 10:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 November 2020, 14:45 https://devapps.diality.us/cru/DG-DEN-5855-1#c6408 Add a TODO on this. Reply by Dara Navaei on 03 December 2020, 10:38 > Done Reply by Sean Nash on 07 December 2020, 10:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 November 2020, 14:47 https://devapps.diality.us/cru/DG-DEN-5855-1#c6409 Is there a better place to put this note? Maybe in a function header brief where it is used? Reply by Dara Navaei on 03 December 2020, 10:42 > Moved it to execDrainPumpMonitor function. Reply by Sean Nash on 07 December 2020, 10:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 November 2020, 14:48 https://devapps.diality.us/cru/DG-DEN-5855-1#c6410 I don't see any inputs to this function. Why change from none? Reply by Dara Navaei on 03 December 2020, 10:44 > The variable hasClosedLoopBeenRequested which is a boolean is > initialized here. This is an input. Reply by Dara Navaei on 08 December 2020, 14:08 > Changed the input to none. Reply by Sean Nash on 09 December 2020, 11:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 November 2020, 14:59 https://devapps.diality.us/cru/DG-DEN-5855-1#c6411 Add blank line between declarations and code. Reply by Dara Navaei on 03 December 2020, 10:46 > Done Reply by Sean Nash on 07 December 2020, 10:51 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 30 November 2020, 10:50 https://devapps.diality.us/cru/DG-DEN-5855-1#c6326 Do we need this check if there is range check inside the function setDrainPumpTargetRPM? Reply by Dara Navaei on 03 December 2020, 10:48 > We do check inside that function and raise an alarm if > needed. But for Dialin we check in the function too so if the > RPM is out of range, it will return a FALSE rather than > raising an alarm. Reply by qnguyen on 07 December 2020, 10:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 30 November 2020, 10:52 https://devapps.diality.us/cru/DG-DEN-5855-1#c6328 Do we need this check if there is range check inside the function setDrainPumpTargetDeltaPressure? Reply by Dara Navaei on 03 December 2020, 10:50 > We do check inside that function and raise an alarm if > needed. But for Dialin we check in the function too so if the > RPM is out of range, it will return a FALSE rather than > raising an alarm. Reply by qnguyen on 07 December 2020, 10:40 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/PIControllers.h Revision Comment by Sean Nash on 30 November 2020, 14:34 https://devapps.diality.us/cru/DG-DEN-5855-1#c6401 This enum does not match the count or order of the controller table in the .c file. Reply by Dara Navaei on 03 December 2020, 10:52 > Done Reply by Sean Nash on 07 December 2020, 10:54 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemComm.c Revision Comment by Sean Nash on 30 November 2020, 14:36 https://devapps.diality.us/cru/DG-DEN-5855-1#c6403 Why removed? Reply by Dara Navaei on 03 December 2020, 10:53 > It has been renamed and also has been moved to the RO pump > branch. Reply by Sean Nash on 07 December 2020, 10:54 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by qnguyen on 30 November 2020, 10:38 https://devapps.diality.us/cru/DG-DEN-5855-1#c6322 Remove extra space between "Inputs", "Outputs" and colon. Reply by Dara Navaei on 03 December 2020, 10:55 > Done Reply by qnguyen on 07 December 2020, 10:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 30 November 2020, 10:39 https://devapps.diality.us/cru/DG-DEN-5855-1#c6323 Missing param name. Reply by Sean Nash on 30 November 2020, 14:38 > And missing one of the params completely. Reply by Dara Navaei on 03 December 2020, 10:56 > Sorry I forgot to put the param name completely. Reply by qnguyen on 07 December 2020, 10:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 10 December 2020, 13:52 https://devapps.diality.us/cru/DG-DEN-5855-1#c6777 Update param info. Reply by Dara Navaei on 10 December 2020, 14:04 > Done Reply by qnguyen on 10 December 2020, 16:14 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 10 December 2020, 13:47 https://devapps.diality.us/cru/DG-DEN-5855-1#c6773 Missing param name. Reply by Dara Navaei on 10 December 2020, 13:58 > Done Reply by qnguyen on 10 December 2020, 16:13 > RESOLVED in CODE WLA Revision Comment by qnguyen on 10 December 2020, 13:48 https://devapps.diality.us/cru/DG-DEN-5855-1#c6774 Remove extra comment. Reply by Dara Navaei on 10 December 2020, 13:59 > Done Reply by qnguyen on 10 December 2020, 16:13 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 30 November 2020, 10:37 https://devapps.diality.us/cru/DG-DEN-5855-1#c6320 Revert this change. The other format for doxygen is correct. Reply by Sean Nash on 30 November 2020, 14:40 > And also revert spacing in function code below. Reply by Dara Navaei on 03 December 2020, 11:03 > Done Reply by qnguyen on 07 December 2020, 10:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 November 2020, 14:41 https://devapps.diality.us/cru/DG-DEN-5855-1#c6407 If changing function name, change name in function header as well. Reply by Dara Navaei on 03 December 2020, 11:03 > Done. Sorry I don't know what happened to this header. Reply by Sean Nash on 07 December 2020, 10:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 30 November 2020, 10:38 https://devapps.diality.us/cru/DG-DEN-5855-1#c6321 Revert this change. Reply by Sean Nash on 30 November 2020, 14:41 > And also revert spacing in function code below. Reply by Dara Navaei on 03 December 2020, 11:05 > Done Reply by qnguyen on 07 December 2020, 10:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 10 December 2020, 13:51 https://devapps.diality.us/cru/DG-DEN-5855-1#c6776 Mismatch function name. Reply by Dara Navaei on 10 December 2020, 14:03 > Done Reply by qnguyen on 10 December 2020, 16:14 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 10 December 2020, 13:49 https://devapps.diality.us/cru/DG-DEN-5855-1#c6775 Misspell "Publish" Reply by Dara Navaei on 10 December 2020, 14:00 > Done Reply by qnguyen on 10 December 2020, 16:14 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.h Revision Comment by Sean Nash on 30 November 2020, 14:35 https://devapps.diality.us/cru/DG-DEN-5855-1#c6402 Why include twice? Reply by Dara Navaei on 03 December 2020, 10:54 > Done Reply by Sean Nash on 07 December 2020, 10:54 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ROPump.c Revision Comment by qnguyen on 10 December 2020, 13:56 https://devapps.diality.us/cru/DG-DEN-5855-1#c6779 These defines has been defined at line number 60-64. Reply by Dara Navaei on 10 December 2020, 14:06 > Done Reply by qnguyen on 10 December 2020, 16:15 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 10 December 2020, 13:58 https://devapps.diality.us/cru/DG-DEN-5855-1#c6780 Add TODO. Reply by Dara Navaei on 10 December 2020, 14:06 > Done Reply by qnguyen on 10 December 2020, 16:15 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 10 December 2020, 14:00 https://devapps.diality.us/cru/DG-DEN-5855-1#c6783 Suggest grouping static (local) functions together below. Reply by Dara Navaei on 10 December 2020, 14:10 > Done Reply by qnguyen on 10 December 2020, 16:16 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 10 December 2020, 14:31 https://devapps.diality.us/cru/DG-DEN-5855-1#c6798 Do we need the variable roPumpPWMDutyCyclePct anymore? Reply by Dara Navaei on 10 December 2020, 14:41 > Yes, we still use it. Reply by qnguyen on 10 December 2020, 16:16 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 10 December 2020, 14:04 https://devapps.diality.us/cru/DG-DEN-5855-1#c6786 Should we allow 1.0 LPM if that is the defined MAX? Reply by Dara Navaei on 10 December 2020, 14:11 > Done Reply by qnguyen on 10 December 2020, 16:16 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 10 December 2020, 14:10 https://devapps.diality.us/cru/DG-DEN-5855-1#c6792 Do we need the conversion from U16 to S32? If we do, suggest combine into one line and remove roFlow variable. Reply by Dara Navaei on 10 December 2020, 14:40 > Done Reply by qnguyen on 10 December 2020, 16:15 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 10 December 2020, 14:12 https://devapps.diality.us/cru/DG-DEN-5855-1#c6794 Suggest combining into one line since there is no other use for variable isFlowOutOfRange. Reply by Dara Navaei on 10 December 2020, 14:41 > Done Reply by qnguyen on 10 December 2020, 16:15 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 07 December 2020, 10:14 https://devapps.diality.us/cru/DG-DEN-5855-1#c6676 If line needs to be commented, our process is to add a TODO as line comment so as not to forget to delete or modify this line down the road. Reply by Dara Navaei on 08 December 2020, 14:07 > Done Reply by pmontazemi on 09 December 2020, 11:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 10 December 2020, 14:49 https://devapps.diality.us/cru/DG-DEN-5855-1#c6802 We should allow MAX value here (1.0). Reply by Dara Navaei on 10 December 2020, 14:52 > Done Reply by qnguyen on 10 December 2020, 16:16 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-5855-1 https://devapps.diality.us/cru/DG-DEN-5855-1 Title: DG-DEN-5855_DG Drain Pump (2 of 2) Statement of Objectives: State: Closed Summary: Author: Dara Navaei Reviewers: (0 active, 3 completed*) qnguyen (*) Sean Nash (*) pmontazemi (*)