This is a list of all comments for HD-DEN-13598-2. Review Summary: No summary General Comment by Michael Garthwaite on 31 August 2022, 14:41 https://devapps.diality.us/cru/HD-DEN-13598-2#c13658 [~snash] [~dnavaei] Does this code review also need the common repo as well? There are changes within this branch that look like they have changes in alarmdefs.h. Reply by Sean Nash on 22 September 2022, 14:06 > common and fwcommon repos are covered in the 13598 DG code > review so not needed here. Reply by Dara Navaei on 22 September 2022, 14:42 > I added the common branch to this code review. Reply by Sean Nash on 26 September 2022, 13:16 > RESOLVED in CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Controllers/DGInterface.c Revision Comment by Sean Nash on 29 August 2022, 11:27 https://devapps.diality.us/cru/HD-DEN-13598-2#c13583 Add blank line above these new constants. Reply by Dong Nguyen on 30 August 2022, 09:04 > I added a blank line above those new constants. Reply by Sean Nash on 26 September 2022, 13:17 > RESOLVED in CODE WALKTHROUGH Revision Comment by Michael Garthwaite on 31 August 2022, 15:11 https://devapps.diality.us/cru/HD-DEN-13598-2#c13667 Should this variable not have any assignment at initialization? Reply by Sean Nash on 02 September 2022, 16:20 > Need to initialize here at declaration or in init function. > I see neither now. Reply by Dong Nguyen on 26 September 2022, 14:18 > Done. Removed the variable, no more needed. Reply by Sean Nash on 27 September 2022, 14:07 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 29 August 2022, 11:28 https://devapps.diality.us/cru/HD-DEN-13598-2#c13584 Do we need counters since we're using persistent alarms? Reply by Dong Nguyen on 30 August 2022, 09:02 > I removed all counters. Reply by Sean Nash on 26 September 2022, 13:16 > RESOLVED in CODE WALKTHROUGH Revision Comment by wbracken on 31 August 2022, 15:51 https://devapps.diality.us/cru/HD-DEN-13598-2#c13671 Update function header. Reply by wbracken on 26 September 2022, 13:54 > RESOLVED IN CODE WALKTHROUGH Revision Comment by Michael Garthwaite on 31 August 2022, 15:09 https://devapps.diality.us/cru/HD-DEN-13598-2#c13666 change comment from reservoirs to op mode Reply by Dong Nguyen on 26 September 2022, 14:19 > Done. Reply by Sean Nash on 27 September 2022, 13:50 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 31 August 2022, 15:49 https://devapps.diality.us/cru/HD-DEN-13598-2#c13670 dgSubMode not an input Reply by wbracken on 26 September 2022, 13:31 > RESOLVED IN CODE WALKTHROUGH Revision Comment by Darren Cox on 26 August 2022, 11:54 https://devapps.diality.us/cru/HD-DEN-13598-2#c13564 Using a get function for the flag, but then setting value directly. Should there be a set function? Also applies to several functions below. Reply by Dong Nguyen on 30 August 2022, 10:30 > This is the existing style of flag implementation in HD code: > The flag is set in "set-value" functions (e.g., the function > setNewLoadCellReadings() sets the flag > dgLoadCellDataFreshFlag = TRUE; ). Reply by Sean Nash on 02 September 2022, 16:26 > I think the style is to use set/get functions if needed by > other modules, otherwise just access the variable directly. Reply by Darren Cox on 26 September 2022, 13:51 > RESOLVED IN CODE WALKTHROUGH Revision Comment by Darren Cox on 26 August 2022, 11:56 https://devapps.diality.us/cru/HD-DEN-13598-2#c13565 Uncomment or add a comment explaining why it is inactive. Reply by Darren Cox on 26 September 2022, 13:52 > RESOLVED IN CODE WALKTHROUGH Revision Comment by Michael Garthwaite on 31 August 2022, 14:59 https://devapps.diality.us/cru/HD-DEN-13598-2#c13665 Should this entire function be commented out? It has the proper SW_CONFIG check to not raise alarms and the variables that being assigned here are local. Reply by Dong Nguyen on 26 September 2022, 14:35 > I didn't comment out the checkDialysateTemperature() > function. I will ask the author of this function to see why > it is commented out. Reply by Sean Nash on 27 September 2022, 14:07 > Restored code. Reply by Sean Nash on 27 September 2022, 14:07 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/Prime.c Revision Comment by Michael Garthwaite on 31 August 2022, 14:51 https://devapps.diality.us/cru/HD-DEN-13598-2#c13661 Should we be raising this alarm with no data to log for debugging and/or service purposes? Reply by Dong Nguyen on 26 September 2022, 14:50 > I am not sure. Please have team discussion about this. Reply by Sean Nash on 27 September 2022, 14:08 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Michael Garthwaite on 31 August 2022, 14:54 https://devapps.diality.us/cru/HD-DEN-13598-2#c13662 Should we be raising this alarm with no data to log for debugging and/or service purposes? Reply by Dong Nguyen on 26 September 2022, 14:52 > This comment looks like the same as the previous one? > I am not sure. Please have team discussion about this. Reply by Sean Nash on 27 September 2022, 14:03 > Timeout has no data. Reply by Sean Nash on 27 September 2022, 14:07 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemComm.c Revision Comment by Michael Garthwaite on 31 August 2022, 15:15 https://devapps.diality.us/cru/HD-DEN-13598-2#c13669 Please remove commented code. Reply by Dong Nguyen on 26 September 2022, 14:03 > Removed. Reply by Sean Nash on 27 September 2022, 13:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Darren Cox on 26 August 2022, 11:58 https://devapps.diality.us/cru/HD-DEN-13598-2#c13566 If only used for debug, this should be removed. If it is to be activated later, a more verbose comment is needed. Reply by Darren Cox on 26 September 2022, 13:52 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Michael Garthwaite on 31 August 2022, 15:13 https://devapps.diality.us/cru/HD-DEN-13598-2#c13668 Please remove double include. Reply by Dong Nguyen on 26 September 2022, 14:11 > Done. Reply by Sean Nash on 27 September 2022, 14:06 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Temperatures.c Revision Comment by Michael Garthwaite on 31 August 2022, 14:56 https://devapps.diality.us/cru/HD-DEN-13598-2#c13663 This variable is still being used when we trigger the alarm down at line 302. Should it be removed? Reply by Dong Nguyen on 26 September 2022, 14:42 > Already removed the variable. No more needed. Reply by Sean Nash on 27 September 2022, 14:01 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Tasks/TaskGeneral.c Revision Comment by Michael Garthwaite on 07 September 2022, 14:01 https://devapps.diality.us/cru/HD-DEN-13598-2#c13716 Remove blank line. Reply by Dong Nguyen on 26 September 2022, 14:01 > Already removed blank line. Reply by Sean Nash on 27 September 2022, 13:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 16 September 2022, 16:28 https://devapps.diality.us/cru/HD-DEN-13598-2#c13788 What is this comment for? Reply by wbracken on 26 September 2022, 13:26 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Modes/SelfTests.c Revision Comment by wbracken on 16 September 2022, 11:02 https://devapps.diality.us/cru/HD-DEN-13598-2#c13783 Update header Reply by wbracken on 26 September 2022, 13:29 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Services/FPGA.c Revision Comment by wbracken on 16 September 2022, 16:25 https://devapps.diality.us/cru/HD-DEN-13598-2#c13786 Remove if not needed. Reply by wbracken on 26 September 2022, 13:28 > RESOLVED IN CODE WALKTHROUGH Revision Comment by wbracken on 16 September 2022, 16:25 https://devapps.diality.us/cru/HD-DEN-13598-2#c13787 Remove Reply by Sean Nash on 22 September 2022, 13:58 > Bill, I think this #ifndef is here because this test will > fail if you are debugging and ever stop for a break point > (FPGA timer keeps going but processor timer stops). So the > idea here is that this test only runs on a release build. Reply by wbracken on 26 September 2022, 13:26 > RESOLVED IN CODE WALKTHROUGH --- ID: HD-DEN-13598-2 https://devapps.diality.us/cru/HD-DEN-13598-2 Title: HD-DEN-13598_DG HD Dev Sprint 77 FW Dong Statement of Objectives: State: Closed Summary: Author: Dong Nguyen Moderator: Dong Nguyen Reviewers: (2 active, 3 completed*) Sean Nash (*) wbracken (*) Darren Cox (*) Michael Garthwaite Dara Navaei