This is a list of all comments for HD-DEN-14344-3. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/AirPump.c Revision Comment by Sean Nash on 22 November 2022, 09:15 https://devapps.diality.us/cru/HD-DEN-14344-3#c15034 Add blank line after separator. Reply by Sean Nash on 19 December 2022, 17:21 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:15 https://devapps.diality.us/cru/HD-DEN-14344-3#c15035 Remove extra blank line. Reply by Sean Nash on 19 December 2022, 17:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 December 2022, 17:20 https://devapps.diality.us/cru/HD-DEN-14344-3#c15719 Set this to something between 0 and 19 (for staggering different broadcasts over typical 1 second interval). Use it to initialize data publication timer counter (which is currently not initialized at all). Reply by Michael Garthwaite on 22 December 2022, 10:56 > Fixed. Thanks! Reply by Sean Nash on 22 December 2022, 13:01 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:17 https://devapps.diality.us/cru/HD-DEN-14344-3#c15038 Remove extra blank line. Reply by Sean Nash on 19 December 2022, 17:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:18 https://devapps.diality.us/cru/HD-DEN-14344-3#c15039 Remove extra blank lines. Add function headers. Reply by Sean Nash on 19 December 2022, 17:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:19 https://devapps.diality.us/cru/HD-DEN-14344-3#c15040 Put constant on right side of compare operator. State should be < num of states. Reply by Michael Garthwaite on 23 November 2022, 09:27 > Fixed. Thanks! Reply by Sean Nash on 19 December 2022, 17:20 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:21 https://devapps.diality.us/cru/HD-DEN-14344-3#c15041 Remove space before "state". Reply by Michael Garthwaite on 23 November 2022, 09:27 > Fixed. Thanks! Reply by Sean Nash on 19 December 2022, 17:21 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:21 https://devapps.diality.us/cru/HD-DEN-14344-3#c15042 Put constant on right side of compare operator. State should be < num of states. Reply by Michael Garthwaite on 23 November 2022, 09:27 > Fixed. Thanks! Reply by Sean Nash on 19 December 2022, 17:24 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:22 https://devapps.diality.us/cru/HD-DEN-14344-3#c15043 This pin is an output (or it should be), so I don't think a get function makes sense. Where is this function used? Reply by Michael Garthwaite on 23 November 2022, 09:19 > Do we have an alternative to reading if we successfully set > the pin high/low? Reply by Sean Nash on 19 December 2022, 17:26 > I think you're stuck with what you last commanded. You > might try the get to see if it works, but I think those > only work for input pins. > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 17:23 https://devapps.diality.us/cru/HD-DEN-14344-3#c15372 Need function header. Reply by Sean Nash on 22 December 2022, 13:13 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:24 https://devapps.diality.us/cru/HD-DEN-14344-3#c15044 Ok to have this function - just wondering where it would be used? If not needed, remove function and make pump state enum private. Reply by Michael Garthwaite on 23 November 2022, 09:17 > Used in airtrap.c and presoccl.c Reply by Sean Nash on 22 December 2022, 13:14 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:25 https://devapps.diality.us/cru/HD-DEN-14344-3#c15045 How would you check motor is running? Not clear to me that we need a monitor. Can move broadcast to control exec. Reply by Michael Garthwaite on 23 November 2022, 09:27 > Function Removed. Thanks! Reply by Sean Nash on 19 December 2022, 17:27 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 17:30 https://devapps.diality.us/cru/HD-DEN-14344-3#c15377 Keep blank line between declaration and return statement. Reply by Michael Garthwaite on 22 December 2022, 10:54 > Fixed? Reply by Sean Nash on 22 December 2022, 13:11 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 17:30 https://devapps.diality.us/cru/HD-DEN-14344-3#c15378 Keep a blank line before return statement. Reply by Michael Garthwaite on 22 December 2022, 10:54 > Fixed? Reply by Sean Nash on 22 December 2022, 13:11 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 17:29 https://devapps.diality.us/cru/HD-DEN-14344-3#c15376 How does the getAirPumpMotorState() give a T/F for alarm detection? Reply by Michael Garthwaite on 22 December 2022, 13:19 > It can only be 0 or 1 in a U32 for the time being. I can cast > it as a bool Reply by Sean Nash on 22 December 2022, 13:26 > Would rather you set a BOOL based on state and then pass > the BOOL to persistent alarm function. I'm afraid we could > add more states later and forget about this. Reply by Michael Garthwaite on 22 December 2022, 13:28 > The only time we would be adding more states is if we are > no longer on HIGH/LOW control, like switching to PWM. > That would be such a significant change that this would > be reviewed. Reply by Sean Nash on 22 December 2022, 13:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 17:34 https://devapps.diality.us/cru/HD-DEN-14344-3#c15379 Keep blank line between declaration and code and between code and return statement. Reply by Michael Garthwaite on 22 December 2022, 10:55 > Fixed? Reply by Sean Nash on 22 December 2022, 13:12 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 December 2022, 17:07 https://devapps.diality.us/cru/HD-DEN-14344-3#c15713 Remove extra blank line. Reply by Sean Nash on 22 December 2022, 13:08 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 December 2022, 17:07 https://devapps.diality.us/cru/HD-DEN-14344-3#c15714 Need function header. Reply by Sean Nash on 22 December 2022, 13:07 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 December 2022, 17:08 https://devapps.diality.us/cru/HD-DEN-14344-3#c15715 Keep blank line between functions. Reply by Michael Garthwaite on 22 December 2022, 10:53 > Fixed? There is a blank line here Reply by Sean Nash on 22 December 2022, 13:07 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:27 https://devapps.diality.us/cru/HD-DEN-14344-3#c15046 Not sure how we would test pump other than in pre-treatment when we do pressure leak test (bad pump will fail to pressurize venous side). Remove function unless there is some way to test pump directly. Reply by Michael Garthwaite on 23 November 2022, 09:28 > Function removed. Thanks! Reply by Sean Nash on 19 December 2022, 17:37 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 17:38 https://devapps.diality.us/cru/HD-DEN-14344-3#c15383 I think we want one more blank line (should be 2 total) before TEST SUPPORT banner. Reply by Michael Garthwaite on 22 December 2022, 10:55 > Fixed? Reply by Sean Nash on 22 December 2022, 13:12 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/AirPump.h Revision Comment by Sean Nash on 22 November 2022, 09:12 https://devapps.diality.us/cru/HD-DEN-14344-3#c15030 Add blank line before separator. Reply by Michael Garthwaite on 23 November 2022, 09:25 > Fixed. Thanks! Reply by Sean Nash on 19 December 2022, 17:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:12 https://devapps.diality.us/cru/HD-DEN-14344-3#c15028 Remove extra blank line. Reply by Michael Garthwaite on 23 November 2022, 09:25 > Fixed. Thanks! Reply by Sean Nash on 19 December 2022, 17:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:13 https://devapps.diality.us/cru/HD-DEN-14344-3#c15032 Need a doxygen module comment here. Reply by Michael Garthwaite on 23 November 2022, 09:25 > Fixed. Thanks! Reply by Sean Nash on 19 December 2022, 17:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:16 https://devapps.diality.us/cru/HD-DEN-14344-3#c15037 Add doxygen comment to top of structs and enums. Reply by Michael Garthwaite on 23 November 2022, 09:25 > Fixed. Thanks! Reply by Sean Nash on 19 December 2022, 17:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:16 https://devapps.diality.us/cru/HD-DEN-14344-3#c15036 Align comments. Reply by Michael Garthwaite on 23 November 2022, 09:26 > Fixed. Thanks! Reply by Sean Nash on 19 December 2022, 17:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:12 https://devapps.diality.us/cru/HD-DEN-14344-3#c15031 Add blank line after separator. Reply by Michael Garthwaite on 23 November 2022, 09:26 > Fixed. Thanks! Reply by Sean Nash on 19 December 2022, 17:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:14 https://devapps.diality.us/cru/HD-DEN-14344-3#c15033 What is there to monitor? Reply by Michael Garthwaite on 23 November 2022, 09:26 > Function removed. Thanks! Reply by Sean Nash on 19 December 2022, 17:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 17:41 https://devapps.diality.us/cru/HD-DEN-14344-3#c15391 Should be a group ending /**@}*/ at bottom here. Reply by Sean Nash on 22 December 2022, 13:25 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Sean Nash on 22 November 2022, 09:08 https://devapps.diality.us/cru/HD-DEN-14344-3#c15022 Need function header. Reply by Michael Garthwaite on 22 December 2022, 10:36 > Fixed. Thanks! Reply by Sean Nash on 22 December 2022, 13:00 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:09 https://devapps.diality.us/cru/HD-DEN-14344-3#c15023 Need function header. Reply by Michael Garthwaite on 22 December 2022, 10:36 > Fixed. Thanks! Reply by Sean Nash on 22 December 2022, 13:00 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:09 https://devapps.diality.us/cru/HD-DEN-14344-3#c15024 Remove extra blank line. Reply by Michael Garthwaite on 22 December 2022, 10:36 > Fixed. Thanks! Reply by Sean Nash on 22 December 2022, 12:59 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/HD.dil Revision Comment by wbracken on 28 November 2022, 13:51 https://devapps.diality.us/cru/HD-DEN-14344-3#c15077 Is this an output file from HalCoGen? If so seems like one shouldn't need to modify this file directly. Reply by Michael Garthwaite on 28 November 2022, 14:00 > HalCoGen is currently editing het.h, het.c, sys_link.cmd, > HD.dil, and sys_selftest.c in this branch. Any and all the > changes within those files are autogenerated from HaloCoGen. Reply by wbracken on 28 November 2022, 16:05 > RESOLVED IN CODE WALKTHROUGH. ---------------------------------------- File: firmware/source/sys_link.cmd Revision Comment by Sean Nash on 22 November 2022, 09:06 https://devapps.diality.us/cru/HD-DEN-14344-3#c15021 What happened here? Why changed? Reply by Michael Garthwaite on 22 November 2022, 09:12 > This code was auto-generated by HALCoGen to link PIN 36. Did > you want me to revert the auto-generated code? Reply by Sean Nash on 22 November 2022, 09:40 > Ok. Not sure how HALCoGen got ahead of generated code for > a time. > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/source/sys_selftest.c Revision Comment by Sean Nash on 22 November 2022, 09:03 https://devapps.diality.us/cru/HD-DEN-14344-3#c15020 Shouldn't make changes outside of user code scope. Looks like blank line added - remove. Reply by Michael Garthwaite on 22 November 2022, 09:11 > This code was auto-generated by HALCoGen. Did you want me to > revert the auto-generated code? Reply by Sean Nash on 22 November 2022, 09:30 > Oh, ok. Then somebody removed a blank line previously and > should not have. > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: MsgDefs.h Revision Comment by Sean Nash on 22 November 2022, 09:10 https://devapps.diality.us/cru/HD-DEN-14344-3#c15025 Align comment. Reply by Sean Nash on 22 December 2022, 13:15 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 November 2022, 09:11 https://devapps.diality.us/cru/HD-DEN-14344-3#c15026 There are 2 available msg IDs above that should be used before adding new ones. Reply by Sean Nash on 19 December 2022, 17:17 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/source/sys_main.c Revision Comment by wbracken on 23 November 2022, 13:19 https://devapps.diality.us/cru/HD-DEN-14344-3#c15068 AirPump.h should be before AirTrap.h. Reply by Michael Garthwaite on 22 December 2022, 10:38 > Fixed. Thanks! Reply by wbracken on 22 December 2022, 12:22 > RESOLVED IN CODE WALKTHROUGH. ---------------------------------------- File: firmware/include/het.h Revision Comment by wbracken on 28 November 2022, 13:47 https://devapps.diality.us/cru/HD-DEN-14344-3#c15072 Alignment throughout the file. However, this file is auto-generated by HalCoGen. Reply by wbracken on 22 December 2022, 13:19 > RESOLVED IN CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/AlarmMgmtSWFaults.h Revision Comment by Sean Nash on 22 December 2022, 09:18 https://devapps.diality.us/cru/HD-DEN-14344-3#c15721 Add // 150. Reply by Michael Garthwaite on 22 December 2022, 10:34 > Fixed. Thanks! Reply by Sean Nash on 22 December 2022, 13:02 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/AirTrap.c Revision Comment by Sean Nash on 29 December 2022, 09:44 https://devapps.diality.us/cru/HD-DEN-14344-3#c15787 Add doxygen comment for persistence. Try to capture thinking on why this is such a brief period. Reply by Michael Garthwaite on 29 December 2022, 11:10 > Fixed. Thanks! Reply by Sean Nash on 29 December 2022, 11:23 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 17:43 https://devapps.diality.us/cru/HD-DEN-14344-3#c15392 Capitalize Level and Ctr. Add doxygen comment. Reply by Sean Nash on 22 December 2022, 13:24 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 17:43 https://devapps.diality.us/cru/HD-DEN-14344-3#c15393 Indent 2nd and 3rd line of if by 2 more spaces. Reply by Michael Garthwaite on 22 December 2022, 10:31 > Fixed. Thanks! Reply by Sean Nash on 22 December 2022, 13:10 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 December 2022, 17:10 https://devapps.diality.us/cru/HD-DEN-14344-3#c15717 Remove blank line. Reply by Michael Garthwaite on 22 December 2022, 10:32 > Fixed. Thanks! Reply by Sean Nash on 22 December 2022, 13:06 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 17:57 https://devapps.diality.us/cru/HD-DEN-14344-3#c15394 Why are we not turning off the air pump here? I do not see any delay in open state where pump is turned off, so we are only delaying by 50 ms. And I don't see any reason why pump should be on while VBT is open. Reply by Sean Nash on 22 December 2022, 13:05 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 17:58 https://devapps.diality.us/cru/HD-DEN-14344-3#c15395 This new else if would not be necessary I think if the pump were just turned off on way out of closed state (my comment from closed state handler). Reply by Michael Garthwaite on 22 December 2022, 10:32 > Fixed. Thanks! Reply by Sean Nash on 22 December 2022, 13:09 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/SelfTests.c Revision Comment by Sean Nash on 29 December 2022, 09:47 https://devapps.diality.us/cru/HD-DEN-14344-3#c15788 For a release build, looks like we would do nothing and get stuck in this state. Reply by Michael Garthwaite on 29 December 2022, 11:10 > Fixed. Thanks! Reply by Sean Nash on 29 December 2022, 11:22 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 18:07 https://devapps.diality.us/cru/HD-DEN-14344-3#c15398 If we're using air pump (nominal case), we don't need to defer DG fills anymore - can signal these wherever we used to start filling before we added this delay. Reply by Michael Garthwaite on 22 December 2022, 10:33 > added the function call within the end of consumable self > tests when air pump is enabled. Reply by Sean Nash on 22 December 2022, 13:08 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: NVDataMgmtHDRecords.h Revision Comment by Dara Navaei on 02 December 2022, 10:55 https://devapps.diality.us/cru/HD-DEN-14344-3#c15095 Please add comment. Reply by Michael Garthwaite on 22 December 2022, 13:35 > Fixed. Thanks! Reply by Dara Navaei on 18 October 2023, 20:49 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: AlarmDefs.h Revision Comment by Sean Nash on 21 December 2022, 17:38 https://devapps.diality.us/cru/HD-DEN-14344-3#c15720 Can't have gaps in alarm enum/tables - will not compile. Need to pull in alarms from other branches if necessary to fill the gaps. Reply by Sean Nash on 22 December 2022, 13:04 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 17:16 https://devapps.diality.us/cru/HD-DEN-14344-3#c15364 When you merge staging locally, please add your new alarm to the new table down here as well. Reply by Michael Garthwaite on 22 December 2022, 10:46 > Unsure what table you are specifying. Does the current commit > look correct? Reply by Sean Nash on 22 December 2022, 13:04 > Referring to manual alarm info table below. It looks > correct. > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/PresOccl.c Revision Comment by wbracken on 19 December 2022, 15:48 https://devapps.diality.us/cru/HD-DEN-14344-3#c15299 Other than the fact that it was previously done, why use the parenthesis? Reply by Sean Nash on 19 December 2022, 18:00 > Agree not necessary, but also no harm. This may have been > done in response to a code review comment in early days. Reply by wbracken on 21 December 2022, 14:59 > RESOLVED IN CODEWALKTHROUGH. Revision Comment by Sean Nash on 19 December 2022, 18:01 https://devapps.diality.us/cru/HD-DEN-14344-3#c15397 Add comment explaining what we're doing here. Reply by Sean Nash on 22 December 2022, 13:09 > RESOLVED in CODE WALKTHROUGH. --- ID: HD-DEN-14344-3 https://devapps.diality.us/cru/HD-DEN-14344-3 Title: HD-DEN-14344_SW S85 MG Air Pump Statement of Objectives: State: Closed Summary: Author: Michael Garthwaite Moderator: Michael Garthwaite Reviewers: (2 active, 2 completed*) Sean Nash (*) wbracken (*) Dara Navaei jtaylor