This is a list of all comments for DD-LEAH-227-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/ConcentratePumps.c Revision Comment by Sean Nash on 30 September 2024, 10:25 https://devapps.diality.us/cru/DD-LEAH-227-1#c20294 ... if we are parked. Reply by Vinayakam Mani on 01 October 2024, 11:52 > Done. Revision Comment by Sean Nash on 30 September 2024, 10:28 https://devapps.diality.us/cru/DD-LEAH-227-1#c20295 Use CONCENTRATEPUMPS_FIRST. Reply by Vinayakam Mani on 01 October 2024, 11:52 > Done. Revision Comment by Sean Nash on 30 September 2024, 10:30 https://devapps.diality.us/cru/DD-LEAH-227-1#c20296 We could say calibration is an input. Reply by Vinayakam Mani on 01 October 2024, 11:52 > Done. Revision Comment by Sean Nash on 30 September 2024, 10:31 https://devapps.diality.us/cru/DD-LEAH-227-1#c20297 Should be "hasTurnOnPumpsBeenRequested". There are also 2 clear flags being set. Reply by Vinayakam Mani on 01 October 2024, 11:52 > Done. Revision Comment by Sean Nash on 30 September 2024, 10:33 https://devapps.diality.us/cru/DD-LEAH-227-1#c20298 This is not an output - it is what gets returned which is already covered below. Reply by Vinayakam Mani on 01 October 2024, 11:52 > Done. Revision Comment by Sean Nash on 30 September 2024, 10:34 https://devapps.diality.us/cru/DD-LEAH-227-1#c20299 Change to concentratePumps[] or isConcPumpParkInProgress. Reply by Vinayakam Mani on 01 October 2024, 11:52 > Done. Revision Comment by Sean Nash on 30 September 2024, 10:35 https://devapps.diality.us/cru/DD-LEAH-227-1#c20300 Should be "hasTurnOnPumpsBeenRequested". Also, "hasParkBeenRequested" and "pumpTargetSpeed". Reply by Vinayakam Mani on 01 October 2024, 11:52 > Done. Revision Comment by Sean Nash on 30 September 2024, 10:36 https://devapps.diality.us/cru/DD-LEAH-227-1#c20301 Also setting pump direction. Reply by Vinayakam Mani on 01 October 2024, 11:52 > Done. Revision Comment by Sean Nash on 30 September 2024, 12:39 https://devapps.diality.us/cru/DD-LEAH-227-1#c20303 Change to "... the target flow rate for a given concentrate pump." Reply by Vinayakam Mani on 01 October 2024, 11:51 > Done. Revision Comment by Sean Nash on 30 September 2024, 12:34 https://devapps.diality.us/cru/DD-LEAH-227-1#c20302 function name and get get function's name suggest you are returning a target flow rate. If this is returning a target flow rate, the return comment should make that clear. Change to "... current target flow rate (in mL/min) for the given concentrate pump." Reply by Vinayakam Mani on 01 October 2024, 11:51 > Done. Revision Comment by Sean Nash on 30 September 2024, 12:40 https://devapps.diality.us/cru/DD-LEAH-227-1#c20304 Change to "... current target speed for the given concentrate pump." Reply by Vinayakam Mani on 01 October 2024, 11:51 > Done. Revision Comment by Sean Nash on 30 September 2024, 12:41 https://devapps.diality.us/cru/DD-LEAH-227-1#c20305 Change to "... the measured flow rate for a given concentrate pump." Reply by Vinayakam Mani on 01 October 2024, 11:51 > Done. Revision Comment by Sean Nash on 30 September 2024, 12:42 https://devapps.diality.us/cru/DD-LEAH-227-1#c20306 Change to "... current measured flow rate (in mL/min) for the given concentrate pump." Reply by Vinayakam Mani on 01 October 2024, 11:51 > Done. Revision Comment by Sean Nash on 30 September 2024, 12:43 https://devapps.diality.us/cru/DD-LEAH-227-1#c20307 Is this measured speed or measured flow? The function header says it's mL/min, so should be flow if that is the case. Reply by Vinayakam Mani on 01 October 2024, 11:51 > Done. Revision Comment by Sean Nash on 30 September 2024, 12:44 https://devapps.diality.us/cru/DD-LEAH-227-1#c20308 Change to "TRUE if a park request is pending for the given concentrate pump, FALSE if not." Reply by Vinayakam Mani on 01 October 2024, 11:51 > Done. Revision Comment by Sean Nash on 30 September 2024, 12:45 https://devapps.diality.us/cru/DD-LEAH-227-1#c20309 Change to "TRUE if the given concentrate pump is currently parked, FALSE if not." Reply by Vinayakam Mani on 01 October 2024, 11:51 > Done. Revision Comment by Sean Nash on 30 September 2024, 12:46 https://devapps.diality.us/cru/DD-LEAH-227-1#c20310 Change to "TRUE if the given concentrate pump has failed a park command." Reply by Vinayakam Mani on 01 October 2024, 11:51 > Done. Revision Comment by Sean Nash on 30 September 2024, 12:48 https://devapps.diality.us/cru/DD-LEAH-227-1#c20311 Change "... the concentrate pump ..." to "... the given concentrate pump ...". Reply by Vinayakam Mani on 01 October 2024, 11:51 > Done. Revision Comment by Sean Nash on 30 September 2024, 12:49 https://devapps.diality.us/cru/DD-LEAH-227-1#c20312 Change to "... turns on a given concentrate pump ...". Reply by Vinayakam Mani on 01 October 2024, 11:51 > Done. Revision Comment by Sean Nash on 30 September 2024, 12:51 https://devapps.diality.us/cru/DD-LEAH-227-1#c20313 Change to "... pump turned on." Reply by Vinayakam Mani on 01 October 2024, 11:51 > Done. Revision Comment by Sean Nash on 30 September 2024, 12:52 https://devapps.diality.us/cru/DD-LEAH-227-1#c20314 Shouldn't the "hasTurnOnPumpsBeenRequested" flag be set to FALSE in here somewhere? Reply by Vinayakam Mani on 01 October 2024, 11:16 > This flag remains true until the PUMP requested to be OFF > state. No change is needed. Revision Comment by Sean Nash on 30 September 2024, 12:53 https://devapps.diality.us/cru/DD-LEAH-227-1#c20315 Change to "ID of concentrate pump to ramp." Reply by Vinayakam Mani on 01 October 2024, 11:50 > Done. Revision Comment by Sean Nash on 30 September 2024, 16:14 https://devapps.diality.us/cru/DD-LEAH-227-1#c20320 Update brief and param to say that this function is only operating on the given pump. This and all functions with pump ID as param. Reply by Vinayakam Mani on 01 October 2024, 11:50 > Done. Revision Comment by Sean Nash on 30 September 2024, 16:23 https://devapps.diality.us/cru/DD-LEAH-227-1#c20321 Can target speed be negative (for reverse)? If so, why are we not doing a fabs() above (about 8 lines up)? If not, why are we doing fabs() here? Reply by Vinayakam Mani on 01 October 2024, 11:08 > Target speed is always positive. Fabs is not required in this > place and removed. Revision Comment by Sean Nash on 30 September 2024, 16:25 https://devapps.diality.us/cru/DD-LEAH-227-1#c20322 Should hasTgtBeenReached be set to TRUE here? Reply by Vinayakam Mani on 01 October 2024, 11:13 > Not required, since the pump will be in "OFF" state when > target speed is close to zero. The flag is used to > communicate when ramp reached and moved to control state. Revision Comment by Sean Nash on 30 September 2024, 16:27 https://devapps.diality.us/cru/DD-LEAH-227-1#c20323 I know we've tended to only mention data that is static to this unit in the Input/Output sections, but I think we should also mentions important inputs from outside the unit (e.g. FPGA readings in this case). Reply by Vinayakam Mani on 01 October 2024, 11:50 > Done. Revision Comment by Sean Nash on 30 September 2024, 16:29 https://devapps.diality.us/cru/DD-LEAH-227-1#c20324 Add details item for given alarm that may be triggered in this function. Reply by Vinayakam Mani on 01 October 2024, 11:50 > Done. Revision Comment by Sean Nash on 30 September 2024, 16:31 https://devapps.diality.us/cru/DD-LEAH-227-1#c20325 This looks like a copy/paste error. I don't think we're setting step speed in this function. Reply by Vinayakam Mani on 01 October 2024, 11:50 > Done. Revision Comment by Sean Nash on 30 September 2024, 16:32 https://devapps.diality.us/cru/DD-LEAH-227-1#c20326 I thought we were using "\b Message \b Sent:", not "\b Messages:". Reply by Vinayakam Mani on 01 October 2024, 11:50 > Done. Revision Comment by Sean Nash on 30 September 2024, 16:34 https://devapps.diality.us/cru/DD-LEAH-227-1#c20327 Are you going to switch to common functions for overrides in next branch? Reply by Vinayakam Mani on 01 October 2024, 11:50 > Will be done once your changes available in staging. ---------------------------------------- File: firmware/App/Controllers/ConcentratePumps.h Revision Comment by Sean Nash on 30 September 2024, 10:20 https://devapps.diality.us/cru/DD-LEAH-227-1#c20293 Remove blank line. Reply by Vinayakam Mani on 01 October 2024, 11:49 > Done. ---------------------------------------- File: firmware/App/Tasks/TaskGeneral.c Revision Comment by Sean Nash on 30 September 2024, 16:46 https://devapps.diality.us/cru/DD-LEAH-227-1#c20333 Change comment to "control concentrate pumps". Controls should be called after operation modes, so move this call down. Reply by Vinayakam Mani on 01 October 2024, 12:09 > This is monitor of concentrate pump ( > execConcentratePumpMonitor()). control one( > execConcentratePumpController()) is called after operation > mode. ---------------------------------------- File: firmware/App/Drivers/TemperatureSensors.c Revision Comment by Sean Nash on 30 September 2024, 16:55 https://devapps.diality.us/cru/DD-LEAH-227-1#c20336 I would move these up to definitions section. Reply by Vinayakam Mani on 01 October 2024, 11:49 > Done. Revision Comment by Sean Nash on 30 September 2024, 16:57 https://devapps.diality.us/cru/DD-LEAH-227-1#c20337 Why not initialize to lastBaroTempReadCounter.data as assigned below? Then we don't need assignment below. Reply by Vinayakam Mani on 01 October 2024, 11:49 > Done. Revision Comment by Sean Nash on 30 September 2024, 16:58 https://devapps.diality.us/cru/DD-LEAH-227-1#c20338 Too broad. At least narrow down to "temperature sensor readings from FPGA". Reply by Vinayakam Mani on 01 October 2024, 11:49 > Done. Revision Comment by Sean Nash on 30 September 2024, 17:01 https://devapps.diality.us/cru/DD-LEAH-227-1#c20339 Change to "... updates the ADC count moving average for a given temperature sensor." Reply by Vinayakam Mani on 01 October 2024, 11:49 > Done. Revision Comment by Sean Nash on 30 September 2024, 17:03 https://devapps.diality.us/cru/DD-LEAH-227-1#c20340 Seems like we don't need a switch here. All sensors do the same thing. Remove switch. Reply by Vinayakam Mani on 01 October 2024, 11:48 > Done. Revision Comment by Sean Nash on 30 September 2024, 17:03 https://devapps.diality.us/cru/DD-LEAH-227-1#c20341 Remove blank line. Reply by Vinayakam Mani on 01 October 2024, 11:48 > Done. Revision Comment by Sean Nash on 30 September 2024, 17:04 https://devapps.diality.us/cru/DD-LEAH-227-1#c20342 Alarm appears to be on a CRC mismatch or a timeout, not a CRC change. Reply by Vinayakam Mani on 01 October 2024, 11:48 > Done. Revision Comment by Sean Nash on 30 September 2024, 17:06 https://devapps.diality.us/cru/DD-LEAH-227-1#c20343 Are you doing overrides later? Reply by Vinayakam Mani on 01 October 2024, 11:48 > Will be done once your changes available in staging. ---------------------------------------- File: firmware/App/Monitors/Pressure.c Revision Comment by Sean Nash on 30 September 2024, 16:51 https://devapps.diality.us/cru/DD-LEAH-227-1#c20334 Looks like it's returning converted pressure (and that it will eventually return converted & calibrated pressure). Reply by Vinayakam Mani on 01 October 2024, 11:58 > Done Revision Comment by Sean Nash on 30 September 2024, 16:53 https://devapps.diality.us/cru/DD-LEAH-227-1#c20335 I don't see these params in the function header and they don't appear to be used or needed. Reply by Vinayakam Mani on 01 October 2024, 11:58 > Done. ---------------------------------------- File: firmware/App/Monitors/Temperature.c Revision Comment by Sean Nash on 30 September 2024, 16:41 https://devapps.diality.us/cru/DD-LEAH-227-1#c20329 It's actually the temperature sensors state, not the sensor that may be invalid. Reply by Vinayakam Mani on 01 October 2024, 12:03 > Done. Revision Comment by Sean Nash on 30 September 2024, 16:42 https://devapps.diality.us/cru/DD-LEAH-227-1#c20330 elapsedTime seems more like a start time. Reply by Vinayakam Mani on 01 October 2024, 12:03 > Done Revision Comment by Sean Nash on 30 September 2024, 16:44 https://devapps.diality.us/cru/DD-LEAH-227-1#c20331 There does not appear to be any variable named tempValuesForPublication. If this is intended to be a general description, use plain English instead of camel case. Reply by Vinayakam Mani on 01 October 2024, 12:05 > Done. ---------------------------------------- File: firmware/App/Monitors/Temperature.h Revision Comment by Sean Nash on 30 September 2024, 16:38 https://devapps.diality.us/cru/DD-LEAH-227-1#c20328 Module to unit. Reply by Vinayakam Mani on 01 October 2024, 12:06 > Done. ---------------------------------------- File: firmware/App/Tasks/TaskPriority.c Revision Comment by Sean Nash on 30 September 2024, 16:45 https://devapps.diality.us/cru/DD-LEAH-227-1#c20332 Change comment to "Monitor temperatures". Reply by Vinayakam Mani on 01 October 2024, 12:11 > done. --- ID: DD-LEAH-227-1 https://devapps.diality.us/cru/DD-LEAH-227-1 Title: DD-LEAH-227_FW DD Concentrate Pumps Control Statement of Objectives: State: Closed Summary: Author: Vinayakam Mani Moderator: Vinayakam Mani Reviewers: (2 active, 2 completed*) Sean Nash (*) Michael Garthwaite (*) jpaguio Dara Navaei