This is a list of all comments for DD-LEAH-225-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/DialysatePumps.c Revision Comment by Sean Nash on 16 October 2024, 08:38 https://devapps.diality.us/cru/DD-LEAH-225-1#c20446 I don't see a low-level driver unit for these pumps. Consider breaking this unit up into a driver and a controller. Reply by Vinayakam Mani on 16 October 2024, 11:21 > looks the low-level monitoring of speed, current and > directions are coupled with the pumps state machines. When > the pump is in OFF state, you expect certain speed and > current, raise alarm if they are not in range and same way > for ramp and control state as well. Will Re-evaluate in > future the possibility of separation. Revision Comment by Sean Nash on 16 October 2024, 09:33 https://devapps.diality.us/cru/DD-LEAH-225-1#c20457 Are both pumps going to have the same coefficients? How did you come up with 1.0 for P and I? Reply by Vinayakam Mani on 16 October 2024, 15:23 > Added separate pump coefficients. The pump coefficients to be > finetuned once we get more information on the pump speed/PSI > relationship. Revision Comment by Sean Nash on 16 October 2024, 09:00 https://devapps.diality.us/cru/DD-LEAH-225-1#c20451 For target/measured speeds, can they be negative? If so, does negative indicate reverse? Can pumps be run in both directions? Clarify these things in comments. Reply by Vinayakam Mani on 16 October 2024, 15:25 > Its Forward direction only. added comments. Revision Comment by Sean Nash on 16 October 2024, 08:33 https://devapps.diality.us/cru/DD-LEAH-225-1#c20444 Remove extra space before "=". Reply by Vinayakam Mani on 16 October 2024, 15:25 > done. Revision Comment by Sean Nash on 16 October 2024, 08:34 https://devapps.diality.us/cru/DD-LEAH-225-1#c20445 The .override field of overrideable variables should also be initialized to OVERRIDE_RESET. Might as well initialize .ovData too. Reply by Vinayakam Mani on 16 October 2024, 15:26 > done. Revision Comment by Sean Nash on 16 October 2024, 08:53 https://devapps.diality.us/cru/DD-LEAH-225-1#c20447 Why the "XXX" in alarm enums? Reply by Vinayakam Mani on 16 October 2024, 15:26 > Added both fresh and spent dialysate alarms in comments. > done. Revision Comment by Sean Nash on 16 October 2024, 08:54 https://devapps.diality.us/cru/DD-LEAH-225-1#c20448 This will always fail. Until calibration record available, set this to TRUE. Reply by Vinayakam Mani on 16 October 2024, 15:26 > done. Revision Comment by Sean Nash on 16 October 2024, 08:55 https://devapps.diality.us/cru/DD-LEAH-225-1#c20449 dialysatePumps[] is an input. Reply by Vinayakam Mani on 16 October 2024, 15:26 > done. Revision Comment by Sean Nash on 16 October 2024, 08:56 https://devapps.diality.us/cru/DD-LEAH-225-1#c20450 I think we can use 0.0F here instead of NEARLY_ZERO. I see no reason or benefit to using NEARLY_ZERO. Based on this code, I am assuming pump is not or cannot be run in reverse (target speed never negative). If this is not correct, this function needs to handle negative target speeds. Reply by Vinayakam Mani on 16 October 2024, 15:26 > done. Revision Comment by Sean Nash on 16 October 2024, 09:25 https://devapps.diality.us/cru/DD-LEAH-225-1#c20454 If a new target is set while in this state, are we just staying here and letting control handle the change? Reply by Vinayakam Mani on 16 October 2024, 15:27 > handled in the 'setDialysatePumpTargetRPM' function. Revision Comment by Sean Nash on 16 October 2024, 09:26 https://devapps.diality.us/cru/DD-LEAH-225-1#c20455 Remove extra space before "=". Reply by Vinayakam Mani on 16 October 2024, 15:29 > done. Revision Comment by Sean Nash on 16 October 2024, 09:27 https://devapps.diality.us/cru/DD-LEAH-225-1#c20456 Should we change state to off state? Reply by Vinayakam Mani on 16 October 2024, 15:32 > Merged two stop functions. ---------------------------------------- File: firmware/App/Controllers/DialysatePumps.h Revision Comment by Sean Nash on 16 October 2024, 09:53 https://devapps.diality.us/cru/DD-LEAH-225-1#c20459 Do we really need a minimum? Reply by Vinayakam Mani on 16 October 2024, 15:34 > Drain pump has minimum speed condition. Will reevaluate with > the actual hardware once. ---------------------------------------- File: firmware/App/Controllers/Heaters.c Revision Comment by Sean Nash on 16 October 2024, 10:19 https://devapps.diality.us/cru/DD-LEAH-225-1#c20471 Do we need a stopHeaterSignal? Reply by Vinayakam Mani on 16 October 2024, 17:44 > startHeaterSignal used for both start and stop state > handling. Revision Comment by Sean Nash on 16 October 2024, 10:01 https://devapps.diality.us/cru/DD-LEAH-225-1#c20460 Use OVERRIDE_RESET to initialize the override field. Reply by Vinayakam Mani on 16 October 2024, 17:44 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:07 https://devapps.diality.us/cru/DD-LEAH-225-1#c20464 Should duty cycle be zeroed? Reply by Vinayakam Mani on 16 October 2024, 17:45 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:07 https://devapps.diality.us/cru/DD-LEAH-225-1#c20465 Consider adding a FIRST heater to enum. Reply by Vinayakam Mani on 16 October 2024, 17:45 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:09 https://devapps.diality.us/cru/DD-LEAH-225-1#c20466 Why does trimmer heater have its own ramp and control states? I understand the functions may be different, but that can be handled in a shared state. Reply by Vinayakam Mani on 16 October 2024, 17:45 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:11 https://devapps.diality.us/cru/DD-LEAH-225-1#c20467 Why doesn't the stopHeater function set state to off? Reply by Vinayakam Mani on 16 October 2024, 17:45 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:14 https://devapps.diality.us/cru/DD-LEAH-225-1#c20468 isHeaterOn is a strange name for a request. I suspect this field is being used as a state and a request which is a little confusing. Reply by Vinayakam Mani on 16 October 2024, 17:46 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:14 https://devapps.diality.us/cru/DD-LEAH-225-1#c20469 Why not just call stopHeater() here? And why are we checking for request at bottom of exec - shouldn't we do this first? Reply by Vinayakam Mani on 16 October 2024, 17:46 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:17 https://devapps.diality.us/cru/DD-LEAH-225-1#c20470 Why use a switch statement in some cases and an if/else here? Reply by Vinayakam Mani on 16 October 2024, 17:46 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:20 https://devapps.diality.us/cru/DD-LEAH-225-1#c20472 This is another if/else. When doing this, we are saying there will only ever be 2 heaters. Where we use a switch statement, we are making it possible to add more heaters later. We should at least be consistent about how we do this throughout the unit. Reply by Vinayakam Mani on 16 October 2024, 17:46 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:22 https://devapps.diality.us/cru/DD-LEAH-225-1#c20473 Is this TODO done in line below? Reply by Vinayakam Mani on 16 October 2024, 17:46 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:23 https://devapps.diality.us/cru/DD-LEAH-225-1#c20474 What is the difference between target temp and calculated target temp? I'm assuming this came from DG and probably not necessary in DD. Reply by Vinayakam Mani on 16 October 2024, 17:46 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:25 https://devapps.diality.us/cru/DD-LEAH-225-1#c20475 Looks like this TODO is done. Reply by Vinayakam Mani on 16 October 2024, 17:46 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:28 https://devapps.diality.us/cru/DD-LEAH-225-1#c20476 Old style Set/Reset functions. Update to new style with message as parameter and handles both set and reset. Reply by Vinayakam Mani on 16 October 2024, 17:47 > Done. ---------------------------------------- File: firmware/App/Controllers/Heaters.h Revision Comment by Sean Nash on 16 October 2024, 10:05 https://devapps.diality.us/cru/DD-LEAH-225-1#c20461 Do we need a calculated target temp? Reply by Vinayakam Mani on 16 October 2024, 17:47 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:05 https://devapps.diality.us/cru/DD-LEAH-225-1#c20462 Comment does not make clear what this is doing. Do we need it? Reply by Vinayakam Mani on 16 October 2024, 17:48 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:06 https://devapps.diality.us/cru/DD-LEAH-225-1#c20463 Maybe add Control to end of function name. Reply by Vinayakam Mani on 16 October 2024, 17:51 > Done. ---------------------------------------- File: firmware/App/Monitors/Level.c Revision Comment by Sean Nash on 16 October 2024, 10:49 https://devapps.diality.us/cru/DD-LEAH-225-1#c20479 Recommend U32 here. Reply by Vinayakam Mani on 16 October 2024, 17:55 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:49 https://devapps.diality.us/cru/DD-LEAH-225-1#c20480 Recommend U32 for i and currentLevelStatus. Reply by Vinayakam Mani on 16 October 2024, 17:55 > Done. ---------------------------------------- File: firmware/App/Monitors/Level.h Revision Comment by Sean Nash on 16 October 2024, 10:46 https://devapps.diality.us/cru/DD-LEAH-225-1#c20477 Remove extra blank line. Reply by Vinayakam Mani on 16 October 2024, 17:55 > Done. Revision Comment by Sean Nash on 16 October 2024, 10:47 https://devapps.diality.us/cru/DD-LEAH-225-1#c20478 I don't see init called in sys_main and I don't see exec called in priority task. Reply by Vinayakam Mani on 16 October 2024, 17:55 > Looks already taken care. ---------------------------------------- File: firmware/App/Monitors/Pressure.c Revision Comment by Sean Nash on 16 October 2024, 10:53 https://devapps.diality.us/cru/DD-LEAH-225-1#c20481 Keep blank line between declaration and code. Reply by Vinayakam Mani on 16 October 2024, 17:56 > Done. --- ID: DD-LEAH-225-1 https://devapps.diality.us/cru/DD-LEAH-225-1 Title: DD-LEAH-225_FW DD Dialysate Pumps Controlle Statement of Objectives: State: Closed Summary: Author: Vinayakam Mani Moderator: Vinayakam Mani Reviewers: (2 active, 2 completed*) Sean Nash (*) Michael Garthwaite (*) jpaguio Dara Navaei