This is a list of all comments for HD-MASTER-1. Review Summary: No summary ---------------------------------------- File: App/Drivers/SafetyShutdown.c Revision Comment by Dara Navaei on 30 September 2019, 19:51 https://devapps.diality.us/cru/HD-MASTER-1#c9 Do we need this space? Reply by Sean Nash on 01 October 2019, 08:52 > I like to put a blank line between "system" includes and > "app" includes. Reply by lbaloa on 25 October 2019, 10:34 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 01 October 2019, 09:36 https://devapps.diality.us/cru/HD-MASTER-1#c18 Use parenthesis around the function. Reply by Sean Nash on 01 October 2019, 10:24 > Coding standard only requires when there are operators in the > expansion. Reply by lbaloa on 15 November 2019, 10:14 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 01 October 2019, 09:36 https://devapps.diality.us/cru/HD-MASTER-1#c19 Same as before Reply by Sean Nash on 01 October 2019, 10:24 > Coding standard only requires when there are operators in the > expansion. Reply by lbaloa on 15 November 2019, 10:15 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: App/Contollers/AlarmLamp.c Revision Comment by pmontazemi on 04 October 2019, 11:19 https://devapps.diality.us/cru/HD-MASTER-1#c29 RESOLVED. Revision Comment by pmontazemi on 04 October 2019, 11:19 https://devapps.diality.us/cru/HD-MASTER-1#c30 Now reviewed. Reply by lbaloa on 25 October 2019, 10:35 > RESOLVED IN CODE WALKTHROUGH Revision Comment by pmontazemi on 04 October 2019, 11:19 https://devapps.diality.us/cru/HD-MASTER-1#c31 Now reviewed. Reply by lbaloa on 25 October 2019, 10:35 > RESOLVED IN CODE WALKTHROUGH Revision Comment by pmontazemi on 04 October 2019, 11:19 https://devapps.diality.us/cru/HD-MASTER-1#c32 Now reviewed. Revision Comment by Dara Navaei on 05 October 2019, 21:29 https://devapps.diality.us/cru/HD-MASTER-1#c35 Reviewed. Reply by lbaloa on 25 October 2019, 10:35 > RESOLVED IN CODE WALKTHROUGH Revision Comment by pmontazemi on 04 October 2019, 11:19 https://devapps.diality.us/cru/HD-MASTER-1#c33 Now reviewed. Revision Comment by pmontazemi on 04 October 2019, 11:19 https://devapps.diality.us/cru/HD-MASTER-1#c34 Now reviewed. Reply by lbaloa on 25 October 2019, 10:35 > RESOLVED IN CODE WALKTHROUGH Revision Comment by Dara Navaei on 30 September 2019, 19:42 https://devapps.diality.us/cru/HD-MASTER-1#c8 Does the variable supposed to be U32 instead of uint32_t? Reply by Sean Nash on 01 October 2019, 08:49 > Fixed. Reply by lbaloa on 25 October 2019, 10:34 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 01 October 2019, 09:23 https://devapps.diality.us/cru/HD-MASTER-1#c14 Isn't lamp state 1 and 2 LAMP_STATE_ON or LAMP_STATE_OFF? If so, I suggest to correct comments Reply by Sean Nash on 01 October 2019, 09:30 > These are 2-state patterns to support blinking. If blinking, > then yes it could be ON and OFF (or vice-versa). If no > blink, then both states would be ON. If color shouldn't even > be on at all, both states would be OFF. Reply by lbaloa on 25 October 2019, 10:34 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: App/Contollers/Buttons.c Revision Comment by Dara Navaei on 01 October 2019, 08:41 https://devapps.diality.us/cru/HD-MASTER-1#c10 Extra space? Reply by Sean Nash on 01 October 2019, 08:50 > There is no extra space. Reply by Dara Navaei on 14 November 2019, 15:41 > RESOLVED Reply by pmontazemi on 03 December 2019, 15:36 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: App/Modes/ModeFault.c Revision Comment by Dara Navaei on 06 October 2019, 09:23 https://devapps.diality.us/cru/HD-MASTER-1#c37 Reviewed. ---------------------------------------- File: App/Drivers/CPLD.c Revision Comment by lbaloa on 01 October 2019, 09:31 https://devapps.diality.us/cru/HD-MASTER-1#c16 Can you spell out what does CPLD stand for at least one here? Reply by Sean Nash on 01 October 2019, 10:18 > Fixed. ---------------------------------------- File: App/Drivers/CPLD.h Revision Comment by lbaloa on 01 October 2019, 09:33 https://devapps.diality.us/cru/HD-MASTER-1#c17 PIN_SIGNAL_STATE_T is not defined in this h file. Shouldn't there be the h file (defining PIN_SIGNAL_STATE_T) here instead of the c file? Reply by Sean Nash on 01 October 2019, 10:20 > Fixed. Reply by lbaloa on 15 November 2019, 10:14 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: App/Modes/ModeTreatment.c Revision Comment by Dara Navaei on 06 October 2019, 09:22 https://devapps.diality.us/cru/HD-MASTER-1#c36 Reviewed. ---------------------------------------- File: App/Modes/OperationModes.h Revision Comment by lbaloa on 04 October 2019, 08:21 https://devapps.diality.us/cru/HD-MASTER-1#c26 Recommend complete spell out, e.g., from MODE_FAUL to MODE_FAULT Reply by Sean Nash on 04 October 2019, 08:28 > I abbreviated them to a fixed 4 characters to help make > transition table (in .c file) more readable. Reply by lbaloa on 25 October 2019, 10:33 > RESOLVED IN CODE WALKTHROUGH --- ID: HD-MASTER-1 https://devapps.diality.us/cru/HD-MASTER-1 Title: HD Master Branch Review Statement of Objectives: State: Closed Summary: Author: Sean Nash Reviewers: (0 active, 3 completed*) Dara Navaei (*) pmontazemi (*) lbaloa (*)