This is a list of all comments for HD-DEN-431-1. Review Summary: No summary General Comment by lbaloa on 06 January 2020, 11:14 https://devapps.diality.us/cru/HD-DEN-431-1#c766 There is a lot of commented code that seems to be essential. Are you thinking of uncommenting them before merge? If so, we need to verify it. Reply by Dara Navaei on 06 January 2020, 11:18 > Yes, the commented code is only for calling the API functions > somewhere from outside. I marked that section as test code > and I will delete it before merging. Reply by lbaloa on 07 January 2020, 17:56 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/RTC.c Revision Comment by Behrouz NematiPour on 07 January 2020, 17:18 https://devapps.diality.us/cru/HD-DEN-431-1#c812 Please change it to SHIFT ... Reply by Behrouz NematiPour on 07 January 2020, 17:54 > RESOLVED IN CODE WALKTHROUGH Revision Comment by Sean Nash on 30 December 2019, 08:10 https://devapps.diality.us/cru/HD-DEN-431-1#c707 Add a "NUM_OF_..." to end of all enums - this will help with unit testing in VectorCAST. Reply by Dara Navaei on 06 January 2020, 19:50 > Done! Reply by Sean Nash on 07 January 2020, 17:48 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:12 https://devapps.diality.us/cru/HD-DEN-431-1#c708 Add descriptive comments to right of variables. Reply by Dara Navaei on 06 January 2020, 19:50 > Done! Reply by Sean Nash on 07 January 2020, 17:49 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 03 January 2020, 17:55 https://devapps.diality.us/cru/HD-DEN-431-1#c757 Why the space? Reply by Dara Navaei on 06 January 2020, 14:01 > Removed the space. Reply by Sean Nash on 07 January 2020, 17:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:14 https://devapps.diality.us/cru/HD-DEN-431-1#c709 Make a #define for 2000. Reply by Dara Navaei on 06 January 2020, 19:46 > Added #define for the years. Reply by Sean Nash on 07 January 2020, 17:52 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Behrouz NematiPour on 30 December 2019, 11:12 https://devapps.diality.us/cru/HD-DEN-431-1#c753 Please, Please, Don't make codes unnecessarily long. Your code is readable enough. To see a function you have to scroll more than required. Reply by Behrouz NematiPour on 07 January 2020, 17:57 > RESOLVED IN CODE WALKTHROUGH Revision Comment by Sean Nash on 30 December 2019, 08:19 https://devapps.diality.us/cru/HD-DEN-431-1#c711 Software fault if invalid state. Reply by Dara Navaei on 06 January 2020, 19:46 > Software fault will be added. Reply by Sean Nash on 07 January 2020, 17:49 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:21 https://devapps.diality.us/cru/HD-DEN-431-1#c712 length should be range checked. Reply by Dara Navaei on 06 January 2020, 19:29 > Length is range checked in the above else if statement. Reply by Sean Nash on 07 January 2020, 17:49 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:25 https://devapps.diality.us/cru/HD-DEN-431-1#c719 There are macros in Common.h for getting high and low bytes of a word. Reply by Dara Navaei on 06 January 2020, 19:38 > Done! Reply by Sean Nash on 07 January 2020, 17:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:21 https://devapps.diality.us/cru/HD-DEN-431-1#c713 Local variables should be declared at top of scope. Reply by Dara Navaei on 06 January 2020, 19:45 > Done! Reply by Sean Nash on 07 January 2020, 17:51 > Move to top of scope instead of top of function. Reply by Dara Navaei on 07 January 2020, 18:30 > Moved the local variable into the scope Reply by Sean Nash on 10 January 2020, 10:08 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 07 January 2020, 17:48 https://devapps.diality.us/cru/HD-DEN-431-1#c834 Move to top of scope where i is used. Reply by Dara Navaei on 07 January 2020, 18:30 > Move the local variable into the scope. Reply by Sean Nash on 10 January 2020, 10:06 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:22 https://devapps.diality.us/cru/HD-DEN-431-1#c714 length should be range checked. Reply by Dara Navaei on 06 January 2020, 19:45 > Length is range checked in the above else if statement. Reply by Sean Nash on 07 January 2020, 17:47 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:24 https://devapps.diality.us/cru/HD-DEN-431-1#c717 There are macros in Common.h for getting high and low bytes of a word. Reply by Dara Navaei on 06 January 2020, 19:39 > Done! Reply by pmontazemi on 10 January 2020, 10:23 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:22 https://devapps.diality.us/cru/HD-DEN-431-1#c715 Local variables should be declared at top of scope. Reply by Dara Navaei on 06 January 2020, 19:44 > Done Reply by pmontazemi on 10 January 2020, 10:23 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:28 https://devapps.diality.us/cru/HD-DEN-431-1#c721 Boolean variables should have names that reflect their T/F nature (i.e. bufferIsOk). Reply by Dara Navaei on 06 January 2020, 19:11 > Changed it to isBufferOk. Reply by Sean Nash on 07 January 2020, 17:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:26 https://devapps.diality.us/cru/HD-DEN-431-1#c720 bufferLength should be range checked. Or does setMibSPIBufferLength() range check it? Reply by Dara Navaei on 06 January 2020, 19:12 > setMibSPIBufferLength() checks the range. Reply by Sean Nash on 07 January 2020, 17:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:31 https://devapps.diality.us/cru/HD-DEN-431-1#c725 It doesn't look like the transfer failed here. Should this be set to zero? Reply by Dara Navaei on 06 January 2020, 19:08 > Changed the value to 0. Reply by Sean Nash on 07 January 2020, 17:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:32 https://devapps.diality.us/cru/HD-DEN-431-1#c726 Should there be an alarm triggered here? Reply by Dara Navaei on 06 January 2020, 19:07 > Software alarms will be added. Reply by Sean Nash on 07 January 2020, 17:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:32 https://devapps.diality.us/cru/HD-DEN-431-1#c727 Software fault if invalid state. Reply by Dara Navaei on 06 January 2020, 19:04 > The software faults will be added Reply by Sean Nash on 07 January 2020, 17:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:35 https://devapps.diality.us/cru/HD-DEN-431-1#c729 Why do we care if an unused bit is set? Reply by Dara Navaei on 06 January 2020, 19:03 > In the datasheet, it is mentioned that bits labeled as T must > always be written with logic 0. The unused bit is labeled as > T so I am checking it all the time. Reply by Sean Nash on 07 January 2020, 17:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:38 https://devapps.diality.us/cru/HD-DEN-431-1#c734 Consider moving these conversion functions to Utilities.c so any module can use them. Reply by Dara Navaei on 06 January 2020, 16:38 > These are specifically used in RTC so I would keep them here. Reply by Sean Nash on 07 January 2020, 17:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 December 2019, 08:39 https://devapps.diality.us/cru/HD-DEN-431-1#c735 Let's not assume. Range check "decimal" parameter. Software fault if > 99. Reply by Dara Navaei on 06 January 2020, 18:46 > Done! Reply by Sean Nash on 07 January 2020, 17:37 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 30 December 2019, 08:29 https://devapps.diality.us/cru/HD-DEN-431-1#c722 Define constants for 2000, 1970, etc. Reply by Dara Navaei on 06 January 2020, 19:11 > Added #defines for the years Reply by Dara Navaei on 19 October 2023, 08:57 > RESOLVED in CODE WALKTHROUGH Revision Comment by Sean Nash on 30 December 2019, 08:42 https://devapps.diality.us/cru/HD-DEN-431-1#c736 Use #defines for 2000 and 1970. Reply by Dara Navaei on 06 January 2020, 16:15 > Added #defines for the years Reply by pmontazemi on 07 January 2020, 17:31 > RESOLVED in CODE WALKTHROUGH. Reply by Sean Nash on 07 January 2020, 17:37 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Behrouz NematiPour on 03 January 2020, 16:55 https://devapps.diality.us/cru/HD-DEN-431-1#c755 shorten the length of the argument you are using as length since you are only filling the txBuffer and it can't have more than 127U. And it's using more space (at least 3 bytes) than requires. Reply by Dara Navaei on 06 January 2020, 14:23 > Done! Reply by Behrouz NematiPour on 07 January 2020, 17:57 > RESOLVED IN CODE WALKTHROUGH Revision Comment by Sean Nash on 30 December 2019, 08:48 https://devapps.diality.us/cru/HD-DEN-431-1#c737 Throughout module - function declarations with no parameters should have void. Reply by Dara Navaei on 06 January 2020, 15:44 > Done! Reply by Sean Nash on 07 January 2020, 17:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Behrouz NematiPour on 03 January 2020, 17:16 https://devapps.diality.us/cru/HD-DEN-431-1#c756 In general, please consider using "txBuffer" and not "&txBuffer[0]" . Reply by Dara Navaei on 06 January 2020, 14:21 > Done! Reply by Behrouz NematiPour on 07 January 2020, 17:56 > RESOLVED IN CODE WALKTHROUGH Revision Comment by pmontazemi on 30 December 2019, 08:29 https://devapps.diality.us/cru/HD-DEN-431-1#c723 Only one (1) extra line at the end of each file. Reply by Dara Navaei on 06 January 2020, 19:08 > Done! Reply by pmontazemi on 07 January 2020, 17:29 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/RTC.h Revision Comment by Sean Nash on 30 December 2019, 08:08 https://devapps.diality.us/cru/HD-DEN-431-1#c706 Definitions should come before function prototypes. Reply by Behrouz NematiPour on 30 December 2019, 11:08 > Very good point does it build if the definition is not > present at the function definition ? Reply by Behrouz NematiPour on 30 December 2019, 11:15 > I understood the usage, thanks. Reply by Dara Navaei on 06 January 2020, 14:03 > Done! Reply by Sean Nash on 07 January 2020, 17:52 > RESOLVED in CODE WALKTHROUGH. Reply by Behrouz NematiPour on 07 January 2020, 17:58 > RESOLVED IN CODE WALKTHROUGH Revision Comment by pmontazemi on 30 December 2019, 08:30 https://devapps.diality.us/cru/HD-DEN-431-1#c724 Only one (1) extra line at the end of each file. Reply by Dara Navaei on 06 January 2020, 14:03 > Removed the extra lines. Reply by pmontazemi on 07 January 2020, 17:29 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Tasks/TaskGeneral.c Revision Comment by Sean Nash on 30 December 2019, 08:05 https://devapps.diality.us/cru/HD-DEN-431-1#c705 Please remember to remove all of this test code before merging the branch. Reply by Dara Navaei on 06 January 2020, 19:51 > This code will be removed prior to merging this branch to > master. Reply by Sean Nash on 07 January 2020, 17:52 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/source/sys_main.c Revision Comment by Behrouz NematiPour on 30 December 2019, 10:49 https://devapps.diality.us/cru/HD-DEN-431-1#c749 Doesn't RTC be initialized at the beginning since it might be initialized to be used by the other modules ? Reply by Dara Navaei on 06 January 2020, 15:11 > initRTC() only sets the self test to start so it does not > have to be in the beginning. Reply by Behrouz NematiPour on 07 January 2020, 18:01 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Drivers/CPLD.c Revision Comment by Behrouz NematiPour on 30 December 2019, 11:05 https://devapps.diality.us/cru/HD-DEN-431-1#c751 Only as a suggestion : I would create a toggle function since you used the same code @258 as well. Reply by Dara Navaei on 06 January 2020, 14:25 > This is Sean's code. Reply by Sean Nash on 07 January 2020, 16:08 > I have a toggle macro in use on my branch. Reply by Behrouz NematiPour on 07 January 2020, 18:02 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Services/FPGA.c Revision Comment by pmontazemi on 30 December 2019, 08:34 https://devapps.diality.us/cru/HD-DEN-431-1#c728 I would reverse the order of arguments. Reply by Behrouz NematiPour on 30 December 2019, 10:58 > If Peman means to have "if ( function() != 0 )" I totally > agree with him and I don't know why it should be in reverse? Reply by Dara Navaei on 06 January 2020, 19:04 > This code belongs to Sean. Reply by Sean Nash on 07 January 2020, 16:10 > I probably had that as "==" at one point and had to flip > it later. When "==" I always put the constant first so > that if I accidentally use "=" the compiler will flag it > for me. I addressed this in my branch of code. Reply by pmontazemi on 07 January 2020, 17:28 > RESOLVED in CODE WALKTHROUGH. --- ID: HD-DEN-431-1 https://devapps.diality.us/cru/HD-DEN-431-1 Title: HD-DEN-431_Real Time Clock Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (1 active, 3 completed*) Sean Nash (*) pmontazemi (*) lbaloa (*) Behrouz NematiPour