This is a list of all comments for DG-DEN-3421-2-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/Fans.h Revision Comment by pmontazemi on 17 November 2020, 11:29 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6249 Not a big fan of numbering variables, can we be more descriptive of location of fans? Reply by Dara Navaei on 21 November 2020, 09:56 > There are 6 fans. Three inlet and three outlet. They are > controlled as if there are only two fans, meaning 1 duty > cycle will be fed to inlet fans and 1 duty cycle will be fed > to the outlet fans. The only time that are treated > individually is when we read their RPM. That is why I called > them inlet 1, 2. Reply by pmontazemi on 23 November 2020, 10:32 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/TemperatureSensors.c Revision Comment by pmontazemi on 17 November 2020, 12:03 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6250 5//10 ? Reply by Dara Navaei on 29 November 2020, 10:13 > Please refer to DEN-5846 code review. Reply by Sean Nash on 30 November 2020, 15:51 > Dara, rather than referring to another code review, I would > just reply that it will be corrected after that code review > is completed, branch merged to master, and then master > pulled into this branch. Commenter can resolve then at > that time when the fix shows up. Reply by Dara Navaei on 01 December 2020, 14:28 > I removed this #define as it will not be used anymore. Reply by pmontazemi on 07 December 2020, 10:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 17 November 2020, 12:04 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6251 Remove commented line or add TODO. Reply by Dara Navaei on 29 November 2020, 10:14 > Please refer to DEN-5846 code review. Reply by pmontazemi on 01 December 2020, 10:58 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 17 November 2020, 12:04 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6252 Remove empty lines. Reply by Dara Navaei on 21 November 2020, 09:58 > Done Reply by pmontazemi on 23 November 2020, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 24 November 2020, 09:02 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6302 Align the last line. Should these number be defined? Reply by Dara Navaei on 29 November 2020, 10:19 > Please refer to DEN-5846 code review. Reply by qnguyen on 01 December 2020, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 01 December 2020, 09:10 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6487 Spaces between characters and parentheses, everywhere. Reply by Dara Navaei on 01 December 2020, 14:24 > Done Reply by pmontazemi on 07 December 2020, 10:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 17 November 2020, 12:07 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6253 What is extra line for? Reply by Dara Navaei on 21 November 2020, 09:59 > Done Reply by pmontazemi on 23 November 2020, 10:33 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Thermistors.c Revision Comment by pmontazemi on 23 November 2020, 10:39 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6299 How variable these parameters are between thermistors (across machines)? Reply by Dara Navaei on 29 November 2020, 10:16 > The values come from the datasheets of the sensors. Reply by pmontazemi on 01 December 2020, 11:00 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 13 November 2020, 09:05 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6175 Align with the line above. Reply by Dara Navaei on 13 November 2020, 09:23 > Done Reply by qnguyen on 13 November 2020, 10:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Thermistors.h Revision Comment by Sean Nash on 10 November 2020, 13:31 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6074 Are these two sensors different than the two dialysate temp sensors in the TemperatureSensors module? Reply by Dara Navaei on 11 November 2020, 15:26 > Yes. These are the internal temperature sensors. I added the > word internal for better clarity. Reply by Sean Nash on 13 November 2020, 10:37 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/UVReactors.c Revision Comment by Sean Nash on 10 November 2020, 13:14 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6065 Why is this here and also outside of status in an override array? Can these be consolidated? Can state be overridden? I think it should be something that Dialin user can set (by requesting on or off) but not override. Reply by Dara Navaei on 12 November 2020, 12:53 > This is exec state of each reactor and it cannot be > overridden. The Dialin user will be able to turn on and off > the UV reactors but not override them. Reply by Sean Nash on 13 November 2020, 10:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 13:16 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6066 Should these two pins (enable output and health input) be something that can be overridden? I would think at least the input health pin should be. Reply by Dara Navaei on 13 November 2020, 13:53 > The health status is an override. Reply by Sean Nash on 17 November 2020, 10:29 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 13:08 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6063 There are two reactors in this array. This line only initializes the first. I would initialize all in a loop in init function and leave it uninitialized here. Also, don't include the word override in the variable name. Also, maybe this shouldn't be an override. And there is already a state in the status record, so maybe this should be removed. Reply by Dara Navaei on 12 November 2020, 13:24 > Removed the override variable completely. Reply by Sean Nash on 17 November 2020, 10:30 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 13:10 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6064 Consider initializing these status and the exec states in a loop. Reply by Dara Navaei on 12 November 2020, 13:26 > Two of the items are reactor specific. I initialized the rest > of them in a loop. Reply by Sean Nash on 13 November 2020, 10:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 13:19 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6067 Validate given reactor before using it. Reply by Dara Navaei on 12 November 2020, 13:34 > Done Reply by Sean Nash on 13 November 2020, 10:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 13:19 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6068 Validate given reactor before using it. Reply by Dara Navaei on 12 November 2020, 13:34 > Done Reply by Sean Nash on 13 November 2020, 10:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 13 November 2020, 15:04 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6218 Do we need this if statement? if the reactor is off, then a request to turn off should generate a TRUE too? Reply by Dara Navaei on 14 November 2020, 11:10 > You are right. Removed the if statement. Reply by qnguyen on 17 November 2020, 10:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 13 November 2020, 08:49 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6169 Remove extra line. Reply by Dara Navaei on 13 November 2020, 09:18 > Done Reply by qnguyen on 13 November 2020, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 13 November 2020, 08:50 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6170 It should be an AND rather than OR if we would like to check both are healthy. Reply by Dara Navaei on 13 November 2020, 09:19 > That is right. Changed to AND. Reply by qnguyen on 13 November 2020, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 13:26 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6073 As implemented, I don't think you should set this flag to false. Will cause it to turn off immediately once in ON state. Reply by Dara Navaei on 12 November 2020, 13:36 > That is right. Removed the line. Reply by Sean Nash on 13 November 2020, 10:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 13:22 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6069 This comments doesn't make sense. Reply by Dara Navaei on 12 November 2020, 13:37 > Rephrased the sentence. Reply by Sean Nash on 13 November 2020, 10:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 13 November 2020, 08:54 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6172 Consider update the pin signal state inside setReactorEnableStatus() function. So there is no need to keep updating the pin signal state after each call to function setReactorEnableStatus(). Reply by Dara Navaei on 13 November 2020, 13:52 > Done Reply by qnguyen on 17 November 2020, 10:33 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/UVReactors.h Revision Comment by Sean Nash on 10 November 2020, 13:03 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6061 For modules that control or monitor h/w, I would like details of the h/w added to the module brief (e.g. mfg, p/n, some summary details from datasheet, ...). Reply by Dara Navaei on 12 November 2020, 08:52 > Done Reply by Sean Nash on 13 November 2020, 10:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 13:04 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6062 Should we also include whether each UV is on/off? Reply by Dara Navaei on 12 November 2020, 09:00 > Added on/off states to the publish structure. Reply by Sean Nash on 13 November 2020, 10:42 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/AlarmMgmt.h Revision Comment by qnguyen on 13 November 2020, 15:06 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6219 Misspell REACOTRS and SELECTD? Reply by Dara Navaei on 14 November 2020, 11:09 > Done Reply by qnguyen on 17 November 2020, 10:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 07 December 2020, 10:41 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6692 SELECTD is misspelled. Reply by Dara Navaei on 07 December 2020, 12:33 > Done Reply by pmontazemi on 09 December 2020, 11:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 19 November 2020, 09:14 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6263 Misspelled "INVAID"? Reply by Dara Navaei on 21 November 2020, 10:04 > Yes, thank you. Done. Reply by qnguyen on 23 November 2020, 10:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by qnguyen on 13 November 2020, 09:25 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6180 Consider moving to corresponding module (ModeHeatDisinfect). Reply by Dara Navaei on 14 November 2020, 15:42 > Done Reply by qnguyen on 17 November 2020, 10:32 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DrainPump.c Revision Comment by qnguyen on 17 November 2020, 11:13 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6248 Suggest add () around each condition. Reply by Dara Navaei on 21 November 2020, 09:55 > Done Reply by qnguyen on 23 November 2020, 10:36 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ROPump.c Revision Comment by qnguyen on 19 November 2020, 09:39 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6264 Suggest grouping each condition in (). maxPressure should be less than MAX_ALLOWED_PRESSURE_PSI? Reply by Dara Navaei on 21 November 2020, 10:06 > Added the (). I had fixed the MIN to MAX pressure but I did > not push it. Done. Reply by qnguyen on 23 November 2020, 10:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 19 November 2020, 09:00 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6262 We might need to obtain absolute value here to account for negative error when current flow is higher than target flow. Reply by Dara Navaei on 29 November 2020, 10:15 > Please refer to DEN-5864 code review. Reply by qnguyen on 01 December 2020, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 20 November 2020, 09:54 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6270 Floating point equation - literals should have a decimal point. Reply by Dara Navaei on 21 November 2020, 10:02 > Done Reply by Sean Nash on 23 November 2020, 10:38 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemComm.c Revision Comment by qnguyen on 17 November 2020, 11:07 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6247 Remove extra blank lines. Reply by Dara Navaei on 21 November 2020, 09:54 > Done Reply by qnguyen on 23 November 2020, 10:36 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/include/het.h Revision Comment by pmontazemi on 17 November 2020, 12:31 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6254 Why indentation? Reply by Dara Navaei on 21 November 2020, 10:00 > This is het.h. This is auto generated by HALCoGen and we do > not touch it. Reply by pmontazemi on 23 November 2020, 10:34 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 17 November 2020, 12:31 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6255 Why indentation? Reply by Dara Navaei on 21 November 2020, 10:00 > This is het.h. This is auto generated by HALCoGen and we do > not touch it. Reply by pmontazemi on 23 November 2020, 10:34 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/PIControllers.c Revision Comment by Sean Nash on 10 November 2020, 13:01 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6060 I know I made this change when helping you troubleshoot drain pump control, but looking at this again, I think we need both options for calculating the error signal (depending on which control is in play). I'm concerned that this change may damage some/all of the other controls. Reply by Dara Navaei on 12 November 2020, 13:38 > I added another hard coded property to be able to have both > options. Reply by Sean Nash on 13 November 2020, 10:43 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemComm.h Revision Comment by qnguyen on 13 November 2020, 09:27 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6181 Why are these function declarations deleted? Reply by Dara Navaei on 13 November 2020, 11:16 > Brought the function declarations back. Reply by qnguyen on 17 November 2020, 10:32 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Fans.c Revision Comment by qnguyen on 13 November 2020, 09:18 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6177 Suggest replace 60000000 with ( SEC_PER_MIN * US_PER_SECOND ). Reply by Dara Navaei on 13 November 2020, 09:34 > Done Reply by qnguyen on 13 November 2020, 10:34 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 12:16 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6049 Why do we need both target and calculated. Looking at how these are used, they both appear to be calculated. Consider consolidating target and calculated PWM. Also, at least in comments, do not abbreviate PWM duty cycle % to just "PWM". It's important to keep description and units clear. Reply by Dara Navaei on 11 November 2020, 10:06 > Calculated is the value that is calculated using the > equation. Target the value that is set to the fans, for > instance if it is ramp up mode and the calculated value is > greater than the limit, the target becomes the limit and not > the calculated. Also, the calculated value is used to compare > it with the newest value to decide whether it is in ramp up > mode or ramp down. I renamed it to previous duty cycle for > better clarity. Reply by Dara Navaei on 13 November 2020, 13:53 > I removed the calculated and there is only the target. Reply by Sean Nash on 17 November 2020, 10:30 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 12:16 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6048 In comment, make clear these are the measured speeds from tachometers. Also give units of speed (RPM). Reply by Dara Navaei on 11 November 2020, 09:55 > Done Reply by Sean Nash on 13 November 2020, 10:50 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 12:20 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6050 Change PWM in comment to PWM duty cycle %. This equation looks upside down. If you want a conversion factor from °C to duty cycle %, you will want the % on top (i.e. °C x % / °C = %). Reply by Dara Navaei on 11 November 2020, 10:18 > Fixed the equation and the names. Reply by Sean Nash on 13 November 2020, 10:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 12:37 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6051 I don't understand this equation. If slope converts temp to PWM duty cycle % (which is what you want), why do we need the min % and min temp terms? Reply by Dara Navaei on 11 November 2020, 14:07 > I fixed the equation and renamed the variables. Reply by Sean Nash on 13 November 2020, 10:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 12:46 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6055 Before this "if", I think you should verify pwm is within min/max range (0.1 to 0.95) and cap if out of range. Reply by Dara Navaei on 11 November 2020, 14:12 > Added it statements to check for the caps. Reply by Sean Nash on 13 November 2020, 10:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 12:41 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6053 Why compare a candidate PWM duty cycle % with a max delta? That doesn't make sense. Should compare delta of pwm and last calculated with max delta. Reply by Dara Navaei on 11 November 2020, 14:28 > Fixed the the statement. Reply by Sean Nash on 13 November 2020, 10:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 12:43 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6054 Why set a PWM duty cycle % with a max PWM delta. That doesn't make sense either. Should set to last calculated + max PWM delta. Reply by Dara Navaei on 11 November 2020, 14:28 > That is right. I fixed it. Reply by Sean Nash on 13 November 2020, 10:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 12:48 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6056 Again, doesn't make sense to assign a delta % to a %. If candidate PWM duty cycle % is decreasing, need the same logic as above with increase to ensure you don't exceed max decrement. As I understand it, a decrease is more important when considering how quickly the fan speed should be allowed to change. Reply by Dara Navaei on 11 November 2020, 14:30 > That is right, I changed it. Reply by Sean Nash on 13 November 2020, 10:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 12:55 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6058 Remove blank line. Reply by Dara Navaei on 11 November 2020, 10:23 > Done Reply by Sean Nash on 13 November 2020, 10:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 12:55 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6057 Floating point literals should always have a decimal point. Change 0 to 0.0. Reply by Dara Navaei on 11 November 2020, 10:23 > Done Reply by Sean Nash on 13 November 2020, 10:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 10 November 2020, 12:58 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6059 I think this local variable is poorly named. Looking below, it appears to be a pulse count from FPGA. If it is a pulse count, I don't know about the term "toggle" for describing it. Reply by Dara Navaei on 11 November 2020, 10:25 > Yes that is right. I had changed this to togglePeriod but did > not push it yet. It is called toggle period in FPGA. Reply by Sean Nash on 13 November 2020, 10:43 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Tasks/TaskGeneral.c Revision Comment by pmontazemi on 23 November 2020, 10:51 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6300 Reactos? Reply by Dara Navaei on 29 November 2020, 10:17 > This branch was covering all the drivers that were needed for > heat disinfection. Each driver now has a branch and code > review. Reply by Sean Nash on 30 November 2020, 16:02 > Dara, it is misspelled. Reply by Dara Navaei on 01 December 2020, 14:21 > Done Reply by pmontazemi on 07 December 2020, 10:31 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Drivers/InternalADC.c Revision Comment by qnguyen on 13 November 2020, 09:31 https://devapps.diality.us/cru/DG-DEN-3421-2-1#c6182 Align comment. Reply by Dara Navaei on 13 November 2020, 09:35 > Done Reply by qnguyen on 13 November 2020, 10:31 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-3421-2-1 https://devapps.diality.us/cru/DG-DEN-3421-2-1 Title: DG-DEN-3421_DG Heat Disinfection Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (0 active, 3 completed*) qnguyen (*) Sean Nash (*) pmontazemi (*)