This is a list of all comments for HD-DEN-17093-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/BloodLeak.c Revision Comment by Sean Nash on 16 October 2024, 13:31 https://devapps.diality.us/cru/HD-DEN-17093-1#c20485 Focus of name and comment should be on division by 16 - not on shift. Reply by Dara Navaei on 16 October 2024, 14:29 > Removed the #define Reply by Sean Nash on 16 October 2024, 14:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 16 October 2024, 13:31 https://devapps.diality.us/cru/HD-DEN-17093-1#c20486 This is a minimum interval, not "the" interval. Reply by Dara Navaei on 16 October 2024, 14:30 > Done Reply by Sean Nash on 16 October 2024, 14:58 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 16 October 2024, 13:32 https://devapps.diality.us/cru/HD-DEN-17093-1#c20487 This is too brief. Assume we want at least 3 samples to show below threshold. That's 12 seconds. Reply by Dara Navaei on 21 October 2024, 08:59 > I have changed the sampling time from 2 seconds to 1 second > and I am planning to make it 500 ms. Reply by Sean Nash on 22 October 2024, 13:14 > So now it's enough if 1 or 2 samples in the drift range is > enough to re-zero? Still seems too brief to me. Reply by Dara Navaei on 22 October 2024, 17:17 > I changed it to 4 seconds. Reply by Sean Nash on 28 October 2024, 09:31 > Systems to provide appropriate debounce time based on > max expected drift rate. Reply by Dara Navaei on 30 October 2024, 22:11 > Per the Systems team the debounce time was set to 10 > seconds. Reply by Sean Nash on 31 October 2024, 16:48 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 16 October 2024, 14:39 https://devapps.diality.us/cru/HD-DEN-17093-1#c20502 I don't think we need this field (been set). It will always be set. Reply by Dara Navaei on 16 October 2024, 14:40 > Removed it Reply by Sean Nash on 16 October 2024, 14:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 16 October 2024, 13:34 https://devapps.diality.us/cru/HD-DEN-17093-1#c20488 Average should be a float. Allows much more precision. Reduces noise which is primary reason why we are doing a moving average. Reply by Dara Navaei on 16 October 2024, 14:29 > Done Reply by Sean Nash on 16 October 2024, 14:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 31 October 2024, 16:40 https://devapps.diality.us/cru/HD-DEN-17093-1#c20692 Perhaps just a naming issue, but I can't figure out what these are for based on their name or comment. Reply by Dara Navaei on 31 October 2024, 16:44 > The variables have been removed and/or renamed Reply by Sean Nash on 31 October 2024, 16:47 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 31 October 2024, 16:41 https://devapps.diality.us/cru/HD-DEN-17093-1#c20693 Not really a range on upper side. Reply by Dara Navaei on 31 October 2024, 16:42 > Done Reply by Sean Nash on 31 October 2024, 16:50 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 October 2024, 12:50 https://devapps.diality.us/cru/HD-DEN-17093-1#c20601 Do we need a else if timeout check here? Can we get stuck waiting for response indefinitely? Reply by Dara Navaei on 22 October 2024, 16:58 > The state machine that is in charge of receiving the response > back from the sensor has a timeout and sets the command > response as ready. Reply by Sean Nash on 28 October 2024, 09:23 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 October 2024, 09:12 https://devapps.diality.us/cru/HD-DEN-17093-1#c20554 I thought we decided not to have the range anymore. Just a threshold crossing now. So > minimum drift only. Reply by Sean Nash on 28 October 2024, 09:23 > Decision is to keep the range. RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 16 October 2024, 14:47 https://devapps.diality.us/cru/HD-DEN-17093-1#c20504 You need to cast numerator and denominator as F32 or it doesn't really help. Reply by Sean Nash on 16 October 2024, 14:57 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 16 October 2024, 13:43 https://devapps.diality.us/cru/HD-DEN-17093-1#c20489 I know I told you to do shift division, but I want a float for average so we need to do real division by sample count. Reply by Dara Navaei on 16 October 2024, 14:25 > Done Reply by Sean Nash on 16 October 2024, 14:56 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 October 2024, 09:02 https://devapps.diality.us/cru/HD-DEN-17093-1#c20551 Need a default case or change switch statement to a compound if statement. Reply by Dara Navaei on 21 October 2024, 09:04 > Done Reply by Sean Nash on 21 October 2024, 13:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 16 October 2024, 13:46 https://devapps.diality.us/cru/HD-DEN-17093-1#c20490 upper and lower limits of range are BLD_NOMINAL_INTENSITY - ( setPoint * BLD_XXX_INTENSITY_OUT_OF_RANGE ). Reply by Dara Navaei on 16 October 2024, 14:24 > Done Reply by Sean Nash on 16 October 2024, 14:49 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 16 October 2024, 13:58 https://devapps.diality.us/cru/HD-DEN-17093-1#c20491 Too complicated. Simplify. Reply by Dara Navaei on 16 October 2024, 14:24 > Done Reply by Sean Nash on 16 October 2024, 14:49 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 16 October 2024, 14:04 https://devapps.diality.us/cru/HD-DEN-17093-1#c20493 I think we should do this on exit from self-test state on its way to normal. Reply by Dara Navaei on 16 October 2024, 14:51 > Done Reply by Sean Nash on 16 October 2024, 14:54 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 October 2024, 09:04 https://devapps.diality.us/cru/HD-DEN-17093-1#c20552 Is this really a "signal" function? Seems more like a "reset" function. Reply by Dara Navaei on 21 October 2024, 10:28 > Done Reply by Sean Nash on 21 October 2024, 13:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 31 October 2024, 17:07 https://devapps.diality.us/cru/HD-DEN-17093-1#c20701 Add a comment explaining we are resetting drift in range debounce timer if we are not in drift range. Reply by Dara Navaei on 01 November 2024, 15:47 > Done. Reply by Sean Nash on 04 November 2024, 08:57 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 31 October 2024, 17:15 https://devapps.diality.us/cru/HD-DEN-17093-1#c20706 Add comment explaining what we are checking here. Reply by Dara Navaei on 01 November 2024, 15:48 > Done. Reply by Sean Nash on 04 November 2024, 08:58 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 31 October 2024, 17:08 https://devapps.diality.us/cru/HD-DEN-17093-1#c20702 I thought drift timeout (30 min) was overrideable. Looks like it's hard coded #define here. Reply by Dara Navaei on 31 October 2024, 17:11 > The drift timeout does not have an override it is the zeroing > interval that has overrides. Reply by Sean Nash on 31 October 2024, 17:14 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 31 October 2024, 17:09 https://devapps.diality.us/cru/HD-DEN-17093-1#c20703 Sending event here is problematic now that we will likely defer zeroing until reservoir switch. It will send every 50ms until we finally zero. Move this to location where we are actually going to zero OR just remove the event as its maybe no longer applicable. Reply by Dara Navaei on 31 October 2024, 17:16 > Relocated the event. Reply by Sean Nash on 01 November 2024, 14:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 31 October 2024, 17:22 https://devapps.diality.us/cru/HD-DEN-17093-1#c20708 Move this too if you didn't already. Reply by Dara Navaei on 31 October 2024, 17:35 > Done Reply by Sean Nash on 31 October 2024, 17:37 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 31 October 2024, 17:36 https://devapps.diality.us/cru/HD-DEN-17093-1#c20710 Should be 2 blank lines above test support banner. Reply by Dara Navaei on 31 October 2024, 17:37 > Done Reply by Sean Nash on 01 November 2024, 14:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 31 October 2024, 17:38 https://devapps.diality.us/cru/HD-DEN-17093-1#c20713 I thought this was going to be an indexed override like when you have an array of temp sensors and the index gives you the enum of the temp sensor you want to override. You have a BOOL flag instead. Does that mean you're using a custom payload instead of the array override payload? Reply by Dara Navaei on 31 October 2024, 17:40 > No it is not custom please look at systemcommmessages. Reply by Sean Nash on 01 November 2024, 08:47 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/BloodLeak.h Revision Comment by Sean Nash on 16 October 2024, 13:28 https://devapps.diality.us/cru/HD-DEN-17093-1#c20484 Average should be float. Reply by Dara Navaei on 16 October 2024, 14:30 > Done Reply by Sean Nash on 16 October 2024, 14:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 31 October 2024, 11:05 https://devapps.diality.us/cru/HD-DEN-17093-1#c20690 Why are we distinguishing between these 2 types of zeroes? Does it matter why we zeroed? Reply by Dara Navaei on 31 October 2024, 16:43 > Removed the new varaible. Reply by Sean Nash on 31 October 2024, 16:50 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeTreatment.c Revision Comment by Sean Nash on 21 October 2024, 09:13 https://devapps.diality.us/cru/HD-DEN-17093-1#c20555 comment is too terse. describe better. Reply by Dara Navaei on 21 October 2024, 10:32 > Done. Reply by Sean Nash on 21 October 2024, 13:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 October 2024, 09:15 https://devapps.diality.us/cru/HD-DEN-17093-1#c20556 I didn't see a new state added to enum in HDDefs.h. Do we want to add a new state? I thought we were going to model this after saline bolus which is a sub-state of existing dialysis state. Reply by Dara Navaei on 21 October 2024, 10:32 > The state has been removed. Reply by Sean Nash on 21 October 2024, 13:37 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 01 November 2024, 14:47 https://devapps.diality.us/cru/HD-DEN-17093-1#c20728 Blank line between break and next case. Reply by Dara Navaei on 01 November 2024, 15:04 > Done Reply by Sean Nash on 04 November 2024, 08:59 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 01 November 2024, 14:47 https://devapps.diality.us/cru/HD-DEN-17093-1#c20729 default should come last. Reply by Dara Navaei on 01 November 2024, 15:04 > Done Reply by Sean Nash on 04 November 2024, 08:54 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 October 2024, 12:23 https://devapps.diality.us/cru/HD-DEN-17093-1#c20560 Should be &&, not ||. Reply by Dara Navaei on 21 October 2024, 12:24 > Done Reply by Sean Nash on 21 October 2024, 13:37 > I'm not seeing fix. Reply by Dara Navaei on 21 October 2024, 17:42 > Just pushed. Reply by Sean Nash on 22 October 2024, 08:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 October 2024, 13:08 https://devapps.diality.us/cru/HD-DEN-17093-1#c20566 Should this exec be moved to Dialysis.c? Seems like it should be with UF and saline bolus execs that are at the same state level. Reply by Dara Navaei on 21 October 2024, 13:17 > This exec is also used by treatment stop at the moment. I > have to make some changes then. Reply by Dara Navaei on 21 October 2024, 17:27 > Done Reply by Sean Nash on 22 October 2024, 08:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 October 2024, 12:25 https://devapps.diality.us/cru/HD-DEN-17093-1#c20562 ...stop the pump(s) before going ... Reply by Dara Navaei on 21 October 2024, 12:26 > This comment is temporary. Update the comment. Reply by Sean Nash on 22 October 2024, 12:56 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 October 2024, 13:16 https://devapps.diality.us/cru/HD-DEN-17093-1#c20568 Not clear why you're using a ternary here. Why not a simple if status == TRUE then move on to zero state? Reply by Dara Navaei on 21 October 2024, 17:31 > Added handlers for the state machine. Reply by Sean Nash on 22 October 2024, 08:47 > That doesn't answer my question. Reply by Dara Navaei on 22 October 2024, 09:08 > This code has been removed, please check the handler > functions. Also, ternary is a one-liner so sometimes it > reduces the lines of code. Reply by Sean Nash on 22 October 2024, 09:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 October 2024, 13:20 https://devapps.diality.us/cru/HD-DEN-17093-1#c20570 I think you only want to call zeroBloodLeak() once. So this state needs to move on immediately. Or, if you're doing this because the queue might be full (it really shouldn't be), then you need to have some kind of timeout so you don't get stuck here forever. Reply by Dara Navaei on 21 October 2024, 16:46 > The zeroBloodLeak() only tries up to 3 calls and then alarms. Reply by Sean Nash on 22 October 2024, 08:49 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 October 2024, 13:27 https://devapps.diality.us/cru/HD-DEN-17093-1#c20571 I'm pretty sure we need to do more to restore treatment. Look at saline bolus for what might be needed. What about restoring dialyzer (no bypass) and getting DPo back on? What about resuming UF if it wasn't already paused before zero started? Reply by Dara Navaei on 21 October 2024, 17:40 > Please check the complete handler. Reply by Sean Nash on 22 October 2024, 13:05 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/Dialysis.c Revision Comment by Sean Nash on 21 October 2024, 12:58 https://devapps.diality.us/cru/HD-DEN-17093-1#c20565 Can we just set the state and that's all we need? Look at saline bolus initiation. A quick look by me suggests we should be initiating a pressure stabilization period, for example, because bypassing the dialyzer is going to likely change the venous pressure and potentially trigger a low/high venous pressure alarm. Also, what about UF pause? We need to pause UF before starting zero process. And if UF already paused, we need to record that so that when we're done we will remember not to resume UF. There may be other things too, so take a close look at saline bolus. Reply by Dara Navaei on 21 October 2024, 13:15 > So then don't you think we should stop all the pumps, wait > for pumps to stop and then stabilize and go to bypass? Reply by Sean Nash on 21 October 2024, 13:38 > What does that have to do with my comment? Stopping the > pumps will also change the pressure. Doesn't help. I'm > asking you to look at and understand how the saline bolus > works and follow that model more closely. I think you are > missing things that we need to deal with. Reply by Dara Navaei on 21 October 2024, 13:42 > I understand your comment. I am saying stabilizing when > the pumps are running might be harder as you stop in > other places. I will match to saline state. Reply by Sean Nash on 22 October 2024, 13:13 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 October 2024, 13:12 https://devapps.diality.us/cru/HD-DEN-17093-1#c20605 This call to signal pressure stabilization should probably be moved into the request zero function. Reply by Dara Navaei on 22 October 2024, 13:26 > Done Reply by Sean Nash on 28 October 2024, 09:30 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 31 October 2024, 17:59 https://devapps.diality.us/cru/HD-DEN-17093-1#c20716 Once reservoir switch occurs, do we have to wait until the "fresh" dialysate from new reservoir reaches the BLD? Reply by Dara Navaei on 31 October 2024, 20:12 > This requests the blood zeroing and then the zeroing state > machine does the flush prior to zeroing. Reply by Dara Navaei on 01 November 2024, 10:47 > Added another state to check the extra volume. Reply by Sean Nash on 01 November 2024, 14:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 31 October 2024, 17:57 https://devapps.diality.us/cru/HD-DEN-17093-1#c20715 This looks like it would reset the flag way too soon. Shouldn't flag reset be done in the resetBloodLeakZeroing() function? Reply by Dara Navaei on 31 October 2024, 20:15 > If a zero is requested, it is checked first and then this > flag is reset. I also want this flag to be reset all the time > so we do not zero on old reservoir. Reply by Dara Navaei on 01 November 2024, 10:47 > Removed the reset function. Reply by Sean Nash on 01 November 2024, 14:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 October 2024, 12:34 https://devapps.diality.us/cru/HD-DEN-17093-1#c20564 Should this exec return next state that gets assigned to result here? Will we never exit zeroing before we get to complete? Reply by Dara Navaei on 21 October 2024, 17:30 > I added a static pointer to change the state inside one the > exec handlers. Reply by Sean Nash on 22 October 2024, 09:04 > I'm not seeing that. And why isn't the exec returning a > state that you assign to result? Reply by Sean Nash on 22 October 2024, 13:13 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 October 2024, 08:58 https://devapps.diality.us/cru/HD-DEN-17093-1#c20590 Not clear to me that this is any better than bypassing the dialyzer in previous state and is likely worse. The way you have it now, the dialysate being pushed by DPi is going to run into a stopped DPo and cause a big pressure spike and push dialysate into blood side. Reply by Dara Navaei on 22 October 2024, 10:29 > Removed the state. Reply by Sean Nash on 22 October 2024, 12:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 01 November 2024, 14:36 https://devapps.diality.us/cru/HD-DEN-17093-1#c20727 Not really all pumps are being stopped here. Just DPi and syringe (do we need to stop syringe pump?). We are also bypassing dialyzer here. Reply by Dara Navaei on 01 November 2024, 15:34 > Done Reply by Sean Nash on 04 November 2024, 08:59 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 October 2024, 09:03 https://devapps.diality.us/cru/HD-DEN-17093-1#c20593 Why are you treating bloodLeakZeroingStatus as a pointer here? Reply by Dara Navaei on 22 October 2024, 10:30 > Removed the static pointer. Reply by Sean Nash on 22 October 2024, 12:51 > RESOLVED in CODE WALKTHROUGH. --- ID: HD-DEN-17093-1 https://devapps.diality.us/cru/HD-DEN-17093-1 Title: HD-DEN-17093_Blood Leak Zeroing IN Treatment Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (2 active, 2 completed*) Sean Nash (*) Vinayakam Mani (*) jpaguio Michael Garthwaite