This is a list of all comments for DG-DEN-2652-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/Valves.c Revision Comment by Sean Nash on 04 May 2020, 17:24 https://devapps.diality.us/cru/DG-DEN-2652-1#c1734 This appears to be more like a valves mismatch counter for your monitor. Consider changing variable name. Reply by pmontazemi on 05 May 2020, 12:19 > Agreed and done. Reply by Sean Nash on 05 May 2020, 15:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 05 May 2020, 13:55 https://devapps.diality.us/cru/DG-DEN-2652-1#c1766 Why are you using int instead of defined integer types? Reply by pmontazemi on 05 May 2020, 14:04 > Agreed and changed to U08. Reply by Dara Navaei on 05 May 2020, 15:39 > RESOLVED in CODE WALKTHROUGH Revision Comment by Sean Nash on 05 May 2020, 13:47 https://devapps.diality.us/cru/DG-DEN-2652-1#c1762 Why aren't these two functions static? Reply by pmontazemi on 05 May 2020, 14:04 > They should be static, agreed and done. Reply by Sean Nash on 05 May 2020, 15:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 May 2020, 17:13 https://devapps.diality.us/cru/DG-DEN-2652-1#c1730 This ends the doxygen module. Should move to end of module (after last function). Reply by pmontazemi on 05 May 2020, 12:51 > Done, please check LoadCell.c, too as it does not have this, > currently. Reply by Sean Nash on 05 May 2020, 15:47 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 May 2020, 17:28 https://devapps.diality.us/cru/DG-DEN-2652-1#c1736 Should loop through valves[] and initialize them. Reply by pmontazemi on 05 May 2020, 12:16 > Agreed and done. Reply by Sean Nash on 05 May 2020, 15:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 05 May 2020, 13:57 https://devapps.diality.us/cru/DG-DEN-2652-1#c1767 For consistency, I would use U08 instead of int Reply by pmontazemi on 05 May 2020, 14:05 > Agreed and done. Reply by Dara Navaei on 05 May 2020, 15:38 > RESOLVED in CODE WALKTHROUGH Revision Comment by Sean Nash on 05 May 2020, 13:48 https://devapps.diality.us/cru/DG-DEN-2652-1#c1763 I would use DEENERGIZED instead of zero to clarify intent for these inits. Reply by pmontazemi on 05 May 2020, 13:54 > Agreed, I had done that but the compiler originally gave me > trouble because it was expecting U32, now done. Reply by Sean Nash on 05 May 2020, 15:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 May 2020, 17:26 https://devapps.diality.us/cru/DG-DEN-2652-1#c1735 Seems like this should only be incremented if there is a mismatch between commanded valve states and the valve states read back from the FPGA. And of course reset to zero when they match. Reply by pmontazemi on 05 May 2020, 12:23 > Agreed, but if condition check needs to become < 2 instead of > <= 2, also modified that. Reply by Sean Nash on 05 May 2020, 15:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 May 2020, 17:29 https://devapps.diality.us/cru/DG-DEN-2652-1#c1737 I would consider always giving FPGA the current valve states at the end of this function instead of only when valve states change. And I would also set currentValvesStates just prior to sending to FPGA instead of above on line 89 - that way the monitor portion here will be checking read FPGA states against the current states from last exec (more likely to match). Reply by pmontazemi on 05 May 2020, 11:14 > Agreed and done, makes sense to move them at the end of > function with publish, together. Reply by Sean Nash on 05 May 2020, 15:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 May 2020, 17:34 https://devapps.diality.us/cru/DG-DEN-2652-1#c1738 I would move this to end of function so that it's always called. Reply by pmontazemi on 05 May 2020, 10:58 > I actually had a doubt on this, whether we needed to publish > the valves states after getting an error, thanks for > clarifying, it makes sense, will also apply this FW/SYS > pattern, moving forward. Reply by Sean Nash on 05 May 2020, 15:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 May 2020, 17:43 https://devapps.diality.us/cru/DG-DEN-2652-1#c1740 I would rename this function to checkValveStateName and change the 2nd param to valveStateName. Reply by pmontazemi on 05 May 2020, 10:56 > Agreed and done. Reply by Sean Nash on 05 May 2020, 13:53 > 2nd param name still same. Reply by Sean Nash on 05 May 2020, 15:49 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 May 2020, 17:17 https://devapps.diality.us/cru/DG-DEN-2652-1#c1732 Override logic does not belong here. I would replace this entire if...else... with the following statement: result |= ( getValveState( i ) == 1 ? 0x0001 << i : 0 ); Reply by pmontazemi on 05 May 2020, 12:47 > Agreed, if distinction between override or note does not need > to be made, then it becomes a simple bitwise operation. Reply by Sean Nash on 05 May 2020, 15:47 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 May 2020, 17:44 https://devapps.diality.us/cru/DG-DEN-2652-1#c1741 I would rename function to convertValveStateNameToValveState and rename the param to valveStateName. Reply by pmontazemi on 05 May 2020, 10:54 > Agreed and done. Reply by Sean Nash on 05 May 2020, 15:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 May 2020, 17:45 https://devapps.diality.us/cru/DG-DEN-2652-1#c1742 Replace 0 with your de-energized #define. Reply by pmontazemi on 05 May 2020, 10:49 > Agreed and done. Reply by Sean Nash on 05 May 2020, 15:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 May 2020, 17:46 https://devapps.diality.us/cru/DG-DEN-2652-1#c1743 Use your energized / de-energized #defines instead of 1 / 0 for all of these cases - then won't need comments. Reply by pmontazemi on 05 May 2020, 10:45 > Excellent point, much cleaner, now it will be optimized from > a legibility point, too. Reply by Sean Nash on 05 May 2020, 15:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 05 May 2020, 14:03 https://devapps.diality.us/cru/DG-DEN-2652-1#c1768 I would use U08 instead of int Reply by pmontazemi on 05 May 2020, 14:04 > Agreed and done. Reply by Dara Navaei on 05 May 2020, 15:38 > RESOLVED in CODE WALKTHROUGH Revision Comment by Sean Nash on 04 May 2020, 17:52 https://devapps.diality.us/cru/DG-DEN-2652-1#c1744 Fault? Reply by pmontazemi on 05 May 2020, 10:45 > Added. Reply by Sean Nash on 05 May 2020, 15:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 May 2020, 17:54 https://devapps.diality.us/cru/DG-DEN-2652-1#c1745 I think if checkValves() throws an alarm in default case of switch, then we don't need this. Reply by pmontazemi on 05 May 2020, 10:43 > Agreed and changed. Reply by Sean Nash on 05 May 2020, 15:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 May 2020, 17:14 https://devapps.diality.us/cru/DG-DEN-2652-1#c1731 Redundant. If already moved the first one of these from near top of module to bottom, this can be deleted. Reply by pmontazemi on 05 May 2020, 12:49 > Deleted. Reply by Sean Nash on 05 May 2020, 15:47 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Valves.h Revision Comment by Sean Nash on 04 May 2020, 17:20 https://devapps.diality.us/cru/DG-DEN-2652-1#c1733 Some logic in Valves.c (e.g. the array to U16 function) will require two unused spare enums (where VR1 and VR2 were) be inserted before VPD. Reply by pmontazemi on 05 May 2020, 12:27 > Good call, I added them and commented them as spare. Reply by Sean Nash on 05 May 2020, 15:47 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/source/sys_main.c Revision Comment by Sean Nash on 04 May 2020, 16:50 https://devapps.diality.us/cru/DG-DEN-2652-1#c1729 This is HalCOGen code (not in a /* USER CODE */ section. Changes will be lost when code re-generated. Reply by pmontazemi on 05 May 2020, 12:56 > Agreed, removed spaces within parentheses. Reply by Sean Nash on 05 May 2020, 15:47 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-2652-1 https://devapps.diality.us/cru/DG-DEN-2652-1 Title: DG-DEN-2652_DG Valve Control 1 of 2 Statement of Objectives: State: Closed Summary: Author: pmontazemi Moderator: pmontazemi Reviewers: (0 active, 2 completed*) Sean Nash (*) Dara Navaei (*)