This is a list of all comments for DG-DEN-14197-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Services/PIControllers.c Revision Comment by Sean Nash on 21 November 2022, 14:07 https://devapps.diality.us/cru/DG-DEN-14197-1#c14961 We are changing the step limit on a control so I would expect an output. Reply by Michael Garthwaite on 21 November 2022, 14:44 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:51 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 November 2022, 14:06 https://devapps.diality.us/cru/DG-DEN-14197-1#c14960 Remove signalID and add stepLimit as params. Reply by Michael Garthwaite on 21 November 2022, 14:43 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:51 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 November 2022, 14:10 https://devapps.diality.us/cru/DG-DEN-14197-1#c14963 Do we need a maximum? If so, magic number. Reply by Michael Garthwaite on 21 November 2022, 14:20 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:29 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 November 2022, 14:00 https://devapps.diality.us/cru/DG-DEN-14197-1#c14952 Based on function name, this function only expects to change step limit. So shouldn't need to pass signalID as param and shouldn't need a switch. Reply by Michael Garthwaite on 21 November 2022, 14:04 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:11 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 November 2022, 14:08 https://devapps.diality.us/cru/DG-DEN-14197-1#c14962 Invalid step limit (not signal)? Reply by Michael Garthwaite on 21 November 2022, 14:20 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:30 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 November 2022, 14:01 https://devapps.diality.us/cru/DG-DEN-14197-1#c14954 Remove blank line. Reply by Michael Garthwaite on 21 November 2022, 14:04 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:11 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ROPump.c Revision Comment by jtaylor on 18 November 2022, 09:22 https://devapps.diality.us/cru/DG-DEN-14197-1#c14863 Is this a placeholder? Should have a "TODO" comment for the planned updates. Are the flow profiles intended to change eventually? Do some of these states have different pump load conditions? Test/tuning plans? Where will setROPIFlowProfile() be called? etc. Reply by Michael Garthwaite on 21 November 2022, 13:58 > Yes and no for the place holder. The profiles expected to > change in the future. They may have different load states but > for now we have tuned Kp and Ki along with our maximum PWM > step limit to get the level of responsiveness we wanted to > achieve with this optimization task. We can further fine tune > the pump profiles with this data structure if necessary. > setROPIFlowProfile will be called in when we transition > operation modes when we intend to differentiate between > profiles. Reply by jtaylor on 21 November 2022, 16:44 > RESOLVED IN CODE WALKTHROUGH Revision Comment by wbracken on 21 November 2022, 10:37 https://devapps.diality.us/cru/DG-DEN-14197-1#c14867 Update header. Reply by Michael Garthwaite on 21 November 2022, 13:54 > Fixed. Thanks! Reply by wbracken on 21 November 2022, 14:23 > RESOLVED IN CODE WALKTHROUGH Revision Comment by wbracken on 21 November 2022, 10:41 https://devapps.diality.us/cru/DG-DEN-14197-1#c14868 Update header. Looks like header may have been incorrect before changes. Reply by Michael Garthwaite on 21 November 2022, 13:54 > Fixed. Thanks! Reply by wbracken on 21 November 2022, 14:23 > RESOLVED IN CODE WALKTHROUGH Revision Comment by wbracken on 21 November 2022, 10:41 https://devapps.diality.us/cru/DG-DEN-14197-1#c14869 Alignment. Reply by Michael Garthwaite on 21 November 2022, 13:54 > Fixed. Thanks! Reply by wbracken on 21 November 2022, 14:22 > RESOLVED IN CODE WALKTHROUGH Revision Comment by Sean Nash on 21 November 2022, 14:48 https://devapps.diality.us/cru/DG-DEN-14197-1#c15000 getROFeedbackVoltage() has been renamed, right? Reply by Michael Garthwaite on 21 November 2022, 14:55 > Correct. Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:56 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 November 2022, 14:32 https://devapps.diality.us/cru/DG-DEN-14197-1#c14992 Change voltage to duty cycle. Reply by Michael Garthwaite on 21 November 2022, 14:43 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 November 2022, 13:13 https://devapps.diality.us/cru/DG-DEN-14197-1#c14894 Variable suggests this is a duty cycle (not a voltage). Suggest renaming function. Reply by Michael Garthwaite on 21 November 2022, 13:46 > Testers requested the feature as feed back voltage. see > http://dvm-linux02:8080/browse/DIAL-212 > > The static variable name is roPumpFeedbackDutyCyclePct. We > get data from the INT_ADC_RO_PUMP_FEEDBACK_DUTY_CYCLE > register on the FPGA in execROPumpMonitor/ > > No issue renaming it but is it a voltage or a duty cycle? Reply by Sean Nash on 21 November 2022, 14:18 > Dara confirmed it is a duty cycle. We expect it to match > our commanded duty cycle for the pump. Reply by Michael Garthwaite on 21 November 2022, 14:27 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 November 2022, 13:14 https://devapps.diality.us/cru/DG-DEN-14197-1#c14895 Voltage mis-spelled. Should rename anyway per comment above. Reply by Michael Garthwaite on 21 November 2022, 14:27 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 November 2022, 13:17 https://devapps.diality.us/cru/DG-DEN-14197-1#c14897 Put parenthesis around inner conditions. Put constant on left side of "==". Reply by Michael Garthwaite on 21 November 2022, 13:56 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:13 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 November 2022, 13:21 https://devapps.diality.us/cru/DG-DEN-14197-1#c14901 Add another blank line here (separates normal code from test code). Reply by Michael Garthwaite on 21 November 2022, 13:56 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:12 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ROPump.h Revision Comment by Sean Nash on 21 November 2022, 13:09 https://devapps.diality.us/cru/DG-DEN-14197-1#c14893 Remove extra blank line. Reply by Michael Garthwaite on 21 November 2022, 13:56 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:17 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeFill.c Revision Comment by wbracken on 21 November 2022, 10:48 https://devapps.diality.us/cru/DG-DEN-14197-1#c14870 Input is integratedVolumeML Reply by Michael Garthwaite on 21 November 2022, 13:54 > Fixed. Thanks! Reply by wbracken on 21 November 2022, 14:21 > RESOLVED IN CODE WALKTHROUGH Revision Comment by wbracken on 21 November 2022, 10:49 https://devapps.diality.us/cru/DG-DEN-14197-1#c14871 Remove blank lines Reply by Michael Garthwaite on 21 November 2022, 13:54 > Fixed. Thanks! Reply by wbracken on 21 November 2022, 14:21 > RESOLVED IN CODE WALKTHROUGH Revision Comment by wbracken on 21 November 2022, 10:50 https://devapps.diality.us/cru/DG-DEN-14197-1#c14872 Should be using variable names? or is description OK? Reply by Sean Nash on 21 November 2022, 13:23 > If it's a global static variable, use variable name. If it's > accessed via another module's get function, description is > ok. Reply by wbracken on 21 November 2022, 15:37 > RESOLVED IN CODE WALKTHROUGH Revision Comment by wbracken on 21 November 2022, 10:53 https://devapps.diality.us/cru/DG-DEN-14197-1#c14873 Where is inactiveRsrv declared. Reply by Sean Nash on 21 November 2022, 13:27 > I think it's abbreviated - should be spelled out > (inactiveReservoir) I think. Reply by Michael Garthwaite on 21 November 2022, 13:54 > Fixed. Thanks! > > This complied with no issue... it was thinking > inactiveRsrvr was from the inactiveRsrvr within the > HEATER_STATUS_T data structure. Reply by wbracken on 21 November 2022, 14:20 > RESOLVED IN CODE WALKTHROUGH Reply by Sean Nash on 21 November 2022, 14:22 > This line of code is not referencing a field in a > structure. I do not see it declared anywhere. I've been > unable to build this branch (I think because there is no > fwcommon branch) so somebody may have fixed this in > develop. Reply by Michael Garthwaite on 21 November 2022, 14:32 > inactiveReservoir is the correct variable name. it is > defined at line 752 within this same function. > > In our current staging branch, this is named > inactiveRsrvr. In develop, it's inactiveReservoir. Note > that commit 2b6abbe does not show the change of name. > > Regardless, it compiled when merging the working branch > into develop. using crtl+f within the file shows that > no other variable has the name inactiveRsrvr in my > working branch. Using code composer's "Open > declaration" of the variable points to to this data > structure with this name. > > /// Heaters data structure > typedef struct > > F32 targetTemp; > ///< Heater target temperature. > HEATERS_STATE_T state; > ///< Heater state. > BOOL startHeaterSignal; > ///< Heater start indication flag. > BOOL isHeaterOn; > ///< Heater on/off status flag. > F32 dutyCycle; > ///< Heater duty cycle. > F32 targetFlow; > ///< Heater target flow. > BOOL hasTargetTempChanged; > ///< Heater target temperature change flag indicator. > F32 heaterEstGain; > ///< Heater estimation gain during the run. > BOOL hasTargetBeenReached; > ///< Heater flag to indicate whether the target > temperature has been reached. > F32 calculatedTemperature; > ///< Heater calculated temperature. > DG_RESERVOIR_ID_T inactiveRsrvr; > ///< Heater inactive reservoir. > HEATER_STATUS_T; > > in heaters.c > > > So this is an odd one. maybe something got mixed up in > a merge conflict? Reply by Sean Nash on 21 November 2022, 14:51 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 21 November 2022, 10:54 https://devapps.diality.us/cru/DG-DEN-14197-1#c14874 Update header. Reply by Michael Garthwaite on 21 November 2022, 13:56 > Fixed. Thanks! Reply by wbracken on 21 November 2022, 14:20 > RESOLVED IN CODE WALKTHROUGH Revision Comment by wbracken on 21 November 2022, 10:56 https://devapps.diality.us/cru/DG-DEN-14197-1#c14875 Update header. Reply by Michael Garthwaite on 21 November 2022, 13:56 > Fixed. Thanks! Reply by wbracken on 21 November 2022, 14:19 > RESOLVED IN CODE WALKTHROUGH Revision Comment by wbracken on 21 November 2022, 10:56 https://devapps.diality.us/cru/DG-DEN-14197-1#c14876 Update header. Reply by Michael Garthwaite on 21 November 2022, 13:56 > Fixed. Thanks! Reply by wbracken on 21 November 2022, 14:19 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Services/PIControllers.h Revision Comment by Sean Nash on 21 November 2022, 13:29 https://devapps.diality.us/cru/DG-DEN-14197-1#c14904 Remove extra blank line. Reply by Michael Garthwaite on 21 November 2022, 13:57 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:02 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 November 2022, 13:29 https://devapps.diality.us/cru/DG-DEN-14197-1#c14905 Remove extra blank line. Reply by Michael Garthwaite on 21 November 2022, 13:57 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:02 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 November 2022, 13:51 https://devapps.diality.us/cru/DG-DEN-14197-1#c14928 Remove extra blank line. Reply by Michael Garthwaite on 21 November 2022, 13:53 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:02 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeInitPOST.c Revision Comment by wbracken on 21 November 2022, 10:58 https://devapps.diality.us/cru/DG-DEN-14197-1#c14877 Update header. systemREG1 input and output. Reply by Michael Garthwaite on 21 November 2022, 13:56 > Fixed. Thanks! Reply by wbracken on 21 November 2022, 14:18 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: MsgDefs.h Revision Comment by Sean Nash on 21 November 2022, 14:36 https://devapps.diality.us/cru/DG-DEN-14197-1#c14993 I thought we changed this to include "DG" in the enum name. Why is it deleted again? Reply by Michael Garthwaite on 21 November 2022, 14:42 > Fixed. Thanks! Reply by Sean Nash on 21 November 2022, 14:44 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-14197-1 https://devapps.diality.us/cru/DG-DEN-14197-1 Title: DG-DEN-14197_SW S83 MG PI Optimizations Statement of Objectives: State: Closed Summary: Author: Michael Garthwaite Moderator: Michael Garthwaite Reviewers: (1 active, 4 completed*) Sean Nash (*) wbracken (*) Dara Navaei (*) jtaylor (*) Darren Cox