This is a list of all comments for HD-DEN-7347-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/PresOccl.c Revision Comment by Sean Nash on 01 April 2021, 09:11 https://devapps.diality.us/cru/HD-DEN-7347-1#c8892 Is this function necessary? Couldn't you just call the function above and look for FALSE return value? Reply by qnguyen on 01 April 2021, 09:43 > FALSE value on the function above does not necessarily mean > all of the occlusion sensors' reading is below the threshold. > It can be one of them is below the threshold. > This function makes sure all of them are below the threshold. Reply by Sean Nash on 02 April 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 25 April 2021, 18:20 https://devapps.diality.us/cru/HD-DEN-7347-1#c9584 Inputs and outputs are missing. Reply by qnguyen on 25 April 2021, 22:09 > The function has been removed. Reply by Dara Navaei on 26 April 2021, 10:39 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModePostTreat.c Revision Comment by pmontazemi on 06 April 2021, 07:24 https://devapps.diality.us/cru/HD-DEN-7347-1#c9011 Align comments. Reply by qnguyen on 06 April 2021, 09:19 > Done. Reply by pmontazemi on 06 April 2021, 10:22 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 01 April 2021, 09:14 https://devapps.diality.us/cru/HD-DEN-7347-1#c8894 I think memset is in string.h. Should you add a #include at top of module? Reply by qnguyen on 01 April 2021, 21:04 > Done. Reply by Sean Nash on 02 April 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 25 April 2021, 18:59 https://devapps.diality.us/cru/HD-DEN-7347-1#c9589 Do we not need a fault here? Reply by qnguyen on 25 April 2021, 22:14 > Added. Reply by Dara Navaei on 26 April 2021, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 25 April 2021, 19:05 https://devapps.diality.us/cru/HD-DEN-7347-1#c9590 Do we need a fault here? Reply by qnguyen on 26 April 2021, 10:41 > Done. Reply by Dara Navaei on 26 April 2021, 10:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 25 April 2021, 19:34 https://devapps.diality.us/cru/HD-DEN-7347-1#c9593 This function needs a header. Reply by qnguyen on 25 April 2021, 22:21 > Done. Reply by Dara Navaei on 26 April 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 25 April 2021, 19:28 https://devapps.diality.us/cru/HD-DEN-7347-1#c9591 This function needs a header. Reply by qnguyen on 25 April 2021, 22:21 > Done. Reply by Dara Navaei on 26 April 2021, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 25 April 2021, 19:33 https://devapps.diality.us/cru/HD-DEN-7347-1#c9592 Is this function in charge of completely draining a reservoir? If yes, why should we provide a target parameter? Why not drain to 0 automatically? Reply by qnguyen on 25 April 2021, 22:16 > EMPTY_RESERVOIR_VOLUME_ML is defined as 0. Reply by Dara Navaei on 26 April 2021, 08:19 > That is what I meant. Why are we even passing this when we > know the target is 0? Reply by Dara Navaei on 26 April 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Sean Nash on 07 April 2021, 07:52 https://devapps.diality.us/cru/HD-DEN-7347-1#c9043 Not needed. Included in SystemCommMessages.h. Reply by qnguyen on 07 April 2021, 13:13 > Removed. Reply by Sean Nash on 08 April 2021, 10:41 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DGInterface.h Revision Comment by Dara Navaei on 25 April 2021, 18:15 https://devapps.diality.us/cru/HD-DEN-7347-1#c9583 All these structures need doxygen comments. Reply by qnguyen on 25 April 2021, 22:32 > Added. Reply by Dara Navaei on 26 April 2021, 10:39 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/SyringePump.c Revision Comment by Dara Navaei on 25 April 2021, 18:29 https://devapps.diality.us/cru/HD-DEN-7347-1#c9585 As per our design decision, reverse the second part of the and. Reply by qnguyen on 25 April 2021, 22:08 > Done. Reply by Sean Nash on 26 April 2021, 10:36 > Only reverse when using == operator Reply by Dara Navaei on 26 April 2021, 10:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 25 April 2021, 18:37 https://devapps.diality.us/cru/HD-DEN-7347-1#c9586 For consistency, I recommend reversing the second part of and in the if statement. Reply by qnguyen on 25 April 2021, 22:10 > Done. Reply by Dara Navaei on 26 April 2021, 10:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 25 April 2021, 18:37 https://devapps.diality.us/cru/HD-DEN-7347-1#c9587 For consistency, I recommend reversing the second part of and in the if statement. Reply by qnguyen on 25 April 2021, 22:10 > Done. Reply by Dara Navaei on 26 April 2021, 10:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 25 April 2021, 18:43 https://devapps.diality.us/cru/HD-DEN-7347-1#c9588 Will this variable be ever used? Reply by qnguyen on 25 April 2021, 22:11 > I think the variable will be used in future to detect > incorrect direction. > [~snash] Please elaborate more. Reply by Dara Navaei on 26 April 2021, 10:37 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModePreTreat.c Revision Comment by Dara Navaei on 25 April 2021, 20:24 https://devapps.diality.us/cru/HD-DEN-7347-1#c9594 This function does not return anything. Reply by qnguyen on 25 April 2021, 22:22 > Changed to return none. Reply by Dara Navaei on 26 April 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. --- ID: HD-DEN-7347-1 https://devapps.diality.us/cru/HD-DEN-7347-1 Title: HD-DEN-7347_DG HD Dev Post Statement of Objectives: State: Closed Summary: Author: qnguyen Reviewers: (0 active, 3 completed*) Sean Nash (*) Dara Navaei (*) pmontazemi (*)