This is a list of all comments for DG-DEN-3922-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/ConductivitySensors.c Revision Comment by Sean Nash on 21 July 2020, 09:32 https://devapps.diality.us/cru/DG-DEN-3922-1#c2851 Why TBD? Reply by qnguyen on 21 July 2020, 09:36 > The SYS team says they will need more data and testing to > determine the value. This value I use is more like a hardware > value. Reply by qnguyen on 21 July 2020, 10:37 > Agreed! that should have a description of the constant value. Reply by qnguyen on 21 July 2020, 12:30 > Done Reply by Sean Nash on 22 July 2020, 10:52 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 July 2020, 09:32 https://devapps.diality.us/cru/DG-DEN-3922-1#c2852 Add ///< comment for Doxygen. Reply by qnguyen on 21 July 2020, 12:30 > Done Reply by Sean Nash on 22 July 2020, 10:52 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 July 2020, 09:33 https://devapps.diality.us/cru/DG-DEN-3922-1#c2853 Add ///< comments for Doxygen. Reply by qnguyen on 21 July 2020, 12:30 > Done Reply by Sean Nash on 22 July 2020, 10:52 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 July 2020, 09:54 https://devapps.diality.us/cru/DG-DEN-3922-1#c2856 I would add that conductivity sensor data is updated with latest readings. Reply by qnguyen on 22 July 2020, 14:27 > Done Reply by Sean Nash on 23 July 2020, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 21 July 2020, 11:27 https://devapps.diality.us/cru/DG-DEN-3922-1#c2864 Space between 0 and ) per C Coding Standard. Reply by qnguyen on 21 July 2020, 12:32 > Done Reply by pmontazemi on 22 July 2020, 10:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 July 2020, 09:53 https://devapps.diality.us/cru/DG-DEN-3922-1#c2855 Missing conductivity param. Should we even have a conductivity parameter for this function? Why should caller have to provide this to the module that owns it? Reply by Sean Nash on 22 July 2020, 10:51 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 21 July 2020, 11:27 https://devapps.diality.us/cru/DG-DEN-3922-1#c2865 Remove extra line. Reply by pmontazemi on 22 July 2020, 10:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 July 2020, 09:56 https://devapps.diality.us/cru/DG-DEN-3922-1#c2857 This function seems more like a "calc" function than a "get" function. Consider renaming. Reply by Sean Nash on 22 July 2020, 10:50 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 July 2020, 09:58 https://devapps.diality.us/cru/DG-DEN-3922-1#c2858 Give each param a separate line and describe it. Reply by Sean Nash on 22 July 2020, 10:50 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 July 2020, 10:03 https://devapps.diality.us/cru/DG-DEN-3922-1#c2860 "{" goes on separate line. Also, this condition seems like a complicated way of saying if ( readCount[ sensorId ] != fpgaReadCount ). Reply by Sean Nash on 22 July 2020, 10:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 July 2020, 10:09 https://devapps.diality.us/cru/DG-DEN-3922-1#c2861 Should this be a conductivity sensor fault? Reply by Sean Nash on 22 July 2020, 10:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 July 2020, 10:00 https://devapps.diality.us/cru/DG-DEN-3922-1#c2859 I think we may want to be a little more forgiving on this. Maybe allow for n errors within some window of time before triggering a fault. I added some functions to support this kind of thing in Utilities module. Also - should this be a conductivity sensor fault? Need to somehow differentiate from the other fault above so when we see it in a log file we know which occurred. Reply by Sean Nash on 22 July 2020, 10:47 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ConductivitySensors.h Revision Comment by Sean Nash on 21 July 2020, 09:29 https://devapps.diality.us/cru/DG-DEN-3922-1#c2850 More specific comment here. This will be picked up by Doxygen. Describe this enumeration. Reply by qnguyen on 21 July 2020, 12:30 > Done Reply by Sean Nash on 22 July 2020, 10:53 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeFill.c Revision Comment by pmontazemi on 21 July 2020, 11:31 https://devapps.diality.us/cru/DG-DEN-3922-1#c2866 Remove extra line. Reply by qnguyen on 21 July 2020, 12:28 > Done Reply by pmontazemi on 22 July 2020, 10:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 21 July 2020, 11:31 https://devapps.diality.us/cru/DG-DEN-3922-1#c2867 Remove extra line. Reply by qnguyen on 21 July 2020, 12:28 > Done Reply by pmontazemi on 22 July 2020, 10:41 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeRecirculate.c Revision Comment by pmontazemi on 21 July 2020, 11:32 https://devapps.diality.us/cru/DG-DEN-3922-1#c2868 Align comment. Reply by qnguyen on 21 July 2020, 12:28 > Done Reply by pmontazemi on 22 July 2020, 10:40 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by pmontazemi on 21 July 2020, 12:53 https://devapps.diality.us/cru/DG-DEN-3922-1#c2879 If adding cd1/cd2 here, then if needs to be done through the entire code. I strongly recommend adding them when doing their related Story. Reply by pmontazemi on 22 July 2020, 10:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 21 July 2020, 12:53 https://devapps.diality.us/cru/DG-DEN-3922-1#c2880 Same comment regarding cd1/cd2, here. Reply by pmontazemi on 22 July 2020, 10:39 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/FPGA.c Revision Comment by pmontazemi on 21 July 2020, 11:34 https://devapps.diality.us/cru/DG-DEN-3922-1#c2869 Why was the FPGA bulk write start address changed? Reply by qnguyen on 21 July 2020, 11:56 > The updated FPGA added fpgaRevLab and fpgaRevMajor, which > pushed the address offset up by 2. Reply by pmontazemi on 22 July 2020, 10:40 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemComm.c Revision Comment by Dara Navaei on 22 July 2020, 17:46 https://devapps.diality.us/cru/DG-DEN-3922-1#c2927 Why there is no break at the end of the case? Reply by qnguyen on 23 July 2020, 09:33 > Probably a merge error. Added back. Reply by Dara Navaei on 23 July 2020, 10:37 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Services/SystemCommMessages.h Revision Comment by pmontazemi on 21 July 2020, 12:56 https://devapps.diality.us/cru/DG-DEN-3922-1#c2881 1. I would call them consistently, cdi, cdo, inlet outlet is too confusing because there is also pressure and temperature;2 2. I would also focus only on cdi cdo and remove cd1/cd2, which have their own Stories. Reply by qnguyen on 21 July 2020, 13:04 > Let's call them cpi and cpo? This will match with the DG HDD. Reply by pmontazemi on 22 July 2020, 10:37 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: MsgDefs.h Revision Comment by pmontazemi on 23 July 2020, 08:29 https://devapps.diality.us/cru/DG-DEN-3922-1#c2928 We either finish all comments everywhere with dots or we don't, but we need to be consistent. Reply by qnguyen on 23 July 2020, 11:17 > Done Reply by pmontazemi on 24 July 2020, 10:32 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: NVDataMgmt.c Revision Comment by pmontazemi on 21 July 2020, 13:09 https://devapps.diality.us/cru/DG-DEN-3922-1#c2883 What is our C Coding Standard saying for the maximum width of a code line? Do we need to move things to the next line? Reply by pmontazemi on 22 July 2020, 10:35 > Update C Coding Standard to add max. width. Reply by pmontazemi on 22 July 2020, 10:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: TestSupport.h Revision Comment by pmontazemi on 21 July 2020, 13:12 https://devapps.diality.us/cru/DG-DEN-3922-1#c2884 What about DG_SOFTWARE_FAULT? Where is that being handled? Reply by Sean Nash on 21 July 2020, 13:28 > Used build switch to handle DG vs. HD. Reply by pmontazemi on 22 July 2020, 10:33 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: Utilities.h Revision Comment by Sean Nash on 22 July 2020, 10:40 https://devapps.diality.us/cru/DG-DEN-3922-1#c2897 Why was this vectorcast area needed? Reply by qnguyen on 22 July 2020, 11:21 > Removed after adding appropriate build flag to vectorcast. Reply by Sean Nash on 23 July 2020, 10:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DrainPump.c Revision Comment by pmontazemi on 22 July 2020, 10:51 https://devapps.diality.us/cru/DG-DEN-3922-1#c2908 Remove extra line. Reply by qnguyen on 22 July 2020, 14:25 > Done Reply by pmontazemi on 23 July 2020, 10:31 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/AlarmMgmt.c Revision Comment by pmontazemi on 22 July 2020, 11:01 https://devapps.diality.us/cru/DG-DEN-3922-1#c2914 Remove extra lines. Reply by qnguyen on 22 July 2020, 14:24 > Done Reply by pmontazemi on 23 July 2020, 10:32 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/MsgQueues.c Revision Comment by Dara Navaei on 22 July 2020, 16:50 https://devapps.diality.us/cru/DG-DEN-3922-1#c2923 Why the private data and private definitions do not have the doxygen comments? Reply by qnguyen on 22 July 2020, 17:32 > Added doxygen comments Reply by Dara Navaei on 23 July 2020, 10:38 > RESOLVED IN CODE WALKTHROUGH Revision Comment by pmontazemi on 22 July 2020, 11:01 https://devapps.diality.us/cru/DG-DEN-3922-1#c2915 Remove extra lines. Reply by qnguyen on 22 July 2020, 14:24 > Done Reply by pmontazemi on 23 July 2020, 10:32 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/PIControllers.c Revision Comment by Dara Navaei on 22 July 2020, 16:52 https://devapps.diality.us/cru/DG-DEN-3922-1#c2924 Please add the doxygen comment Reply by qnguyen on 22 July 2020, 17:29 > /// is also picked up by doxygen as well. However, changed to > ///< as prefer. Reply by Dara Navaei on 23 July 2020, 10:38 > RESOLVED IN CODE WALKTHROUGH --- ID: DG-DEN-3922-1 https://devapps.diality.us/cru/DG-DEN-3922-1 Title: DG-DEN-3922_DG Water Inlet Quality Statement of Objectives: State: Closed Summary: Author: qnguyen Moderator: qnguyen Reviewers: (0 active, 3 completed*) Sean Nash (*) Dara Navaei (*) pmontazemi (*)