This is a list of all comments for DG-DEN-4217-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/DrainPump.c Revision Comment by pmontazemi on 12 August 2020, 10:54 https://devapps.diality.us/cru/DG-DEN-4217-1#c3158 Remove extra line, also where is the /*@}/ Doxygen comment at eof? Reply by Sean Nash on 12 August 2020, 11:50 > Done. Reply by pmontazemi on 17 August 2020, 15:38 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/TemperatureSensors.c Revision Comment by pmontazemi on 12 August 2020, 10:56 https://devapps.diality.us/cru/DG-DEN-4217-1#c3162 Start coefficients right after the first "{". Reply by Sean Nash on 17 August 2020, 15:39 > Done. Reply by pmontazemi on 17 August 2020, 15:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 10:57 https://devapps.diality.us/cru/DG-DEN-4217-1#c3163 Start coefficients right after the first "{" and left adjust/align them. Reply by Sean Nash on 12 August 2020, 11:52 > Done Reply by pmontazemi on 17 August 2020, 15:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 10:57 https://devapps.diality.us/cru/DG-DEN-4217-1#c3164 One liner Reply by Sean Nash on 12 August 2020, 11:53 > Done Reply by pmontazemi on 17 August 2020, 15:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 10:57 https://devapps.diality.us/cru/DG-DEN-4217-1#c3165 Space after // in comments Reply by Sean Nash on 12 August 2020, 11:53 > Done Reply by pmontazemi on 17 August 2020, 15:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 10:57 https://devapps.diality.us/cru/DG-DEN-4217-1#c3167 Space after // in comments Reply by Sean Nash on 12 August 2020, 11:54 > Done Reply by pmontazemi on 17 August 2020, 15:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 20 August 2020, 14:40 https://devapps.diality.us/cru/DG-DEN-4217-1#c3608 Spaces between arithmetic operation signs. Reply by Sean Nash on 20 August 2020, 14:46 > This isn't code - it's a commented equation explaining next 2 > lines of code. That said, Dara can add spaces for > readability on his branch. Reply by pmontazemi on 20 August 2020, 15:08 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 20 August 2020, 14:40 https://devapps.diality.us/cru/DG-DEN-4217-1#c3609 Spaces between arithmetic operation signs. Reply by Sean Nash on 20 August 2020, 14:45 > This isn't code - it's a commented equation explaining next 2 > lines of code. That said, Dara can add spaces for > readability on his branch. Reply by pmontazemi on 20 August 2020, 15:09 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 10:58 https://devapps.diality.us/cru/DG-DEN-4217-1#c3168 Space after // in comments Reply by Sean Nash on 12 August 2020, 11:54 > Done Reply by pmontazemi on 17 August 2020, 15:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 11:07 https://devapps.diality.us/cru/DG-DEN-4217-1#c3170 Remove commented line (or add a TODO with clear explanation why this is commented out). Reply by Sean Nash on 12 August 2020, 11:59 > Looks like this self test is not fully implemented. Added > TODO comment. Reply by pmontazemi on 17 August 2020, 15:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 11:07 https://devapps.diality.us/cru/DG-DEN-4217-1#c3171 Remove commented line (or add a TODO with clear explanation why this is commented out). Reply by Sean Nash on 12 August 2020, 12:01 > Not sure if we want to keep this self test. Not sure it is > workable. Added TODO. Reply by qnguyen on 12 August 2020, 13:46 > This self test has been modified to only check the > consistency between TPi and Tpo in branch > DEN-4169-dg-inlet-water-temperature. Reply by pmontazemi on 17 August 2020, 15:36 > Please remove when doing the merge. Reply by pmontazemi on 17 August 2020, 15:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 11:07 https://devapps.diality.us/cru/DG-DEN-4217-1#c3172 Remove commented line (or add a TODO with clear explanation why this is commented out). Reply by Sean Nash on 12 August 2020, 12:04 > I think Quang moved this check to somewhere else where only > checked in treatment modes in one of his branches. Added > TODO. Reply by qnguyen on 12 August 2020, 13:44 > The temperature sensor has been refactored in branch > DEN-4169-dg-inlet-water-temperature that has been reviewed. > The code here will be obsoleted once we merge the branch > in. Reply by pmontazemi on 17 August 2020, 15:33 > Be careful merging to make sure this part gets obsoleted. Reply by pmontazemi on 17 August 2020, 15:33 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/DGCommon.h Revision Comment by qnguyen on 11 August 2020, 15:19 https://devapps.diality.us/cru/DG-DEN-4217-1#c3052 Are these flags enabled as intention? Reply by Sean Nash on 11 August 2020, 16:03 > All of these flags are for DEBUG build only (not release) and > most are not see by Vectorcast either - so for code review > purposes it doesn't really matter what the last DEBUG build > configuration was. Reply by qnguyen on 17 August 2020, 15:04 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by pmontazemi on 12 August 2020, 14:17 https://devapps.diality.us/cru/DG-DEN-4217-1#c3264 Packing alignment removed? Reply by Sean Nash on 12 August 2020, 15:16 > Moved these records to MessagePayloads.h Reply by pmontazemi on 17 August 2020, 15:14 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 14:16 https://devapps.diality.us/cru/DG-DEN-4217-1#c3263 Where were these moved to? Reply by Sean Nash on 12 August 2020, 15:16 > To MessagePayloads.h. I think I'll eventually redistribute > these back to the appropriate individual module headers and > remove MessagePayloads.h - still working out what makes most > sense. Reply by pmontazemi on 17 August 2020, 15:14 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 11 August 2020, 15:28 https://devapps.diality.us/cru/DG-DEN-4217-1#c3056 The message should be DG accelerometer data? Reply by Sean Nash on 11 August 2020, 16:12 > Done. Reply by qnguyen on 17 August 2020, 15:03 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 14:18 https://devapps.diality.us/cru/DG-DEN-4217-1#c3266 Add extra line between } and function. Reply by Sean Nash on 12 August 2020, 15:35 > Done Reply by pmontazemi on 17 August 2020, 15:13 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 14:19 https://devapps.diality.us/cru/DG-DEN-4217-1#c3267 Add extra line between } and function. Reply by Sean Nash on 12 August 2020, 15:34 > Done Reply by pmontazemi on 17 August 2020, 15:13 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 14:19 https://devapps.diality.us/cru/DG-DEN-4217-1#c3268 Add extra line between } and function. Reply by Sean Nash on 12 August 2020, 15:34 > Done Reply by pmontazemi on 17 August 2020, 15:13 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 14:19 https://devapps.diality.us/cru/DG-DEN-4217-1#c3269 Add extra line between } and function. Reply by Sean Nash on 12 August 2020, 15:34 > Done Reply by pmontazemi on 17 August 2020, 15:13 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 14:19 https://devapps.diality.us/cru/DG-DEN-4217-1#c3270 Add extra line between } and function. Reply by Sean Nash on 12 August 2020, 15:33 > Done Reply by pmontazemi on 17 August 2020, 15:13 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 14:19 https://devapps.diality.us/cru/DG-DEN-4217-1#c3271 Add extra line between } and function. Reply by Sean Nash on 12 August 2020, 15:33 > Done Reply by pmontazemi on 17 August 2020, 15:12 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 14:19 https://devapps.diality.us/cru/DG-DEN-4217-1#c3272 Add extra line between } and function. Reply by Sean Nash on 12 August 2020, 15:32 > Done Reply by pmontazemi on 17 August 2020, 15:12 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 14:20 https://devapps.diality.us/cru/DG-DEN-4217-1#c3273 Doxygen /**@}*/ missing. Reply by Sean Nash on 12 August 2020, 15:11 > This module not yet converted to new Doxygen format. Reply by pmontazemi on 17 August 2020, 15:11 > Planned to be taken care of in DEN S26. Reply by pmontazemi on 17 August 2020, 15:11 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/FPGA.c Revision Comment by pmontazemi on 20 August 2020, 14:46 https://devapps.diality.us/cru/DG-DEN-4217-1#c3612 What is lab rev number and how it is different from minor rev number? Reply by Sean Nash on 20 August 2020, 14:49 > It's a test build #. They may do several lab revisions > before test results looks good, then roll minor revision. Reply by pmontazemi on 20 August 2020, 15:08 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 13:41 https://devapps.diality.us/cru/DG-DEN-4217-1#c3223 FPGA ALL CAPS Reply by Sean Nash on 12 August 2020, 15:27 > Done Reply by pmontazemi on 17 August 2020, 15:24 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 13:41 https://devapps.diality.us/cru/DG-DEN-4217-1#c3225 Where is the Doxygen eof /**@}*/? Reply by Sean Nash on 12 August 2020, 15:26 > This module not yet converted to new Doxygen format. Reply by pmontazemi on 17 August 2020, 15:23 > Planned to be taken care of in DEN S26. Reply by pmontazemi on 17 August 2020, 15:23 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/FPGA.h Revision Comment by pmontazemi on 12 August 2020, 13:41 https://devapps.diality.us/cru/DG-DEN-4217-1#c3226 Where is the Doxygen eof /**@}*/? Reply by Sean Nash on 12 August 2020, 15:25 > This module not yet converted to new Doxygen format. Reply by pmontazemi on 17 August 2020, 15:22 > Planned to be taken care of in DEN S26. Reply by Dara Navaei on 19 October 2023, 08:27 > RESOLVED in CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Services/MessagePayloads.h Revision Comment by qnguyen on 11 August 2020, 15:25 https://devapps.diality.us/cru/DG-DEN-4217-1#c3055 Suggest renaming the structure to make it common for both DG and HD or distinguish it from HD. Reply by Sean Nash on 11 August 2020, 16:11 > Good catch. Removed HD_ prefix and moved to common Accel.h. Reply by qnguyen on 17 August 2020, 15:04 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 14:25 https://devapps.diality.us/cru/DG-DEN-4217-1#c3278 Doxygen /**@}*/ missing. Reply by Sean Nash on 12 August 2020, 15:09 > This was part of SystemCommMessages.c which has not been > updated to new Doxygen format. I think these structures will > be moved eventually to their respective module header files > and this header file will go away. Reply by pmontazemi on 17 August 2020, 15:09 > Planned to be taken care of in DEN S26. Reply by pmontazemi on 17 August 2020, 15:09 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Reservoirs.c Revision Comment by pmontazemi on 12 August 2020, 14:24 https://devapps.diality.us/cru/DG-DEN-4217-1#c3276 What is our plan to generate different builds for various prototypes? Aren't we removing the capability of being able to build release v0.4 by removing this code? Reply by Sean Nash on 12 August 2020, 15:04 > Release builds (i.e. Bamboo) will not have any of these build > switches defined. As we get closer to DVT, we should be able > to get rid of most of these debug build switches, but they > will never interfere with a release build. Reply by pmontazemi on 17 August 2020, 15:10 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemComm.c Revision Comment by qnguyen on 11 August 2020, 12:22 https://devapps.diality.us/cru/DG-DEN-4217-1#c3046 Replace # with number. Reply by Sean Nash on 11 August 2020, 14:17 > Done. Reply by qnguyen on 17 August 2020, 15:06 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 14:21 https://devapps.diality.us/cru/DG-DEN-4217-1#c3274 No more DMA needed? Reply by Sean Nash on 12 August 2020, 15:03 > Moved DMA records a little further down inside #ifdef > DEBUG_ENABLED because we only do DMA for UART and release > versions will not have UART. Reply by pmontazemi on 17 August 2020, 15:11 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 20 August 2020, 14:54 https://devapps.diality.us/cru/DG-DEN-4217-1#c3614 Recommend replacing 2 with DG and do the same for other Software Items (HD, etc.). Reply by Sean Nash on 20 August 2020, 14:58 > Agreed. Will address in another branch - maybe Dara's? Reply by pmontazemi on 20 August 2020, 15:06 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.h Revision Comment by pmontazemi on 12 August 2020, 14:13 https://devapps.diality.us/cru/DG-DEN-4217-1#c3261 No space between function name and "(" in both functions newly added. Reply by Sean Nash on 17 August 2020, 15:44 > Done. Reply by pmontazemi on 17 August 2020, 15:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 14:14 https://devapps.diality.us/cru/DG-DEN-4217-1#c3262 Doxygen /**@}*/ missing at eof. Reply by Sean Nash on 12 August 2020, 15:18 > This module not yet converted to new Doxygen format. Reply by pmontazemi on 17 August 2020, 15:14 > Planned to be taken care of in DEN S26. Reply by pmontazemi on 17 August 2020, 15:15 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Tasks/TaskPriority.c Revision Comment by pmontazemi on 12 August 2020, 14:12 https://devapps.diality.us/cru/DG-DEN-4217-1#c3258 Remove commented section if not used, otherwise add TODO comment. Reply by Sean Nash on 17 August 2020, 15:41 > Done. Reply by pmontazemi on 17 August 2020, 15:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 14:12 https://devapps.diality.us/cru/DG-DEN-4217-1#c3259 Missing the Doxygen /**@}*/ at eof. Reply by Sean Nash on 12 August 2020, 15:18 > This module not yet converted to new Doxygen format. Reply by pmontazemi on 17 August 2020, 15:16 > Planned to be taken care of in DEN S26. Reply by Dara Navaei on 19 October 2023, 08:28 > RESOLVED in CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Controllers/Valves.c Revision Comment by pmontazemi on 12 August 2020, 11:09 https://devapps.diality.us/cru/DG-DEN-4217-1#c3174 Argument no longer needed? Reply by Sean Nash on 17 August 2020, 15:42 > Argument not used in function so I removed. Reply by pmontazemi on 17 August 2020, 15:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 11:09 https://devapps.diality.us/cru/DG-DEN-4217-1#c3175 Argument no longer needed? Reply by Sean Nash on 12 August 2020, 12:07 > Argument was not being used in function so I removed it. Reply by pmontazemi on 17 August 2020, 15:29 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 11:09 https://devapps.diality.us/cru/DG-DEN-4217-1#c3176 Argument no longer needed? Reply by Sean Nash on 12 August 2020, 12:08 > Argument not being used in function so I removed it. Reply by pmontazemi on 17 August 2020, 15:28 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 11:08 https://devapps.diality.us/cru/DG-DEN-4217-1#c3173 Why was this deleted to be re-added? I remember I had this in. Reply by Sean Nash on 12 August 2020, 12:06 > Not sure how/where it got dropped if you had it before. Reply by pmontazemi on 17 August 2020, 15:30 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/CommBuffers.c Revision Comment by qnguyen on 12 August 2020, 10:40 https://devapps.diality.us/cru/DG-DEN-4217-1#c3120 Remove unused variable i. Reply by Sean Nash on 12 August 2020, 11:50 > Done Reply by qnguyen on 17 August 2020, 15:03 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 13:39 https://devapps.diality.us/cru/DG-DEN-4217-1#c3216 One liner Reply by Sean Nash on 12 August 2020, 15:31 > Done Reply by pmontazemi on 17 August 2020, 15:27 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 13:39 https://devapps.diality.us/cru/DG-DEN-4217-1#c3217 Good candidate for a two liner... Reply by Sean Nash on 12 August 2020, 15:30 > Done Reply by pmontazemi on 17 August 2020, 15:26 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 13:40 https://devapps.diality.us/cru/DG-DEN-4217-1#c3218 Why this check is no longer required? Reply by Sean Nash on 12 August 2020, 15:28 > The buffer lock was added long ago to try to help thread > protect the buffers. Since IRQs prioritized and FIQs changed > to IRQs, this is no longer needed. Reply by pmontazemi on 17 August 2020, 15:26 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 13:40 https://devapps.diality.us/cru/DG-DEN-4217-1#c3220 Where is the Doxygen eof /**@}*/? Reply by Sean Nash on 12 August 2020, 15:27 > This module not yet converted to new Doxygen format. Reply by pmontazemi on 17 August 2020, 15:25 > Planned to be taken care of in DEN S26. Reply by pmontazemi on 17 August 2020, 15:25 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/CommBuffers.h Revision Comment by pmontazemi on 12 August 2020, 13:40 https://devapps.diality.us/cru/DG-DEN-4217-1#c3221 Where is the Doxygen eof /**@}*/? Reply by Sean Nash on 12 August 2020, 15:27 > This module not yet converted to new Doxygen format. Reply by pmontazemi on 17 August 2020, 15:24 > Planned to be taken care of in DEN S26. Reply by pmontazemi on 17 August 2020, 15:24 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeDrain.c Revision Comment by pmontazemi on 12 August 2020, 11:10 https://devapps.diality.us/cru/DG-DEN-4217-1#c3178 Only 1 space between ) and // comment Reply by Sean Nash on 12 August 2020, 12:09 > Done Reply by pmontazemi on 17 August 2020, 15:28 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeFill.c Revision Comment by pmontazemi on 12 August 2020, 11:10 https://devapps.diality.us/cru/DG-DEN-4217-1#c3179 Only one space between ) and // comment. Reply by Sean Nash on 12 August 2020, 12:10 > Done Reply by pmontazemi on 17 August 2020, 15:29 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 11:10 https://devapps.diality.us/cru/DG-DEN-4217-1#c3180 What about R2? We are not handling it anymore? Reply by Sean Nash on 12 August 2020, 12:11 > fillWeightLoadCell initialized to A1 at top of function > (assuming active reservoir is R2 and so we would want load > cell associated with the inactive reservoir R1 which is A1. > So we only need to see if that assumption is wrong here. Reply by pmontazemi on 17 August 2020, 15:27 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/AlarmMgmt.c Revision Comment by pmontazemi on 12 August 2020, 11:15 https://devapps.diality.us/cru/DG-DEN-4217-1#c3183 Add empty line between } and next function call. Reply by Sean Nash on 12 August 2020, 12:14 > Done Reply by pmontazemi on 17 August 2020, 15:27 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 13:38 https://devapps.diality.us/cru/DG-DEN-4217-1#c3214 Add empty line after }. Reply by Sean Nash on 12 August 2020, 15:32 > Done Reply by pmontazemi on 17 August 2020, 15:27 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 13:38 https://devapps.diality.us/cru/DG-DEN-4217-1#c3215 Add empty line after }. Reply by Sean Nash on 12 August 2020, 15:31 > Done Reply by pmontazemi on 17 August 2020, 15:27 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/MsgQueues.c Revision Comment by pmontazemi on 12 August 2020, 14:25 https://devapps.diality.us/cru/DG-DEN-4217-1#c3277 1) Remove extra line; 2) Doxygen /**@}*/ missing. Reply by Sean Nash on 12 August 2020, 15:07 > Some modules that were implemented early like this one have > not yet been converted to new Doxygen format. Eventually, we > will get all of these older modules caught up, but it takes > time so need to create a task in coming sprints to allot some > time for it. Reply by pmontazemi on 17 August 2020, 15:09 > Planned to be taken care of in DEN S26. Reply by pmontazemi on 17 August 2020, 15:09 > RESOLVED IN CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/WatchdogMgmt.c Revision Comment by qnguyen on 14 August 2020, 10:55 https://devapps.diality.us/cru/DG-DEN-4217-1#c3438 Remove extra space between value and override. Should we capitalize override to signal the beginning of the description for that parameter? Reply by Sean Nash on 14 August 2020, 11:03 > Done Reply by qnguyen on 17 August 2020, 15:02 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Tasks/TaskGeneral.c Revision Comment by qnguyen on 11 August 2020, 15:14 https://devapps.diality.us/cru/DG-DEN-4217-1#c3049 Should this not be commented out since there is a ifdef guarding it? Reply by Sean Nash on 11 August 2020, 15:22 > At the moment, I only have one GPIO (that I borrow from drain > pump enable) to share for all tasks so I can't leave them all > un-commented. Ideally, we find spare GPIO pin for each task. > For now, the #ifdef is just determining whether borrowed pin > is for drain pump or task timing. Reply by qnguyen on 17 August 2020, 15:05 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 11 August 2020, 15:15 https://devapps.diality.us/cru/DG-DEN-4217-1#c3050 Should this not be commented out since there is a ifdef guarding it? Reply by Sean Nash on 11 August 2020, 15:24 > Same comment as above. Reply by qnguyen on 17 August 2020, 15:05 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 14:13 https://devapps.diality.us/cru/DG-DEN-4217-1#c3260 Missing the Doxygen /**@}*/ at eof. Reply by Sean Nash on 12 August 2020, 15:18 > This module not yet converted to new Doxygen format. Reply by pmontazemi on 17 August 2020, 15:16 > Planned to be taken care of in DEN S26. Reply by pmontazemi on 17 August 2020, 15:16 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Tasks/TaskTimer.c Revision Comment by pmontazemi on 12 August 2020, 14:11 https://devapps.diality.us/cru/DG-DEN-4217-1#c3257 1) Remove extra line; 2) Missing the Doxygen /**@}*/ at eof. Reply by Sean Nash on 12 August 2020, 15:22 > 1) Done > 2) This module not yet converted to new Doxygen format. Reply by pmontazemi on 17 August 2020, 15:18 > Planned to be taken care of in DEN S26. Reply by pmontazemi on 19 August 2020, 10:39 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: Utilities.c Revision Comment by pmontazemi on 12 August 2020, 13:52 https://devapps.diality.us/cru/DG-DEN-4217-1#c3242 We should provide somewhere a description of what critical data is, at least few examples given at this level. Reply by Sean Nash on 12 August 2020, 15:19 > I think SHA will (maybe already does) call out certain data > items as critical and control measure is to have some kind of > means to verify integrity. I think all of the treatment > parameters that I'm adding in this story will qualify as > critical so I added these critical data structures and > functions. Reply by pmontazemi on 17 August 2020, 15:18 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: Accel.c Revision Comment by pmontazemi on 19 August 2020, 10:40 https://devapps.diality.us/cru/DG-DEN-4217-1#c3593 Commented -1.0 on purpose? Reply by Sean Nash on 19 August 2020, 10:44 > Waiting for DG FPGA fix to remove comment permanently as this > is a common module. Reply by pmontazemi on 19 August 2020, 10:45 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: NVDataMgmt.c Revision Comment by qnguyen on 11 August 2020, 10:11 https://devapps.diality.us/cru/DG-DEN-4217-1#c3043 status is the same as calRecordIsValid. Suggest removing the if statement and assign calRecordIsValid to status. Reply by Sean Nash on 11 August 2020, 10:42 > Done. Reply by qnguyen on 17 August 2020, 15:06 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 13:50 https://devapps.diality.us/cru/DG-DEN-4217-1#c3237 Figure out where this should go. Reply by Dara Navaei on 13 August 2020, 09:15 > Added to my list of TODOs Reply by pmontazemi on 17 August 2020, 15:21 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 17 August 2020, 15:20 https://devapps.diality.us/cru/DG-DEN-4217-1#c3531 No space between function name and "(" Reply by Sean Nash on 17 August 2020, 15:44 > Done. Reply by pmontazemi on 17 August 2020, 15:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 13:50 https://devapps.diality.us/cru/DG-DEN-4217-1#c3238 One liner. Reply by Sean Nash on 12 August 2020, 15:24 > Done Reply by pmontazemi on 17 August 2020, 15:20 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 12 August 2020, 13:51 https://devapps.diality.us/cru/DG-DEN-4217-1#c3240 Missing the Doxygen /**@}*/ at eof. Reply by Sean Nash on 12 August 2020, 15:23 > This module not yet converted to new Doxygen format. Reply by pmontazemi on 17 August 2020, 15:19 > Planned to be taken care of in DEN S26. Reply by pmontazemi on 17 August 2020, 15:42 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: NVDataMgmt.h Revision Comment by pmontazemi on 12 August 2020, 13:51 https://devapps.diality.us/cru/DG-DEN-4217-1#c3241 Missing the Doxygen /**@}*/ at eof. Reply by Sean Nash on 12 August 2020, 15:23 > This module not yet converted to new Doxygen format. Reply by pmontazemi on 17 August 2020, 15:19 > Planned to be taken care of in DEN S26. Reply by pmontazemi on 17 August 2020, 15:19 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: AlarmDefs.h Revision Comment by qnguyen on 12 August 2020, 16:48 https://devapps.diality.us/cru/DG-DEN-4217-1#c3365 Remove period at the end to make it consistent with the rest. Reply by Sean Nash on 13 August 2020, 17:59 > Done. Reply by qnguyen on 17 August 2020, 15:02 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-4217-1 https://devapps.diality.us/cru/DG-DEN-4217-1 Title: DG-DEN-4217_Level and Shock Detection Statement of Objectives: State: Closed Summary: Author: Sean Nash Reviewers: (2 active, 1 completed*) Dara Navaei (*) qnguyen pmontazemi