This is a list of all comments for DG-RESTART-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Services/SystemComm.c Revision Comment by pmontazemi on 27 February 2020, 10:06 https://devapps.diality.us/cru/DG-RESTART-1#c1288 Align numbers. Reply by Sean Nash on 28 February 2020, 14:35 > Done. Reply by pmontazemi on 28 February 2020, 14:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 24 February 2020, 14:01 https://devapps.diality.us/cru/DG-RESTART-1#c1201 Space? Reply by Sean Nash on 26 February 2020, 10:26 > Done. Reply by pmontazemi on 26 February 2020, 10:29 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Sean Nash on 24 February 2020, 17:22 https://devapps.diality.us/cru/DG-RESTART-1#c1218 Why 2 structures that appear to hold the same 4 load cell values? Reply by snejatali on 25 February 2020, 14:10 > I had not noticed existing one and made a new one. Removed > one of them accordingly. Reply by Sean Nash on 26 February 2020, 10:18 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 27 February 2020, 10:08 https://devapps.diality.us/cru/DG-RESTART-1#c1289 Remove extra line at EOF. Reply by Sean Nash on 28 February 2020, 14:37 > Done. Reply by pmontazemi on 28 February 2020, 14:38 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Timers.c Revision Comment by pmontazemi on 24 February 2020, 14:03 https://devapps.diality.us/cru/DG-RESTART-1#c1202 Why removed? Reply by pmontazemi on 26 February 2020, 10:27 > Because code got moved to common. Reply by pmontazemi on 26 February 2020, 10:27 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Timers.h Revision Comment by pmontazemi on 24 February 2020, 14:03 https://devapps.diality.us/cru/DG-RESTART-1#c1203 Why removed? Reply by pmontazemi on 26 February 2020, 10:26 > Because code got moved to common. Reply by pmontazemi on 26 February 2020, 10:27 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Utilities.c Revision Comment by pmontazemi on 24 February 2020, 14:03 https://devapps.diality.us/cru/DG-RESTART-1#c1204 Why removed? Reply by pmontazemi on 26 February 2020, 10:26 > Because code got moved to common. Reply by pmontazemi on 26 February 2020, 10:26 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Utilities.h Revision Comment by pmontazemi on 24 February 2020, 14:03 https://devapps.diality.us/cru/DG-RESTART-1#c1205 Why removed? Reply by pmontazemi on 26 February 2020, 10:25 > Because code got moved to common. Reply by pmontazemi on 26 February 2020, 10:25 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/irqDispatch_a.asm Revision Comment by pmontazemi on 24 February 2020, 14:09 https://devapps.diality.us/cru/DG-RESTART-1#c1210 Why removed? Reply by pmontazemi on 26 February 2020, 10:22 > It's because code got moved to common Reply by pmontazemi on 26 February 2020, 10:22 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/irqDispatch_c.c Revision Comment by pmontazemi on 24 February 2020, 14:09 https://devapps.diality.us/cru/DG-RESTART-1#c1209 Why removed? Reply by pmontazemi on 26 February 2020, 10:23 > Because code got moved to common. Reply by pmontazemi on 26 February 2020, 10:23 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: .gitignore Revision Comment by pmontazemi on 24 February 2020, 14:10 https://devapps.diality.us/cru/DG-RESTART-1#c1211 Why removed? Reply by Dara Navaei on 26 February 2020, 11:09 > This file was in the wrong directory. All the corresponding > extensions that need to be ignored, are added to the > .gitignore of the firmware or VectorCAST. Reply by pmontazemi on 27 February 2020, 08:06 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: .project Revision Comment by pmontazemi on 24 February 2020, 14:10 https://devapps.diality.us/cru/DG-RESTART-1#c1212 Why removed? Reply by Dara Navaei on 26 February 2020, 11:09 > This file was in the wrong directory. Reply by pmontazemi on 27 February 2020, 08:06 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Common.h Revision Comment by pmontazemi on 24 February 2020, 14:04 https://devapps.diality.us/cru/DG-RESTART-1#c1207 Why removed? Reply by pmontazemi on 26 February 2020, 10:24 > Because code got moved to common. Reply by pmontazemi on 26 February 2020, 10:24 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Drivers/Comm.c Revision Comment by pmontazemi on 24 February 2020, 13:40 https://devapps.diality.us/cru/DG-RESTART-1#c1191 We need to get rid of UART. It is planned for DEN S15. Reply by pmontazemi on 26 February 2020, 10:30 > Will do so in DEN S15. Reply by pmontazemi on 26 February 2020, 10:30 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 24 February 2020, 13:41 https://devapps.diality.us/cru/DG-RESTART-1#c1192 Must get rid of UART, it is planned for DEN S15. Reply by pmontazemi on 26 February 2020, 10:18 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 24 February 2020, 13:41 https://devapps.diality.us/cru/DG-RESTART-1#c1193 If related to UART, must be removed. Reply by pmontazemi on 26 February 2020, 10:21 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeInitPOST.c Revision Comment by pmontazemi on 24 February 2020, 13:42 https://devapps.diality.us/cru/DG-RESTART-1#c1194 What is this comment for? Reply by Sean Nash on 26 February 2020, 10:00 > Looks like POSTs were being skipped. Fixed. Reply by pmontazemi on 26 February 2020, 10:30 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModePostTreat.c Revision Comment by pmontazemi on 24 February 2020, 13:43 https://devapps.diality.us/cru/DG-RESTART-1#c1195 What are we doing in transitionToPostTreatmentMode? Reply by pmontazemi on 26 February 2020, 10:15 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeStandby.c Revision Comment by pmontazemi on 24 February 2020, 13:43 https://devapps.diality.us/cru/DG-RESTART-1#c1196 What are we doing in transitionToStandbyMode? Reply by Sean Nash on 24 February 2020, 17:14 > All of these modes act as a state machine. They each have a > transition function where you can take any actions that you > want to happen when transitioning into the mode from some > other mode. Reply by pmontazemi on 26 February 2020, 10:14 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/CommBuffers.c Revision Comment by pmontazemi on 24 February 2020, 13:46 https://devapps.diality.us/cru/DG-RESTART-1#c1197 What are these comments for? Reply by Sean Nash on 25 February 2020, 16:26 > I was checking to see if interrupt protection helped with any > of the CAN issues. They are not needed. I'll remove. Reply by Sean Nash on 26 February 2020, 10:26 > Done. Reply by pmontazemi on 26 February 2020, 10:29 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 24 February 2020, 13:47 https://devapps.diality.us/cru/DG-RESTART-1#c1198 Remove extra line at EOF. Reply by Sean Nash on 26 February 2020, 10:01 > Done. Reply by pmontazemi on 26 February 2020, 10:11 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/FPGA.c Revision Comment by Sean Nash on 24 February 2020, 17:29 https://devapps.diality.us/cru/DG-RESTART-1#c1219 Any reason why you're keeping the HD structure? Reply by snejatali on 25 February 2020, 14:10 > It should be removed. Kept it only for the moment as > reference to help with DG FPGA register assignment. Reply by Sean Nash on 26 February 2020, 10:18 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Interrupts.c Revision Comment by pmontazemi on 24 February 2020, 13:49 https://devapps.diality.us/cru/DG-RESTART-1#c1199 Consolidate all includes. Reply by Sean Nash on 26 February 2020, 09:58 > Done. Reply by pmontazemi on 26 February 2020, 10:10 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 24 February 2020, 13:50 https://devapps.diality.us/cru/DG-RESTART-1#c1200 Remove all UART-related debugging ports (but the one connected to FPGA). This is planned to be done in DEN S15. Reply by pmontazemi on 26 February 2020, 10:17 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/WatchdogMgmt.c Revision Comment by pmontazemi on 24 February 2020, 14:04 https://devapps.diality.us/cru/DG-RESTART-1#c1206 Remove 20+ extra lines. Reply by Sean Nash on 26 February 2020, 10:08 > Done. Reply by pmontazemi on 26 February 2020, 10:19 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/TestSupport.h Revision Comment by pmontazemi on 24 February 2020, 14:05 https://devapps.diality.us/cru/DG-RESTART-1#c1208 Why removed? Reply by pmontazemi on 26 February 2020, 10:23 > Because code got moved to common Reply by pmontazemi on 26 February 2020, 10:24 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/SensorProcess.c Revision Comment by Sean Nash on 24 February 2020, 16:46 https://devapps.diality.us/cru/DG-RESTART-1#c1216 Is this module intended to process all sensor data or just load cells? If just load cells, this module seems too broadly named. If all sensor data, are you sure you want to process all sensor data in one module? Reply by snejatali on 25 February 2020, 14:06 > Now I am calling execLoadCell from within execSensorData. New > functions can be later added to execSensorData(). Reply by Sean Nash on 26 February 2020, 10:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 25 February 2020, 09:14 https://devapps.diality.us/cru/DG-RESTART-1#c1221 This doxygen comment (group end) looks out of place - should be at the bottom of the module after all code. What should be up here is a group start. Check HD code for examples. Reply by snejatali on 25 February 2020, 15:18 > Removed for now. Reply by Sean Nash on 26 February 2020, 10:17 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 25 February 2020, 09:10 https://devapps.diality.us/cru/DG-RESTART-1#c1220 The scope on these variables is too narrow. You will want to make latest load cell values available to other modules via a "get" function and you'll need to move these up to module level to do that. Also, generally speaking, it is difficult to unit test functions that have static variables declared in them so I would not recommend doing this. Reply by snejatali on 25 February 2020, 14:24 > Good idea. > Make changes accordingly. Please review again. Reply by Sean Nash on 26 February 2020, 10:17 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 24 February 2020, 16:38 https://devapps.diality.us/cru/DG-RESTART-1#c1213 The LOAD_CELL_REPORT_PERIOD is 10 and this function gets called every 10ms - so this function produces and broadcasts a new set of averaged load cell weights every 100ms based on last 10 samples. This is what we talked about, but it took me a while to convince myself ot that. And if the priority task timing changes it will all fall apart. There is a TASK_PRIORITY_INTERVAL defined in TaskPriority.h as 10 (10ms interval) - if the task timing changes, this definition would also be changed. I would define LOAD_CELL_REPORT_PERIOD as ( 100 / TASK_PRIORITY_INTERVAL ) to couple it with task timing and clarify the desired report period is 100ms. I would also make a separate #define LOAD_CELL_SAMPLES_TO_AVERAGE and define it to be same as LOAD_CELL_REPORT_PERIOD just to clarify its other purpose (used in SensorProcess.h where it's folded into conversion factor). Reply by snejatali on 25 February 2020, 14:02 > Implemented suggestions. Please review again. Reply by Sean Nash on 26 February 2020, 10:21 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/SensorProcess.h Revision Comment by Sean Nash on 24 February 2020, 16:42 https://devapps.diality.us/cru/DG-RESTART-1#c1214 Where is prototype for execSensorProcess()? Reply by snejatali on 25 February 2020, 14:07 > Added prototype. Reply by Sean Nash on 26 February 2020, 10:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 24 February 2020, 16:43 https://devapps.diality.us/cru/DG-RESTART-1#c1215 You will likely also need a function for other DG modules to get the current load cell weights. Some DG features like disinfection will probably require load cell feedback. Reply by snejatali on 26 February 2020, 10:22 > Done. Reply by Sean Nash on 26 February 2020, 10:23 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/LoadCell.c Revision Comment by pmontazemi on 28 February 2020, 14:42 https://devapps.diality.us/cru/DG-RESTART-1#c1298 Comments should go. Reply by snejatali on 28 February 2020, 14:47 > Done. Reply by pmontazemi on 28 February 2020, 14:48 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by pmontazemi on 28 February 2020, 14:45 https://devapps.diality.us/cru/DG-RESTART-1#c1299 Remove extra lines. Reply by snejatali on 28 February 2020, 14:46 > Done. Reply by pmontazemi on 28 February 2020, 14:47 > RESOLVED IN CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/DGCommon.h Revision Comment by pmontazemi on 27 February 2020, 10:09 https://devapps.diality.us/cru/DG-RESTART-1#c1290 What is this blank section for? Reply by Sean Nash on 28 February 2020, 14:32 > Placeholder for future unit test accomodations. Reply by pmontazemi on 28 February 2020, 14:32 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-RESTART-1 https://devapps.diality.us/cru/DG-RESTART-1 Title: DG-RESTART Statement of Objectives: State: Closed Summary: Author: snejatali Moderator: snejatali Reviewers: (2 active, 2 completed*) Sean Nash (*) pmontazemi (*) Dara Navaei Behrouz NematiPour