This is a list of all comments for DG-DEN-5963-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Sean Nash on 04 January 2021, 10:20 https://devapps.diality.us/cru/DG-DEN-5963-1#c7166 A lot of these #includes are already in SystemCommMessages.h and so should probably be removed from this area as they are redundant. Reply by Dara Navaei on 14 April 2021, 09:21 > Done. Reply by Sean Nash on 15 April 2021, 11:01 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 08 April 2021, 12:18 https://devapps.diality.us/cru/DG-DEN-5963-1#c9103 Comments shall start with capital letter. Reply by Dara Navaei on 13 April 2021, 09:31 > Done. Reply by pmontazemi on 19 April 2021, 10:50 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 10:24 https://devapps.diality.us/cru/DG-DEN-5963-1#c7167 Shouldn't these functions return a boolean? Then assign result to return value instead of always TRUE. Reply by Dara Navaei on 14 April 2021, 09:19 > I changed the functions. Reply by Sean Nash on 15 April 2021, 11:01 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeHeatDisinfect.c Revision Comment by Sean Nash on 04 January 2021, 10:26 https://devapps.diality.us/cru/DG-DEN-5963-1#c7169 I would avoid the \ and new line here. If comment won't align, put comment on line above with ///. Reply by Dara Navaei on 15 April 2021, 08:46 > I changed it to one line. Reply by Sean Nash on 15 April 2021, 11:00 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 15:47 https://devapps.diality.us/cru/DG-DEN-5963-1#c8510 This looks tight. Will we pass? Do we need a bigger filter for temp sensors? Reply by Dara Navaei on 03 April 2021, 18:39 > This was in the flow chart. So far both values to do the > difference comes from TRo so it should be tested in V3. Reply by Sean Nash on 12 April 2021, 10:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 16:03 https://devapps.diality.us/cru/DG-DEN-5963-1#c8511 Align #define values on left. Reply by Dara Navaei on 03 April 2021, 18:38 > The #defines are aligned. Reply by Sean Nash on 12 April 2021, 10:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 08 April 2021, 12:06 https://devapps.diality.us/cru/DG-DEN-5963-1#c9101 Align comment. Reply by Dara Navaei on 13 April 2021, 09:32 > Done. Reply by pmontazemi on 19 April 2021, 10:54 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 15 April 2021, 22:41 https://devapps.diality.us/cru/DG-DEN-5963-1#c9376 Use SEC_PER_MIN to replace 60. Reply by Dara Navaei on 16 April 2021, 08:50 > Done. Reply by qnguyen on 16 April 2021, 10:34 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 15 April 2021, 22:42 https://devapps.diality.us/cru/DG-DEN-5963-1#c9377 Align the define value (for most of the define section). Reply by Dara Navaei on 16 April 2021, 08:54 > Done. Reply by qnguyen on 16 April 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 10:53 https://devapps.diality.us/cru/DG-DEN-5963-1#c7176 There should already be an enum for reservoirs somewhere. Use that one. Reply by Dara Navaei on 14 April 2021, 08:57 > This enum has been deleted. Reply by Sean Nash on 15 April 2021, 10:58 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 10:29 https://devapps.diality.us/cru/DG-DEN-5963-1#c7170 I don't see that you're using this status type. Why not just remove it now? Reply by Dara Navaei on 14 April 2021, 09:18 > This enum has been deleted. Reply by Sean Nash on 15 April 2021, 10:58 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 16:08 https://devapps.diality.us/cru/DG-DEN-5963-1#c8513 I thought hot and cold paths got merged into one? Reply by Dara Navaei on 03 April 2021, 18:34 > They are both in Water Cancellation Path state. They are > separate because hot cancellation requires mix drain. Reply by Sean Nash on 12 April 2021, 10:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 16:07 https://devapps.diality.us/cru/DG-DEN-5963-1#c8512 When is status unknown? And shouldn't there be a partially full status? Reply by Dara Navaei on 14 April 2021, 09:19 > This enum has been deleted. Reply by Sean Nash on 15 April 2021, 10:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 16:10 https://devapps.diality.us/cru/DG-DEN-5963-1#c8514 This doesn't look like a complete list of possible status. What are these used for? Reply by Dara Navaei on 03 April 2021, 18:36 > These enums are the possible status that can occur during the > actual heat disinfect. Reply by Sean Nash on 12 April 2021, 10:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 10:31 https://devapps.diality.us/cru/DG-DEN-5963-1#c7171 Didn't Sunny say we need at least 25 PSI at inlet? Reply by Dara Navaei on 14 April 2021, 09:18 > That is tested in the start state. Reply by Sean Nash on 15 April 2021, 11:00 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 16:12 https://devapps.diality.us/cru/DG-DEN-5963-1#c8515 Should actuators be reset here too? Reply by Dara Navaei on 03 April 2021, 18:32 > I added the function to reset the actuators. Reply by Sean Nash on 15 April 2021, 10:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 10:39 https://devapps.diality.us/cru/DG-DEN-5963-1#c7172 Should return boolean. Should return FALSE and not request standby if we are not in heat disinfect mode. Reply by Dara Navaei on 14 April 2021, 09:17 > Done. Reply by Sean Nash on 15 April 2021, 10:59 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 10:45 https://devapps.diality.us/cru/DG-DEN-5963-1#c7174 Is ppiPressure being checked here or in global check I saw at top of exec? Since VPi is closed for much of this mode, the global check is probably not right. Reply by Dara Navaei on 14 April 2021, 08:58 > The global check has been removed. PPi is checked here. Reply by Sean Nash on 15 April 2021, 10:59 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 10:42 https://devapps.diality.us/cru/DG-DEN-5963-1#c7173 Function name is confusing. Change to something like failHeatDisinfection(). Also, this state handler will return DG_HEAT_DISINFECT_STATE_DRAIN_R1 - overriding the complete state being set in your function. Reply by Dara Navaei on 14 April 2021, 09:01 > I renamed the function. This part has been changed and the > current state will not be overridden. Reply by Sean Nash on 15 April 2021, 10:59 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 10:48 https://devapps.diality.us/cru/DG-DEN-5963-1#c7175 Should drain pump be turned off? Reply by Dara Navaei on 14 April 2021, 08:58 > It depends on where it is going. If it is the last drain, it > turns off the drain pump. Reply by Sean Nash on 15 April 2021, 10:58 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 15 April 2021, 22:48 https://devapps.diality.us/cru/DG-DEN-5963-1#c9378 Should this be an OR (according to the comment)? Reply by Dara Navaei on 16 April 2021, 08:55 > This is an and. I changed the comment. Reply by qnguyen on 16 April 2021, 10:34 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 16:15 https://devapps.diality.us/cru/DG-DEN-5963-1#c8516 I prefer boolean conditions be explicit (e.g. TRUE == isThisLastDrain). This way, if value gets corrupted to a garbage value (like 5) it will be same as FALSE. Reply by Dara Navaei on 03 April 2021, 18:31 > Done. Reply by Sean Nash on 15 April 2021, 10:52 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 16:32 https://devapps.diality.us/cru/DG-DEN-5963-1#c8518 Why no actuators set for draining R2 here? I see VRd being set a few lines down. Maybe move this comment to go with the valve change. Reply by Dara Navaei on 03 April 2021, 18:31 > I moved the location of the comment. Reply by Sean Nash on 15 April 2021, 10:52 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 16:38 https://devapps.diality.us/cru/DG-DEN-5963-1#c8519 Recommend being much more descriptive of these states. What exactly is the state trying to accomplish. Reply by Dara Navaei on 03 April 2021, 18:29 > I added more description for all the states. Reply by Sean Nash on 15 April 2021, 10:52 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 16:41 https://devapps.diality.us/cru/DG-DEN-5963-1#c8522 This comment needs a rephrase. I don't think range has anything to do with it. This is a retry counter check. Reply by Dara Navaei on 03 April 2021, 18:28 > I rephrased it. Reply by Sean Nash on 15 April 2021, 10:51 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 15 April 2021, 22:55 https://devapps.diality.us/cru/DG-DEN-5963-1#c9379 Recommend adding TRUE or FALSE to check explicitly. Apply to the rest of the file. Reply by Dara Navaei on 16 April 2021, 08:56 > Done. Reply by qnguyen on 16 April 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 08 April 2021, 00:15 https://devapps.diality.us/cru/DG-DEN-5963-1#c9066 Do we need to check for maximum limit here? What is the use of conductivity in this case? Reply by Dara Navaei on 13 April 2021, 09:52 > This is just to check that everything above minimum we are > about to start the heat disifnect mode. I added the max limit > too. Reply by Dara Navaei on 15 April 2021, 11:28 > The minimum is not needed and I removed it. Reply by qnguyen on 16 April 2021, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 15 April 2021, 22:58 https://devapps.diality.us/cru/DG-DEN-5963-1#c9381 This should be a post-increment. Otherwise, it will never retry with MAX_ALLOWED_STATE_TRIALS = 1. Apply to the rest of the file. Reply by Dara Navaei on 16 April 2021, 09:02 > Done. Reply by qnguyen on 16 April 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 16:46 https://devapps.diality.us/cru/DG-DEN-5963-1#c8524 Is Thd same as TRo? Does it belong in this group? Reply by Dara Navaei on 03 April 2021, 18:26 > THd belongs to this group. TRo is temporarily used as THd. Reply by Sean Nash on 15 April 2021, 10:51 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 16:48 https://devapps.diality.us/cru/DG-DEN-5963-1#c8525 Set these booleans to TRUE or FALSE explicitly. Reply by Dara Navaei on 03 April 2021, 18:26 > This is the oneliner syntax, what do you mean by setting them > explicitly? Reply by Dara Navaei on 15 April 2021, 12:40 > I set them explicitly. Reply by Sean Nash on 16 April 2021, 10:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 16:49 https://devapps.diality.us/cru/DG-DEN-5963-1#c8526 Check these booleans explicitly. Reply by Dara Navaei on 03 April 2021, 18:25 > Done. Reply by Sean Nash on 15 April 2021, 10:48 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 16:53 https://devapps.diality.us/cru/DG-DEN-5963-1#c8528 Check boolean explicitly. Reply by Dara Navaei on 03 April 2021, 18:24 > Done. Reply by Sean Nash on 15 April 2021, 10:47 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 16:57 https://devapps.diality.us/cru/DG-DEN-5963-1#c8529 I prefer to reverse the order of these to change accidental = instead of ==. Reply by Dara Navaei on 15 April 2021, 08:48 > Done. Reply by Sean Nash on 15 April 2021, 10:47 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 17:08 https://devapps.diality.us/cru/DG-DEN-5963-1#c8531 Are these the only two faults that can happen in R1 to R2 state? Reply by Dara Navaei on 03 April 2021, 18:22 > Yes. In Disinfect R1 to R2 and Disinfect R2 to R1, the > reservoirs might leak, or we might not be able to reach to > 81C within the specified time. Reply by Sean Nash on 15 April 2021, 10:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 17:08 https://devapps.diality.us/cru/DG-DEN-5963-1#c8532 What is this state doing? Reply by Dara Navaei on 03 April 2021, 18:21 > This state has been removed. Reply by Sean Nash on 15 April 2021, 10:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 17:11 https://devapps.diality.us/cru/DG-DEN-5963-1#c8533 Can drain time out in this state? Why no check? Reply by Dara Navaei on 03 April 2021, 18:20 > Yes. Drain can time out. It transitions to Basic Cancellation > path. Reply by Sean Nash on 15 April 2021, 10:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 17:12 https://devapps.diality.us/cru/DG-DEN-5963-1#c8534 Is this the new "coldest spot" sensor? Reply by Dara Navaei on 03 April 2021, 18:19 > TRo is used as the coldest spot until V3. Reply by Sean Nash on 12 April 2021, 10:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 10:56 https://devapps.diality.us/cru/DG-DEN-5963-1#c7179 Not a great name. Should be resFull or something like that. Reply by Dara Navaei on 14 April 2021, 08:56 > This variable has been deleted. Reply by Sean Nash on 15 April 2021, 10:58 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 10:55 https://devapps.diality.us/cru/DG-DEN-5963-1#c7178 Should be an else (or default if you turn this into a switch) that triggers a s/w fault (bad reservoir param). Reply by Dara Navaei on 18 April 2021, 11:37 > Done. Reply by Sean Nash on 19 April 2021, 10:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 10:57 https://devapps.diality.us/cru/DG-DEN-5963-1#c7181 Change to resEmpty. Reply by Dara Navaei on 14 April 2021, 08:55 > This variable has been deleted. Reply by Sean Nash on 16 April 2021, 10:47 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 10:57 https://devapps.diality.us/cru/DG-DEN-5963-1#c7180 Add else for s/w fault. Or maybe load cell driver or Reservoirs module should have a generic get weight function for a given reservoir. Reply by Dara Navaei on 18 April 2021, 11:36 > The above code has been deleted. Reply by Sean Nash on 19 April 2021, 10:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 11:01 https://devapps.diality.us/cru/DG-DEN-5963-1#c7182 fabs will indicate a leak even if the volume goes up. Is that what you want here? Reply by Dara Navaei on 14 April 2021, 08:53 > Yes, the purpose is to check whether the level changes, > either going up or down. Reply by Sean Nash on 15 April 2021, 10:57 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 11:03 https://devapps.diality.us/cru/DG-DEN-5963-1#c7183 Instead of "if the flag is TRUE", comment should say something like "if disinfect temperature has been reached". Reply by Dara Navaei on 14 April 2021, 08:52 > Done. Reply by Sean Nash on 15 April 2021, 10:57 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 17:17 https://devapps.diality.us/cru/DG-DEN-5963-1#c8536 Can we log 2 data with alarms (set alarm and a data value when detected, log with alarm here)? Reply by Dara Navaei on 15 April 2021, 11:20 > The alarm is different for each condition, so that explains > what failed and in what state (i.e reservoir drain time out > in drain R1 state) Reply by Sean Nash on 16 April 2021, 10:37 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ROPump.c Revision Comment by Sean Nash on 04 January 2021, 10:13 https://devapps.diality.us/cru/DG-DEN-5963-1#c7162 In HD, AlarmDefs.h is always included via common.h which includes AlarmMgmt.h which includes AlarmDefs.h. DG should be same way so you shouldn't need to include it here. Reply by Dara Navaei on 14 April 2021, 10:12 > Done. Reply by Sean Nash on 15 April 2021, 11:02 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 10:15 https://devapps.diality.us/cru/DG-DEN-5963-1#c7163 We're making all comments start with a capital letter globally. I did HD. This is probably the branch that we should do same for DG. Reply by Dara Navaei on 14 April 2021, 10:11 > Done. Reply by Sean Nash on 15 April 2021, 11:02 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 March 2021, 19:49 https://devapps.diality.us/cru/DG-DEN-5963-1#c8368 If we're not doing all of this commented out monitoring, just delete it. Reply by Dara Navaei on 18 April 2021, 15:31 > I uncommented the code. Reply by Sean Nash on 19 April 2021, 10:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 March 2021, 19:50 https://devapps.diality.us/cru/DG-DEN-5963-1#c8369 If we don't need this check anymore, just delete it. Reply by Dara Navaei on 16 April 2021, 09:18 > Done. Reply by Sean Nash on 16 April 2021, 10:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 March 2021, 19:51 https://devapps.diality.us/cru/DG-DEN-5963-1#c8370 Delete blank line. Reply by Dara Navaei on 03 April 2021, 18:41 > Done. Reply by Sean Nash on 15 April 2021, 10:54 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 March 2021, 19:51 https://devapps.diality.us/cru/DG-DEN-5963-1#c8371 Delete blank line. Reply by Dara Navaei on 03 April 2021, 18:41 > Done. Reply by Sean Nash on 15 April 2021, 10:54 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeHeatDisinfect.h Revision Comment by Sean Nash on 14 March 2021, 20:09 https://devapps.diality.us/cru/DG-DEN-5963-1#c8372 Shouldn't there be a few of these disinfect times? R1, R2, etc... Reply by Dara Navaei on 03 April 2021, 18:40 > No, this variable is used to time both the disinfect states. Reply by Sean Nash on 15 April 2021, 10:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 January 2021, 10:25 https://devapps.diality.us/cru/DG-DEN-5963-1#c7168 Should this return a boolean? What if we're not in heat disinfect mode when this function is called? Should return FALSE then. Reply by Dara Navaei on 14 April 2021, 09:19 > Done. Reply by Sean Nash on 15 April 2021, 11:01 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeStandby.c Revision Comment by Sean Nash on 04 January 2021, 10:17 https://devapps.diality.us/cru/DG-DEN-5963-1#c7164 Add TODO on this to restore condition later. Reply by Dara Navaei on 14 April 2021, 10:10 > This section has been uncommented. Reply by Sean Nash on 15 April 2021, 11:02 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 14 April 2021, 14:14 https://devapps.diality.us/cru/DG-DEN-5963-1#c9291 Each condition should be put in a parenthesis (). Reply by Dara Navaei on 14 April 2021, 19:10 > Done. Reply by qnguyen on 15 April 2021, 10:36 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Heaters.c Revision Comment by Sean Nash on 04 January 2021, 10:10 https://devapps.diality.us/cru/DG-DEN-5963-1#c7161 This arrangement looks odd. Should be desired time / interval. You have double desired time / double interval. Why? Reply by Dara Navaei on 14 April 2021, 10:13 > I removed 2. If faster publish time is needed, it should be > overridden via Dialin. Reply by Sean Nash on 14 April 2021, 22:16 > Still too many () now. You can still have 2 Hz default > publish time if you want - I just didn't like the > arrangement. Should be more like ( 500 / > TASK_PRIORITY_INTERVAL ) so it is clear that you want every > 500 ms. Reply by Dara Navaei on 15 April 2021, 12:44 > I changed the publish interval to 1 second. I also > removed the extra parentheses. Reply by Sean Nash on 16 April 2021, 10:48 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 07 April 2021, 22:38 https://devapps.diality.us/cru/DG-DEN-5963-1#c9064 Align the defines. Doxygen comment can be moved up one line using ///. Reply by Dara Navaei on 13 April 2021, 10:18 > Done. Reply by qnguyen on 15 April 2021, 10:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 March 2021, 19:45 https://devapps.diality.us/cru/DG-DEN-5963-1#c8367 Why commented out here? If you don't want this disabled, comment the build switch out in DGCommon.h. Reply by Dara Navaei on 03 April 2021, 18:42 > Done. Reply by Sean Nash on 15 April 2021, 10:54 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 08 April 2021, 10:53 https://devapps.diality.us/cru/DG-DEN-5963-1#c9089 If both of these functions are using the same constant, why do we have two functions? Use just one (unless it is planned for them to inherit different constants down the road). Reply by Dara Navaei on 13 April 2021, 09:47 > The small and main primary duty cycles are on different PWM > channels. They cannot be controlled with one function. Reply by pmontazemi on 19 April 2021, 10:59 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 08 April 2021, 10:54 https://devapps.diality.us/cru/DG-DEN-5963-1#c9090 Why commented out? Reply by Dara Navaei on 19 April 2021, 10:58 > I uncommented it. Reply by pmontazemi on 19 April 2021, 10:59 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Heaters.h Revision Comment by pmontazemi on 08 April 2021, 11:14 https://devapps.diality.us/cru/DG-DEN-5963-1#c9091 ...operating temperatures between 5 degrees C and 95 degrees C. Reply by Dara Navaei on 13 April 2021, 09:46 > Done. Reply by pmontazemi on 19 April 2021, 10:57 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 08 April 2021, 11:14 https://devapps.diality.us/cru/DG-DEN-5963-1#c9092 ...operating temperatures between 5 degrees C and 95 degrees C. Reply by Dara Navaei on 13 April 2021, 09:46 > Done. Reply by pmontazemi on 19 April 2021, 10:57 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 March 2021, 19:43 https://devapps.diality.us/cru/DG-DEN-5963-1#c8365 Why is this banner deleted? Reply by Dara Navaei on 15 April 2021, 12:43 > I brought it back. Reply by Sean Nash on 16 April 2021, 10:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 08 April 2021, 11:15 https://devapps.diality.us/cru/DG-DEN-5963-1#c9093 Why using structs instead of enums? Reply by Dara Navaei on 13 April 2021, 09:40 > This struct is the structure that is used to set the > dialysate target temperature using the CAN bus. I do not > think we can use enums for this purpose. Reply by pmontazemi on 19 April 2021, 10:56 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 March 2021, 19:43 https://devapps.diality.us/cru/DG-DEN-5963-1#c8366 Why banner deleted? Reply by Dara Navaei on 03 April 2021, 18:43 > I brought it back. Reply by Sean Nash on 15 April 2021, 10:55 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/MessagePayloads.h Revision Comment by Sean Nash on 04 January 2021, 10:18 https://devapps.diality.us/cru/DG-DEN-5963-1#c7165 Can you go ahead and move the rest of these to their respective module .h files and delete this header file from project? Reply by Dara Navaei on 16 April 2021, 10:11 > Done. Reply by Sean Nash on 16 April 2021, 10:47 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DrainPump.c Revision Comment by Sean Nash on 14 March 2021, 19:34 https://devapps.diality.us/cru/DG-DEN-5963-1#c8359 Why is all of this commented out? Just build switch the checks that aren't working and leave everything else alone. Reply by Dara Navaei on 16 April 2021, 09:15 > I added a TODO on top to explain that this part will be > addressed once the drain pump RPM can be read reliably. I > added a build switch to ignore this part. Reply by Sean Nash on 16 April 2021, 10:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 March 2021, 19:34 https://devapps.diality.us/cru/DG-DEN-5963-1#c8358 If this check isn't working, make a build switch to exclude them. What is the plan for this check? Can we even do it? If not, remove. Reply by Dara Navaei on 16 April 2021, 09:15 > I added a build switch to ignore this part. Reply by Sean Nash on 16 April 2021, 10:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 March 2021, 19:36 https://devapps.diality.us/cru/DG-DEN-5963-1#c8360 Why is this check commented out? Even if our RPM conversion is not great, we still need to be able to distinguish between off and on. Set a threshold and keep this check. Reply by Dara Navaei on 16 April 2021, 09:15 > I added a build switch to ignore this part. Reply by Sean Nash on 16 April 2021, 10:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 March 2021, 19:38 https://devapps.diality.us/cru/DG-DEN-5963-1#c8361 Not sure what's happening here. Restore this as it was. Reply by Dara Navaei on 16 April 2021, 09:15 > I added a build switch to ignore this part. Reply by Sean Nash on 21 April 2021, 10:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 08 April 2021, 10:50 https://devapps.diality.us/cru/DG-DEN-5963-1#c9087 Spell out P in variable name (or use "Press" at least). Reply by Dara Navaei on 13 April 2021, 09:49 > I spelled it completely. Reply by pmontazemi on 19 April 2021, 11:00 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 March 2021, 19:39 https://devapps.diality.us/cru/DG-DEN-5963-1#c8362 What is the purpose of this? If there is a problem, shouldn't there be an alarm/fault? Seems like current mode may not be aware that the drain pump has been stopped here. Reply by Dara Navaei on 16 April 2021, 09:16 > This is another way to be able to turn off the pump by just > setting the RPM to 0. Reply by Sean Nash on 16 April 2021, 10:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 March 2021, 19:41 https://devapps.diality.us/cru/DG-DEN-5963-1#c8363 Why remove this check? Reply by Dara Navaei on 03 April 2021, 18:47 > I do not know what happened. I brought it back. Reply by Sean Nash on 15 April 2021, 10:55 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Fans.c Revision Comment by Sean Nash on 14 March 2021, 19:42 https://devapps.diality.us/cru/DG-DEN-5963-1#c8364 Why commented out? Reply by Dara Navaei on 18 April 2021, 11:25 > I uncommented the code. Reply by Sean Nash on 19 April 2021, 10:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 07 April 2021, 22:33 https://devapps.diality.us/cru/DG-DEN-5963-1#c9063 This should be an or. Reply by Dara Navaei on 13 April 2021, 10:19 > Nice catch. Done. Reply by qnguyen on 15 April 2021, 10:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 08 April 2021, 10:52 https://devapps.diality.us/cru/DG-DEN-5963-1#c9088 Why commented out? Reply by Dara Navaei on 18 April 2021, 11:25 > I uncommented the code. Reply by pmontazemi on 19 April 2021, 11:00 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Pressures.c Revision Comment by qnguyen on 07 April 2021, 22:58 https://devapps.diality.us/cru/DG-DEN-5963-1#c9065 load cell enum needs to be fixed. Reply by Dara Navaei on 13 April 2021, 10:16 > This section was for debugging only. I removed it. Reply by qnguyen on 15 April 2021, 10:41 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Thermistors.c Revision Comment by pmontazemi on 08 April 2021, 11:27 https://devapps.diality.us/cru/DG-DEN-5963-1#c9099 Add TODO or delete. Reply by Dara Navaei on 18 April 2021, 11:18 > Done. Reply by pmontazemi on 19 April 2021, 10:54 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 19 April 2021, 11:57 https://devapps.diality.us/cru/DG-DEN-5963-1#c9508 Should we remove = so we only error when greater than? Reply by Dara Navaei on 19 April 2021, 20:45 > I removed the = sign. Reply by qnguyen on 21 April 2021, 10:36 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/LoadCell.c Revision Comment by pmontazemi on 08 April 2021, 11:17 https://devapps.diality.us/cru/DG-DEN-5963-1#c9094 Remove "for" at end of @param sentence. Reply by Dara Navaei on 19 April 2021, 11:46 > Done. Reply by pmontazemi on 21 April 2021, 10:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 08 April 2021, 11:17 https://devapps.diality.us/cru/DG-DEN-5963-1#c9095 Remove "for" at end of @param sentence. Reply by Dara Navaei on 13 April 2021, 09:39 > Done. Reply by pmontazemi on 19 April 2021, 10:55 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/LoadCell.h Revision Comment by pmontazemi on 08 April 2021, 11:17 https://devapps.diality.us/cru/DG-DEN-5963-1#c9096 Where did these go? Reply by Sean Nash on 12 April 2021, 10:02 > Moved to common DGDefs.h (in staging branch). Reply by pmontazemi on 12 April 2021, 11:04 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeDrain.c Revision Comment by pmontazemi on 08 April 2021, 11:28 https://devapps.diality.us/cru/DG-DEN-5963-1#c9100 Align comment. Reply by Dara Navaei on 13 April 2021, 09:33 > Done. Reply by pmontazemi on 19 April 2021, 10:54 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/AlarmMgmt.c Revision Comment by pmontazemi on 08 April 2021, 12:12 https://devapps.diality.us/cru/DG-DEN-5963-1#c9102 Replace a with a more meaningful variable name. Reply by Dara Navaei on 13 April 2021, 09:32 > Done. Reply by pmontazemi on 19 April 2021, 11:01 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/TemperatureSensors.c Revision Comment by pmontazemi on 08 April 2021, 11:25 https://devapps.diality.us/cru/DG-DEN-5963-1#c9097 Why not using a union? Much more optimized for memory-limited devices. Reply by qnguyen on 13 April 2021, 13:26 > Union is not applicable in this case. Every field of this > struct has a different value to be stored. Reply by pmontazemi on 19 April 2021, 10:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 18 April 2021, 22:55 https://devapps.diality.us/cru/DG-DEN-5963-1#c9429 Is load cell A1 with A2, and B1 with B2? Reply by Dara Navaei on 19 April 2021, 08:45 > Yes, they only have one temperature sensor per board. Reply by qnguyen on 19 April 2021, 10:20 > Should we refactor the name then? It looks like A1 goes > with B1, and A2 goes with B2 with the current name. Reply by qnguyen on 19 April 2021, 10:48 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 08 April 2021, 11:26 https://devapps.diality.us/cru/DG-DEN-5963-1#c9098 Replace logic with a temp parameter (call it whatever you like) and then use that parameter in the function call. This will make the code a lot more legible. Reply by Dara Navaei on 13 April 2021, 09:38 > Done. Reply by pmontazemi on 19 April 2021, 10:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 13 April 2021, 13:24 https://devapps.diality.us/cru/DG-DEN-5963-1#c9250 Both false and true cases are needed (for clear and trigger alarm). Recommend turning the if statement into a variable and pass in as parameter. Reply by Dara Navaei on 14 April 2021, 10:24 > Done. Reply by qnguyen on 15 April 2021, 10:39 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ConcentratePumps.h Revision Comment by pmontazemi on 08 April 2021, 10:48 https://devapps.diality.us/cru/DG-DEN-5963-1#c9086 Did we want to mention "set" in the two variables' name? Reply by Dara Navaei on 13 April 2021, 09:50 > [~qnguyen] Could you please address? Reply by qnguyen on 13 April 2021, 13:34 > [~dnavaei] Please refactor the name to cp1CurrentSetSpeed > and cp2CurrentSetSpeed. Thank you! Reply by Dara Navaei on 18 April 2021, 11:21 > Done. Reply by pmontazemi on 19 April 2021, 11:01 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-5963-1 https://devapps.diality.us/cru/DG-DEN-5963-1 Title: DG-DEN-5963_DG Heat Disinfect (2 of 2) Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (0 active, 3 completed*) qnguyen (*) Sean Nash (*) pmontazemi (*)