This is a list of all comments for DG-DEN-7568-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Modes/ModeFlush.c Revision Comment by qnguyen on 21 April 2021, 12:19 https://devapps.diality.us/cru/DG-DEN-7568-1#c9546 Check and remove unused include (ConductivitySensors.h, TemperatureSensors.h, etc.) Reply by Dara Navaei on 23 April 2021, 08:43 > Done. Reply by qnguyen on 23 April 2021, 10:33 > RESOLVED in Revision Comment by qnguyen on 21 April 2021, 12:17 https://devapps.diality.us/cru/DG-DEN-7568-1#c9545 Remove unused defines Reply by Dara Navaei on 23 April 2021, 08:57 > Done. Reply by qnguyen on 23 April 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 April 2021, 22:43 https://devapps.diality.us/cru/DG-DEN-7568-1#c9300 I think this is actually a time stamp for when flush mode began - and you are using it to calculate total flush mode elapsed time. I would add the word "Start" to the end of the variable name and clarify the comment. stateTimer below is likely also a starting time - so same comment. Reply by Dara Navaei on 19 April 2021, 09:09 > Done. Reply by Sean Nash on 22 April 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 April 2021, 23:04 https://devapps.diality.us/cru/DG-DEN-7568-1#c9306 Recommend making this variable name a little more descripting. Something like alarmDetectedPendingTrigger. Reply by Dara Navaei on 19 April 2021, 09:08 > Done. Reply by Sean Nash on 22 April 2021, 10:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 April 2021, 23:02 https://devapps.diality.us/cru/DG-DEN-7568-1#c9305 This function has no inputs. It only sets variables. It never references any. Reply by Dara Navaei on 19 April 2021, 09:11 > These are the inputs that are changed in this function. Is > this not right? Reply by Sean Nash on 19 April 2021, 09:16 > Inputs are data looked at in this function (e.g. in an if > statement). Outputs are data that are changed in this > function. Reply by Dara Navaei on 19 April 2021, 09:23 > Done. Reply by Sean Nash on 22 April 2021, 10:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 21 April 2021, 12:15 https://devapps.diality.us/cru/DG-DEN-7568-1#c9542 Align or remove extra space. Reply by Dara Navaei on 23 April 2021, 08:58 > Done. Reply by qnguyen on 23 April 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 April 2021, 23:00 https://devapps.diality.us/cru/DG-DEN-7568-1#c9304 Not necessary. Standby mode is going to set actuators on entry 50ms from here so why bother. Reply by Dara Navaei on 19 April 2021, 09:10 > Done. Reply by Sean Nash on 22 April 2021, 10:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 April 2021, 22:52 https://devapps.diality.us/cru/DG-DEN-7568-1#c9301 Is this function something we can have just once somewhere? I feel like this same function is going to be in both disinfect modes and this flush mode. Reply by Dara Navaei on 14 April 2021, 23:06 > Yes, the exact same function is used in heat disinfect and > one will be used in chemical disinfect. Where do you suggest > we should put them? Reply by Sean Nash on 14 April 2021, 23:16 > So this function looks a lot like the transition function > in standby mode. So maybe standby mode could have a public > function that resets all of the actuators and then the > transition function in standby mode could call that > function (and your modes could also call that function > instead of this one). Reply by Dara Navaei on 24 April 2021, 11:04 > Moved this function to the FaultMode.c. Reply by Sean Nash on 26 April 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 21 April 2021, 16:23 https://devapps.diality.us/cru/DG-DEN-7568-1#c9552 Only two places call this function. Should we inline and remove this function? Reply by Dara Navaei on 23 April 2021, 08:42 > This code is very similar to heat disinfect, so I decided to > keep this function. Reply by qnguyen on 23 April 2021, 10:34 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 April 2021, 23:11 https://devapps.diality.us/cru/DG-DEN-7568-1#c9308 I think "full" should be changed to target volume reached. Review function header descriptions for any updates needed. Reply by Dara Navaei on 19 April 2021, 09:14 > Done. Reply by Sean Nash on 22 April 2021, 10:32 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by qnguyen on 21 April 2021, 12:12 https://devapps.diality.us/cru/DG-DEN-7568-1#c9541 Should result be set the the return of startDGFlush() and stopDGFlush() functions? Reply by Dara Navaei on 23 April 2021, 10:09 > Yes, this was on hold. I updated it. Reply by qnguyen on 23 April 2021, 10:34 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ROPump.c Revision Comment by Sean Nash on 14 April 2021, 22:34 https://devapps.diality.us/cru/DG-DEN-7568-1#c9297 Didn't these RO pump control changes happen a long time ago? Why are they showing up here as new changes? Reply by Dara Navaei on 19 April 2021, 09:02 > I copied and pasted this module from heat disinfect. Reply by Sean Nash on 22 April 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 April 2021, 22:36 https://devapps.diality.us/cru/DG-DEN-7568-1#c9299 Why did this change so much? This is for the initial PWM estimate, right? Reply by Dara Navaei on 19 April 2021, 09:05 > This is for the initial estimation. This has changed during > the revisit of the RO pump module. Reply by Sean Nash on 22 April 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 21 April 2021, 12:00 https://devapps.diality.us/cru/DG-DEN-7568-1#c9538 Revert the deleted comment. Reply by Dara Navaei on 22 April 2021, 09:32 > Done. Reply by qnguyen on 23 April 2021, 10:36 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeStandby.c Revision Comment by qnguyen on 21 April 2021, 12:05 https://devapps.diality.us/cru/DG-DEN-7568-1#c9539 DG_MODE_SOLO does not have DG_STANDBY_MODE_STATE_IDLE. Reply by Dara Navaei on 23 April 2021, 10:11 > I re-ordered the logic. Reply by qnguyen on 23 April 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Reservoirs.c Revision Comment by qnguyen on 21 April 2021, 12:11 https://devapps.diality.us/cru/DG-DEN-7568-1#c9540 Update to latest revision. This function should be deleted. Reply by Dara Navaei on 23 April 2021, 10:16 > Done. Reply by qnguyen on 23 April 2021, 10:34 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DrainPump.c Revision Comment by Sean Nash on 14 April 2021, 22:35 https://devapps.diality.us/cru/DG-DEN-7568-1#c9298 When do you think we will have something for this? I thought I heard Blaine say he thought we could get RPM conversion close enough to re-implement check. Reply by Dara Navaei on 19 October 2023, 08:06 > RESOLVED in CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Controllers/TemperatureSensors.h Revision Comment by qnguyen on 25 April 2021, 22:36 https://devapps.diality.us/cru/DG-DEN-7568-1#c9606 This struct needs doxygen comment on individual member. Reply by Dara Navaei on 26 April 2021, 10:16 > Done. Reply by qnguyen on 26 April 2021, 10:32 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-7568-1 https://devapps.diality.us/cru/DG-DEN-7568-1 Title: DG-DEN-7568_DG Mode Flush Statement of Objectives: State: Closed Summary: Author: Dara Navaei Reviewers: (3 active, 0 completed*) qnguyen Sean Nash pmontazemi