This is a list of all comments for HD-DEN-15306-3. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/DGInterface.c Revision Comment by Sean Nash on 01 June 2023, 20:11 https://devapps.diality.us/cru/HD-DEN-15306-3#c17699 This was safer the other way. Why change? Reply by jtaylor on 02 June 2023, 12:45 > Done. Changed each instance to the negative form for > consistency. Reply by Sean Nash on 02 June 2023, 14:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 02 June 2023, 15:04 https://devapps.diality.us/cru/HD-DEN-15306-3#c17728 This check is too far out. Each check function inside should check this with DG comm. Reply by jtaylor on 02 June 2023, 16:01 > Done. Reply by Sean Nash on 02 June 2023, 16:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 02 June 2023, 16:34 https://devapps.diality.us/cru/HD-DEN-15306-3#c17735 Remove blank line. Reply by jtaylor on 02 June 2023, 16:58 > Done. Reply by Sean Nash on 02 June 2023, 17:20 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 01 June 2023, 20:11 https://devapps.diality.us/cru/HD-DEN-15306-3#c17700 This was safer the other way. Why change? Reply by jtaylor on 02 June 2023, 13:46 > Done. Changed each instance to the negative form for > consistency. Reply by Sean Nash on 02 June 2023, 14:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 02 June 2023, 16:36 https://devapps.diality.us/cru/HD-DEN-15306-3#c17736 Restore blank line. Should be 2 blank lines above and below test support banner. Reply by jtaylor on 02 June 2023, 17:08 > Done. Reply by Sean Nash on 02 June 2023, 17:21 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DialInFlow.c Revision Comment by Sean Nash on 01 June 2023, 20:10 https://devapps.diality.us/cru/HD-DEN-15306-3#c17698 This was safer the other way. Why change? Reply by jtaylor on 02 June 2023, 13:52 > Done. Changed each instance to the negative form for > consistency. Reply by Sean Nash on 02 June 2023, 14:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 02 June 2023, 14:59 https://devapps.diality.us/cru/HD-DEN-15306-3#c17727 Remove this condition. Reply by jtaylor on 02 June 2023, 16:05 > Done. Reply by Sean Nash on 02 June 2023, 16:39 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Fans.c Revision Comment by Sean Nash on 01 June 2023, 20:10 https://devapps.diality.us/cru/HD-DEN-15306-3#c17697 This was safer the other way. Why change? Reply by jtaylor on 02 June 2023, 13:52 > Done. Changed each instance to the negative form for > consistency. Reply by Sean Nash on 02 June 2023, 14:44 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Valves.c Revision Comment by Sean Nash on 01 June 2023, 20:18 https://devapps.diality.us/cru/HD-DEN-15306-3#c17704 Seems like we should be calling CPLD function here and in Fans, DialInFlow, and DGInterface too. Also recommend calling the isACPowerLost() function in just one place - maybe alarm mgmt exec to give you your 10 sec alarm block. Maybe rename the function to checkACPowerLost() with no return value and make it static (private). Reply by jtaylor on 02 June 2023, 11:40 > Use power loss to reset the out of position counter for the > valves. CPLD and isACPowerLost used to give the valves time > to settle before checking. Reply by Sean Nash on 02 June 2023, 16:20 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 02 June 2023, 15:07 https://devapps.diality.us/cru/HD-DEN-15306-3#c17729 Needs an else to zero out of range counter. Reply by jtaylor on 02 June 2023, 15:12 > Done. Reply by Sean Nash on 02 June 2023, 16:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 01 June 2023, 20:02 https://devapps.diality.us/cru/HD-DEN-15306-3#c17695 Remove extra blank line. Reply by jtaylor on 02 June 2023, 14:04 > Done. Reply by Sean Nash on 02 June 2023, 14:46 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/AlarmMgmt.c Revision Comment by Sean Nash on 01 June 2023, 20:13 https://devapps.diality.us/cru/HD-DEN-15306-3#c17701 This is technically a timer counter based on task tick of 50ms. Reply by jtaylor on 02 June 2023, 13:48 > Renamed for clarification. Reply by Sean Nash on 02 June 2023, 14:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 01 June 2023, 20:16 https://devapps.diality.us/cru/HD-DEN-15306-3#c17702 This seems unnecessary now with your new function doing this all the time. Consider removing. Reply by jtaylor on 02 June 2023, 13:49 > Done. Reply by Sean Nash on 02 June 2023, 16:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 02 June 2023, 16:43 https://devapps.diality.us/cru/HD-DEN-15306-3#c17742 Add blank line between declaration and code. Prefer we not assume bool and BOOL are the same. Use if statement and explicit assignment of TRUE to result if all conditions are true. First condition seems backward. Second and third conditions seem problematic. You will never trigger an AC power loss alarm. Should remove them I think. Reply by jtaylor on 02 June 2023, 17:06 > Oops. DeMorganized and done. Reply by Sean Nash on 02 June 2023, 17:20 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 01 June 2023, 15:36 https://devapps.diality.us/cru/HD-DEN-15306-3#c17690 Add alarmsBlockedTimer to header. Is alarmsBlockedTimer a timer or is it a counter? Reply by jtaylor on 01 June 2023, 16:18 > A 10 second timeout timer, driven by the 50ms tick that > drives the alarm state machine. Reply by Sean Nash on 01 June 2023, 20:16 > It is a timer counter. Counts down to zero. > > And still need to address function header per Bill's first > comment. Reply by jtaylor on 02 June 2023, 12:54 > Done. The countdown timer was renamed for clarity. Reply by Sean Nash on 02 June 2023, 14:45 > Still need header fixed. Reply by jtaylor on 02 June 2023, 16:16 > Done. Error propagration. Reply by wbracken on 02 June 2023, 16:51 > RESOLVED IN CODE WALKTHROUGH Revision Comment by Sean Nash on 01 June 2023, 20:07 https://devapps.diality.us/cru/HD-DEN-15306-3#c17696 How is this different than checking CPLD? Condition is cleared when CPLD no longer sees power loss (w/ 150ms delay I guess). Seems simpler to use CPLD function instead of checking 2 alarms. Reply by jtaylor on 02 June 2023, 13:23 > We appear to need a delay from AC's return to the first alarm > check. We have natural times at CPLD, Alarm clear, and the > end of the alarm block interval. The alarm block timer > currently clears 10 seconds after CPLD; the alarm is clear at > a random interval from milliseconds to minutes after CPLD. > I have modified the calls for consistent use of CPLD or of > isACPowerLost. Reply by Sean Nash on 02 June 2023, 14:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 02 June 2023, 16:51 https://devapps.diality.us/cru/HD-DEN-15306-3#c17743 Remove space between (). Reply by jtaylor on 02 June 2023, 17:54 > Done. Reply by Sean Nash on 02 June 2023, 18:27 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DialOutFlow.c Revision Comment by Sean Nash on 02 June 2023, 14:58 https://devapps.diality.us/cru/HD-DEN-15306-3#c17726 Remove this condition. Reply by Sean Nash on 02 June 2023, 16:40 > RESOLVED in CODE WALKTHROUGH. --- ID: HD-DEN-15306-3 https://devapps.diality.us/cru/HD-DEN-15306-3 Title: HD-DEN-15306_HD Fault Valve Position Alarm After Restoring AC Power 4 - Ready Statement of Objectives: State: Closed Summary: Author: jtaylor Moderator: jtaylor Reviewers: (4 active, 2 completed*) Sean Nash (*) wbracken (*) Michael Garthwaite Dara Navaei Darren Cox Steve Jarpe