This is a list of all comments for LEAHI-DD-FIRMWARE-LDT-2198-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Drivers/PressureSensor.c Revision Comment by Sean Nash on 09 September 2025, 08:37 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24127 If we don't need these, remove them. Reply by Raghu Kallala on 09 September 2025, 11:05 > removed Revision Comment by Sean Nash on 10 September 2025, 14:26 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24212 Remove blank line. Reply by Raghu Kallala on 11 September 2025, 06:50 > removed Revision Comment by Sean Nash on 09 September 2025, 08:40 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24130 These M sensors were treated differently above depending on beta h/w test config. Now that you have moved them down here, that differentiation is missing and appears to be doing the alpha thing only. Check w/ Vinay on desired handling. Do we even need this test config on beta build? Reply by Raghu Kallala on 09 September 2025, 11:15 > M sensors are high pressure i.e. -1 to 10 bar > Also removed the check for test config for D9_PRES Revision Comment by Sean Nash on 09 September 2025, 08:45 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24131 Why aren't we doing these checks anymore? Reply by Raghu Kallala on 09 September 2025, 11:06 > ReadCount and ErrorCount support from FPGA has been removed ---------------------------------------- File: firmware/App/Services/Messaging.c Revision Comment by Sean Nash on 10 September 2025, 09:28 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24181 Remove extra blank line. Reply by Raghu Kallala on 11 September 2025, 06:52 > removed Revision Comment by Sean Nash on 10 September 2025, 09:32 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24182 Remove extra blank line. Reply by Raghu Kallala on 11 September 2025, 06:52 > removed Revision Comment by Sean Nash on 10 September 2025, 09:33 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24183 Remove extra blank line. Reply by Raghu Kallala on 11 September 2025, 06:53 > removed ---------------------------------------- File: firmware/App/Controllers/Valves.c Revision Comment by Michael Garthwaite on 11 September 2025, 14:02 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24254 needs assignment alignment. Reply by Sean Nash on 12 September 2025, 09:53 > Fixed. Revision Comment by Sean Nash on 25 September 2025, 08:47 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24445 Why the extra leading zeroes? If you want to ensure the literal is treated like a U32, you can do 0x00000001 or simply 1U, but I suspect the compiler will default to 32 bits anyway. Reply by Vinayakam Mani on 03 October 2025, 14:45 > Done. ---------------------------------------- File: firmware/App/Modes/ModeGenDialysate.c Revision Comment by Vinayakam Mani on 16 September 2025, 13:35 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24303 [~mgarthwaite]: Can we remove the M4 valve controls (across) in DD and keep it only as part of the IO/FP? So that the M4 valve actuation is done by one entity. Reply by Michael Garthwaite on 16 September 2025, 14:33 > fixed. removed m4 from dd side Reply by Vinayakam Mani on 18 September 2025, 09:32 > I have removed multiple M4 valve control across in DD. > IO/FP will govern the M4 valve control. ---------------------------------------- File: firmware/App/FPCommon.h Revision Comment by Sean Nash on 09 September 2025, 14:34 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24166 Do we even need a FPCommon.h? Reply by Michael Garthwaite on 11 September 2025, 08:32 > I dont think the file or the versioning it if we plan to have > the same SW version for the DD/IOFP. Dialin will always > display the same version # for both sub systems. Reply by Michael Garthwaite on 11 September 2025, 13:55 > Removed FPcommon.h Revision Comment by Sean Nash on 09 September 2025, 14:33 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24165 Needed? Reply by Michael Garthwaite on 11 September 2025, 13:55 > Removed FPcommon.h Revision Comment by Sean Nash on 09 September 2025, 14:33 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24164 Is FP version meaningful? I think we will only have a DD f/w version and a DD FPGA version. Reply by Michael Garthwaite on 11 September 2025, 13:55 > Removed FPcommon.h ---------------------------------------- File: firmware/App/Modes/FPModes/FPOperationModes.c Revision Comment by Vinayakam Mani on 16 September 2025, 13:41 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24304 Is it FP software fault, instead of TD fault? Reply by Michael Garthwaite on 16 September 2025, 14:34 > fixed. thanks ---------------------------------------- File: firmware/App/Modes/FPModes/ModePreGenPermeateDefeatured.c Revision Comment by Vinayakam Mani on 16 September 2025, 13:46 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24305 Align comments. Reply by Michael Garthwaite on 16 September 2025, 14:34 > fixed. thanks ---------------------------------------- File: firmware/App/Services/AlarmMgmtSWFaults.h Revision Comment by Sean Nash on 09 September 2025, 14:12 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24157 DD_SW_FAULT_ID_T Reply by Michael Garthwaite on 11 September 2025, 08:41 > Fixed. thanks! Revision Comment by Sean Nash on 09 September 2025, 14:12 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24158 FP_SW_FAULT_ID_T Reply by Michael Garthwaite on 11 September 2025, 08:41 > Fixed. thanks! ---------------------------------------- File: firmware/App/Services/Messaging.h Revision Comment by Sean Nash on 09 September 2025, 14:09 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24156 Didn't you add a source param to these broadcast alarm functions? Reply by Michael Garthwaite on 11 September 2025, 08:31 > yes they should have the parameters in the last spot ---------------------------------------- File: firmware/App/Tasks/TaskGeneral.c Revision Comment by Sean Nash on 09 September 2025, 14:26 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24161 run DD operation ... Reply by Michael Garthwaite on 11 September 2025, 08:51 > renamed. Reply by Sean Nash on 11 September 2025, 11:16 > Was looking to clarify comment actually. Reply by Michael Garthwaite on 11 September 2025, 14:00 > also added. Revision Comment by Sean Nash on 09 September 2025, 14:29 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24163 Expecting to see added FP controllers executed from general task. Reply by Michael Garthwaite on 11 September 2025, 08:46 > they are added. ---------------------------------------- File: firmware/source/sys_main.c Revision Comment by Sean Nash on 09 September 2025, 14:34 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24167 Don't touch code outside of USER CODE sections. Remove blank line. Reply by Michael Garthwaite on 11 September 2025, 08:45 > removed Revision Comment by Sean Nash on 09 September 2025, 14:36 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24168 What about non-mode FP init calls (e.g. monitors, etc...) Reply by Michael Garthwaite on 11 September 2025, 08:34 > They are added ---------------------------------------- File: MsgDefs.h Revision Comment by Sean Nash on 09 September 2025, 14:39 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24170 Remove DD => FP requests and FP => DD responses. Reply by Michael Garthwaite on 11 September 2025, 08:44 > Removed ---------------------------------------- File: firmware/App/Monitors/Level.h Revision Comment by Sean Nash on 09 September 2025, 13:46 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24136 Do we need markers for first/last DD and IOFP floats? Reply by Michael Garthwaite on 11 September 2025, 08:22 > Ive added the markers for DD. Since the IOFP is only one > float, i thought markers for IOFP weren't needed. Reply by Sean Nash on 11 September 2025, 11:10 > I think even with 1 float, we should have markers to > generalize handling of IOFP floats (easier if we add a > float later). Reply by Michael Garthwaite on 11 September 2025, 13:58 > added. ---------------------------------------- File: firmware/App/Services/FPInterface.h Revision Comment by Sean Nash on 09 September 2025, 14:15 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24159 Opmode functions removed? Reply by Michael Garthwaite on 11 September 2025, 08:49 > removed headers ---------------------------------------- File: firmware/App/Services/TDInterface.c Revision Comment by Sean Nash on 09 September 2025, 14:25 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24160 Remove extra blank lines. Reply by Michael Garthwaite on 11 September 2025, 08:49 > removed ---------------------------------------- File: firmware/App/Monitors/Pressure.c Revision Comment by Sean Nash on 09 September 2025, 13:47 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24137 Can we just remove these? Reply by Raghu Kallala on 10 September 2025, 09:47 > removed Revision Comment by Sean Nash on 09 September 2025, 13:47 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24138 Align comments. Reply by Raghu Kallala on 10 September 2025, 09:47 > Fixed Revision Comment by Sean Nash on 09 September 2025, 13:48 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24139 Align "=". Reply by Raghu Kallala on 10 September 2025, 09:47 > Fixed Revision Comment by Sean Nash on 09 September 2025, 13:49 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24140 Remove comment? And TODO comments should be all caps. Reply by Raghu Kallala on 10 September 2025, 10:22 > removed and added TODO comment in line 335 Revision Comment by Sean Nash on 09 September 2025, 13:50 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24142 Add TODO comment. Reply by Raghu Kallala on 10 September 2025, 10:21 > Added Revision Comment by jpaguio on 23 September 2025, 10:23 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24424 Do you want to add a space for the baro pressure sensor from which we receive from the TD? Reply by Sean Nash on 24 September 2025, 09:10 > I believe baro is being moved to the TD firmware. DD will > have to get baro readings from TD broadcasts. ---------------------------------------- File: MessagePayloads.h Revision Comment by Sean Nash on 10 September 2025, 10:31 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24199 Is there a reason why a few payloads are defined here while most are defined in the associated unit's header file? If we want to define payloads here, shouldn't we define them all? And we would want to wrap each payload in a doxygen group so that they get documented in the write place. Reply by Michael Garthwaite on 11 September 2025, 08:38 > [~vmani] Reply by Vinayakam Mani on 11 September 2025, 09:39 > I guess, the common message payloads (TD <-> DD, DD <-> > IOFP) are defined here to avoid duplicate copy in their > subsystems. For instance, TD wants to know the TMP pressure > value, but it's been measured in DD subsystem. > > These are defined under "Messaging" group as part of > doxygen grouping for now. Revision Comment by Sean Nash on 10 September 2025, 13:17 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24208 Are these RO commands still relevant? Reply by Michael Garthwaite on 11 September 2025, 08:37 > these are still relevant for the DD to call > requestFPGeneratePermeate() Revision Comment by Sean Nash on 09 September 2025, 14:40 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24171 Add blank line before pragma. Reply by Raghu Kallala on 10 September 2025, 10:28 > Added ---------------------------------------- File: firmware/App/Monitors/Temperature.c Revision Comment by Sean Nash on 09 September 2025, 13:53 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24145 Can we remove this line of code? Reply by Raghu Kallala on 10 September 2025, 09:50 > Removed Revision Comment by Sean Nash on 09 September 2025, 13:56 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24148 Combine to one publish function w/ separate counters, intervals, msg ID, payload structures and CAN channels (see pressures for example). Revision Comment by Sean Nash on 09 September 2025, 13:54 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24146 Add blank line before function header. Reply by Raghu Kallala on 10 September 2025, 09:50 > Added Revision Comment by Sean Nash on 09 September 2025, 13:54 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24147 Add TODO comment. Reply by Vinayakam Mani on 10 September 2025, 12:07 > Done. Revision Comment by Sean Nash on 09 September 2025, 13:57 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24149 Why are we only filtering D4 and D50? Reply by Vinayakam Mani on 10 September 2025, 12:03 > it's based on the application need. Currently D4 and D50 set > based on the heaters control need. Also D28, D30 temprature > moving average done for alarm handling. rest will be handled > as needed. Revision Comment by Sean Nash on 09 September 2025, 14:00 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24150 Why are these commented out. They are in the payload, so we should set them. Reply by Raghu Kallala on 10 September 2025, 09:52 > Was waiting for changes from Conductivity, uncommented now. ---------------------------------------- File: firmware/App/Monitors/Temperature.h Revision Comment by Sean Nash on 09 September 2025, 13:53 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24143 Should IO sensors come first? Reply by Raghu Kallala on 10 September 2025, 09:55 > Moved M1, M3 to top Revision Comment by Sean Nash on 09 September 2025, 13:53 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24144 Remove extra blank line. Reply by Raghu Kallala on 10 September 2025, 09:55 > removed ---------------------------------------- File: firmware/App/Services/AlarmMgmtDD.c Revision Comment by Sean Nash on 11 September 2025, 11:12 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24246 Align "=" Revision Comment by Sean Nash on 09 September 2025, 14:01 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24151 Do we need separate versions of this flag? Reply by Michael Garthwaite on 11 September 2025, 08:41 > added FP flag and public get function Revision Comment by Sean Nash on 09 September 2025, 14:03 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24152 Add a blank line between declarations and code. Reply by Michael Garthwaite on 11 September 2025, 08:41 > Fixed. thanks! Revision Comment by Sean Nash on 09 September 2025, 14:04 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24153 Let's change "index" to "alarm" for clarity. Reply by Michael Garthwaite on 11 September 2025, 08:40 > Fixed. thanks! Revision Comment by Sean Nash on 11 September 2025, 11:13 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24247 Should be 2 blank lines above and below test support banner. Revision Comment by Sean Nash on 09 September 2025, 14:06 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24154 Fix spacing at end of if statement. Reply by Michael Garthwaite on 11 September 2025, 08:40 > Fixed. thanks! Revision Comment by Sean Nash on 09 September 2025, 14:06 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24155 Fix spacing at end of if statement. Reply by Michael Garthwaite on 11 September 2025, 08:40 > Fixed. thanks! ---------------------------------------- File: FPGA.c Revision Comment by Sean Nash on 10 September 2025, 09:34 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24184 Fix comment. Reply by Vinayakam Mani on 10 September 2025, 12:00 > Done. Revision Comment by Sean Nash on 10 September 2025, 09:40 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24185 Read page starts at 0x200 now. Reply by Vinayakam Mani on 10 September 2025, 12:00 > Done. ---------------------------------------- File: firmware/App/Controllers/DialysatePumps.c Revision Comment by Sean Nash on 10 September 2025, 09:11 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24176 Initialize in init function, not here. Reply by Vinayakam Mani on 10 September 2025, 12:02 > Done. Revision Comment by Sean Nash on 10 September 2025, 09:16 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24177 Add a TODO comment. Is there a workable value to set target pressure to for now until we get baro from TD? Reply by Vinayakam Mani on 10 September 2025, 12:01 > Done. Yes. In the Init function, there is a default value > set. ---------------------------------------- File: firmware/App/Drivers/ConductivitySensors.c Revision Comment by Raghu Kallala on 11 September 2025, 08:39 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24229 Should it be FPGA_PERS_ERROR_P18_COND_SENSOR instead of P9? Reply by Sean Nash on 12 September 2025, 09:57 > Fixed. ---------------------------------------- File: firmware/App/Drivers/TemperatureSensors.c Revision Comment by Sean Nash on 10 September 2025, 09:22 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24178 Should we just remove this whole if statement? Reply by Raghu Kallala on 10 September 2025, 14:25 > removed Revision Comment by Sean Nash on 10 September 2025, 09:23 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24179 Should we remove this whole function? Reply by Raghu Kallala on 10 September 2025, 14:25 > removed ---------------------------------------- File: firmware/App/Monitors/Conductivity.c Revision Comment by Sean Nash on 10 September 2025, 09:24 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24180 Add blank line below banner. Reply by Vinayakam Mani on 10 September 2025, 12:00 > Done. ---------------------------------------- File: firmware/App/Monitors/Level.c Revision Comment by Sean Nash on 16 September 2025, 09:42 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24299 Align "=" Reply by Michael Garthwaite on 16 September 2025, 14:34 > fixed. thanks Revision Comment by Sean Nash on 15 September 2025, 13:58 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24266 Consider making function name a little clearer that we are getting the status from the FPGA. Reply by Michael Garthwaite on 15 September 2025, 14:50 > renamed to readFloaterLevelstatus() Revision Comment by Sean Nash on 15 September 2025, 13:56 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24263 Read FPGA D6 status once at top of D6 if statement into a local var and use that in these if conditions so you don't have to keep calling the fpga function. Reply by Michael Garthwaite on 15 September 2025, 14:49 > fixed. thanks Revision Comment by Sean Nash on 15 September 2025, 13:56 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24264 Read FPGA P25 status once at top of P25 if statement into a local var and use that in these if conditions so you don't have to keep calling the fpga function. Reply by Michael Garthwaite on 15 September 2025, 14:49 > fixed. thanks Revision Comment by Sean Nash on 15 September 2025, 13:57 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24265 Add blank line before return statement. Reply by Michael Garthwaite on 15 September 2025, 14:49 > fixed. thanks ---------------------------------------- File: firmware/App/Controllers/ConcentratePumps.h Revision Comment by Sean Nash on 16 September 2025, 09:38 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1#c24298 Are we still supporting FMI pumps? Reply by Vinayakam Mani on 16 September 2025, 11:07 > Removed FMI support for Beta. will maintain FMI support only > in Alpha --- ID: LEAHI-DD-FIRMWARE-LDT-2198-1 https://devapps.diality.us/cru/LEAHI-DD-FIRMWARE-LDT-2198-1 Title: LEAHI-DD-FIRMWARE-LDT-2198_Beta 1.0 FW Integration - DD Implementation Statement of Objectives: State: Closed Summary: Author: Sean Nash Moderator: Sean Nash Reviewers: (4 active, 3 completed*) Vinayakam Mani (*) Michael Garthwaite (*) Raghu Kallala (*) jpaguio Dara Navaei Vendor - TEL - Sivvanarayana Kurapati Daniel Ho