This is a list of all comments for HD-SPR3-1. Review Summary: No summary General Comment by lbaloa on 17 October 2019, 09:45 https://devapps.diality.us/cru/HD-SPR3-1#c76 We might want to change Contollers directory to Controllers Reply by lbaloa on 22 October 2019, 10:06 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: App/Services/SystemComm.c Revision Comment by lbaloa on 17 October 2019, 09:36 https://devapps.diality.us/cru/HD-SPR3-1#c72 This if statement makes no sense Reply by Sean Nash on 17 October 2019, 09:58 > The comment explains. I haven't implemented the transmitter > idle check yet - so I'm assuming it is idle for now (as it > generally would be). Reply by lbaloa on 18 October 2019, 14:21 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 17 October 2019, 09:38 https://devapps.diality.us/cru/HD-SPR3-1#c73 Shouldn't we test if this is done by hardware... In other words, if I pull a queue of messages. Doesn't the hardware figure it out what to send first? Reply by Sean Nash on 17 October 2019, 10:37 > Hardware will only handle which packet wins when 2 or more > nodes are trying to send a packet at the same time. For each > node (sub-system), software still needs to decide which > packet it wants to send next. Reply by lbaloa on 18 October 2019, 14:19 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 17 October 2019, 09:41 https://devapps.diality.us/cru/HD-SPR3-1#c74 blank line after blankMessageInWrapper for readability Reply by Sean Nash on 17 October 2019, 10:40 > Why? Reply by lbaloa on 18 October 2019, 14:14 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 17 October 2019, 09:41 https://devapps.diality.us/cru/HD-SPR3-1#c75 blank line after dataPtr. for readability Reply by Sean Nash on 17 October 2019, 10:40 > I will add more comment lines instead of blank lines. See > HD-SPR4-1 review for fix. Reply by lbaloa on 18 October 2019, 14:15 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: source/can.c Revision Comment by lbaloa on 10 October 2019, 08:43 https://devapps.diality.us/cru/HD-SPR3-1#c47 General comment. We need to define these constant with #define Reply by Sean Nash on 14 October 2019, 13:09 > Leaving as is because it is HALCoGen generated code. Reply by lbaloa on 25 October 2019, 10:29 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: App/Services/MsgQueues.c Revision Comment by lbaloa on 17 October 2019, 09:30 https://devapps.diality.us/cru/HD-SPR3-1#c70 I couldn't find the definition of MESSAGE_WRAPPER_T... Words like wrapper is too generic. Are this denali message. Let's give name similar to our documentation. Reply by Sean Nash on 17 October 2019, 09:55 > I have the MESSAGE_T type as our denali message (minus the > CRC) and I have the MESSAGE_WRAPPER_T as a MESSAGE_T with a > CRC wrapped in. This makes it easier to implement the CRC > check (calc CRC for the message only) and then throw away the > wrapper (CRC) after it checks out. Reply by lbaloa on 18 October 2019, 14:23 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 17 October 2019, 09:31 https://devapps.diality.us/cru/HD-SPR3-1#c71 same comment apply in regards the word wrapper Reply by Sean Nash on 17 October 2019, 09:57 > Same response. Reply by lbaloa on 18 October 2019, 14:23 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: App/Services/CommBuffers.c Revision Comment by lbaloa on 10 October 2019, 08:35 https://devapps.diality.us/cru/HD-SPR3-1#c44 define 2 as a #define Reply by Sean Nash on 14 October 2019, 13:14 > Used #define. Reply by lbaloa on 25 October 2019, 10:30 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 10 October 2019, 08:35 https://devapps.diality.us/cru/HD-SPR3-1#c45 Suggest using variable different than l. l might be confused with 1 ("one") Reply by Sean Nash on 14 October 2019, 13:16 > Addressed in code-walkthrough. Reply by lbaloa on 25 October 2019, 10:30 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 17 October 2019, 09:10 https://devapps.diality.us/cru/HD-SPR3-1#c64 No need for srcPtr pointer... data can be use directly Reply by Sean Nash on 17 October 2019, 09:46 > Agreed. See HD-SPR4-1 review for fix. Reply by lbaloa on 18 October 2019, 14:26 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 17 October 2019, 09:20 https://devapps.diality.us/cru/HD-SPR3-1#c66 inactiveBuffer = !activeBuffer isn't more readable? Reply by Sean Nash on 17 October 2019, 09:46 > I prefer as is. Reply by lbaloa on 15 November 2019, 10:16 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 17 October 2019, 09:19 https://devapps.diality.us/cru/HD-SPR3-1#c65 Don't we have a max, min macro? Reply by Sean Nash on 17 October 2019, 09:49 > I used the MIN macro. See fix in the HD-SPR4-1 review. Reply by lbaloa on 15 November 2019, 10:16 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 17 October 2019, 09:22 https://devapps.diality.us/cru/HD-SPR3-1#c67 Isn't copyFromCommBuffer a clearer name for this function? Reply by Sean Nash on 17 October 2019, 09:50 > That would have worked too. Since I've got unit and > integration test cases looking for this name, I'd prefer not > to change it. Reply by lbaloa on 15 November 2019, 10:17 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 17 October 2019, 09:23 https://devapps.diality.us/cru/HD-SPR3-1#c68 Same comment as before Reply by Sean Nash on 17 October 2019, 09:51 > I prefer it this way. Reply by lbaloa on 15 November 2019, 10:17 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 17 October 2019, 09:24 https://devapps.diality.us/cru/HD-SPR3-1#c69 Shouldn't you have a variable that indicates which is the active buffer and have it managed it internally by the module. That way, you wouldn't need to pass the active buffer. It would be switchDoubleBuffer() Reply by Sean Nash on 17 October 2019, 09:53 > I'm not sure I understand this issue. There is a variable > that indicates the active buffer for each buffer. I am not > passing the active buffer - I'm passing the buffer of > interest and this function is using the active buffer > variable to manage it internally as you say. Reply by lbaloa on 18 October 2019, 14:25 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: App/Drivers/CPLD.c Revision Comment by lbaloa on 11 October 2019, 08:39 https://devapps.diality.us/cru/HD-SPR3-1#c53 I think this need to be round parethenses Reply by Sean Nash on 14 October 2019, 13:07 > Addressed in code walk-through. Reply by lbaloa on 25 October 2019, 10:29 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 10 October 2019, 08:33 https://devapps.diality.us/cru/HD-SPR3-1#c42 Please use parenthesis Reply by Sean Nash on 17 October 2019, 08:50 > Addressed in code walk-through. Reply by lbaloa on 25 October 2019, 10:31 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 10 October 2019, 08:33 https://devapps.diality.us/cru/HD-SPR3-1#c41 Please use parenthesis Reply by Sean Nash on 14 October 2019, 13:17 > Addressed in code-walkthrough. Reply by Sean Nash on 14 October 2019, 13:18 > Addressed in code-walkthrough. Reply by lbaloa on 25 October 2019, 10:31 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: App/Drivers/CPLD.h Revision Comment by lbaloa on 10 October 2019, 08:34 https://devapps.diality.us/cru/HD-SPR3-1#c43 Label to something that indicates is temp Reply by Sean Nash on 14 October 2019, 13:16 > Used build switch. Reply by lbaloa on 25 October 2019, 10:31 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: App/Contollers/Buttons.c Revision Comment by Dara Navaei on 10 October 2019, 08:13 https://devapps.diality.us/cru/HD-SPR3-1#c38 Do we need to define 1 in a variable instead of using a magic number? Reply by Sean Nash on 10 October 2019, 08:45 > Fixed. Reply by Dara Navaei on 14 November 2019, 15:40 > RESOLVED Reply by pmontazemi on 15 November 2019, 11:17 > This is CLASS C code that needs to be reviewed in a group > setting. Reply by Dara Navaei on 26 November 2019, 16:49 > RESOLVED in CODE WALKTHROUGH Revision Comment by lbaloa on 10 October 2019, 08:29 https://devapps.diality.us/cru/HD-SPR3-1#c39 Define 1 as confirmed and 0 as rejected. Create #define variables or use true or false. Reply by Sean Nash on 14 October 2019, 13:18 > Used build switch. Reply by lbaloa on 25 October 2019, 10:31 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: App/Services/WatchdogMgmt.c Revision Comment by lbaloa on 10 October 2019, 08:38 https://devapps.diality.us/cru/HD-SPR3-1#c46 create a temp #define for 0 that indicates intend Reply by Sean Nash on 14 October 2019, 13:10 > Addressed in code walk-through. Reply by lbaloa on 25 October 2019, 10:30 > RESOLVED IN CODE WALKTHROUGH --- ID: HD-SPR3-1 https://devapps.diality.us/cru/HD-SPR3-1 Title: HD Sprint3 Branch Review Statement of Objectives: State: Closed Summary: Author: Sean Nash Reviewers: (0 active, 3 completed*) Dara Navaei (*) pmontazemi (*) lbaloa (*)