This is a list of all comments for HD-SPR4-1. Review Summary: No summary ---------------------------------------- File: App/Services/CommBuffers.c Revision Comment by lbaloa on 24 October 2019, 08:35 https://devapps.diality.us/cru/HD-SPR4-1#c111 rename data to dataPtr Reply by Sean Nash on 24 October 2019, 09:01 > Is this a convention we want to impose? I have "data" in > many places already. Reply by lbaloa on 25 October 2019, 10:21 > As is. RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: App/Controllers/AlarmLamp.c Revision Comment by lbaloa on 24 October 2019, 08:29 https://devapps.diality.us/cru/HD-SPR4-1#c107 Not clear what is the purpose of DATA_DECL Reply by Sean Nash on 24 October 2019, 08:37 > It's a macro to declare a data that you want to be able to > override via script command. Reply by lbaloa on 25 October 2019, 10:24 > as is RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 24 October 2019, 08:28 https://devapps.diality.us/cru/HD-SPR4-1#c106 where does currentLampPattern.data come from? This is very confusing. It would be ideal to create simple structures. currentLampPattern is a enum. Where does .data come from? Need to make it simple. Reply by Sean Nash on 24 October 2019, 08:38 > Comes from the DATA_DECL macro above. Reply by lbaloa on 25 October 2019, 10:25 > as is RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 24 October 2019, 08:24 https://devapps.diality.us/cru/HD-SPR4-1#c104 This pattern is confusing. Not very clear what is meant to do Reply by lbaloa on 25 October 2019, 10:27 > as is RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 24 October 2019, 08:25 https://devapps.diality.us/cru/HD-SPR4-1#c105 currentLampPattern should be a local variable and it is OK to use it. Using a function add unnecessary extra lower level step. If a function is used, should be for modules outside this one. Reply by Sean Nash on 24 October 2019, 08:57 > Need data that can be overridden to go through getter > function. Reply by lbaloa on 25 October 2019, 10:26 > as is RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: App/Controllers/Buttons.c Revision Comment by lbaloa on 24 October 2019, 08:31 https://devapps.diality.us/cru/HD-SPR4-1#c108 Make the ? operator a macro to make it more readable Reply by Sean Nash on 24 October 2019, 08:59 > Why is ? operator not readable? Reply by lbaloa on 25 October 2019, 10:24 > as is RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 24 October 2019, 08:33 https://devapps.diality.us/cru/HD-SPR4-1#c110 .data need to clarify. It makes the code confusing. Reply by lbaloa on 15 November 2019, 10:18 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: App/Services/SystemCommMessages.c Revision Comment by lbaloa on 24 October 2019, 09:00 https://devapps.diality.us/cru/HD-SPR4-1#c116 Can you revert the order? output should be on the left, input to the right. Reply by Sean Nash on 24 October 2019, 09:07 > Is this a convention we want to impose? Why should output be > on left? Reply by lbaloa on 25 October 2019, 10:17 > RESOLVED IN CODE WALKTHROUGH Revision Comment by pmontazemi on 29 October 2019, 08:37 https://devapps.diality.us/cru/HD-SPR4-1#c176 I like to always place spaces between arguments and mathematical signs. Reply by Sean Nash on 29 October 2019, 09:19 > Done. Reply by pmontazemi on 30 October 2019, 10:05 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: source/can.c Revision Comment by pmontazemi on 28 October 2019, 15:40 https://devapps.diality.us/cru/HD-SPR4-1#c161 Were these CAN registers re-generated from HALCoGen or manually changed? Reply by Sean Nash on 29 October 2019, 09:21 > Generated from HALCoGen Reply by pmontazemi on 30 October 2019, 10:06 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: App/Services/CommInterrupts.c Revision Comment by lbaloa on 25 October 2019, 08:08 https://devapps.diality.us/cru/HD-SPR4-1#c131 Shouldn't this be |= . If yes, rethink for the remainder of the function in this module Reply by lbaloa on 25 October 2019, 10:09 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: App/Services/Utilities.c Revision Comment by lbaloa on 24 October 2019, 09:07 https://devapps.diality.us/cru/HD-SPR4-1#c120 Because this is a calculation, add how many bits are you shifting. SHIFT_5_BITS_IN_BYTE to make it more readable. Reply by Sean Nash on 24 October 2019, 12:40 > Renamed #define to clarify # of bits. Reply by lbaloa on 25 October 2019, 10:11 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 24 October 2019, 09:08 https://devapps.diality.us/cru/HD-SPR4-1#c121 do not use address++ in calculation. It is confusing. Use it after. Reply by Sean Nash on 24 October 2019, 12:41 > I thought this might be a little faster this way, but I guess > this function will always be working on small memory ranges > as it's used for messages - so I'll make this change as you > recommended. Reply by lbaloa on 15 November 2019, 10:19 > RESOLVED IN CODE WALKTHROUGH Revision Comment by lbaloa on 24 October 2019, 09:09 https://devapps.diality.us/cru/HD-SPR4-1#c122 MASK_OFF_MSB (indicates how many bits), e.g., MASK_OFF_4_MSB Reply by Sean Nash on 24 October 2019, 12:43 > I think this should be clear enough. MSB stands for most > significant BYTE, so it should be clear we are masking off 8 > bits. Reply by lbaloa on 25 October 2019, 10:12 > RESOLVED IN CODE WALKTHROUGH --- ID: HD-SPR4-1 https://devapps.diality.us/cru/HD-SPR4-1 Title: HD Sprint4 Branch Review Statement of Objectives: State: Closed Summary: Author: Sean Nash Reviewers: (0 active, 3 completed*) Dara Navaei (*) pmontazemi (*) lbaloa (*)