This is a list of all comments for DD-LEAH-210-5. Review Summary: No summary ---------------------------------------- File: firmware/App/Services/AlarmMgmtDD.h Revision Comment by Sean Nash on 28 August 2024, 10:29 https://devapps.diality.us/cru/DD-LEAH-210-5#c19946 Remove extra blank line. Reply by Vinayakam Mani on 30 August 2024, 12:19 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:29 https://devapps.diality.us/cru/DD-LEAH-210-5#c19947 Remove this. Reply by Vinayakam Mani on 30 August 2024, 12:19 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:31 https://devapps.diality.us/cru/DD-LEAH-210-5#c19948 Remove extra blank line. Reply by Vinayakam Mani on 30 August 2024, 12:19 > Done. ---------------------------------------- File: firmware/App/DDCommon.h Revision Comment by Sean Nash on 29 August 2024, 14:53 https://devapps.diality.us/cru/DD-LEAH-210-5#c20038 Should be Vinay Reply by Vinayakam Mani on 03 September 2024, 12:27 > Done. ---------------------------------------- File: firmware/App/Drivers/InternalADC.c Revision Comment by Sean Nash on 27 August 2024, 13:35 https://devapps.diality.us/cru/DD-LEAH-210-5#c19885 Needs a /// comment above. Reply by Vinayakam Mani on 29 August 2024, 13:48 > Done. ---------------------------------------- File: firmware/App/Drivers/InternalADC.h Revision Comment by Sean Nash on 27 August 2024, 13:34 https://devapps.diality.us/cru/DD-LEAH-210-5#c19884 Driver. Please describe a little more. Reply by Vinayakam Mani on 29 August 2024, 13:48 > Done. ---------------------------------------- File: firmware/App/Drivers/PAL.c Revision Comment by Sean Nash on 27 August 2024, 13:38 https://devapps.diality.us/cru/DD-LEAH-210-5#c19886 These DD units should have your name on them. Reply by Vinayakam Mani on 29 August 2024, 13:49 > Done. ---------------------------------------- File: firmware/App/Drivers/SafetyShutdown.h Revision Comment by Sean Nash on 27 August 2024, 13:40 https://devapps.diality.us/cru/DD-LEAH-210-5#c19887 In TD, I removed this unit and merged safety shutdown content into GPIO and CPLD Interface (would be FPGA Interface for DD and RO) units. I don't see that you have a GPIO unit, but you should I think. It would have watchdog and safety shutdown pin defs, macros, and functions. Reply by Vinayakam Mani on 29 August 2024, 13:49 > Done. Created GPIO.C which contains only watchdog signals for > now. can be expanded based on the design. ---------------------------------------- File: firmware/App/Modes/ModeFault.c Revision Comment by Sean Nash on 28 August 2024, 09:24 https://devapps.diality.us/cru/DD-LEAH-210-5#c19906 Why not call this function? Reply by Vinayakam Mani on 29 August 2024, 14:54 > Will be addressed once Concentrate pump driver is included. > this function carries the parameter (#define) related to > concentrate pump. Revision Comment by Sean Nash on 28 August 2024, 09:24 https://devapps.diality.us/cru/DD-LEAH-210-5#c19907 Remove this. DD will not have LEDs as far as I know. Reply by Vinayakam Mani on 29 August 2024, 14:55 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:27 https://devapps.diality.us/cru/DD-LEAH-210-5#c19908 Why not call this function? Reply by Vinayakam Mani on 29 August 2024, 14:56 > Will be addressed once Concentrate pump driver is included. > this function carries the parameter (#define) related to > concentrate pump. ---------------------------------------- File: firmware/App/Modes/ModeInitPOST.c Revision Comment by Sean Nash on 28 August 2024, 09:29 https://devapps.diality.us/cru/DD-LEAH-210-5#c19909 I don't think DD will have CPLD, drain pump, fans, or flow sensors. Reply by Vinayakam Mani on 29 August 2024, 17:14 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:30 https://devapps.diality.us/cru/DD-LEAH-210-5#c19910 FpgaDD.h. Reply by Vinayakam Mani on 29 August 2024, 17:14 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:30 https://devapps.diality.us/cru/DD-LEAH-210-5#c19911 DD has no load cells. Reply by Vinayakam Mani on 29 August 2024, 17:14 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:30 https://devapps.diality.us/cru/DD-LEAH-210-5#c19912 DD has no reservoirs or RTC. Reply by Vinayakam Mani on 29 August 2024, 17:15 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:31 https://devapps.diality.us/cru/DD-LEAH-210-5#c19913 Messaging is out of order. Alphabetize. Reply by Vinayakam Mani on 29 August 2024, 17:15 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:31 https://devapps.diality.us/cru/DD-LEAH-210-5#c19914 DD has no UV reactors. Reply by Vinayakam Mani on 29 August 2024, 17:15 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:32 https://devapps.diality.us/cru/DD-LEAH-210-5#c19915 Missing postPassed. I prefer we just generalize outputs for init functions to say unit variables are initialized. Reply by Vinayakam Mani on 29 August 2024, 17:15 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:33 https://devapps.diality.us/cru/DD-LEAH-210-5#c19916 DD has not LEDs, RTC, accelerometer, or UV reactor. Reply by Vinayakam Mani on 29 August 2024, 17:15 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:34 https://devapps.diality.us/cru/DD-LEAH-210-5#c19917 Remove. Reply by Vinayakam Mani on 29 August 2024, 17:15 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:34 https://devapps.diality.us/cru/DD-LEAH-210-5#c19918 Remove cases for RTC, accelerometer, drain pump, reservoirs, UV reactors, fans, flow sensors, and load cells. Reply by Vinayakam Mani on 29 August 2024, 17:15 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:36 https://devapps.diality.us/cru/DD-LEAH-210-5#c19919 m at end of comment should be mode. Reply by Vinayakam Mani on 29 August 2024, 17:15 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:37 https://devapps.diality.us/cru/DD-LEAH-210-5#c19920 HD s/b DD. Reply by Vinayakam Mani on 29 August 2024, 17:15 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:37 https://devapps.diality.us/cru/DD-LEAH-210-5#c19921 HD s/b DD in return and brief sections. Reply by Vinayakam Mani on 29 August 2024, 17:15 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:38 https://devapps.diality.us/cru/DD-LEAH-210-5#c19922 Remove "passed" from comment. We are broadcasting regardless of pass/fail. Reply by Vinayakam Mani on 29 August 2024, 17:15 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:39 https://devapps.diality.us/cru/DD-LEAH-210-5#c19923 Add a details for event messages sent. Reply by Vinayakam Mani on 29 August 2024, 17:15 > Done. Reply by Sean Nash on 30 August 2024, 09:00 > For messages sent, I've been doing details \b Message \b > Sent: ... Reply by Vinayakam Mani on 30 August 2024, 09:37 > Done. ---------------------------------------- File: firmware/App/Modes/ModeService.c Revision Comment by Sean Nash on 28 August 2024, 09:41 https://devapps.diality.us/cru/DD-LEAH-210-5#c19925 Remove CPLD include and replace with FpgaDD.h. Reply by Vinayakam Mani on 30 August 2024, 09:38 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:42 https://devapps.diality.us/cru/DD-LEAH-210-5#c19926 Remove LED and load cell reset calls. Why not call the deenergize function? Reply by Vinayakam Mani on 29 August 2024, 17:16 > Done. Once Concentrate pump driver added, deenergize function > will be enabled ---------------------------------------- File: firmware/App/Modes/ModeService.h Revision Comment by Sean Nash on 28 August 2024, 09:41 https://devapps.diality.us/cru/DD-LEAH-210-5#c19924 Why is ModeFault.h included? Reply by Vinayakam Mani on 29 August 2024, 17:17 > Done. ---------------------------------------- File: firmware/App/Modes/ModeStandby.c Revision Comment by Sean Nash on 28 August 2024, 09:44 https://devapps.diality.us/cru/DD-LEAH-210-5#c19928 Remove drain pump and RTC includes. Reorder alphabetically. Reply by Vinayakam Mani on 29 August 2024, 17:18 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:48 https://devapps.diality.us/cru/DD-LEAH-210-5#c19929 Just generalize outputs for init. Reply by Vinayakam Mani on 29 August 2024, 17:17 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:49 https://devapps.diality.us/cru/DD-LEAH-210-5#c19931 Remove - no LEDs on DD. Reply by Vinayakam Mani on 29 August 2024, 17:17 > Done. Revision Comment by Sean Nash on 28 August 2024, 09:49 https://devapps.diality.us/cru/DD-LEAH-210-5#c19930 Remove - no reservoirs in DD. Reply by Vinayakam Mani on 29 August 2024, 17:17 > Done. ---------------------------------------- File: firmware/App/Modes/ModeStandby.h Revision Comment by Sean Nash on 28 August 2024, 09:44 https://devapps.diality.us/cru/DD-LEAH-210-5#c19927 Replace HD in all comments with TD. Reply by Vinayakam Mani on 29 August 2024, 17:28 > Done. ---------------------------------------- File: firmware/App/Modes/OperationModes.c Revision Comment by Sean Nash on 28 August 2024, 10:24 https://devapps.diality.us/cru/DD-LEAH-210-5#c19941 Why is gio.h needed? Reply by Vinayakam Mani on 30 August 2024, 10:11 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:24 https://devapps.diality.us/cru/DD-LEAH-210-5#c19942 Remove commented out includes. Alphabetize remaining. Reply by Vinayakam Mani on 30 August 2024, 10:12 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:23 https://devapps.diality.us/cru/DD-LEAH-210-5#c19940 Change DG/HD to DD/TD. Reply by Vinayakam Mani on 30 August 2024, 10:12 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:23 https://devapps.diality.us/cru/DD-LEAH-210-5#c19939 Generalize initialization outputs. Reply by Vinayakam Mani on 30 August 2024, 10:13 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:22 https://devapps.diality.us/cru/DD-LEAH-210-5#c19938 Add details for event messages sent. Reply by Vinayakam Mani on 30 August 2024, 10:14 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:21 https://devapps.diality.us/cru/DD-LEAH-210-5#c19937 Add details for event messages sent and s/w fault alarm. Reply by Vinayakam Mani on 30 August 2024, 10:14 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:20 https://devapps.diality.us/cru/DD-LEAH-210-5#c19936 Add details for broadcast message sent. Reply by Vinayakam Mani on 30 August 2024, 10:14 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:20 https://devapps.diality.us/cru/DD-LEAH-210-5#c19935 Change DG to DD. Reply by Vinayakam Mani on 30 August 2024, 10:14 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:19 https://devapps.diality.us/cru/DD-LEAH-210-5#c19934 Add details for event message sent. Reply by Vinayakam Mani on 30 August 2024, 10:14 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:18 https://devapps.diality.us/cru/DD-LEAH-210-5#c19933 Not really a queue operation. But we want the mode and sub-mode to be updated together and the set is not atomic so we need thread protection. Reply by Vinayakam Mani on 30 August 2024, 10:15 > Done. ---------------------------------------- File: firmware/App/Modes/OperationModes.h Revision Comment by Sean Nash on 28 August 2024, 10:16 https://devapps.diality.us/cru/DD-LEAH-210-5#c19932 Add blank line before separator comment. Reply by Vinayakam Mani on 30 August 2024, 10:16 > Done ---------------------------------------- File: firmware/App/Services/AlarmMgmtDD.c Revision Comment by Sean Nash on 28 August 2024, 10:31 https://devapps.diality.us/cru/DD-LEAH-210-5#c19949 Alphabetize includes. Reply by Vinayakam Mani on 30 August 2024, 12:16 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:32 https://devapps.diality.us/cru/DD-LEAH-210-5#c19950 Make this comment an inline like the others. Reply by Vinayakam Mani on 30 August 2024, 12:16 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:32 https://devapps.diality.us/cru/DD-LEAH-210-5#c19951 DD has no LEDs. Reply by Vinayakam Mani on 30 August 2024, 12:16 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:33 https://devapps.diality.us/cru/DD-LEAH-210-5#c19952 Add a blank line before this comment. Reply by Vinayakam Mani on 30 August 2024, 12:17 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:34 https://devapps.diality.us/cru/DD-LEAH-210-5#c19953 Generalize initialization outputs. Reply by Vinayakam Mani on 30 August 2024, 12:17 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:35 https://devapps.diality.us/cru/DD-LEAH-210-5#c19954 Change to "... executes periodic alarm management operations." Reply by Vinayakam Mani on 30 August 2024, 12:17 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:38 https://devapps.diality.us/cru/DD-LEAH-210-5#c19955 Add details for trigger message sent. Reply by Vinayakam Mani on 30 August 2024, 12:17 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:41 https://devapps.diality.us/cru/DD-LEAH-210-5#c19956 Add details for alarm cleared message sent. Reply by Vinayakam Mani on 30 August 2024, 12:18 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:42 https://devapps.diality.us/cru/DD-LEAH-210-5#c19957 Add details for alarm condition cleared message sent. Reply by Vinayakam Mani on 30 August 2024, 12:18 > Done. ---------------------------------------- File: firmware/App/Services/AlarmMgmtSWFaults.h Revision Comment by Sean Nash on 28 August 2024, 10:25 https://devapps.diality.us/cru/DD-LEAH-210-5#c19943 Should be Vinay. Reply by Vinayakam Mani on 30 August 2024, 12:20 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:26 https://devapps.diality.us/cru/DD-LEAH-210-5#c19944 In TD, I removed the comments on every 5th enum and instead explicitly enumerated each one. Recommend we do that here too. Reply by Vinayakam Mani on 30 August 2024, 12:20 > Done. Revision Comment by Sean Nash on 28 August 2024, 10:28 https://devapps.diality.us/cru/DD-LEAH-210-5#c19945 I would remove all enums that aren't already in this code base. Reply by Vinayakam Mani on 30 August 2024, 12:20 > Done. ---------------------------------------- File: firmware/App/Services/CommBuffers.c Revision Comment by Sean Nash on 29 August 2024, 10:18 https://devapps.diality.us/cru/DD-LEAH-210-5#c19986 Why not include this? Reply by Vinayakam Mani on 30 August 2024, 12:21 > I guess, this file will be deleted, and common source will be > used. ---------------------------------------- File: firmware/App/Services/FpgaDD.c Revision Comment by Sean Nash on 29 August 2024, 14:12 https://devapps.diality.us/cru/DD-LEAH-210-5#c20009 Move down to app includes section and replace <> with "". Reply by Vinayakam Mani on 30 August 2024, 16:28 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:12 https://devapps.diality.us/cru/DD-LEAH-210-5#c20010 Add blank line between Halcogen includes and normal app includes for separation. Reply by Vinayakam Mani on 30 August 2024, 16:28 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:12 https://devapps.diality.us/cru/DD-LEAH-210-5#c20011 Alphabetize the app includes group. Reply by Vinayakam Mani on 30 August 2024, 16:28 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:16 https://devapps.diality.us/cru/DD-LEAH-210-5#c20015 Move this comment down below the pragma and use 3 slashes so doxygen picks it up. Reply by Vinayakam Mani on 30 August 2024, 16:29 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:17 https://devapps.diality.us/cru/DD-LEAH-210-5#c20016 Change sensors' to sensor. And let's not abbreviate words like structure in doxygen comments. Reply by Vinayakam Mani on 30 August 2024, 16:29 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:18 https://devapps.diality.us/cru/DD-LEAH-210-5#c20017 Remove blank line. Revision Comment by Sean Nash on 29 August 2024, 14:18 https://devapps.diality.us/cru/DD-LEAH-210-5#c20018 Remove blank line. Reply by Vinayakam Mani on 30 August 2024, 16:29 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:15 https://devapps.diality.us/cru/DD-LEAH-210-5#c20014 Remove this comment. Redundant. Reply by Vinayakam Mani on 30 August 2024, 16:29 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:13 https://devapps.diality.us/cru/DD-LEAH-210-5#c20012 I've been trying to replace all references to a "module" with "unit". Reply by Vinayakam Mani on 30 August 2024, 16:29 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:14 https://devapps.diality.us/cru/DD-LEAH-210-5#c20013 Add a blank line between THd control setting and other valve settings because the comment above only applies to the THd assignment. The other valve assignments should get their own comment. Reply by Vinayakam Mani on 30 August 2024, 16:29 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:19 https://devapps.diality.us/cru/DD-LEAH-210-5#c20019 Remove blank line. Reply by Vinayakam Mani on 30 August 2024, 16:29 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:20 https://devapps.diality.us/cru/DD-LEAH-210-5#c20020 Indent looks weird here. Maybe just say "time windowed error count for FPGA clock speed alarm" for readability. Revision Comment by Sean Nash on 29 August 2024, 14:22 https://devapps.diality.us/cru/DD-LEAH-210-5#c20021 Move this up to function header as a "@warning". Reply by Vinayakam Mani on 30 August 2024, 16:30 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:26 https://devapps.diality.us/cru/DD-LEAH-210-5#c20022 I think using component code in function names is good, but not in the actual text that is describing the function. Reply by Vinayakam Mani on 30 August 2024, 16:30 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:27 https://devapps.diality.us/cru/DD-LEAH-210-5#c20023 Add details for alarm. Reply by Vinayakam Mani on 30 August 2024, 16:32 > Done. ---------------------------------------- File: firmware/App/Services/FpgaDD.h Revision Comment by Sean Nash on 29 August 2024, 14:05 https://devapps.diality.us/cru/DD-LEAH-210-5#c20005 Add _ between fpga and dd. Reply by Vinayakam Mani on 30 August 2024, 16:33 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:08 https://devapps.diality.us/cru/DD-LEAH-210-5#c20006 I don't think Interrupts or Utilities is needed here. Reply by Vinayakam Mani on 30 August 2024, 16:33 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:09 https://devapps.diality.us/cru/DD-LEAH-210-5#c20007 I see these are grouped as set or get. Consider grouping all get and sets together by h/w component. Revision Comment by Sean Nash on 29 August 2024, 14:10 https://devapps.diality.us/cru/DD-LEAH-210-5#c20008 Let's be consistent about to do comments so they are easily searched for. We've been using // TODO blah blah blah... Are these to dos going to be addressed now or later? Reply by Vinayakam Mani on 30 August 2024, 16:33 > Done. deleted unused function prototypes. ---------------------------------------- File: firmware/App/Services/Interrupts.c Revision Comment by Sean Nash on 29 August 2024, 14:30 https://devapps.diality.us/cru/DD-LEAH-210-5#c20024 Move down to app includes grouping. Reply by Vinayakam Mani on 30 August 2024, 16:34 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:30 https://devapps.diality.us/cru/DD-LEAH-210-5#c20025 Should not be necessary since SystemCommDD.h will include this. Reply by Vinayakam Mani on 30 August 2024, 16:34 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:31 https://devapps.diality.us/cru/DD-LEAH-210-5#c20027 Alphabetize the app includes. Reply by Vinayakam Mani on 30 August 2024, 16:34 > Done. ---------------------------------------- File: firmware/App/Services/Messaging.c Revision Comment by Sean Nash on 29 August 2024, 14:37 https://devapps.diality.us/cru/DD-LEAH-210-5#c20028 Add a 2nd blank line here. Reply by Vinayakam Mani on 03 September 2024, 12:23 > Done. ---------------------------------------- File: firmware/App/Services/MsgQueues.c Revision Comment by Sean Nash on 29 August 2024, 14:40 https://devapps.diality.us/cru/DD-LEAH-210-5#c20029 Remove * at end of line. Reply by Vinayakam Mani on 03 September 2024, 12:23 > Done. ---------------------------------------- File: firmware/App/Services/SystemCommDD.c Revision Comment by Sean Nash on 29 August 2024, 14:42 https://devapps.diality.us/cru/DD-LEAH-210-5#c20030 Move down to normal app includes group and replace <> with "". Reply by Vinayakam Mani on 03 September 2024, 12:23 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:42 https://devapps.diality.us/cru/DD-LEAH-210-5#c20031 There will be no drain pump, right? Reply by Vinayakam Mani on 03 September 2024, 12:23 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:42 https://devapps.diality.us/cru/DD-LEAH-210-5#c20032 Remove extra blank line. Reply by Vinayakam Mani on 03 September 2024, 12:23 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:47 https://devapps.diality.us/cru/DD-LEAH-210-5#c20033 Remove blank line. Reply by Vinayakam Mani on 03 September 2024, 12:24 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:48 https://devapps.diality.us/cru/DD-LEAH-210-5#c20034 Add s/w fault. Reply by Vinayakam Mani on 03 September 2024, 12:24 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:48 https://devapps.diality.us/cru/DD-LEAH-210-5#c20035 Add s/w fault. Reply by Vinayakam Mani on 03 September 2024, 12:24 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:49 https://devapps.diality.us/cru/DD-LEAH-210-5#c20036 Restore extra blank line. ---------------------------------------- File: firmware/App/Tasks/TaskGeneral.c Revision Comment by Sean Nash on 29 August 2024, 14:51 https://devapps.diality.us/cru/DD-LEAH-210-5#c20037 If you include SystemCommDD, you shouldn't have to include lower level SystemComm header. Reply by Vinayakam Mani on 03 September 2024, 12:26 > Done. ---------------------------------------- File: firmware/source/sys_main.c Revision Comment by Sean Nash on 29 August 2024, 14:55 https://devapps.diality.us/cru/DD-LEAH-210-5#c20040 Move down to app includes grouping and replace <> with "". Revision Comment by Sean Nash on 29 August 2024, 14:55 https://devapps.diality.us/cru/DD-LEAH-210-5#c20041 Move up to Halcogen include grouping. All of them. Reply by Vinayakam Mani on 03 September 2024, 12:27 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:56 https://devapps.diality.us/cru/DD-LEAH-210-5#c20043 Go ahead and remove includes that you know we aren't going to have. Reply by Vinayakam Mani on 03 September 2024, 12:27 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:56 https://devapps.diality.us/cru/DD-LEAH-210-5#c20045 Should only have to call FpgaDD.h init function from main to get low-level driver init function called from there. Reply by Vinayakam Mani on 03 September 2024, 12:28 > Done. Revision Comment by Sean Nash on 29 August 2024, 14:57 https://devapps.diality.us/cru/DD-LEAH-210-5#c20046 Go ahead and remove init function call that you know we're not going to have. Reply by Vinayakam Mani on 03 September 2024, 12:28 > Done. ---------------------------------------- File: firmware/App/Controllers/Valves.c Revision Comment by Sean Nash on 27 August 2024, 09:57 https://devapps.diality.us/cru/DD-LEAH-210-5#c19850 Add blank line between file header and first include. Reply by Vinayakam Mani on 29 August 2024, 10:59 > Fixed Revision Comment by Sean Nash on 27 August 2024, 09:58 https://devapps.diality.us/cru/DD-LEAH-210-5#c19851 Add blank line between library includes and app includes. Library includes should come first, so FpgaDD.h above should come down into the app includes group and should have "" instead of <>. App includes should be in alphabetical order. Reply by Vinayakam Mani on 29 August 2024, 11:00 > Fixed Revision Comment by Sean Nash on 27 August 2024, 10:01 https://devapps.diality.us/cru/DD-LEAH-210-5#c19852 Shouldn't this be overrideable? Reply by Vinayakam Mani on 29 August 2024, 11:00 > The 'valveStatesPublishInterval' is override variable. this > variable remains unchanged. Revision Comment by Sean Nash on 27 August 2024, 10:02 https://devapps.diality.us/cru/DD-LEAH-210-5#c19853 Prefer that we initialize variables in the init function so that variables can be re-initialized by simply calling the init function. Reply by Vinayakam Mani on 29 August 2024, 11:01 > Done. Revision Comment by Sean Nash on 27 August 2024, 10:30 https://devapps.diality.us/cru/DD-LEAH-210-5#c19860 Outputs are the 3 bit states variables right now - but consider making those variables local to caller and passed in as reference params. Reply by Vinayakam Mani on 29 August 2024, 11:02 > Looks we need these variables as global variables since the > execvalve checks the last command valve and read back valve > state to report an alarm. To retain the last commanded > valves, we need these variables as global. Revision Comment by Sean Nash on 27 August 2024, 10:07 https://devapps.diality.us/cru/DD-LEAH-210-5#c19855 Add redundant enums for group separation (e.g. FIRST_UF_VALVE) so that this reads better. Reply by Vinayakam Mani on 29 August 2024, 11:04 > Done. Revision Comment by Sean Nash on 27 August 2024, 10:33 https://devapps.diality.us/cru/DD-LEAH-210-5#c19861 This function is not necessary if the only valve states are open and closed. In Denali, there were more state names and many of them only made sense to certain valves (e.g. FILL_RESERVOIR = OPEN and TO_DRAIN = CLOSED for VPo valve in Denali). If we're not going to do that in Leahi, that's fine but then this function should be deleted since the mapping to energized and deenergized is very straight forward. I recommend adding more state names like Denali had though. Reply by Vinayakam Mani on 29 August 2024, 11:05 > Will work on it as we get more clarity on Operation modes. Revision Comment by Sean Nash on 27 August 2024, 10:36 https://devapps.diality.us/cru/DD-LEAH-210-5#c19862 Same comment. If only open and closed state names, we don't really need this function. Reply by Vinayakam Mani on 29 August 2024, 11:05 > Will work on it as we get more clarity on Operation modes. Revision Comment by Sean Nash on 27 August 2024, 10:48 https://devapps.diality.us/cru/DD-LEAH-210-5#c19863 Remove blank line. Reply by Vinayakam Mani on 29 August 2024, 11:05 > Done. Revision Comment by Sean Nash on 27 August 2024, 10:50 https://devapps.diality.us/cru/DD-LEAH-210-5#c19864 Make 2 blank lines before and after test support functions banner. Reply by Vinayakam Mani on 29 August 2024, 11:05 > Done. Revision Comment by Sean Nash on 27 August 2024, 10:55 https://devapps.diality.us/cru/DD-LEAH-210-5#c19866 I think these are going to be refactored. Message handlers need to return BOOL and take a message as a param. See TD BubbleDetector.c for example. Reply by Vinayakam Mani on 29 August 2024, 11:05 > Done. Revision Comment by Sean Nash on 27 August 2024, 10:54 https://devapps.diality.us/cru/DD-LEAH-210-5#c19865 valveStates[]. Reply by Vinayakam Mani on 29 August 2024, 11:05 > Done. ---------------------------------------- File: firmware/App/Controllers/Valves.h Revision Comment by Sean Nash on 27 August 2024, 10:23 https://devapps.diality.us/cru/DD-LEAH-210-5#c19856 Try to be a little more descriptive in these group briefs as this is the summary text in the SDD for the unit. This is a driver and controller/monitor all in one. Reply by Vinayakam Mani on 29 August 2024, 11:06 > Done. Revision Comment by Sean Nash on 27 August 2024, 10:24 https://devapps.diality.us/cru/DD-LEAH-210-5#c19857 Add blank line between doxygen group item and definitions. Reply by Vinayakam Mani on 29 August 2024, 11:06 > Done. Revision Comment by Sean Nash on 27 August 2024, 10:27 https://devapps.diality.us/cru/DD-LEAH-210-5#c19859 Consider turning these into bits like the commanded states above. Dialin can convert them to separate valve state flags on receipt and UI can do the same for logging. Discuss with Dara and Michael. Reply by Vinayakam Mani on 04 September 2024, 10:36 > Ok. Will check and update in next versions. ---------------------------------------- File: DDDefs.h Revision Comment by Sean Nash on 30 August 2024, 10:06 https://devapps.diality.us/cru/DD-LEAH-210-5#c20079 No solo standby mode. Reply by Vinayakam Mani on 03 September 2024, 12:31 > Done. Revision Comment by Sean Nash on 30 August 2024, 10:05 https://devapps.diality.us/cru/DD-LEAH-210-5#c20078 No recirculate mode. Reply by Vinayakam Mani on 03 September 2024, 12:31 > Done. Revision Comment by Sean Nash on 30 August 2024, 10:03 https://devapps.diality.us/cru/DD-LEAH-210-5#c20077 No fill mode. Reply by Vinayakam Mani on 03 September 2024, 12:31 > Done. Revision Comment by Sean Nash on 30 August 2024, 10:02 https://devapps.diality.us/cru/DD-LEAH-210-5#c20076 No drain mode. Reply by Vinayakam Mani on 03 September 2024, 12:31 > Done. Revision Comment by Sean Nash on 30 August 2024, 10:00 https://devapps.diality.us/cru/DD-LEAH-210-5#c20075 This is not a s/w unit. I think groups should be 1:1 with s/w units so that doxygen chapters align with s/w units to be tested. So this should probably be standby mode (TD or DD?). Reply by Vinayakam Mani on 03 September 2024, 12:29 > Done. Revision Comment by Sean Nash on 30 August 2024, 09:58 https://devapps.diality.us/cru/DD-LEAH-210-5#c20073 No chem disinfect. Reply by Vinayakam Mani on 03 September 2024, 12:30 > Done. Revision Comment by Sean Nash on 30 August 2024, 09:59 https://devapps.diality.us/cru/DD-LEAH-210-5#c20074 This is a group end thing and I don't see a group above that it goes with. Reply by Vinayakam Mani on 03 September 2024, 12:30 > Done. Revision Comment by Sean Nash on 30 August 2024, 09:58 https://devapps.diality.us/cru/DD-LEAH-210-5#c20072 No chem disinfect. Reply by Vinayakam Mani on 03 September 2024, 12:30 > Done. ---------------------------------------- File: MsgDefs.h Revision Comment by Sean Nash on 30 August 2024, 10:07 https://devapps.diality.us/cru/DD-LEAH-210-5#c20080 Rename to Messaging so it gets added to that s/w unit. Reply by Vinayakam Mani on 03 September 2024, 12:34 > Done. ---------------------------------------- File: WatchdogMgmt.c Revision Comment by Sean Nash on 30 August 2024, 10:09 https://devapps.diality.us/cru/DD-LEAH-210-5#c20081 Add one for RO too. Reply by Vinayakam Mani on 03 September 2024, 12:35 > Done. --- ID: DD-LEAH-210-5 https://devapps.diality.us/cru/DD-LEAH-210-5 Title: DD-LEAH-210_FW DD Fpga Interface Statement of Objectives: State: Closed Summary: Author: Vinayakam Mani Moderator: Vinayakam Mani Reviewers: (1 active, 3 completed*) Sean Nash (*) Michael Garthwaite (*) Dara Navaei (*) jpaguio