This is a list of all comments for DG-DEN-12224-7. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/ConcentratePumps.c Revision Comment by Sean Nash on 30 March 2022, 13:29 https://devapps.diality.us/cru/DG-DEN-12224-7#c12720 Remove blank line. Reply by Dara Navaei on 23 May 2022, 14:40 > Done. Reply by Sean Nash on 24 May 2022, 16:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 March 2022, 13:34 https://devapps.diality.us/cru/DG-DEN-12224-7#c12721 Do we still need this? Reply by Dara Navaei on 23 May 2022, 14:40 > Yes we will further test this in DVT. Reply by Sean Nash on 24 May 2022, 16:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 March 2022, 13:34 https://devapps.diality.us/cru/DG-DEN-12224-7#c12722 Clear alarm condition, not alarm. Reply by Dara Navaei on 23 May 2022, 14:39 > Done. Reply by Sean Nash on 24 May 2022, 16:53 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ConcentratePumps.h Revision Comment by Sean Nash on 21 May 2022, 23:01 https://devapps.diality.us/cru/DG-DEN-12224-7#c12852 Add doxygen comments. Reply by Dara Navaei on 23 May 2022, 14:38 > Done. Reply by Sean Nash on 24 May 2022, 16:50 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ConductivitySensors.c Revision Comment by Sean Nash on 21 May 2022, 23:07 https://devapps.diality.us/cru/DG-DEN-12224-7#c12853 Why not just increment in the if statement below? Reply by Dara Navaei on 23 May 2022, 14:37 > Done. Reply by Sean Nash on 24 May 2022, 16:50 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DialysateFlow.c Revision Comment by Sean Nash on 21 May 2022, 23:08 https://devapps.diality.us/cru/DG-DEN-12224-7#c12854 Remove blank line. Reply by Dara Navaei on 24 May 2022, 21:08 > Done. Reply by Sean Nash on 25 May 2022, 10:20 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 May 2022, 23:09 https://devapps.diality.us/cru/DG-DEN-12224-7#c12855 add another blank line here. Reply by Dara Navaei on 23 May 2022, 14:36 > Done. Reply by Sean Nash on 24 May 2022, 16:50 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Heaters.c Revision Comment by Sean Nash on 21 May 2022, 23:20 https://devapps.diality.us/cru/DG-DEN-12224-7#c12861 No longer need these new fields - just fault on low flow now. Reply by Dara Navaei on 23 May 2022, 14:20 > The fault has been implemented. As soon as all of the testing > is done, I will remove this code. Reply by Sean Nash on 24 May 2022, 16:48 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 May 2022, 23:14 https://devapps.diality.us/cru/DG-DEN-12224-7#c12858 Why aren't all fields initialized here? Reply by Dara Navaei on 23 May 2022, 14:30 > I added another item to be initialized but some of them are > related to turning of the heater when there is no flow that > are removed. Reply by Sean Nash on 24 May 2022, 16:49 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 May 2022, 23:16 https://devapps.diality.us/cru/DG-DEN-12224-7#c12859 Can we finalize this fault now? Reply by Dara Navaei on 23 May 2022, 14:34 > Done. Reply by Sean Nash on 24 May 2022, 16:49 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 May 2022, 23:18 https://devapps.diality.us/cru/DG-DEN-12224-7#c12860 If we're going to fault on low flow, no longer need this. Reply by Dara Navaei on 23 May 2022, 14:20 > Done. Reply by Sean Nash on 24 May 2022, 16:49 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Pressures.c Revision Comment by Sean Nash on 21 May 2022, 23:22 https://devapps.diality.us/cru/DG-DEN-12224-7#c12862 Remove blank line. Reply by Dara Navaei on 24 May 2022, 21:07 > Done. Reply by Sean Nash on 25 May 2022, 10:20 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 May 2022, 23:22 https://devapps.diality.us/cru/DG-DEN-12224-7#c12863 Remove blank line. Reply by Dara Navaei on 23 May 2022, 14:19 > Done. Reply by Sean Nash on 24 May 2022, 16:48 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ROPump.c Revision Comment by Sean Nash on 21 May 2022, 23:24 https://devapps.diality.us/cru/DG-DEN-12224-7#c12864 Can we put all of the local declarations at the top of the function? Reply by Dara Navaei on 24 May 2022, 21:06 > Done. Reply by Sean Nash on 25 May 2022, 10:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 May 2022, 23:26 https://devapps.diality.us/cru/DG-DEN-12224-7#c12865 Does this volume ever get read from NV memory at startup? Reply by Dara Navaei on 23 May 2022, 14:18 > Yes it is one of the POST states of the NV data management. Reply by Sean Nash on 24 May 2022, 16:47 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 24 May 2022, 16:28 https://devapps.diality.us/cru/DG-DEN-12224-7#c13011 Remove blank line. Reply by Dara Navaei on 24 May 2022, 21:00 > Done. Reply by Sean Nash on 25 May 2022, 10:17 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 24 May 2022, 16:29 https://devapps.diality.us/cru/DG-DEN-12224-7#c13012 Why is this var declared in the if and all others are at top of function? Reply by Dara Navaei on 24 May 2022, 21:01 > The idea was to call these variables once it is time to do > the control but it was abandoned. Reply by Sean Nash on 25 May 2022, 10:17 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeChemicalDisinfect.c Revision Comment by Sean Nash on 21 May 2022, 23:36 https://devapps.diality.us/cru/DG-DEN-12224-7#c12870 Can we put all local declarations at top of function? Reply by Dara Navaei on 23 May 2022, 14:15 > Done. Reply by Sean Nash on 24 May 2022, 16:46 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeDrain.c Revision Comment by Sean Nash on 18 May 2022, 15:18 https://devapps.diality.us/cru/DG-DEN-12224-7#c12819 Why not initialize this variable? Why removed? If removing rinse state, remove variable and state entirely. Reply by Dara Navaei on 23 May 2022, 14:39 > This variable cannot be initialized here, otherwise it will > set the flag to FALSE regardless of the user's request. Reply by Sean Nash on 24 May 2022, 16:52 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeFill.c Revision Comment by Sean Nash on 21 May 2022, 23:40 https://devapps.diality.us/cru/DG-DEN-12224-7#c12871 Can't we just pass a zero in function param list below? Why need a local variable? Reply by Dara Navaei on 23 May 2022, 14:11 > When the NV record data is requested, the number of sensors > to be checked has to be provided for error check. But this is > not a sensor so 0 is passed. I defined a variable for it to > be readable. Reply by Sean Nash on 24 May 2022, 16:59 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 May 2022, 23:42 https://devapps.diality.us/cru/DG-DEN-12224-7#c12873 Need an else so volume isn't undefined if not acid or bicarb. Should probably also trigger a s/w fault. Reply by Dara Navaei on 24 May 2022, 16:50 > Done. Reply by Sean Nash on 24 May 2022, 16:56 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 May 2022, 23:43 https://devapps.diality.us/cru/DG-DEN-12224-7#c12874 Should we s/w fault if not a valid bottle? Reply by Dara Navaei on 24 May 2022, 16:50 > Done. Reply by Sean Nash on 24 May 2022, 16:56 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 May 2022, 23:44 https://devapps.diality.us/cru/DG-DEN-12224-7#c12875 Add a blank line between declarations and code. Reply by Dara Navaei on 23 May 2022, 14:08 > Done. Reply by Sean Nash on 24 May 2022, 16:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 May 2022, 23:46 https://devapps.diality.us/cru/DG-DEN-12224-7#c12876 Is there a rationale for these blank lines in random places? Reply by Dara Navaei on 23 May 2022, 14:07 > Removed the blank lines. Reply by Sean Nash on 24 May 2022, 16:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 May 2022, 23:46 https://devapps.diality.us/cru/DG-DEN-12224-7#c12877 Add another blank line. Reply by Dara Navaei on 23 May 2022, 14:06 > Done. Reply by Sean Nash on 24 May 2022, 16:42 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeGenIdle.c Revision Comment by Sean Nash on 21 May 2022, 23:53 https://devapps.diality.us/cru/DG-DEN-12224-7#c12879 Should we initialize hdMode here since don't want to initialize in init function? Reply by Dara Navaei on 23 May 2022, 14:03 > Done. Reply by Sean Nash on 24 May 2022, 16:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 May 2022, 23:49 https://devapps.diality.us/cru/DG-DEN-12224-7#c12878 Makes me a little nervous that hdMode is never initialized. Reply by Dara Navaei on 23 May 2022, 14:05 > hdMode is initialized now. Reply by Sean Nash on 24 May 2022, 16:42 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeHeatDisinfect.c Revision Comment by Sean Nash on 22 May 2022, 00:02 https://devapps.diality.us/cru/DG-DEN-12224-7#c12880 Can local declarations be moved to top of function? Reply by Dara Navaei on 23 May 2022, 14:01 > Done. Reply by Sean Nash on 24 May 2022, 16:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 May 2022, 00:04 https://devapps.diality.us/cru/DG-DEN-12224-7#c12881 Add blank line between declarations and code. Reply by Dara Navaei on 23 May 2022, 14:00 > Done. Reply by Sean Nash on 24 May 2022, 16:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 May 2022, 00:06 https://devapps.diality.us/cru/DG-DEN-12224-7#c12883 Put declarations at top of scope. Reply by Dara Navaei on 23 May 2022, 13:59 > Done. Reply by Sean Nash on 24 May 2022, 16:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 May 2022, 00:07 https://devapps.diality.us/cru/DG-DEN-12224-7#c12884 Can we change "r" to "reservoir"? Reply by Dara Navaei on 23 May 2022, 13:57 > Done. Reply by Sean Nash on 24 May 2022, 16:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 May 2022, 00:07 https://devapps.diality.us/cru/DG-DEN-12224-7#c12885 Swap if order. Reply by Dara Navaei on 23 May 2022, 13:56 > Done. Reply by Sean Nash on 24 May 2022, 16:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 May 2022, 00:08 https://devapps.diality.us/cru/DG-DEN-12224-7#c12886 Blank line between declarations and code. Reply by Dara Navaei on 23 May 2022, 13:56 > Done. Reply by Sean Nash on 24 May 2022, 16:40 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Reservoirs.c Revision Comment by Sean Nash on 22 May 2022, 00:21 https://devapps.diality.us/cru/DG-DEN-12224-7#c12892 Remove blank line. Reply by Dara Navaei on 23 May 2022, 13:42 > Done. Reply by Sean Nash on 24 May 2022, 16:33 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemComm.c Revision Comment by Sean Nash on 22 May 2022, 00:22 https://devapps.diality.us/cru/DG-DEN-12224-7#c12893 Add another blank line here. Reply by Dara Navaei on 23 May 2022, 13:41 > Done Reply by Sean Nash on 24 May 2022, 16:32 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Sean Nash on 22 May 2022, 00:23 https://devapps.diality.us/cru/DG-DEN-12224-7#c12894 Add another blank line here. Reply by Dara Navaei on 23 May 2022, 13:40 > Done. Reply by Sean Nash on 24 May 2022, 16:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 May 2022, 00:25 https://devapps.diality.us/cru/DG-DEN-12224-7#c12895 We have a banner above separating system message helpers from Dialin message helpers. Should only be Dialin message helpers below that banner. I think there are a lot of these in the wrong place. Reply by Dara Navaei on 25 May 2022, 09:43 > Done. Reply by Sean Nash on 25 May 2022, 10:19 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Switches.c Revision Comment by Sean Nash on 21 May 2022, 23:28 https://devapps.diality.us/cru/DG-DEN-12224-7#c12867 Remove extra blank line. Reply by Dara Navaei on 24 May 2022, 21:05 > Done. Reply by Sean Nash on 25 May 2022, 10:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 May 2022, 23:28 https://devapps.diality.us/cru/DG-DEN-12224-7#c12868 Remove blank line. Reply by Dara Navaei on 23 May 2022, 14:15 > Done. Reply by Sean Nash on 24 May 2022, 16:46 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Switches.h Revision Comment by Sean Nash on 21 May 2022, 23:27 https://devapps.diality.us/cru/DG-DEN-12224-7#c12866 Should be NUM_OF_SWITCHES based on enum name and type. Also, no doors - fix comment. Reply by Dara Navaei on 23 May 2022, 14:17 > They should not be switches. They are all caps. Reply by Sean Nash on 24 May 2022, 16:58 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/FPGA.c Revision Comment by Sean Nash on 22 May 2022, 00:19 https://devapps.diality.us/cru/DG-DEN-12224-7#c12891 Do we still need this build switch? Reply by Dara Navaei on 23 May 2022, 13:43 > This is one the very few build switches in case we need to > isolate the firmware to just an eval board. Reply by Sean Nash on 24 May 2022, 16:56 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeStandby.c Revision Comment by Sean Nash on 22 May 2022, 00:10 https://devapps.diality.us/cru/DG-DEN-12224-7#c12887 Put declarations at top of scope. Reply by Dara Navaei on 23 May 2022, 13:53 > Done. Reply by Sean Nash on 24 May 2022, 16:39 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/FluidLeak.c Revision Comment by Sean Nash on 21 May 2022, 23:12 https://devapps.diality.us/cru/DG-DEN-12224-7#c12857 Remove extra blank line. Reply by Dara Navaei on 23 May 2022, 14:35 > Done. Reply by Sean Nash on 24 May 2022, 16:49 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/FluidLeak.h Revision Comment by Sean Nash on 21 May 2022, 23:11 https://devapps.diality.us/cru/DG-DEN-12224-7#c12856 Remove extra blank line. Reply by Dara Navaei on 23 May 2022, 14:35 > Done. Reply by Sean Nash on 24 May 2022, 16:49 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Tasks/TaskGeneral.c Revision Comment by Sean Nash on 22 May 2022, 00:15 https://devapps.diality.us/cru/DG-DEN-12224-7#c12890 It doesn't look like we are following this order - looks like controllers are before modes. Reply by Dara Navaei on 23 May 2022, 13:48 > I reordered it. Some of the controllers also do the > monitoring as well since we are not planning to use the > priority task rate for all the monitors. Reply by Sean Nash on 24 May 2022, 16:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Tasks/TaskPriority.c Revision Comment by Sean Nash on 22 May 2022, 00:13 https://devapps.diality.us/cru/DG-DEN-12224-7#c12889 Remove blank line. Reply by Dara Navaei on 23 May 2022, 13:50 > Done. Reply by Sean Nash on 24 May 2022, 16:36 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/OperationModes.c Revision Comment by Sean Nash on 22 May 2022, 00:12 https://devapps.diality.us/cru/DG-DEN-12224-7#c12888 Is this the best place to do this? Reply by Dara Navaei on 23 May 2022, 13:51 > This is the place that the RO water usage can be updated with > only one function call. Reply by Sean Nash on 24 May 2022, 16:39 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-12224-7 https://devapps.diality.us/cru/DG-DEN-12224-7 Title: DG-DEN-12224_DG HD Dev Switches Monitor Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (1 active, 2 completed*) Sean Nash (*) Michael Garthwaite (*) Darren Cox