This is a list of all comments for RO-LDT-566-2. Review Summary: No summary ---------------------------------------- File: FPDefs.h Revision Comment by Sean Nash on 21 July 2025, 11:44 https://devapps.diality.us/cru/RO-LDT-566-2#c22986 Add comments to right. Reply by Michael Garthwaite on 30 July 2025, 13:03 > Fixed. Thanks ---------------------------------------- File: MsgDefs.h Revision Comment by Sean Nash on 21 July 2025, 11:45 https://devapps.diality.us/cru/RO-LDT-566-2#c22988 Add comments to right. ---------------------------------------- File: firmware/App/Controllers/BoostPump.c Revision Comment by Sean Nash on 22 July 2025, 13:52 https://devapps.diality.us/cru/RO-LDT-566-2#c23016 FluidPump.h is included in BoostPump.h, so not needed here. Revision Comment by Sean Nash on 22 July 2025, 13:57 https://devapps.diality.us/cru/RO-LDT-566-2#c23020 Not sure this should be overridable. Can still have Dialin set this, but with a set command (instead of an override command). If you allow override, can get into situation where target needs to be programmatically changed (e.g. zeroed when changing control method) and it only changes the .data and not the .ovdata and, if currently overridden, it's not zero and doesn't work right. General rule: -use override cmds for sensor readings and broadcast intervals (i.e. things that you get or are usually constant) -use set cmds for set points and targets (i.e. things that you set) Reply by Michael Garthwaite on 30 July 2025, 13:24 > The intention of this to override was to exercise the control > loop state machine since this isnt the variable used to set > the pwm, but one layer above. We already have the set pwm ( > and dialin functionality ) in the fluid pump driver. Lets > discuss in the in person meeting. Revision Comment by Sean Nash on 22 July 2025, 13:54 https://devapps.diality.us/cru/RO-LDT-566-2#c23017 Put first part of condition in () too. Reply by Michael Garthwaite on 30 July 2025, 13:12 > fixed. thanks! Revision Comment by Sean Nash on 22 July 2025, 13:54 https://devapps.diality.us/cru/RO-LDT-566-2#c23018 Put first part of condition in () too. Reply by Michael Garthwaite on 30 July 2025, 13:12 > fixed. thanks! Revision Comment by Sean Nash on 22 July 2025, 14:04 https://devapps.diality.us/cru/RO-LDT-566-2#c23023 Remove extra blank line. Reply by Michael Garthwaite on 30 July 2025, 13:11 > fixed. thanks! ---------------------------------------- File: firmware/App/Controllers/ROPump.c Revision Comment by Sean Nash on 01 August 2025, 15:16 https://devapps.diality.us/cru/RO-LDT-566-2#c23415 Remove blank line. Revision Comment by Sean Nash on 22 July 2025, 14:10 https://devapps.diality.us/cru/RO-LDT-566-2#c23030 Shouldn't be override. Change Dialin from "override" to "set" cmd. Reply by Michael Garthwaite on 30 July 2025, 13:20 > to discuss in the in person meeting for CR. Revision Comment by Sean Nash on 22 July 2025, 14:08 https://devapps.diality.us/cru/RO-LDT-566-2#c23028 Put first part of condition in () too. Reply by Michael Garthwaite on 30 July 2025, 13:19 > fixed. thanks Revision Comment by Sean Nash on 22 July 2025, 14:09 https://devapps.diality.us/cru/RO-LDT-566-2#c23029 Remove extra blank line. Reply by Michael Garthwaite on 30 July 2025, 13:19 > fixed. thanks ---------------------------------------- File: firmware/App/Drivers/ConductivitySensor.c Revision Comment by Sean Nash on 23 July 2025, 12:21 https://devapps.diality.us/cru/RO-LDT-566-2#c23049 Should sensor temperatures be read in this driver (since coming from this sensor)? Like PressureSensor.c does it. Reply by Michael Garthwaite on 30 July 2025, 12:43 > i believe they are.. this driver will have to be reviewed > once we get the conductivity sensors onto the units. > > getFPGAP9Data() pulls register 336 in the HDD that is 4 > bytes. > getFPGAP9Conductivity pulls register 340 that is 2 bytes. > getFPGAP9Temperature pulls register 342 that is 2 bytes. > > unsure what are the differences between getFPGAP9Data and the > other two. Seem like they are the same ---------------------------------------- File: firmware/App/Drivers/PressureSensor.c Revision Comment by Sean Nash on 23 July 2025, 12:22 https://devapps.diality.us/cru/RO-LDT-566-2#c23050 Isn't there a common override function you can call to fetch the value? Reply by Michael Garthwaite on 30 July 2025, 12:33 > Can you clarify? ---------------------------------------- File: firmware/App/Drivers/TemperatureSensor.c Revision Comment by Sean Nash on 23 July 2025, 11:57 https://devapps.diality.us/cru/RO-LDT-566-2#c23038 Why not just remove these? And, in fact, why do we even have this driver anymore? The conductivity sensor driver has P10 and P19 covered, pressuresensor has others covered and fpgaFP will have PCB temps covered I think and so what's left for this driver to read? Reply by Michael Garthwaite on 30 July 2025, 12:47 > agreed. I dont think the driver is necessary any more Reply by Michael Garthwaite on 30 July 2025, 12:49 > removed driver. ---------------------------------------- File: firmware/App/Modes/FlushConcentrate.c Revision Comment by Sean Nash on 23 July 2025, 12:41 https://devapps.diality.us/cru/RO-LDT-566-2#c23063 Add comment for why needed. Revision Comment by Sean Nash on 23 July 2025, 12:41 https://devapps.diality.us/cru/RO-LDT-566-2#c23064 Add blank line after banner. Reply by Michael Garthwaite on 30 July 2025, 12:17 > fixed. thanks! Revision Comment by Sean Nash on 01 August 2025, 15:06 https://devapps.diality.us/cru/RO-LDT-566-2#c23405 If no comment here, remove. Revision Comment by Sean Nash on 23 July 2025, 12:42 https://devapps.diality.us/cru/RO-LDT-566-2#c23065 Add blank line between different comment styles. I think doxygen messes up if you don't. Reply by Michael Garthwaite on 30 July 2025, 12:19 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 12:42 https://devapps.diality.us/cru/RO-LDT-566-2#c23066 Remove blank line. Reply by Michael Garthwaite on 30 July 2025, 12:19 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 12:43 https://devapps.diality.us/cru/RO-LDT-566-2#c23067 Add blank line before return. Reply by Michael Garthwaite on 30 July 2025, 12:19 > fixed thanks! Revision Comment by Sean Nash on 23 July 2025, 12:43 https://devapps.diality.us/cru/RO-LDT-566-2#c23068 Remove blank line. Reply by Michael Garthwaite on 30 July 2025, 12:21 > fixed. thanks Revision Comment by Sean Nash on 23 July 2025, 12:44 https://devapps.diality.us/cru/RO-LDT-566-2#c23069 No return value. Reply by Michael Garthwaite on 30 July 2025, 12:22 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 12:45 https://devapps.diality.us/cru/RO-LDT-566-2#c23070 Is it ok if this function gets called when we're not paused in concentrate flush? Should we s/w fault? Should we inform caller (return FALSE)? Reply by Michael Garthwaite on 30 July 2025, 12:24 > it should be okay if we call when were not paused.. The idea > is that if it was called the FP would ignore it since it was > already in flush. > > Changed the function to return a boolean. Revision Comment by Sean Nash on 23 July 2025, 12:47 https://devapps.diality.us/cru/RO-LDT-566-2#c23071 No output. Reply by Michael Garthwaite on 30 July 2025, 12:29 > Fixed. Thanks! Revision Comment by Sean Nash on 23 July 2025, 12:47 https://devapps.diality.us/cru/RO-LDT-566-2#c23072 Add isFlushComplete to inputs. Reply by Michael Garthwaite on 30 July 2025, 12:30 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 12:50 https://devapps.diality.us/cru/RO-LDT-566-2#c23073 Oddly worded. I would say "TRUE if concentrate flush is complete, FALSE if not." Reply by Michael Garthwaite on 30 July 2025, 12:30 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 12:50 https://devapps.diality.us/cru/RO-LDT-566-2#c23074 I would say something like "the concentrate flush timeout period in ms" Reply by Michael Garthwaite on 30 July 2025, 12:31 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 12:51 https://devapps.diality.us/cru/RO-LDT-566-2#c23075 \Sent should be \b Sent. Reply by Michael Garthwaite on 30 July 2025, 12:30 > fixed. Thanks! ---------------------------------------- File: firmware/App/Modes/FlushConcentrate.h Revision Comment by Sean Nash on 23 July 2025, 12:41 https://devapps.diality.us/cru/RO-LDT-566-2#c23062 Remove extra blank line. Reply by Michael Garthwaite on 30 July 2025, 12:16 > fixed. thanks! ---------------------------------------- File: firmware/App/Modes/FlushFilter.c Revision Comment by Sean Nash on 23 July 2025, 12:52 https://devapps.diality.us/cru/RO-LDT-566-2#c23076 Add comment for why we need this. Reply by Michael Garthwaite on 30 July 2025, 12:09 > removed. Revision Comment by Sean Nash on 23 July 2025, 13:01 https://devapps.diality.us/cru/RO-LDT-566-2#c23077 Remove blank line. Reply by Michael Garthwaite on 30 July 2025, 12:09 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 13:02 https://devapps.diality.us/cru/RO-LDT-566-2#c23078 Add blank line before return. Reply by Michael Garthwaite on 30 July 2025, 12:10 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 13:02 https://devapps.diality.us/cru/RO-LDT-566-2#c23079 Even though it's via a get function, we should probably include the current filter flush state as an input. Reply by Michael Garthwaite on 30 July 2025, 12:12 > added. thanks! Revision Comment by Sean Nash on 23 July 2025, 13:04 https://devapps.diality.us/cru/RO-LDT-566-2#c23080 No output. Reply by Michael Garthwaite on 30 July 2025, 12:13 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 13:04 https://devapps.diality.us/cru/RO-LDT-566-2#c23081 Add isFlushComplete as input. Reply by Michael Garthwaite on 30 July 2025, 12:13 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 13:04 https://devapps.diality.us/cru/RO-LDT-566-2#c23082 Reword per same comment in other units. Reply by Michael Garthwaite on 30 July 2025, 12:13 > fixed thanks! Revision Comment by Sean Nash on 23 July 2025, 13:04 https://devapps.diality.us/cru/RO-LDT-566-2#c23083 Remove extra blank line. Reply by Michael Garthwaite on 30 July 2025, 12:14 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 13:05 https://devapps.diality.us/cru/RO-LDT-566-2#c23084 \Sent should be \b Sent. Reply by Michael Garthwaite on 30 July 2025, 12:14 > fixed thanks! ---------------------------------------- File: firmware/App/Modes/FlushPermeate.c Revision Comment by Sean Nash on 23 July 2025, 13:06 https://devapps.diality.us/cru/RO-LDT-566-2#c23085 Add comment for why we need this. Reply by Michael Garthwaite on 30 July 2025, 12:01 > removed. thanks! Revision Comment by Sean Nash on 23 July 2025, 13:07 https://devapps.diality.us/cru/RO-LDT-566-2#c23086 Update inputs and outputs. Reply by Michael Garthwaite on 30 July 2025, 12:03 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 13:07 https://devapps.diality.us/cru/RO-LDT-566-2#c23087 Update inputs and outputs. Reply by Michael Garthwaite on 30 July 2025, 12:03 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 13:07 https://devapps.diality.us/cru/RO-LDT-566-2#c23088 Add blank line before return. Reply by Michael Garthwaite on 30 July 2025, 12:04 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 13:17 https://devapps.diality.us/cru/RO-LDT-566-2#c23089 Add s/w fault alarm. Reply by Michael Garthwaite on 30 July 2025, 12:04 > added to the documentation. thanks Revision Comment by Sean Nash on 23 July 2025, 13:17 https://devapps.diality.us/cru/RO-LDT-566-2#c23090 No return. Reply by Michael Garthwaite on 30 July 2025, 12:05 > fixed thanks! Revision Comment by Sean Nash on 23 July 2025, 13:17 https://devapps.diality.us/cru/RO-LDT-566-2#c23091 No output. Reply by Michael Garthwaite on 30 July 2025, 12:05 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 13:18 https://devapps.diality.us/cru/RO-LDT-566-2#c23092 Add isFlushComplete to input. Reply by Michael Garthwaite on 30 July 2025, 12:06 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 13:18 https://devapps.diality.us/cru/RO-LDT-566-2#c23093 Reword per comment in other units. Reply by Michael Garthwaite on 30 July 2025, 12:15 > fixed. thanks! ---------------------------------------- File: firmware/App/Modes/ModeGenPermeate.c Revision Comment by Sean Nash on 24 July 2025, 09:03 https://devapps.diality.us/cru/RO-LDT-566-2#c23109 Align with others. Reply by Michael Garthwaite on 30 July 2025, 11:46 > fixed. thanks! Revision Comment by Sean Nash on 24 July 2025, 09:05 https://devapps.diality.us/cru/RO-LDT-566-2#c23112 Add comments to right. Reply by Michael Garthwaite on 30 July 2025, 11:46 > fixed. thanks! Revision Comment by Sean Nash on 24 July 2025, 09:05 https://devapps.diality.us/cru/RO-LDT-566-2#c23111 25.0F Reply by Michael Garthwaite on 30 July 2025, 11:46 > fixed. thanks! Revision Comment by Sean Nash on 24 July 2025, 09:03 https://devapps.diality.us/cru/RO-LDT-566-2#c23110 Add comments to right. Revision Comment by Sean Nash on 24 July 2025, 09:08 https://devapps.diality.us/cru/RO-LDT-566-2#c23114 genPermeateState is an input. Reply by Michael Garthwaite on 30 July 2025, 11:45 > fixed. thanks! Revision Comment by Sean Nash on 24 July 2025, 09:08 https://devapps.diality.us/cru/RO-LDT-566-2#c23113 Add blank line before return. Reply by Michael Garthwaite on 30 July 2025, 11:45 > fixed. thanks! Revision Comment by Sean Nash on 24 July 2025, 09:09 https://devapps.diality.us/cru/RO-LDT-566-2#c23115 genPermeateState is an input and output. Reply by Michael Garthwaite on 30 July 2025, 11:45 > fixed. thanks! Revision Comment by Sean Nash on 24 July 2025, 09:17 https://devapps.diality.us/cru/RO-LDT-566-2#c23116 Not an output. Reply by Michael Garthwaite on 30 July 2025, 11:44 > function have been removed as we no longer need it Revision Comment by Sean Nash on 24 July 2025, 09:17 https://devapps.diality.us/cru/RO-LDT-566-2#c23117 Add param. Reply by Michael Garthwaite on 30 July 2025, 11:43 > functions have been removed as we no longer need it Revision Comment by Sean Nash on 24 July 2025, 09:18 https://devapps.diality.us/cru/RO-LDT-566-2#c23118 Not an output. Reply by Michael Garthwaite on 30 July 2025, 11:35 > functions have been removed as we no longer need it. ( > Controlling to flow ) Revision Comment by Sean Nash on 24 July 2025, 09:18 https://devapps.diality.us/cru/RO-LDT-566-2#c23119 Add param. Reply by Michael Garthwaite on 30 July 2025, 11:35 > functions have been removed as we no longer need it. ( > Controlling to flow ) Revision Comment by Sean Nash on 24 July 2025, 09:19 https://devapps.diality.us/cru/RO-LDT-566-2#c23120 Not an input. Reply by Michael Garthwaite on 30 July 2025, 11:34 > fixed. Revision Comment by Sean Nash on 24 July 2025, 09:20 https://devapps.diality.us/cru/RO-LDT-566-2#c23121 Why do we need to set a pending flag if we're making the mode transition here? Reply by Michael Garthwaite on 30 July 2025, 11:34 > Removed. It was there before as we had a START_STATE and that > would allow the transition from start to full or fill. ---------------------------------------- File: firmware/App/Modes/ModeGenPermeate.h Revision Comment by Sean Nash on 24 July 2025, 09:02 https://devapps.diality.us/cru/RO-LDT-566-2#c23108 Remove extra blank line. Reply by Michael Garthwaite on 30 July 2025, 12:15 > fixed. thanks! ---------------------------------------- File: firmware/App/Modes/ModeGenPermeateDefeatured.c Revision Comment by Sean Nash on 01 August 2025, 12:05 https://devapps.diality.us/cru/RO-LDT-566-2#c23355 Align "=". Revision Comment by Sean Nash on 01 August 2025, 12:05 https://devapps.diality.us/cru/RO-LDT-566-2#c23357 Add blank line before return. Reply by Michael Garthwaite on 01 August 2025, 12:25 > fixed. thanks Revision Comment by Sean Nash on 01 August 2025, 12:06 https://devapps.diality.us/cru/RO-LDT-566-2#c23358 I don't see any inputs. Reply by Michael Garthwaite on 01 August 2025, 12:25 > fixed. thanks Revision Comment by Sean Nash on 01 August 2025, 12:06 https://devapps.diality.us/cru/RO-LDT-566-2#c23359 Add blank line between declarations and code. Reply by Michael Garthwaite on 01 August 2025, 12:25 > fixed. thanks ---------------------------------------- File: firmware/App/Modes/ModeGenPermeateDefeatured.h Revision Comment by Sean Nash on 01 August 2025, 12:01 https://devapps.diality.us/cru/RO-LDT-566-2#c23351 Comments to right of function prototypes are not required. If you add them, add them for all in a given header file. Reply by Michael Garthwaite on 01 August 2025, 12:27 > fixed. thanks Revision Comment by Sean Nash on 01 August 2025, 12:01 https://devapps.diality.us/cru/RO-LDT-566-2#c23350 Remove extra blank line. ---------------------------------------- File: firmware/App/Modes/ModeInitPOST.c Revision Comment by Sean Nash on 23 July 2025, 14:28 https://devapps.diality.us/cru/RO-LDT-566-2#c23099 FPInitAndPOSTMode Reply by Michael Garthwaite on 30 July 2025, 11:48 > fixed in renamed file. thanks. Revision Comment by Sean Nash on 23 July 2025, 14:27 https://devapps.diality.us/cru/RO-LDT-566-2#c23097 Need function headers. Reply by Michael Garthwaite on 30 July 2025, 11:48 > fixed in renamed file. thanks. Revision Comment by Sean Nash on 23 July 2025, 14:27 https://devapps.diality.us/cru/RO-LDT-566-2#c23098 Add blank line before return. Reply by Michael Garthwaite on 30 July 2025, 11:48 > fixed in renamed file. thanks. ---------------------------------------- File: firmware/App/Modes/ModePreGenPermeate.c Revision Comment by Sean Nash on 01 August 2025, 15:21 https://devapps.diality.us/cru/RO-LDT-566-2#c23417 Remove blank line. ---------------------------------------- File: firmware/App/Modes/ModePreGenPermeateDefeatured.h Revision Comment by Sean Nash on 01 August 2025, 15:24 https://devapps.diality.us/cru/RO-LDT-566-2#c23419 Remove blank line. ---------------------------------------- File: firmware/App/Modes/ModeStandby.c Revision Comment by Sean Nash on 01 August 2025, 12:00 https://devapps.diality.us/cru/RO-LDT-566-2#c23348 Shouldn't this unit be deleted since it's been renamed? Reply by Michael Garthwaite on 01 August 2025, 12:02 > This is the deleted file. Moved to FPModeStandby Revision Comment by Sean Nash on 23 July 2025, 14:28 https://devapps.diality.us/cru/RO-LDT-566-2#c23100 Remove extra blank line. Reply by Michael Garthwaite on 30 July 2025, 11:51 > fixed in renamed file. thanks! Revision Comment by Sean Nash on 23 July 2025, 14:28 https://devapps.diality.us/cru/RO-LDT-566-2#c23101 Remove extra blank line. Revision Comment by Sean Nash on 23 July 2025, 14:30 https://devapps.diality.us/cru/RO-LDT-566-2#c23103 How do we get to pre-gen now? Reply by Michael Garthwaite on 30 July 2025, 11:54 > when the FP receives MSG_ID_DD_FP_START_STOP_CMD_REQUEST, > it'll set the variable within getPreGenRequest to be true. Revision Comment by Sean Nash on 23 July 2025, 14:29 https://devapps.diality.us/cru/RO-LDT-566-2#c23102 Add extra blank line before test support banner. Reply by Michael Garthwaite on 30 July 2025, 11:53 > fixed. thanks! ---------------------------------------- File: firmware/App/Modes/OperationModes.c Revision Comment by Sean Nash on 23 July 2025, 14:31 https://devapps.diality.us/cru/RO-LDT-566-2#c23106 Remove extra blank line. Reply by Michael Garthwaite on 30 July 2025, 11:50 > fixed in renamed file. thanks! Revision Comment by Sean Nash on 23 July 2025, 14:33 https://devapps.diality.us/cru/RO-LDT-566-2#c23107 Add extra blank line before test support banner. Reply by Michael Garthwaite on 30 July 2025, 11:50 > fixed in renamed file. thanks! ---------------------------------------- File: firmware/App/Modes/OperationModes.h Revision Comment by Sean Nash on 23 July 2025, 14:31 https://devapps.diality.us/cru/RO-LDT-566-2#c23105 When we merge with DD for beta, will need unique unit names. Should we change to FPOperationModes? Reply by Michael Garthwaite on 30 July 2025, 11:49 > Agreed. Renamed the common file names ( standby, op modes, > fault, post ) with a FP prefix. Revision Comment by Sean Nash on 23 July 2025, 14:30 https://devapps.diality.us/cru/RO-LDT-566-2#c23104 Change to "isFPDefeatured"? Reply by Michael Garthwaite on 30 July 2025, 11:50 > fixed. thanks! ---------------------------------------- File: firmware/App/Modes/PressureCheck.h Revision Comment by Sean Nash on 01 August 2025, 11:59 https://devapps.diality.us/cru/RO-LDT-566-2#c23346 If not ready to implement this unit, should we remove it for this branch? Reply by Michael Garthwaite on 01 August 2025, 12:02 > agreed. its been removed ---------------------------------------- File: firmware/App/Monitors/Conductivity.c Revision Comment by Sean Nash on 23 July 2025, 12:36 https://devapps.diality.us/cru/RO-LDT-566-2#c23061 Why does this monitor need these mode includes? ---------------------------------------- File: firmware/App/Monitors/Flow.c Revision Comment by Sean Nash on 23 July 2025, 12:35 https://devapps.diality.us/cru/RO-LDT-566-2#c23060 Why does this monitor need these mode includes? ---------------------------------------- File: firmware/App/Monitors/Pressure.c Revision Comment by Sean Nash on 23 July 2025, 12:25 https://devapps.diality.us/cru/RO-LDT-566-2#c23051 Add comments to right. Revision Comment by Sean Nash on 23 July 2025, 12:25 https://devapps.diality.us/cru/RO-LDT-566-2#c23052 Remove extra blank line. Revision Comment by Sean Nash on 23 July 2025, 12:30 https://devapps.diality.us/cru/RO-LDT-566-2#c23056 What is value of this function? Why not bring in functionality of called function? Revision Comment by Sean Nash on 23 July 2025, 12:30 https://devapps.diality.us/cru/RO-LDT-566-2#c23057 Remove blank line. Revision Comment by Sean Nash on 23 July 2025, 12:31 https://devapps.diality.us/cru/RO-LDT-566-2#c23058 Add comment before break to indicate we are doing nothing on purpose. Consider grouping modes that do nothing (no blank lines between, 1 break at end of group, 1 comment). Revision Comment by Sean Nash on 23 July 2025, 12:33 https://devapps.diality.us/cru/RO-LDT-566-2#c23059 Same comment as temperatures. Consider similar approach where modes signal monitors to monitor differently as appropriate for mode instead of having monitor being aware of current mode. Revision Comment by Sean Nash on 23 July 2025, 12:29 https://devapps.diality.us/cru/RO-LDT-566-2#c23055 Remove blank line. Revision Comment by Sean Nash on 23 July 2025, 12:29 https://devapps.diality.us/cru/RO-LDT-566-2#c23054 Remove blank line. Revision Comment by Sean Nash on 23 July 2025, 12:27 https://devapps.diality.us/cru/RO-LDT-566-2#c23053 Should Temperature monitor be publishing these temperatures? Reply by Michael Garthwaite on 30 July 2025, 14:12 > Yes and has been moving to the temperature monitor ---------------------------------------- File: firmware/App/Monitors/Temperature.c Revision Comment by Sean Nash on 01 August 2025, 11:54 https://devapps.diality.us/cru/RO-LDT-566-2#c23340 Remove extra blank lines. Revision Comment by Sean Nash on 01 August 2025, 11:54 https://devapps.diality.us/cru/RO-LDT-566-2#c23341 Remove extra blank line. Revision Comment by Sean Nash on 23 July 2025, 12:05 https://devapps.diality.us/cru/RO-LDT-566-2#c23040 Why not just do monitoring function here in this function instead of calling another? I don't see what value this function is adding. Revision Comment by Sean Nash on 23 July 2025, 12:08 https://devapps.diality.us/cru/RO-LDT-566-2#c23044 If nothing to do in these modes, add a comment before break indicating we're doing nothing on purpose because nothing is required. Also, consider grouping these "do nothing" modes together (no blank lines between, 1 break at end, shared function (which is nothing)). Revision Comment by Sean Nash on 23 July 2025, 12:06 https://devapps.diality.us/cru/RO-LDT-566-2#c23042 Blank line before default. Revision Comment by Sean Nash on 23 July 2025, 12:10 https://devapps.diality.us/cru/RO-LDT-566-2#c23045 Why is s/w fault commented out? Revision Comment by Sean Nash on 23 July 2025, 12:06 https://devapps.diality.us/cru/RO-LDT-566-2#c23041 Add alarm(s) triggered to header. Revision Comment by Sean Nash on 23 July 2025, 12:12 https://devapps.diality.us/cru/RO-LDT-566-2#c23047 Pre gen and gen modes appear to be doing the exact same thing. And defeatured modes are only different in that they are looking at M4 instead of P10. Consider handling monitoring differently. Instead of monitor being aware of mode and acting accordingly, should mode call a signal function in the monitor to configure the monitor appropriately for that mode? With this approach, seems like a lot of these functions could be merged into 1 with a couple "if" conditions checking current monitor configuration. I also think this approach is a better design pattern because monitor does not need to concern itself with higher level details (e.g. current mode) - better to have higher level units (modes, states) telling lower level units (drivers, monitors, controllers) what's appropriate. Revision Comment by Sean Nash on 23 July 2025, 12:07 https://devapps.diality.us/cru/RO-LDT-566-2#c23043 Remove extra blank line. Revision Comment by Sean Nash on 23 July 2025, 12:19 https://devapps.diality.us/cru/RO-LDT-566-2#c23048 Remove blank line. Revision Comment by Sean Nash on 23 July 2025, 12:11 https://devapps.diality.us/cru/RO-LDT-566-2#c23046 Remove blank lines. ---------------------------------------- File: firmware/App/Services/AlarmMgmtFP.h Revision Comment by Sean Nash on 01 August 2025, 15:25 https://devapps.diality.us/cru/RO-LDT-566-2#c23420 Remove blank line. ---------------------------------------- File: firmware/App/Services/DDInterface.c Revision Comment by Sean Nash on 01 August 2025, 15:27 https://devapps.diality.us/cru/RO-LDT-566-2#c23424 Remove blank line. Revision Comment by Sean Nash on 01 August 2025, 15:27 https://devapps.diality.us/cru/RO-LDT-566-2#c23425 Remove blank line. Revision Comment by Sean Nash on 01 August 2025, 15:27 https://devapps.diality.us/cru/RO-LDT-566-2#c23426 Remove blank line. Revision Comment by Sean Nash on 01 August 2025, 15:27 https://devapps.diality.us/cru/RO-LDT-566-2#c23427 Remove blank line. Revision Comment by Sean Nash on 01 August 2025, 15:27 https://devapps.diality.us/cru/RO-LDT-566-2#c23428 Remove blank line. Revision Comment by Sean Nash on 01 August 2025, 15:27 https://devapps.diality.us/cru/RO-LDT-566-2#c23429 Remove blank line. ---------------------------------------- File: firmware/App/Services/DDInterface.h Revision Comment by Sean Nash on 01 August 2025, 15:26 https://devapps.diality.us/cru/RO-LDT-566-2#c23421 Fix this. ---------------------------------------- File: firmware/App/Services/FpgaFP.c Revision Comment by Sean Nash on 01 August 2025, 15:28 https://devapps.diality.us/cru/RO-LDT-566-2#c23430 Remove blank line. ---------------------------------------- File: firmware/App/Services/Messaging.c Revision Comment by Sean Nash on 01 August 2025, 15:29 https://devapps.diality.us/cru/RO-LDT-566-2#c23434 Remove blank line. Revision Comment by Sean Nash on 01 August 2025, 15:35 https://devapps.diality.us/cru/RO-LDT-566-2#c23444 I think FP and DD need to handle alarm trigger, clear, and clear condition messages (msg ID 2,3,4) from other s/w stacks. Reply by Michael Garthwaite on 04 August 2025, 09:25 > Added TODO. It'll be implemented in the next branch regarding > stop/pauses states and alarm requirements. Revision Comment by Sean Nash on 01 August 2025, 15:34 https://devapps.diality.us/cru/RO-LDT-566-2#c23442 I would put normal messages above Dialin messages in this list - basically ordering this list in msg enum order. ---------------------------------------- File: firmware/App/Services/SystemCommFP.c Revision Comment by Sean Nash on 01 August 2025, 15:35 https://devapps.diality.us/cru/RO-LDT-566-2#c23445 Remove blank line. ---------------------------------------- File: firmware/source/sys_main.c Revision Comment by Sean Nash on 01 August 2025, 12:08 https://devapps.diality.us/cru/RO-LDT-566-2#c23360 Remove extra blank line. Reply by Michael Garthwaite on 01 August 2025, 12:24 > fixed. thanks Revision Comment by Sean Nash on 01 August 2025, 12:08 https://devapps.diality.us/cru/RO-LDT-566-2#c23361 Remove extra blank line. Reply by Michael Garthwaite on 01 August 2025, 12:24 > fixed. thanks ---------------------------------------- File: AlarmMgmt.c Revision Comment by Sean Nash on 22 July 2025, 13:46 https://devapps.diality.us/cru/RO-LDT-566-2#c23014 Add comment to right if keeping this variable. Otherwise remove variable. Reply by Michael Garthwaite on 30 July 2025, 13:05 > removed. thanks! Revision Comment by Sean Nash on 22 July 2025, 13:48 https://devapps.diality.us/cru/RO-LDT-566-2#c23015 I don't see that this flag is ever set to TRUE. Where is this needed? Reply by Michael Garthwaite on 30 July 2025, 13:05 > This was supposed to be a place holder for it. it was planned > to be used in modes. After our previous discussion, this is > wasn't the correct approach. It has been removed. ---------------------------------------- File: AlarmMgmt.h Revision Comment by Sean Nash on 22 July 2025, 13:45 https://devapps.diality.us/cru/RO-LDT-566-2#c23013 Assume FP and DD would share this structure. Why do we need it? TD is responsible for publishing this status. Reply by Michael Garthwaite on 30 July 2025, 13:07 > this was used to help read the stop property for alarms. It > has been removed. ---------------------------------------- File: firmware/App/Controllers/ROPump.h Revision Comment by Sean Nash on 22 July 2025, 14:06 https://devapps.diality.us/cru/RO-LDT-566-2#c23025 Where did these slope and intercept definitions get moved to? Reply by Michael Garthwaite on 30 July 2025, 13:17 > They are in the source file. These definitions have also been > moved to the source file as well. ---------------------------------------- File: firmware/App/Modes/ModeFault.c Revision Comment by Sean Nash on 23 July 2025, 14:25 https://devapps.diality.us/cru/RO-LDT-566-2#c23094 FPFaultMode Reply by Michael Garthwaite on 30 July 2025, 11:49 > fixed. thanks! Revision Comment by Sean Nash on 23 July 2025, 14:26 https://devapps.diality.us/cru/RO-LDT-566-2#c23096 Don't we need at least 2 states (energized and de-energized) per SRS? Reply by Michael Garthwaite on 30 July 2025, 11:48 > updated in renamed file. thanks! Revision Comment by Sean Nash on 23 July 2025, 14:26 https://devapps.diality.us/cru/RO-LDT-566-2#c23095 Need function headers. Reply by Michael Garthwaite on 30 July 2025, 11:48 > fixed in renamed file. thanks! ---------------------------------------- File: firmware/App/Monitors/WaterQualityMonitor.c Revision Comment by Sean Nash on 01 August 2025, 11:57 https://devapps.diality.us/cru/RO-LDT-566-2#c23342 Remove extra blank line. Revision Comment by Sean Nash on 01 August 2025, 11:57 https://devapps.diality.us/cru/RO-LDT-566-2#c23343 Remove extra blank lines. Reply by Michael Garthwaite on 01 August 2025, 12:09 > fixed. thanks! Revision Comment by Sean Nash on 01 August 2025, 11:57 https://devapps.diality.us/cru/RO-LDT-566-2#c23344 Remove extra blank line. Reply by Michael Garthwaite on 01 August 2025, 12:09 > fixed. Thanks! Revision Comment by Sean Nash on 01 August 2025, 11:58 https://devapps.diality.us/cru/RO-LDT-566-2#c23345 Remove extra blank line. Reply by Michael Garthwaite on 01 August 2025, 12:08 > fixed. thanks! ---------------------------------------- File: firmware/App/Controllers/PermeateTank.c Revision Comment by Sean Nash on 01 August 2025, 15:12 https://devapps.diality.us/cru/RO-LDT-566-2#c23408 Remove blank line. Revision Comment by Sean Nash on 01 August 2025, 15:15 https://devapps.diality.us/cru/RO-LDT-566-2#c23414 Add blank line between functions. Revision Comment by Sean Nash on 01 August 2025, 15:14 https://devapps.diality.us/cru/RO-LDT-566-2#c23413 Update inputs/outputs Revision Comment by Sean Nash on 01 August 2025, 15:14 https://devapps.diality.us/cru/RO-LDT-566-2#c23412 Update inputs/outputs Revision Comment by Sean Nash on 01 August 2025, 15:13 https://devapps.diality.us/cru/RO-LDT-566-2#c23409 Remove blank line. Revision Comment by Sean Nash on 01 August 2025, 15:14 https://devapps.diality.us/cru/RO-LDT-566-2#c23411 Check inputs/outputs Revision Comment by Sean Nash on 01 August 2025, 15:13 https://devapps.diality.us/cru/RO-LDT-566-2#c23410 Remove blank line. ---------------------------------------- File: firmware/App/Modes/FPModeStandby.c Revision Comment by Sean Nash on 01 August 2025, 15:19 https://devapps.diality.us/cru/RO-LDT-566-2#c23416 Remove blank line. --- ID: RO-LDT-566-2 https://devapps.diality.us/cru/RO-LDT-566-2 Title: RO-LDT-566_Water Treatment And Degassing FP 2 Statement of Objectives: State: Closed Summary: Author: Michael Garthwaite Moderator: Michael Garthwaite Reviewers: (6 active, 2 completed*) Sean Nash (*) Vinayakam Mani (*) Nicholas Ramirez Tiffany Mejia Raghu Kallala Dara Navaei Behrouz NematiPour Daniel Ho