This is a list of all comments for HD-DEN-6372-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Modes/ModeTreatment.c Revision Comment by qnguyen on 11 February 2021, 10:23 https://devapps.diality.us/cru/HD-DEN-6372-1#c7937 Do we need to reset alarm signal flags? Reply by Sean Nash on 16 February 2021, 10:47 > All alarm user actions are blocked in this sub-mode. Alarm > stop signal flag is covered in this reset function. Reply by qnguyen on 16 February 2021, 11:11 > It looks like the alarm stop signal flag is reset in > another function? resetAlarmSignalFlags function. Reply by Sean Nash on 17 February 2021, 11:02 > Stop signal is removed. But other alarm flags still > exist so I added call to reset alarm signal flags as you > suggested. Reply by qnguyen on 19 February 2021, 10:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 27 February 2021, 16:46 https://devapps.diality.us/cru/HD-DEN-6372-1#c8244 1. How are we syncing times between UI RTC and HD RTC? 2. Does not the UI APP know some of these times (like start time) to compute the other times by itself instead of HD FW passing them to the UI APP? Reply by Sean Nash on 28 February 2021, 22:40 > RTCs are sync'd by UI sending its date/time to HD and DG f/w. > > However, RTC time is not used for treatment time. Treatment > time is accumulated using HD timer counter. HD then uses > this and set treatment duration to calculate time remaining. > All are then sent to UI regularly. Reply by pmontazemi on 01 March 2021, 10:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 26 February 2021, 23:57 https://devapps.diality.us/cru/HD-DEN-6372-1#c8242 Consider using TREATMENT_STATE_DATA_T data structure. Reply by Sean Nash on 28 February 2021, 22:39 > Done. Reply by qnguyen on 01 March 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/Rinseback.c Revision Comment by Dara Navaei on 11 February 2021, 09:32 https://devapps.diality.us/cru/HD-DEN-6372-1#c7874 Do we need a #define for 1.2? Reply by Sean Nash on 11 February 2021, 09:41 > In my opinion, no. Should be clear I am setting safety > margin at 120% of target volume. This constant is > essentially a #define itself. Reply by Dara Navaei on 11 February 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 11 February 2021, 09:32 https://devapps.diality.us/cru/HD-DEN-6372-1#c7875 Do we need a #define 0.8? Reply by Sean Nash on 11 February 2021, 09:41 > In my opinion, no. Should be clear I am setting safety > margin at 80% of target volume. Reply by Dara Navaei on 11 February 2021, 10:34 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 11 February 2021, 09:31 https://devapps.diality.us/cru/HD-DEN-6372-1#c7873 This is the reason why at one of my previous companies we used to name variables with f_ or i_ as first characters to know at a glance the variable type definition. Now worth considering though. Reply by Sean Nash on 11 February 2021, 09:43 > Something to consider, but I think we are too far into > development to make a change like that. We have a lot of > unit test cases looking for variables as they are currently > named. A large refactor would kill our dev testing. Reply by pmontazemi on 11 February 2021, 11:10 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 16 February 2021, 10:59 https://devapps.diality.us/cru/HD-DEN-6372-1#c8135 Mismatch function name. Reply by Sean Nash on 16 February 2021, 11:22 > Fixed. Reply by qnguyen on 17 February 2021, 10:32 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by qnguyen on 03 March 2021, 10:27 https://devapps.diality.us/cru/HD-DEN-6372-1#c8266 Need to fix doxygen style. @details Inputs: @details Outputs: Reply by Sean Nash on 03 March 2021, 10:34 > Fixed. Reply by qnguyen on 03 March 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/TreatmentRecirc.c Revision Comment by qnguyen on 16 February 2021, 10:34 https://devapps.diality.us/cru/HD-DEN-6372-1#c8124 Consider changing 60 to SEC_PER_MIN. Reply by Sean Nash on 16 February 2021, 10:45 > Done. Reply by qnguyen on 17 February 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 16 February 2021, 10:36 https://devapps.diality.us/cru/HD-DEN-6372-1#c8125 Mismatch function name. Reply by Sean Nash on 16 February 2021, 10:44 > Fixed. Reply by qnguyen on 17 February 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 22 February 2021, 18:41 https://devapps.diality.us/cru/HD-DEN-6372-1#c8239 Are timeout and countdown in seconds? If so, RECIRC_TIMEOUT_MS might not convert correctly to second. Reply by Sean Nash on 23 February 2021, 10:24 > Timeout is not in ms. Renamed constant. Reply by qnguyen on 23 February 2021, 10:36 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/TreatmentStop.c Revision Comment by pmontazemi on 11 February 2021, 09:28 https://devapps.diality.us/cru/HD-DEN-6372-1#c7870 Delete or add TODO comment. Reply by Sean Nash on 11 February 2021, 09:30 > Removed. Reply by pmontazemi on 11 February 2021, 11:11 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/Dialysis.c Revision Comment by pmontazemi on 11 February 2021, 09:16 https://devapps.diality.us/cru/HD-DEN-6372-1#c7868 Replace with function that sets these valves. Settings valves can be its own module for the various modes/sates and sub-modes/sub-states. This same concept can be applied to all actuators where in mode/sate or sub-mode/sub-state, the valves, the pumps, etc. are all set. I would like to avoid setting just the valves manually here and again manually set somewhere else. Let's keep all states centralized per mode/sub-mode or state/sub-state. Reply by Sean Nash on 11 February 2021, 09:34 > The current design is for each mode to set actuators on entry > to a new mode/sub-mode (e.g. this function) per that mode's > initial state. Then, while in that mode/sub-mode, the mode's > state machine will change actuators as required when moving > from one state to another. Reply by pmontazemi on 11 February 2021, 11:13 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Valves.c Revision Comment by pmontazemi on 11 February 2021, 09:13 https://devapps.diality.us/cru/HD-DEN-6372-1#c7867 Do both #ifndef need to be satisfied for this portion of the code to execute? Where (at what time) are these two parameters set? Reply by Sean Nash on 11 February 2021, 09:38 > These are both engineering build switches (HDCommon.h). If > either build switch is enabled, this code will not be > compiled. Reply by pmontazemi on 11 February 2021, 11:14 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/HDCommon.h Revision Comment by pmontazemi on 11 February 2021, 09:46 https://devapps.diality.us/cru/HD-DEN-6372-1#c7884 Add TODO comments or delete. Reply by Sean Nash on 11 February 2021, 09:50 > Already have a TODO on line 32. Reply by pmontazemi on 11 February 2021, 11:09 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/PIControllers.c Revision Comment by qnguyen on 16 February 2021, 10:47 https://devapps.diality.us/cru/HD-DEN-6372-1#c8128 Remove doxygen comment. Reply by Sean Nash on 16 February 2021, 11:17 > Done. Reply by qnguyen on 17 February 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 16 February 2021, 10:48 https://devapps.diality.us/cru/HD-DEN-6372-1#c8130 Need to review doxygen style in this file. Reply by Sean Nash on 16 February 2021, 11:21 > Fixed. Reply by qnguyen on 17 February 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: MsgDefs.h Revision Comment by pmontazemi on 27 February 2021, 16:37 https://devapps.diality.us/cru/HD-DEN-6372-1#c8243 Align comment. Reply by qnguyen on 28 February 2021, 02:50 > Aligned. Reply by pmontazemi on 01 March 2021, 10:38 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/BloodFlow.c Revision Comment by pmontazemi on 11 February 2021, 09:04 https://devapps.diality.us/cru/HD-DEN-6372-1#c7862 Where is bloodFlowCalGain defined and what is its value? Is it fixed across machines or does it get set at calibration time? Reply by Sean Nash on 11 February 2021, 09:10 > Calibration factors live in non-volatile memory. Set during > calibration via Dialin. Reply by pmontazemi on 11 February 2021, 11:15 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeService.c Revision Comment by pmontazemi on 11 February 2021, 09:20 https://devapps.diality.us/cru/HD-DEN-6372-1#c7869 Is the stop button still part of the design? And why are we ignoring it here? Reply by Sean Nash on 11 February 2021, 09:31 > Not fully clear what the sub-modes of service mode will be. > At this time, I don't know what, if anything, should happen > if the user presses the stop/pause button while in service > mode. For now, there is nothing to stop so we are ignoring. Reply by pmontazemi on 11 February 2021, 11:12 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/BloodPrime.c Revision Comment by qnguyen on 11 February 2021, 10:08 https://devapps.diality.us/cru/HD-DEN-6372-1#c7906 What is 2.0? Can it be defined as a constant to provide more definition? Reply by Sean Nash on 16 February 2021, 10:51 > Divide by 2 is averaging start and end rates of the ramp so > that I can determine estimate of time needed to complete the > ramp. Reply by qnguyen on 17 February 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 11 February 2021, 10:07 https://devapps.diality.us/cru/HD-DEN-6372-1#c7905 Need an extra line. Reply by Sean Nash on 16 February 2021, 10:49 > Where? Reply by qnguyen on 16 February 2021, 11:03 > Right after the if statement ends. Reply by Sean Nash on 16 February 2021, 11:58 > Is that a rule in our coding standard? Reply by qnguyen on 17 February 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. --- ID: HD-DEN-6372-1 https://devapps.diality.us/cru/HD-DEN-6372-1 Title: HD-DEN-6372_HD Rinseback Statement of Objectives: State: Closed Summary: Author: Sean Nash Moderator: Sean Nash Reviewers: (0 active, 3 completed*) qnguyen (*) Dara Navaei (*) pmontazemi (*)