This is a list of all comments for DG-DEN-14316-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Modes/ModeChemicalDisinfect.c Revision Comment by wbracken on 21 November 2022, 11:04 https://devapps.diality.us/cru/DG-DEN-14316-1#c14878 Alignment Reply by wbracken on 20 December 2022, 18:28 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 11:05 https://devapps.diality.us/cru/DG-DEN-14316-1#c14879 Remove comment. Reply by wbracken on 20 December 2022, 18:42 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 11:05 https://devapps.diality.us/cru/DG-DEN-14316-1#c14880 Alignment Reply by Steve Jarpe on 20 December 2022, 11:57 > Fixed Reply by wbracken on 20 December 2022, 13:30 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 11:05 https://devapps.diality.us/cru/DG-DEN-14316-1#c14881 Alignment Reply by wbracken on 21 December 2022, 11:21 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 11:06 https://devapps.diality.us/cru/DG-DEN-14316-1#c14882 Alignment Reply by wbracken on 20 December 2022, 18:43 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 11:07 https://devapps.diality.us/cru/DG-DEN-14316-1#c14883 Doxygen descriptions. Reply by wbracken on 20 December 2022, 18:31 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 December 2022, 08:05 https://devapps.diality.us/cru/DG-DEN-14316-1#c15603 Where is UI state used? Reply by Steve Jarpe on 21 December 2022, 09:18 > The UI state is set to > CHEM_DISINFECT_UI_STATE_DISINFECT_DEVICE when the > disinfectant is being added to the reservoirs. It is one of > the logged parameters. This was in the original chem > disinfect code. Reply by Sean Nash on 21 December 2022, 09:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 11:11 https://devapps.diality.us/cru/DG-DEN-14316-1#c14884 General comment on alignment. Reply by wbracken on 20 December 2022, 18:32 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 20 December 2022, 17:08 https://devapps.diality.us/cru/DG-DEN-14316-1#c15553 R1FullVolume and R2FullVolume are set here but aren't referenced. Reply by Steve Jarpe on 20 December 2022, 17:17 > Actually they are used. Reply by wbracken on 20 December 2022, 17:44 > Just pulled your latest branches and get the following > warnings when I build: > Description Resource Path Location Type > #179-D function "dequeue" was declared but never referenced > NVDataMgmt.c /DG/FWCommon line 2484 C/C++ Problem > #179-D function "setFillInfoToRTCRAM" was declared but > never referenced ModeFill.c /DG/App/Modes line 997 C/C++ > Problem > #552-D variable "chemDisinfectFlushUIState" was set but > never used ModeChemicalDisinfectFlush.c /DG/App/Modes line > 115 C/C++ Problem > #552-D variable "rsrvrFillStableTimeCounter" was set but > never used ModeChemicalDisinfectFlush.c /DG/App/Modes line > 127 C/C++ Problem > #552-D variable "R1ChemDisinfectVol" was set but never used > ModeChemicalDisinfect.c /DG/App/Modes line 171 C/C++ > Problem > #552-D variable "R2ChemDisinfectVol" was set but never used > ModeChemicalDisinfect.c /DG/App/Modes line 172 C/C++ > Problem Reply by Steve Jarpe on 21 December 2022, 09:21 > R1ChemDisinfectVol and "R2ChemDisinfectVol have been > removed from ModeChemicalDisinfect.c Reply by wbracken on 21 December 2022, 11:28 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 11:12 https://devapps.diality.us/cru/DG-DEN-14316-1#c14885 Remove comment. Reply by wbracken on 21 December 2022, 11:43 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 11:16 https://devapps.diality.us/cru/DG-DEN-14316-1#c14886 Remove space Reply by Steve Jarpe on 20 December 2022, 17:01 > I can't find this. Reply by wbracken on 20 December 2022, 17:11 > Before the beginning /* > > Sean asked me to update your branch. I have modifications > to 3 files which I believe address most of my comments. I > can push my changes if that helps. Reply by Steve Jarpe on 20 December 2022, 17:17 > OK I see the space. Reply by wbracken on 20 December 2022, 18:45 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:17 https://devapps.diality.us/cru/DG-DEN-14316-1#c14896 Alignment Reply by wbracken on 20 December 2022, 18:33 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 20 December 2022, 13:37 https://devapps.diality.us/cru/DG-DEN-14316-1#c15455 Remove TODO and extra blank lines. Reply by Steve Jarpe on 20 December 2022, 15:30 > Fixed Reply by wbracken on 20 December 2022, 17:13 > I'll assume this has been fixed but not pushed since it > still shows up here. Reply by wbracken on 20 December 2022, 18:00 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:17 https://devapps.diality.us/cru/DG-DEN-14316-1#c14898 Alignment Reply by wbracken on 20 December 2022, 13:38 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 11:20 https://devapps.diality.us/cru/DG-DEN-14316-1#c14887 Alignment Reply by wbracken on 20 December 2022, 18:34 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 11:22 https://devapps.diality.us/cru/DG-DEN-14316-1#c14888 Indent Reply by wbracken on 20 December 2022, 18:35 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 11:25 https://devapps.diality.us/cru/DG-DEN-14316-1#c14890 Alignment Reply by wbracken on 20 December 2022, 13:40 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:32 https://devapps.diality.us/cru/DG-DEN-14316-1#c15334 Can these 2 declarations be moved up with the others (above "ifndef_RELEASE_")? Reply by Sean Nash on 20 December 2022, 13:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 11:25 https://devapps.diality.us/cru/DG-DEN-14316-1#c14891 Alignment Reply by wbracken on 20 December 2022, 13:40 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 11:24 https://devapps.diality.us/cru/DG-DEN-14316-1#c14889 Indent Reply by wbracken on 20 December 2022, 13:39 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:19 https://devapps.diality.us/cru/DG-DEN-14316-1#c14899 Alignment Reply by wbracken on 20 December 2022, 13:40 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:21 https://devapps.diality.us/cru/DG-DEN-14316-1#c14900 Alignment. Reply by Dara Navaei on 18 October 2023, 21:02 > RESOLVED IN CODE WALKTHROUGH Revision Comment by Sean Nash on 19 December 2022, 16:34 https://devapps.diality.us/cru/DG-DEN-14316-1#c15335 Add blank line between declarations and code. Reply by Sean Nash on 20 December 2022, 13:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:35 https://devapps.diality.us/cru/DG-DEN-14316-1#c15336 Body start bracket indented too far. Reply by Steve Jarpe on 20 December 2022, 06:26 > I can't see this in Code Composer Studio. Reply by Sean Nash on 20 December 2022, 09:43 > I searched this file for tabs and replaced them with 4 > spaces. Please check your editor tab settings. > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 20 December 2022, 13:42 https://devapps.diality.us/cru/DG-DEN-14316-1#c15467 Remove blank line. Reply by Steve Jarpe on 20 December 2022, 16:57 > removed Reply by wbracken on 20 December 2022, 18:01 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:34 https://devapps.diality.us/cru/DG-DEN-14316-1#c14907 Remove commented code. Reply by wbracken on 20 December 2022, 18:36 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:35 https://devapps.diality.us/cru/DG-DEN-14316-1#c14909 Remove. Reply by wbracken on 20 December 2022, 18:37 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:36 https://devapps.diality.us/cru/DG-DEN-14316-1#c14910 Alignment Reply by wbracken on 20 December 2022, 18:37 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:38 https://devapps.diality.us/cru/DG-DEN-14316-1#c14912 Alignment Reply by wbracken on 20 December 2022, 18:22 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:37 https://devapps.diality.us/cru/DG-DEN-14316-1#c15340 Align "="s. Reply by Sean Nash on 20 December 2022, 13:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 20 December 2022, 13:45 https://devapps.diality.us/cru/DG-DEN-14316-1#c15479 Remove blank line. Reply by Steve Jarpe on 20 December 2022, 16:57 > removed Reply by wbracken on 20 December 2022, 18:01 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:39 https://devapps.diality.us/cru/DG-DEN-14316-1#c14913 Alignment Reply by wbracken on 20 December 2022, 14:28 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:40 https://devapps.diality.us/cru/DG-DEN-14316-1#c14914 Alignment Reply by wbracken on 20 December 2022, 14:28 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:40 https://devapps.diality.us/cru/DG-DEN-14316-1#c14915 Alignment Reply by wbracken on 20 December 2022, 18:22 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:36 https://devapps.diality.us/cru/DG-DEN-14316-1#c15339 Check indent. Reply by Steve Jarpe on 20 December 2022, 06:27 > I can't see this in Code Composer Studio. Reply by Sean Nash on 20 December 2022, 09:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:34 https://devapps.diality.us/cru/DG-DEN-14316-1#c14906 Alignment Reply by wbracken on 20 December 2022, 14:29 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:39 https://devapps.diality.us/cru/DG-DEN-14316-1#c15342 Alignment - why is "=" pushed out so far? Reply by Sean Nash on 20 December 2022, 13:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:40 https://devapps.diality.us/cru/DG-DEN-14316-1#c15343 Align "="s. Reply by Sean Nash on 20 December 2022, 13:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:35 https://devapps.diality.us/cru/DG-DEN-14316-1#c14908 Alignment Reply by wbracken on 20 December 2022, 13:45 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:41 https://devapps.diality.us/cru/DG-DEN-14316-1#c14917 Needs function header. Reply by wbracken on 20 December 2022, 18:25 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:42 https://devapps.diality.us/cru/DG-DEN-14316-1#c14918 Remove blank lines and commented out ode. Reply by wbracken on 20 December 2022, 18:26 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:38 https://devapps.diality.us/cru/DG-DEN-14316-1#c15341 Keep blank line before return statement. Reply by Sean Nash on 20 December 2022, 13:34 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:42 https://devapps.diality.us/cru/DG-DEN-14316-1#c14919 Alignment and commented out code. Reply by wbracken on 20 December 2022, 18:46 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:37 https://devapps.diality.us/cru/DG-DEN-14316-1#c14911 Alignment Reply by wbracken on 20 December 2022, 18:38 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:44 https://devapps.diality.us/cru/DG-DEN-14316-1#c14920 Alignment Reply by wbracken on 20 December 2022, 18:47 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 20 December 2022, 18:26 https://devapps.diality.us/cru/DG-DEN-14316-1#c15581 Check alignment of "=" throughout module. Reply by Steve Jarpe on 21 December 2022, 09:33 > I went through it again, and changed a number of them. Reply by wbracken on 21 December 2022, 11:44 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:41 https://devapps.diality.us/cru/DG-DEN-14316-1#c15344 Add blank line before return statement. Reply by Sean Nash on 20 December 2022, 13:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:41 https://devapps.diality.us/cru/DG-DEN-14316-1#c15345 Add blank line between declarations and code. Reply by Sean Nash on 20 December 2022, 13:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:42 https://devapps.diality.us/cru/DG-DEN-14316-1#c15346 Remove blank line. Reply by Sean Nash on 20 December 2022, 13:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:42 https://devapps.diality.us/cru/DG-DEN-14316-1#c15347 Remove blank line. Reply by Sean Nash on 20 December 2022, 13:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:43 https://devapps.diality.us/cru/DG-DEN-14316-1#c15348 Add blank line between declaration and code. Reply by Sean Nash on 20 December 2022, 13:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:43 https://devapps.diality.us/cru/DG-DEN-14316-1#c15349 Prefer local var declarations to be at top of scope where they're used. Reply by Steve Jarpe on 20 December 2022, 15:42 > The declarations have been moved higher, as in > ModeChemicalDisinfectFlush Reply by Steve Jarpe on 20 December 2022, 16:17 > Actually the only other place I could put them is at the > top of the function. Reply by Sean Nash on 21 December 2022, 08:08 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:44 https://devapps.diality.us/cru/DG-DEN-14316-1#c15350 Check indents. Reply by Sean Nash on 20 December 2022, 09:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:44 https://devapps.diality.us/cru/DG-DEN-14316-1#c15351 Align. Reply by Sean Nash on 20 December 2022, 09:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:45 https://devapps.diality.us/cru/DG-DEN-14316-1#c15353 Add blank line between declarations and code. Reply by Sean Nash on 20 December 2022, 13:29 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: DGDefs.h Revision Comment by Sean Nash on 19 December 2022, 15:38 https://devapps.diality.us/cru/DG-DEN-14316-1#c15284 Don't need two of these. Remove one. Reply by Steve Jarpe on 20 December 2022, 12:35 > removed Reply by Sean Nash on 20 December 2022, 13:57 > Still see two. Reply by Steve Jarpe on 20 December 2022, 16:11 > OK, now I see. I removed this one. There are others, > should I remove them also? Reply by Sean Nash on 20 December 2022, 17:06 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 15:44 https://devapps.diality.us/cru/DG-DEN-14316-1#c15294 Why are there two of these? Reply by Steve Jarpe on 20 December 2022, 12:35 > removed Reply by Sean Nash on 20 December 2022, 13:46 > I still see two /**@}*/. Reply by Steve Jarpe on 20 December 2022, 16:11 > Removed it this time. Reply by Sean Nash on 20 December 2022, 17:07 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 15:45 https://devapps.diality.us/cru/DG-DEN-14316-1#c15295 Why are there two of these? /**@}*/ Reply by Steve Jarpe on 20 December 2022, 16:14 > removed Reply by Sean Nash on 20 December 2022, 17:07 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeChemicalDisinfectFlush.c Revision Comment by Sean Nash on 19 December 2022, 16:09 https://devapps.diality.us/cru/DG-DEN-14316-1#c15311 DGChemicalDisinfectFlushMode Reply by Sean Nash on 20 December 2022, 13:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:47 https://devapps.diality.us/cru/DG-DEN-14316-1#c14922 Alignment Reply by wbracken on 20 December 2022, 18:18 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 20 December 2022, 17:09 https://devapps.diality.us/cru/DG-DEN-14316-1#c15555 Does not appear to be referenced. Reply by Steve Jarpe on 21 December 2022, 09:44 > chemDisinfectFlushUIState is set in a number of places, but > it was not set in the publish logging data function. I > corrected that. Reply by wbracken on 21 December 2022, 11:27 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 20 December 2022, 17:10 https://devapps.diality.us/cru/DG-DEN-14316-1#c15556 Does not appear to be used. Reply by Steve Jarpe on 21 December 2022, 09:46 > removed Reply by wbracken on 21 December 2022, 11:26 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:47 https://devapps.diality.us/cru/DG-DEN-14316-1#c14923 Remove Reply by wbracken on 20 December 2022, 17:49 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:48 https://devapps.diality.us/cru/DG-DEN-14316-1#c14924 Alignment Reply by wbracken on 20 December 2022, 17:50 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:48 https://devapps.diality.us/cru/DG-DEN-14316-1#c14925 Remove Reply by wbracken on 20 December 2022, 18:48 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:13 https://devapps.diality.us/cru/DG-DEN-14316-1#c15313 ..CHEM_DISINFECT_FLUSH_INVALID_EXEC_STATE Reply by Sean Nash on 20 December 2022, 13:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 30 November 2022, 14:53 https://devapps.diality.us/cru/DG-DEN-14316-1#c15083 Remove. Reply by wbracken on 20 December 2022, 15:26 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:16 https://devapps.diality.us/cru/DG-DEN-14316-1#c15315 This already called in transition into this mode. Reply by Sean Nash on 20 December 2022, 13:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:17 https://devapps.diality.us/cru/DG-DEN-14316-1#c15316 Align = or bring next to state. Reply by Sean Nash on 20 December 2022, 13:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:17 https://devapps.diality.us/cru/DG-DEN-14316-1#c15317 Align comment. Reply by Sean Nash on 20 December 2022, 09:58 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:17 https://devapps.diality.us/cru/DG-DEN-14316-1#c15318 Align. Reply by Sean Nash on 20 December 2022, 09:58 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:18 https://devapps.diality.us/cru/DG-DEN-14316-1#c15319 Align else body brackets. Reply by Sean Nash on 20 December 2022, 09:59 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 20 December 2022, 13:54 https://devapps.diality.us/cru/DG-DEN-14316-1#c15489 Remove blank line. Reply by Steve Jarpe on 20 December 2022, 16:40 > done Reply by wbracken on 20 December 2022, 18:04 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:50 https://devapps.diality.us/cru/DG-DEN-14316-1#c14926 Update header. Reply by wbracken on 21 December 2022, 11:41 > prevChemDisinfectFlushState is not used. Reply by Sean Nash on 21 December 2022, 12:52 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:18 https://devapps.diality.us/cru/DG-DEN-14316-1#c15321 Align. Reply by Sean Nash on 20 December 2022, 09:59 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:18 https://devapps.diality.us/cru/DG-DEN-14316-1#c15322 Align. Reply by Sean Nash on 20 December 2022, 09:59 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:50 https://devapps.diality.us/cru/DG-DEN-14316-1#c14927 Update header. Reply by Steve Jarpe on 21 December 2022, 09:48 > Could you be more specific? It looks correct to me. Reply by wbracken on 21 December 2022, 11:39 > drainTimer, numberOfPostDisinfectRinses, > rsrvFillFillToFullStableTimerCounter Reply by Sean Nash on 21 December 2022, 12:50 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:19 https://devapps.diality.us/cru/DG-DEN-14316-1#c15323 Remove extra blank line. Reply by Sean Nash on 20 December 2022, 13:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:55 https://devapps.diality.us/cru/DG-DEN-14316-1#c14937 Update header. Reply by Steve Jarpe on 21 December 2022, 09:49 > I don't see what needs to be updated. Reply by wbracken on 21 December 2022, 11:38 > drainTimer, waitTimer Reply by Sean Nash on 21 December 2022, 12:48 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 20 December 2022, 14:17 https://devapps.diality.us/cru/DG-DEN-14316-1#c15504 Remove blank line. Reply by Steve Jarpe on 20 December 2022, 16:44 > removed Reply by wbracken on 20 December 2022, 17:52 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:53 https://devapps.diality.us/cru/DG-DEN-14316-1#c14929 Should the pending alarm state be set here? Reply by wbracken on 20 December 2022, 13:56 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:19 https://devapps.diality.us/cru/DG-DEN-14316-1#c15324 Align. Reply by Sean Nash on 20 December 2022, 09:59 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:55 https://devapps.diality.us/cru/DG-DEN-14316-1#c14938 Set alarm pending here? Reply by wbracken on 20 December 2022, 13:57 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 20 December 2022, 13:59 https://devapps.diality.us/cru/DG-DEN-14316-1#c15493 Remove blank lines. Reply by Steve Jarpe on 20 December 2022, 16:47 > removed Reply by wbracken on 20 December 2022, 18:11 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 30 November 2022, 14:55 https://devapps.diality.us/cru/DG-DEN-14316-1#c15084 Alignment Reply by wbracken on 20 December 2022, 14:18 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:21 https://devapps.diality.us/cru/DG-DEN-14316-1#c15325 Align = or bring back to state. Reply by Sean Nash on 20 December 2022, 13:54 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 13:58 https://devapps.diality.us/cru/DG-DEN-14316-1#c14949 Alignment Reply by wbracken on 20 December 2022, 13:59 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:21 https://devapps.diality.us/cru/DG-DEN-14316-1#c15326 Remove blank line. Reply by Sean Nash on 20 December 2022, 13:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:22 https://devapps.diality.us/cru/DG-DEN-14316-1#c15327 Has this been decided yet? Reply by Steve Jarpe on 20 December 2022, 15:40 > removed Reply by Sean Nash on 20 December 2022, 17:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 14:00 https://devapps.diality.us/cru/DG-DEN-14316-1#c14951 Remove blank line Reply by wbracken on 20 December 2022, 18:48 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:22 https://devapps.diality.us/cru/DG-DEN-14316-1#c15328 Remove blank line. Reply by Sean Nash on 20 December 2022, 13:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 20 December 2022, 18:16 https://devapps.diality.us/cru/DG-DEN-14316-1#c15575 Is this needed for publishing data? Is this a future enhancement? Reply by Steve Jarpe on 21 December 2022, 09:52 > I don't know the answer to this question. Reply by Sean Nash on 21 December 2022, 09:56 > I don't understand the question. Appears to be publishing > data now. Reply by wbracken on 21 December 2022, 11:26 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:26 https://devapps.diality.us/cru/DG-DEN-14316-1#c15333 Prefer to have declarations of local variables at top of scope where they are used. Reply by Steve Jarpe on 20 December 2022, 07:11 > To be clear, does this mean all of the local declarations > should be at the beginning of the function? Reply by Sean Nash on 20 December 2022, 10:00 > Not necessarily at top of function. If variables are only > used in a smaller scope (e.g. an if body), they can be > declared at the top of that scope. I prefer they not be > declared in the middle of code unless there is a good > reason (e.g. a dependency in assignment on code above). Reply by Steve Jarpe on 20 December 2022, 12:05 > I am still not sure where you want the declarations to > go. They are just before the part of the function that > checks the inlet water conditions. The only other logical > place is at the top. Reply by Sean Nash on 20 December 2022, 13:38 > In this case, up around line 1013. Reply by Steve Jarpe on 20 December 2022, 15:41 > The lines have been moved. Reply by Sean Nash on 20 December 2022, 17:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:24 https://devapps.diality.us/cru/DG-DEN-14316-1#c15332 Align. Reply by Sean Nash on 20 December 2022, 10:02 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:24 https://devapps.diality.us/cru/DG-DEN-14316-1#c15330 Ok to not do anything for these cases, but put a comment in here so we know this is intentional. Reply by Steve Jarpe on 20 December 2022, 12:00 > Removed the do nothing cases, added comment to default case. Reply by Sean Nash on 20 December 2022, 13:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:23 https://devapps.diality.us/cru/DG-DEN-14316-1#c15329 Alignment. Reply by Sean Nash on 20 December 2022, 10:02 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:24 https://devapps.diality.us/cru/DG-DEN-14316-1#c15331 Should have a default case with s/w fault or something. Reply by Sean Nash on 20 December 2022, 13:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 14:00 https://devapps.diality.us/cru/DG-DEN-14316-1#c14953 Alignment Reply by wbracken on 20 December 2022, 15:34 > RESOLVED IN CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeChemicalDisinfectFlush.h Revision Comment by Sean Nash on 19 December 2022, 16:01 https://devapps.diality.us/cru/DG-DEN-14316-1#c15310 File header? Reply by Steve Jarpe on 20 December 2022, 12:28 > fixed Reply by Sean Nash on 20 December 2022, 13:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 16:09 https://devapps.diality.us/cru/DG-DEN-14316-1#c15312 "Flusht" - remove t at end of Flush. Reply by Steve Jarpe on 20 December 2022, 12:29 > fixed Reply by Sean Nash on 20 December 2022, 13:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 14:10 https://devapps.diality.us/cru/DG-DEN-14316-1#c14964 Alignment Reply by wbracken on 20 December 2022, 15:28 > RESOLVED IN CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/OperationModes.c Revision Comment by Sean Nash on 19 December 2022, 15:54 https://devapps.diality.us/cru/DG-DEN-14316-1#c15304 Should not allow transition from fault mode to chem flush mode. Reply by Steve Jarpe on 20 December 2022, 12:19 > fixed Reply by Sean Nash on 20 December 2022, 13:44 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/PIControllers.h Revision Comment by Sean Nash on 19 December 2022, 15:49 https://devapps.diality.us/cru/DG-DEN-14316-1#c15301 Does "tank" refer to reservoir? Prefer reservoir if so. Reply by Steve Jarpe on 20 December 2022, 12:21 > fixed Reply by Sean Nash on 20 December 2022, 13:45 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeFlush.c Revision Comment by wbracken on 30 November 2022, 15:07 https://devapps.diality.us/cru/DG-DEN-14316-1#c15085 Alignment Reply by wbracken on 20 December 2022, 17:54 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 30 November 2022, 15:08 https://devapps.diality.us/cru/DG-DEN-14316-1#c15086 Remove. Reply by wbracken on 20 December 2022, 16:16 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 30 November 2022, 15:10 https://devapps.diality.us/cru/DG-DEN-14316-1#c15088 Think the deleted def is correct. Reply by Sean Nash on 19 December 2022, 16:00 > Disagree. This is ModeFlush.c. Looks correct in last > commit. Reply by wbracken on 20 December 2022, 14:05 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 30 November 2022, 15:09 https://devapps.diality.us/cru/DG-DEN-14316-1#c15087 Alignment Reply by wbracken on 20 December 2022, 14:06 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 30 November 2022, 15:10 https://devapps.diality.us/cru/DG-DEN-14316-1#c15089 Remove. Reply by wbracken on 20 December 2022, 14:06 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 20 December 2022, 14:08 https://devapps.diality.us/cru/DG-DEN-14316-1#c15500 Should these two lines be deleted? Reply by Steve Jarpe on 21 December 2022, 10:00 > This may be from an older version? Reply by wbracken on 21 December 2022, 11:36 > It is definitely in the code. Reply by Sean Nash on 21 December 2022, 12:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 30 November 2022, 15:13 https://devapps.diality.us/cru/DG-DEN-14316-1#c15090 Remove Reply by wbracken on 20 December 2022, 14:09 > RESOLVED IN CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeStandby.c Revision Comment by Sean Nash on 19 December 2022, 14:54 https://devapps.diality.us/cru/DG-DEN-14316-1#c15271 Remove blank line. Reply by Sean Nash on 20 December 2022, 13:51 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 30 November 2022, 15:18 https://devapps.diality.us/cru/DG-DEN-14316-1#c15091 Remove blank line. Reply by Steve Jarpe on 21 December 2022, 09:55 > removed Reply by wbracken on 21 December 2022, 11:37 > RESOLVED IN CODEWALKTHROUGH. Revision Comment by wbracken on 30 November 2022, 15:18 https://devapps.diality.us/cru/DG-DEN-14316-1#c15092 Alignment Reply by wbracken on 20 December 2022, 14:11 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 14:55 https://devapps.diality.us/cru/DG-DEN-14316-1#c15273 No response sent if not in idle standby mode? Reply by Steve Jarpe on 20 December 2022, 12:17 > I need help with resolving this. Reply by Sean Nash on 20 December 2022, 13:48 > Need an else at bottom (for not in idle standby mode) where > we would set reject code to something like not in correct > mode (I'm sure we have a code for this already). And then > move the send response functional call to very end of this > function so that it will always send a response in any > case. Reply by Steve Jarpe on 20 December 2022, 15:59 > Done Reply by Sean Nash on 20 December 2022, 17:34 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Sean Nash on 19 December 2022, 15:52 https://devapps.diality.us/cru/DG-DEN-14316-1#c15303 Change both sizeof() calls to BOOL to match type of local var destination of your memcpy(). All are 4 bytes - just for consistency and in case BOOL gets changed. Reply by Steve Jarpe on 20 December 2022, 12:25 > Fixed Reply by Sean Nash on 20 December 2022, 13:44 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: HDDefs.h Revision Comment by Sean Nash on 19 December 2022, 15:21 https://devapps.diality.us/cru/DG-DEN-14316-1#c15282 Seems like there should be code changes in HD standby mode to handle these states. I don't see an HD code branch though. Reply by Steve Jarpe on 20 December 2022, 16:12 > I would need some help with this. Reply by Sean Nash on 20 December 2022, 17:08 > I'll have Dara do it once this branch is closed. It's a > small thing on HD side to manage some UI interactions. Reply by Sean Nash on 21 December 2022, 08:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 15:48 https://devapps.diality.us/cru/DG-DEN-14316-1#c15298 This enum is not in a group. Assign it to one (existing or new). Reply by Steve Jarpe on 20 December 2022, 16:13 > Not sure. Reply by Sean Nash on 21 December 2022, 08:30 > Ok as is. > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 15:47 https://devapps.diality.us/cru/DG-DEN-14316-1#c15296 Does this enum belong in UIUserConfirm group above or some other group? I don't see a group header to capture this enum. Reply by Steve Jarpe on 20 December 2022, 16:13 > I am not sure what should be done. Reply by Sean Nash on 21 December 2022, 08:34 > Added to group. > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: MsgDefs.h Revision Comment by Sean Nash on 19 December 2022, 15:16 https://devapps.diality.us/cru/DG-DEN-14316-1#c15281 DG does not send this message. Should be something like MSG_ID_HD_START_STOP_DG_CHEM_DISINFECT_FLUSH_REQUEST. Reply by Steve Jarpe on 20 December 2022, 12:40 > The start_stop messages for other modes are named similarly. Reply by Sean Nash on 21 December 2022, 08:35 > Leaving as is for now. Will review all msg IDs later. > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-14316-1 https://devapps.diality.us/cru/DG-DEN-14316-1 Title: DG-DEN-14316_Chemical Disinfect Statement of Objectives: State: Closed Summary: Author: Steve Jarpe Moderator: Steve Jarpe Reviewers: (3 active, 2 completed*) Sean Nash (*) Dara Navaei (*) Michael Garthwaite wbracken Darren Cox