•  

Comment Results

Review Name Created Custom Fields Content
HD-DEN-7395-1 11 Apr 2021

Maybe I do not quite understand the architecture, but is there any reason that your are returning the state of the blood leak detector state machine?

HD-DEN-7395-1 12 Apr 2021

RESOLVED in CODE WALKTHROUGH.

HD-DEN-7395-1 11 Apr 2021

A break is needed here.

HD-DEN-9906-1 15 Nov 2021

RESOLVED in CODE WALKTHROUGH.

DIALIN-DEN-7135-1 13 Apr 2021

RESOLVED.

DIALIN-DEN-5963-2 12 Apr 2021

Is this a Dialin msg (0xA...) or regular msg? If regular, should this cmd be moved to UI proxy?

HD-DEN-7395-1 11 Apr 2021

Where did zeroBloodLeak() and selfTestBloodLeak() functions go? You have prototypes for them in .h and I think you still need at least one of them. zeroBloodLeak would be called by Pre-Treatment Mode and would tell FPGA to put sensor in zero state and would also put your state machine in zero state.

HD-DEN-9906-1 15 Nov 2021

RESOLVED in CODE WALKTHROUGH. (On behalf of Behrouz NematiPour who is on vacation)

HD-DEN-9906-1 15 Nov 2021

RESOLVED in CODE WALKTHROUGH

DIALIN-DEN-11750-1 23 Feb 2022

Please see comment in hd_blood_leak_data.py.

HD-DEN-11098-1 19 Nov 2021

Done

HD-DEN-11098-1 19 Nov 2021

Fixed

DIALIN-DEN-11750-1 23 Feb 2022

This class is used to wait for the results to be received from firmware. I fixed the order.

HD-DEN-11114-1 02 Dec 2021

RESOLVED in CODE WALKTHROUGH.

HD-DEN-11114-1 29 Nov 2021

Remove obsolete function prototypes.

DIALIN-DEN-11750-1 23 Feb 2022

RESOLVED.

DIALIN-DEN-11750-1 22 Feb 2022

Please remove comments if they are no longer applicable to the test.

HD-DEN-11098-1 13 Dec 2021

This start time probably needs a new name to better describe what it's doing.

DG-DEN-11750-1 25 Feb 2022

rpm should be in ().

UI-DEN-10205-1 22 Dec 2021

if the parameter is not used, the name will not be specified so the compiler won't complain about the parameter not being used.

Note the difference between parameters and arguments:

  • Function parameters are the names listed in the function's definition.
  • Function arguments are the real values passed to the function.
DIALIN-DEN-10602-2 15 Dec 2021

According to PRS 178:
These are actually:

  • "Final Target Ultrafiltration Volume"
  • "Final Target Ultrafiltration Rate"
    for simplicity, the "Final" portion has been removed and only the "Target" is used since all SW knows them as "Target" only.
UI-DEN-10205-1 22 Dec 2021

Thank you!

UI-DEN-10205-1 21 Dec 2021

With Off and Complete being two different properties, should we have Complete say "Off" to the user and not "Complete?"

DG-DEN-11928-1 28 Feb 2022

Is this average calculation correct? Do we need to weight samples based on CP rate?

HD-DEN-11750-2 26 Feb 2022

It must be removed.

HD-DEN-11114-1 31 Dec 2021

Done.

HD-DEN-11098-1 13 Dec 2021

Add blank line between these two lines.

HD-DEN-11098-1 13 Dec 2021

Should be 120 mL divided by 300 mL/min times 60 sec/min = 24 seconds.

HD-DEN-11114-1 30 Dec 2021

We don't need to calculate digit count - just use strlen(tempCharBuffer) after sprintf creates your string.

DG-DEN-11928-1 22 Mar 2022

Added doxygen comment

ALARM_ID_INLET_WATER_HIGH_CONDUCTIVITY = 106, ///< DG inlet water conductivity is greater than threshold

HD-DEN-11750-2 24 Feb 2022

Venous pressure does not have an error count. Is it needed?

DG-DEN-11928-1 01 Mar 2022

Changed CONCENTRATE_PUMP_MAX_SPEED from 40.0 to 48.0.

DG-DEN-11928-1 01 Mar 2022

Done.

DG-DEN-11750-1 02 Mar 2022

Done.

DG-DEN-11750-1 02 Mar 2022

This will be addressed once we were working on deciding the number of rinses.

DG-DEN-11750-1 02 Mar 2022

This will be addressed once we were working on deciding the number of rinses.

DG-DEN-11750-1 24 Feb 2022

Shouldn't the override check be its own get function like other overidden properties? Ex: getUVReactorHealth() ?

HD-DEN-11750-2 24 Feb 2022

Is this supposed to be assigned to zero? it is unlike the other variables that have been changed

HD-DEN-11750-2 24 Feb 2022

Should 300 be a magic number?

DG-DEN-11928-1 23 Mar 2022

RESOLVED in CODE WALKTHROUGH.

UI-DEN-7135-1 05 Apr 2021

I added these already to staging, but they are named BUILD_FOR_DESKTOP and BUILD_FOR_TARGET. So when you merge staging to your branch or vice versa be sure to rename all of mine or make sure all of them are named the same since there's no need for duplicates. Also, are we still going to review merges as I don't think there is a merge commit yet from staging to this branch in this code review.

DG-DEN-8030-1 16 Jun 2021

RESOLVED in CODE WALKTHROUGH.

HD-DEN-7395-1 12 Apr 2021

Good catch, addressed.

DG-DEN-8030-1 16 Jun 2021

Instead of retry logic, should we just double the time out? This retry is essentially extended the time one more without any action to correct the first time out.

HD-DEN-7395-1 12 Apr 2021

Good catch, addressed.

DIALIN-DEN-7135-1 12 Apr 2021

RESOLVED
Sounds good. Thanks!

UI-DEN-11980-1 16 Feb 2022

Fixed. Thanks!

HD-DEN-13598-2 22 Sep 2022

I added the common branch to this code review.

DIALIN-DEN-8462-1 22 Jun 2021

RESOLVED.

HD-DEN-7395-1 12 Apr 2021

Yes, Sean and I thought that it would be good for the SWVV, SYSVV, MFR, and SVC Teams to know via Dialin API which state the Blood Leak state machine is in. This detector comes with an independent MCU thqat can be programmed and calibrated via UART and via HD FPGA including self-test mode and zeroing mode to zero the sensor when clear dialysate is run through it. Then a threshold of blood can be set to be detected as soon as blood gets across the dialyzer from the blood circuit to the dialysate circuit.