This is a list of all comments for RO-LDT-1809-3. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/ROPump.c Revision Comment by Michael Garthwaite on 13 August 2025, 14:03 https://devapps.diality.us/cru/RO-LDT-1809-3#c23743 We can remove this instead of commenting it out Reply by Raghu Kallala on 13 August 2025, 14:58 > Forgot this during cleanup. > Removed, Thanks Reply by Michael Garthwaite on 14 August 2025, 11:13 > Resolved Revision Comment by Sean Nash on 11 August 2025, 09:35 https://devapps.diality.us/cru/RO-LDT-1809-3#c23591 Remove extra blank line. Reply by Raghu Kallala on 11 August 2025, 10:16 > Removed. Revision Comment by Sean Nash on 11 August 2025, 09:37 https://devapps.diality.us/cru/RO-LDT-1809-3#c23592 What if .data is zero but .ovdata is > 0? Why aren't we using the get function to access the data anymore? Looks like we're still using the get function inside the if to initialize the control. Reply by Raghu Kallala on 11 August 2025, 10:11 > To avoid resetting data and ovdata to zero when we send an > override on top of another override. > Using the data value to check if the control should be > handled or not as the ovdata is been assigned to data in all > set functions Reply by Raghu Kallala on 26 August 2025, 10:48 > Resolved the behavior in latest commit. Revision Comment by Sean Nash on 11 August 2025, 09:40 https://devapps.diality.us/cru/RO-LDT-1809-3#c23593 Remove these blank lines. Reply by Raghu Kallala on 11 August 2025, 10:10 > Done, thank you Revision Comment by Sean Nash on 11 August 2025, 09:42 https://devapps.diality.us/cru/RO-LDT-1809-3#c23594 Why not use get function in these ifs? Reply by Raghu Kallala on 11 August 2025, 10:06 > get functions return data or ovdata based on override flag > value. > That makes it difficult to set the ovdata to zero if we are > sending an override on top of another override. > > Instead checking for data value all the time as we are > assigning the ovdata to data in all set functions. Reply by Raghu Kallala on 26 August 2025, 10:49 > Resolved the behavior in latest commit. Revision Comment by Sean Nash on 26 August 2025, 13:40 https://devapps.diality.us/cru/RO-LDT-1809-3#c23876 Add new @param to function header. Reply by Raghu Kallala on 26 August 2025, 14:42 > Done Revision Comment by Sean Nash on 26 August 2025, 13:41 https://devapps.diality.us/cru/RO-LDT-1809-3#c23877 Add a comment here to help reader understand what we're doing. Reply by Raghu Kallala on 26 August 2025, 14:43 > Added comment line Revision Comment by Sean Nash on 26 August 2025, 13:42 https://devapps.diality.us/cru/RO-LDT-1809-3#c23878 returning here breaks one of our coding standards. Should only be one return at end of a function. Reply by Raghu Kallala on 26 August 2025, 14:43 > Removed return and replaced with skip flag Revision Comment by Michael Garthwaite on 12 August 2025, 13:55 https://devapps.diality.us/cru/RO-LDT-1809-3#c23653 setROPumpTargetPressure doesnt have this check for 0. Is it something that we would need across all 3 set functions or not needed at all? Do we care that ovInitData is zero before assigning it to data if are overriding? Reply by Michael Garthwaite on 13 August 2025, 13:07 > Resolved. Revision Comment by Sean Nash on 19 August 2025, 14:24 https://devapps.diality.us/cru/RO-LDT-1809-3#c23839 Do we not still need to zero these other targets in these set functions? Reply by Raghu Kallala on 20 August 2025, 08:57 > These code lines are removed in latest commit Revision Comment by Sean Nash on 27 August 2025, 13:58 https://devapps.diality.us/cru/RO-LDT-1809-3#c23909 Change to ( skipSet != TRUE ) Reply by Raghu Kallala on 27 August 2025, 14:29 > changed Revision Comment by Sean Nash on 26 August 2025, 13:44 https://devapps.diality.us/cru/RO-LDT-1809-3#c23881 Add @param for new param. Reply by Raghu Kallala on 26 August 2025, 14:43 > Done Revision Comment by Sean Nash on 26 August 2025, 13:44 https://devapps.diality.us/cru/RO-LDT-1809-3#c23880 Add a comment. Reply by Raghu Kallala on 26 August 2025, 14:43 > Added comment line Revision Comment by Sean Nash on 26 August 2025, 13:44 https://devapps.diality.us/cru/RO-LDT-1809-3#c23879 Cannot return here. Reply by Raghu Kallala on 26 August 2025, 14:44 > Removed multiple returns Revision Comment by Sean Nash on 27 August 2025, 13:59 https://devapps.diality.us/cru/RO-LDT-1809-3#c23910 Change to ( skipSet != TRUE ) Reply by Raghu Kallala on 27 August 2025, 14:29 > Changed Revision Comment by Sean Nash on 26 August 2025, 13:44 https://devapps.diality.us/cru/RO-LDT-1809-3#c23882 Add @param for new param. Reply by Raghu Kallala on 26 August 2025, 14:44 > Done Revision Comment by Sean Nash on 26 August 2025, 13:44 https://devapps.diality.us/cru/RO-LDT-1809-3#c23883 Add a comment. Reply by Raghu Kallala on 26 August 2025, 14:44 > Added comment line Revision Comment by Sean Nash on 26 August 2025, 13:45 https://devapps.diality.us/cru/RO-LDT-1809-3#c23884 Cannot return here. Reply by Raghu Kallala on 26 August 2025, 14:44 > Removed multiple return Revision Comment by Sean Nash on 27 August 2025, 13:59 https://devapps.diality.us/cru/RO-LDT-1809-3#c23911 Change to ( skipSet != TRUE ) Reply by Raghu Kallala on 27 August 2025, 14:29 > Changed Revision Comment by Sean Nash on 27 August 2025, 14:00 https://devapps.diality.us/cru/RO-LDT-1809-3#c23912 Indents don't look right. Reply by Raghu Kallala on 27 August 2025, 14:53 > Fixed Revision Comment by Michael Garthwaite on 12 August 2025, 14:00 https://devapps.diality.us/cru/RO-LDT-1809-3#c23657 I think this check needs to happen in the handler rather in the set functions. Please update MIN_RO_PRESSURE_PSI and MIN_FLUID_PUMP_DUTY_CYCLE_PCT to 0 and reflect the change in their respective set functions. Reply by Michael Garthwaite on 13 August 2025, 13:07 > Resolved. Revision Comment by Sean Nash on 26 August 2025, 13:45 https://devapps.diality.us/cru/RO-LDT-1809-3#c23885 We want 2 blank lines before and after test support banner. So restore this deleted row. Reply by Raghu Kallala on 26 August 2025, 14:46 > Added blank lines as needed Revision Comment by Sean Nash on 19 August 2025, 14:34 https://devapps.diality.us/cru/RO-LDT-1809-3#c23840 Can we remove this commented line? Seems redundant anyway. Reply by Raghu Kallala on 20 August 2025, 08:58 > Removed Revision Comment by Sean Nash on 19 August 2025, 09:00 https://devapps.diality.us/cru/RO-LDT-1809-3#c23823 Declare result at top of scope. Keep assignment here. Reply by Raghu Kallala on 19 August 2025, 10:15 > Fixed. ---------------------------------------- File: TestSupport.h Revision Comment by Sean Nash on 26 August 2025, 13:40 https://devapps.diality.us/cru/RO-LDT-1809-3#c23875 Why are these here? If not truly common to all f/w, consider moving these functions to FluidPump or ROPump. Reply by Raghu Kallala on 26 August 2025, 14:49 > Moved to FluidPump --- ID: RO-LDT-1809-3 https://devapps.diality.us/cru/RO-LDT-1809-3 Title: RO-LDT-1809_Iofp WS Setup And Bug Fixing Statement of Objectives: State: Closed Summary: Author: Raghu Kallala Moderator: Raghu Kallala Reviewers: (6 active, 2 completed*) Sean Nash (*) Michael Garthwaite (*) Nicholas Ramirez Tiffany Mejia Vinayakam Mani Dara Navaei Behrouz NematiPour Daniel Ho