This is a list of all comments for DG-DEN-6402-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Services/Reservoirs.c Revision Comment by pmontazemi on 11 February 2021, 10:04 https://devapps.diality.us/cru/DG-DEN-6402-1#c7901 Remove extra "/" from comment. Reply by qnguyen on 11 February 2021, 10:09 > This is doxygen style comment if not in the same line. Reply by pmontazemi on 11 February 2021, 10:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 11 February 2021, 10:04 https://devapps.diality.us/cru/DG-DEN-6402-1#c7902 Remove extra "/" from comment. Reply by qnguyen on 11 February 2021, 10:09 > This is doxygen style comment if not in the same line. Reply by pmontazemi on 11 February 2021, 10:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 11 February 2021, 10:04 https://devapps.diality.us/cru/DG-DEN-6402-1#c7903 Remove extra "/" from comment. Reply by Sean Nash on 11 February 2021, 10:05 > Doxygen needs /// (previous line) or ///< (same line) or it > won't pick up the comment. Reply by pmontazemi on 11 February 2021, 10:55 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Reservoirs.h Revision Comment by Sean Nash on 10 February 2021, 23:14 https://devapps.diality.us/cru/DG-DEN-6402-1#c7857 Should this be moved to common\DG_Defs.h? I see HD has same typedef. Reply by qnguyen on 11 February 2021, 00:47 > Agreed. Moved to DGDefs.h Reply by Sean Nash on 11 February 2021, 10:52 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ROPump.c Revision Comment by Dara Navaei on 11 February 2021, 08:55 https://devapps.diality.us/cru/DG-DEN-6402-1#c7860 Are we planning to provide pressure in float? Like 30.25 psi? Reply by qnguyen on 11 February 2021, 09:13 > In verify state, we change the target pressure with the > average pressure over the verify period. That pressure value > is in float. > For RO pump publish data, the data type for target pressure > is float as well. Reply by Dara Navaei on 11 February 2021, 10:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 11 February 2021, 09:02 https://devapps.diality.us/cru/DG-DEN-6402-1#c7861 Do we need any persistent alarm after a couple of trials? How many times are you planning to go back and forth? Reply by qnguyen on 11 February 2021, 09:40 > Added maximum 5 times retry before alarm. Reply by Sean Nash on 11 February 2021, 10:16 > May be moot after change to flow control. Reply by Dara Navaei on 11 February 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 11 February 2021, 10:00 https://devapps.diality.us/cru/DG-DEN-6402-1#c7895 Remove "to" from sentence. Reply by qnguyen on 11 February 2021, 10:03 > Removed. Reply by pmontazemi on 11 February 2021, 10:57 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeDrain.c Revision Comment by Dara Navaei on 11 February 2021, 09:06 https://devapps.diality.us/cru/DG-DEN-6402-1#c7863 Why the RPM has been lowered? Reply by qnguyen on 11 February 2021, 09:41 > SYS team would like to lower down RPM since there is a risk > of making the drain pump goes bad at 2800 RPM. Reply by Sean Nash on 11 February 2021, 10:08 > Yes, if drain pump is pumping too fast, the combined flow > from drain pump and from RO pump is too much for the length > of tubing after the two merge and go to drain. Starts to > cavitate and causes a lot of turbulence. Reply by Dara Navaei on 11 February 2021, 10:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 11 February 2021, 09:10 https://devapps.diality.us/cru/DG-DEN-6402-1#c7865 For the time that the RO pump is switching between ramp and stabilize to get to the flow, the drain continues, does that affect the drain process? Reply by qnguyen on 11 February 2021, 09:50 > We have slowed down the RO pump flow rate for drain mode. so > the RO pump flow rate should not affect the drain process > much. Reply by Dara Navaei on 11 February 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeRecirculate.c Revision Comment by pmontazemi on 11 February 2021, 10:03 https://devapps.diality.us/cru/DG-DEN-6402-1#c7900 Is this initial RO pump target flow rate or this is fixed? If initial, we should indicate as this will be set by controller to achieve desired flow rate. Reply by qnguyen on 11 February 2021, 10:11 > This is our desired flow rate that we would like the > controller to achieve. Reply by pmontazemi on 11 February 2021, 10:57 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/LoadCell.c Revision Comment by pmontazemi on 11 February 2021, 09:54 https://devapps.diality.us/cru/DG-DEN-6402-1#c7891 Do we finish comments with "." or not? Let us be consistent throughout. Reply by qnguyen on 11 February 2021, 10:01 > Fixed. Elements inside typedef will not finish with ".". Reply by pmontazemi on 11 February 2021, 10:58 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeFill.c Revision Comment by pmontazemi on 11 February 2021, 10:01 https://devapps.diality.us/cru/DG-DEN-6402-1#c7897 Remove extra "/" in comment. Reply by qnguyen on 11 February 2021, 10:02 > This is a doxygen style comment. Not a general code comment. Reply by pmontazemi on 11 February 2021, 10:57 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: AlarmDefs.h Revision Comment by Sean Nash on 17 February 2021, 09:24 https://devapps.diality.us/cru/DG-DEN-6402-1#c8161 Need to add these to end of alarm table below. Reply by qnguyen on 17 February 2021, 10:00 > This should be part of Non-Volatile Data Management code > review. > [~dnavaei] has added them to the alarm table. Reply by Sean Nash on 17 February 2021, 10:36 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: DGDefs.h Revision Comment by pmontazemi on 11 February 2021, 09:52 https://devapps.diality.us/cru/DG-DEN-6402-1#c7888 I would imagine there will be many more valve settings for the various modes/states and sub-modes/sub-states. Reply by qnguyen on 11 February 2021, 09:57 > Those valve settings might not be of interest to the HD and > should stay local to DG. > For now, HD is interest in these two valve settings for wet > self-tests. Reply by pmontazemi on 11 February 2021, 10:59 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 February 2021, 09:25 https://devapps.diality.us/cru/DG-DEN-6402-1#c8162 Why commented out? Reply by qnguyen on 17 February 2021, 10:02 > These have been uncommented. Reply by Sean Nash on 17 February 2021, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 11 February 2021, 09:53 https://devapps.diality.us/cru/DG-DEN-6402-1#c7889 Isn't this already defined somewhere else? It seems redundant. Reply by qnguyen on 11 February 2021, 09:56 > These have been defined both in DG and HD before and now > moved here as a common define. Reply by pmontazemi on 11 February 2021, 10:59 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-6402-1 https://devapps.diality.us/cru/DG-DEN-6402-1 Title: DG-DEN-6402_HD Dev Pretreatment Self Tests Statement of Objectives: State: Closed Summary: Author: qnguyen Reviewers: (1 active, 2 completed*) Sean Nash (*) Dara Navaei (*) pmontazemi