This is a list of all comments for DG-DEN-5873-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/UVReactors.c Revision Comment by Sean Nash on 30 November 2020, 11:05 https://devapps.diality.us/cru/DG-DEN-5873-1#c6339 Blank line between HALCOGEN includes and normal includes. Alphabetize both #include sets. Reply by Dara Navaei on 01 December 2020, 14:36 > Done Reply by Sean Nash on 07 December 2020, 10:56 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 30 November 2020, 10:36 https://devapps.diality.us/cru/DG-DEN-5873-1#c6319 We might not need this condition. Reply by Sean Nash on 30 November 2020, 11:11 > I think condition should stay. Flag name is confusing. > hasTurnOnBeenRequested sounds like it only relates to turning > UV reactor on but it's not. Setting to FALSE indicates we > want UV reactor turned off. Recommend renaming this flag. Reply by Dara Navaei on 01 December 2020, 15:26 > Changed the flag to an enum. Reply by qnguyen on 07 December 2020, 10:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 November 2020, 11:13 https://devapps.diality.us/cru/DG-DEN-5873-1#c6341 For consistency, be explicit when checking boolean conditions (as done below in this function). Include TRUE ==. Reply by Dara Navaei on 01 December 2020, 14:40 > Done Reply by Sean Nash on 07 December 2020, 10:56 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 November 2020, 11:17 https://devapps.diality.us/cru/DG-DEN-5873-1#c6342 Why not using persistent alarm here? Reply by Dara Navaei on 07 December 2020, 15:59 > Done Reply by Sean Nash on 09 December 2020, 11:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 07 December 2020, 19:51 https://devapps.diality.us/cru/DG-DEN-5873-1#c6751 Suggest using alarm management to check if the unhealthy reactor alarm is active. If it is active, we can turn off the reactor and go to off state. Then reactorUnhealthyCounter and count limit can be deleted. Reply by Dara Navaei on 09 December 2020, 10:58 > Done Reply by qnguyen on 09 December 2020, 11:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 November 2020, 11:18 https://devapps.diality.us/cru/DG-DEN-5873-1#c6343 Function name implies a boolean return value. Why U32? Reply by Dara Navaei on 01 December 2020, 16:21 > The function gioGetBit returns a U32 so I made the function > be a U32. I changed it to a BOOL. Reply by Sean Nash on 07 December 2020, 10:56 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 November 2020, 11:19 https://devapps.diality.us/cru/DG-DEN-5873-1#c6344 Remove extra space between U32 and function name. Reply by Dara Navaei on 01 December 2020, 16:23 > Done Reply by Sean Nash on 07 December 2020, 10:56 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeInitPOST.c Revision Comment by Sean Nash on 30 November 2020, 11:02 https://devapps.diality.us/cru/DG-DEN-5873-1#c6335 Is this UT only code temporary? Adding a POST case with no functionality will cause POST to get stuck. Reply by Dara Navaei on 01 December 2020, 16:26 > No it is not. I brought the case into the #ifdef. Reply by Sean Nash on 07 December 2020, 10:57 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 November 2020, 11:03 https://devapps.diality.us/cru/DG-DEN-5873-1#c6337 The case is outside the #ifdef, so the break should be as well (or vice-versa). Reply by Dara Navaei on 01 December 2020, 16:26 > I brought the case to be inside #ifdef. Reply by Sean Nash on 07 December 2020, 10:57 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/source/sys_main.c Revision Comment by Sean Nash on 30 November 2020, 10:51 https://devapps.diality.us/cru/DG-DEN-5873-1#c6327 Move up in order with other inits for controllers/monitors. Reply by Dara Navaei on 02 December 2020, 08:33 > Done Reply by Sean Nash on 07 December 2020, 10:57 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemComm.c Revision Comment by Sean Nash on 30 November 2020, 10:59 https://devapps.diality.us/cru/DG-DEN-5873-1#c6332 Why were these removed? Reply by Dara Navaei on 01 December 2020, 16:27 > This has been renamed. It will be added to the RO pump > branch. Reply by Sean Nash on 07 December 2020, 10:57 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by qnguyen on 02 December 2020, 19:17 https://devapps.diality.us/cru/DG-DEN-5873-1#c6632 This check is not needed since turn on/off function already check for valid reactor index. Reply by Dara Navaei on 07 December 2020, 15:30 > Done Reply by qnguyen on 09 December 2020, 11:36 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.h Revision Comment by Sean Nash on 30 November 2020, 10:57 https://devapps.diality.us/cru/DG-DEN-5873-1#c6330 Why 2 functions for start/stop command? Should just have one message and add a parameter for which UV reactor to start/stop. Reply by Dara Navaei on 02 December 2020, 09:06 > Done Reply by Sean Nash on 07 December 2020, 10:57 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 November 2020, 10:54 https://devapps.diality.us/cru/DG-DEN-5873-1#c6329 Override handlers (and really any handlers for messages that would only come from Dialin) should have the word Test in the name to help distinguish Dialin test functions from other f/w functions. Reply by Dara Navaei on 02 December 2020, 09:09 > Done Reply by Sean Nash on 07 December 2020, 10:57 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-5873-1 https://devapps.diality.us/cru/DG-DEN-5873-1 Title: DG-DEN-5873_DG UV Reactors Statement of Objectives: State: Closed Summary: Author: Dara Navaei Reviewers: (0 active, 3 completed*) qnguyen (*) Sean Nash (*) pmontazemi (*)