This is a list of all comments for DG-DEN-5980-1. Review Summary: No summary ---------------------------------------- File: firmware/.launches/DG.launch Revision Comment by Sean Nash on 22 February 2021, 09:25 https://devapps.diality.us/cru/DG-DEN-5980-1#c8208 Why did this file change? Reply by Dara Navaei on 21 March 2021, 17:09 > Because I used a different debugger and it updated the launch > file. I think we should add this .gitignore. Reply by Sean Nash on 23 March 2021, 11:04 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DrainPump.c Revision Comment by Sean Nash on 22 February 2021, 09:26 https://devapps.diality.us/cru/DG-DEN-5980-1#c8209 Why commented out? Reply by Dara Navaei on 24 March 2021, 12:36 > This section is monitoring the RPM of the drain pump and > makes decision accordingly. This must be commented out until > we figure out how to convert counts into RPM properly. I > added a TODO note for that. Reply by Sean Nash on 29 March 2021, 10:48 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ROPump.c Revision Comment by Sean Nash on 22 February 2021, 09:27 https://devapps.diality.us/cru/DG-DEN-5980-1#c8210 Why is this commented out? Reply by Dara Navaei on 21 March 2021, 17:08 > Uncommented the code. Reply by Sean Nash on 23 March 2021, 11:04 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemComm.c Revision Comment by qnguyen on 27 March 2021, 18:13 https://devapps.diality.us/cru/DG-DEN-5980-1#c8827 Add break. Reply by Dara Navaei on 28 March 2021, 10:23 > Done. Reply by qnguyen on 29 March 2021, 10:38 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Sean Nash on 22 February 2021, 09:40 https://devapps.diality.us/cru/DG-DEN-5980-1#c8216 Maybe these 3 U32s can be passed as a structure so you don't have to do so many memcpys below? Reply by Dara Navaei on 29 March 2021, 10:49 > It will be addressed in the future stories. Reply by Sean Nash on 29 March 2021, 10:51 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 February 2021, 09:41 https://devapps.diality.us/cru/DG-DEN-5980-1#c8217 See comment above on send functions. Consider packaging the 3 U32s in a structure to reduce memcpys. Reply by Dara Navaei on 29 March 2021, 10:53 > It will be addressed in the future stories. Reply by Sean Nash on 29 March 2021, 10:54 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Tasks/TaskGeneral.c Revision Comment by qnguyen on 23 March 2021, 10:55 https://devapps.diality.us/cru/DG-DEN-5980-1#c8601 Update to new build switch. Reply by Dara Navaei on 27 March 2021, 12:06 > Done. Reply by qnguyen on 29 March 2021, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 February 2021, 09:36 https://devapps.diality.us/cru/DG-DEN-5980-1#c8215 Why are all of these commented out? Reply by Dara Navaei on 21 March 2021, 16:31 > To be able to run them on a simple board on my desk. I made a > build switch for this. I used one of the older build switches > instead of commenting out. Reply by Sean Nash on 23 March 2021, 11:08 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 March 2021, 10:59 https://devapps.diality.us/cru/DG-DEN-5980-1#c8604 Not just for calibration record. Reply by Dara Navaei on 24 March 2021, 12:18 > I updated the comment. Reply by Sean Nash on 29 March 2021, 10:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2021, 23:20 https://devapps.diality.us/cru/DG-DEN-5980-1#c8568 Are you sure you want this run here after modes? Reply by Dara Navaei on 24 March 2021, 12:24 > Yes. This state machine is only used when a record data is > scheduled to be sent to Dialin in batches. Reply by Sean Nash on 29 March 2021, 10:41 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Tasks/TaskPriority.c Revision Comment by qnguyen on 23 March 2021, 10:55 https://devapps.diality.us/cru/DG-DEN-5980-1#c8600 Update to new build switch. Reply by Dara Navaei on 24 March 2021, 12:20 > Done. Reply by qnguyen on 29 March 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 February 2021, 09:34 https://devapps.diality.us/cru/DG-DEN-5980-1#c8214 Why are all of these commented out? Reply by Dara Navaei on 21 March 2021, 16:38 > To be able to run it on a board with not hardware. I used one > of the older build switched instead. Reply by Sean Nash on 23 March 2021, 11:08 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 23 March 2021, 10:55 https://devapps.diality.us/cru/DG-DEN-5980-1#c8602 Uncomment code. Reply by Dara Navaei on 24 March 2021, 12:19 > Done. Reply by qnguyen on 29 March 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: NVDataMgmt.c Revision Comment by Sean Nash on 22 February 2021, 09:52 https://devapps.diality.us/cru/DG-DEN-5980-1#c8220 Do these structures require packing? Separate those that do from those that do not and pack only those that do. Reply by Dara Navaei on 21 March 2021, 17:16 > As a common practice, I use #pragma around the structs. Does > that matter? Reply by Sean Nash on 23 March 2021, 10:11 > The compiler generates extra code when we pack structures > so I'd prefer not to unless needed. Generally, we only > need packing if structure contains 8 or 16 bit data fields > and we want to serialize the structure (i.e. for > transmission). Reply by Sean Nash on 29 March 2021, 10:52 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 23 March 2021, 10:59 https://devapps.diality.us/cru/DG-DEN-5980-1#c8603 Add "TRUE == " to the comparison. Reply by Dara Navaei on 24 March 2021, 12:18 > Done. Reply by qnguyen on 29 March 2021, 10:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 23 March 2021, 11:00 https://devapps.diality.us/cru/DG-DEN-5980-1#c8605 Add () between each condition. Reply by Dara Navaei on 24 March 2021, 12:15 > Done. Reply by qnguyen on 29 March 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 05 April 2021, 17:14 https://devapps.diality.us/cru/DG-DEN-5980-1#c9009 Recommend adding TRUE to the comparison and parenthesis around each condition. Reply by Dara Navaei on 06 April 2021, 10:10 > Done. Reply by qnguyen on 06 April 2021, 10:28 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 23 March 2021, 11:05 https://devapps.diality.us/cru/DG-DEN-5980-1#c8610 Update function description. Reply by Dara Navaei on 24 March 2021, 12:14 > Done. Reply by qnguyen on 29 March 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 23 March 2021, 11:06 https://devapps.diality.us/cru/DG-DEN-5980-1#c8612 Why do we need to disable interrupt? Is it a critical function? Reply by Dara Navaei on 24 March 2021, 12:08 > To make sure dequeuing is not interrupted. Reply by Sean Nash on 25 March 2021, 10:42 > Yes, jobs are added to queue from General Task and removed > from queue from Background Task. Need queue operations to > be protected. Reply by qnguyen on 29 March 2021, 10:34 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/source/sys_main.c Revision Comment by Sean Nash on 22 February 2021, 09:33 https://devapps.diality.us/cru/DG-DEN-5980-1#c8213 Position may be ok. Check dependencies though. Reply by Dara Navaei on 21 March 2021, 16:39 > Dependencies are fine. It enables Bank7. Reply by Sean Nash on 22 March 2021, 23:24 > What I meant was, do any of the prior init functions above > need nv-data (and bank 7) to be initialized before they are > run? Reply by Dara Navaei on 24 March 2021, 12:29 > I moved the function to above and before the actuators. Reply by Sean Nash on 29 March 2021, 10:47 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: DGDefs.h Revision Comment by Sean Nash on 06 April 2021, 09:38 https://devapps.diality.us/cru/DG-DEN-5980-1#c9025 Any reason why these are in common header? Does anybody else use them? If not, make private to HeatDisinfect module. Reply by Dara Navaei on 06 April 2021, 10:08 > Yes, they are going to be used in mode flush and chemical > disinfect as well as heat disinfect. Reply by Sean Nash on 06 April 2021, 10:30 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: MsgDefs.h Revision Comment by Sean Nash on 22 February 2021, 09:24 https://devapps.diality.us/cru/DG-DEN-5980-1#c8207 We have a lot of available message IDs above. Why are we using them first before creating new msg IDs? Reply by Dara Navaei on 21 March 2021, 17:10 > I will use the older message IDs for other programs. Reply by Sean Nash on 23 March 2021, 11:03 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: NVDataMgmtDGRecords.h Revision Comment by Sean Nash on 23 March 2021, 10:22 https://devapps.diality.us/cru/DG-DEN-5980-1#c8572 Are these string fields null-terminated? If so, we need an extra character for the null terminator. Reply by Dara Navaei on 24 March 2021, 12:21 > These string fields are not null terminated. We will used > them as characters and they are not used anywhere in the > firmware. Reply by Sean Nash on 29 March 2021, 10:43 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: RTC.c Revision Comment by Sean Nash on 22 February 2021, 09:48 https://devapps.diality.us/cru/DG-DEN-5980-1#c8219 Why is this commented out? Reply by Dara Navaei on 21 March 2021, 16:23 > It was commented out for testing. I un-commented it. Reply by Sean Nash on 29 March 2021, 10:46 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeInitPOST.c Revision Comment by Sean Nash on 22 February 2021, 09:29 https://devapps.diality.us/cru/DG-DEN-5980-1#c8211 Rather than changing this, maybe you could just not define SKIP_POST? The SKIP_POST build flag should skip all POST. Reply by Dara Navaei on 21 March 2021, 17:07 > In this test, I was NVDatMgmt to be tested but not anything > else. Reply by Sean Nash on 22 March 2021, 23:18 > Create a build switch for this then. Reply by Sean Nash on 23 March 2021, 11:02 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 27 March 2021, 18:17 https://devapps.diality.us/cru/DG-DEN-5980-1#c8829 if we skip POST, do we want to execute RTC POST test and other POST tests? Reply by Dara Navaei on 28 March 2021, 10:32 > That is right. I added another build switch to only run POST > on RTC and NVDataMgmt only. Reply by qnguyen on 29 March 2021, 10:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 February 2021, 09:32 https://devapps.diality.us/cru/DG-DEN-5980-1#c8212 These cases should be in same order as enum. POST tests for drivers/controllers/monitors will need NV Data Mgmt to have already read and validated NV Data so they can get/apply calibration. So should this POST be executed earlier? Reply by Dara Navaei on 21 March 2021, 16:44 > That is true. I changed its place to right after RTC. Reply by Sean Nash on 23 March 2021, 11:06 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: NVDataMgmtHDRecords.h Revision Comment by Sean Nash on 23 March 2021, 10:20 https://devapps.diality.us/cru/DG-DEN-5980-1#c8571 Do we want these header files to have a separate doxygen group name? I think we should have all of the NV data files (.h and .c) sharing the same group. Reply by Dara Navaei on 24 March 2021, 12:23 > Changed them to NVDataMgmt NVDataMgmt. Reply by Sean Nash on 29 March 2021, 10:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 March 2021, 10:25 https://devapps.diality.us/cru/DG-DEN-5980-1#c8573 I'm not sure an enum is what we want for mfg location. We don't want to have to make a new build every time we a new location is added. I think location should probably be a 3 or 4 character alpha-numeric location code. Reply by Dara Navaei on 28 March 2021, 10:34 > The enum was in the document that we received from the > systems team. We can discuss with them and change it if > needed. Reply by Sean Nash on 29 March 2021, 10:48 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 March 2021, 10:27 https://devapps.diality.us/cru/DG-DEN-5980-1#c8574 Same comment as for mfg location. Reply by Dara Navaei on 28 March 2021, 10:34 > The enum was in the document that we received from the > systems team. We can discuss with them and change it if > needed. Reply by Sean Nash on 29 March 2021, 10:48 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 March 2021, 10:29 https://devapps.diality.us/cru/DG-DEN-5980-1#c8575 We have a very similar enum in valves.h. Why can't we use that enum? Reply by Dara Navaei on 24 March 2021, 12:21 > I wanted to use enum specifically for calibration purposes. > Their names also contain the word CAL. Reply by Sean Nash on 29 March 2021, 10:41 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: Accel.c Revision Comment by Sean Nash on 22 February 2021, 09:47 https://devapps.diality.us/cru/DG-DEN-5980-1#c8218 You can remove the else - no longer needed. Reply by Dara Navaei on 21 March 2021, 16:26 > Done. Reply by Sean Nash on 29 March 2021, 10:46 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/AlarmMgmt.c Revision Comment by qnguyen on 27 March 2021, 18:14 https://devapps.diality.us/cru/DG-DEN-5980-1#c8828 Revert line deletion. Reply by Dara Navaei on 28 March 2021, 10:24 > Done. Reply by qnguyen on 29 March 2021, 10:40 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-5980-1 https://devapps.diality.us/cru/DG-DEN-5980-1 Title: DG-DEN-5980_Non Volatile Data Management (2 of 2) Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (0 active, 3 completed*) qnguyen (*) Sean Nash (*) pmontazemi (*)