This is a list of all comments for HD-DEN-14000-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Modes/ModeStandby.c Revision Comment by Sean Nash on 07 October 2022, 10:04 https://devapps.diality.us/cru/HD-DEN-14000-1#c14148 Change to "0 == disinfectCancelReqID" Reply by Sean Nash on 13 October 2022, 23:23 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 06 October 2022, 15:47 https://devapps.diality.us/cru/HD-DEN-14000-1#c14139 GENERIC_CONFIGURE == disinfectCancelRegID Reply by Darren Cox on 14 October 2022, 11:36 > Fixed. Reply by wbracken on 14 October 2022, 15:19 > RESOLVED IN CODE WALKTHROUGH Revision Comment by Sean Nash on 07 October 2022, 10:34 https://devapps.diality.us/cru/HD-DEN-14000-1#c14158 I think getting here would maybe indicate flush/disinfect ended naturally or faulted out while waiting for confirmation. I think appropriate thing to do is to send a reject back to UI? No alarm. Reply by Sean Nash on 13 October 2022, 23:20 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Sean Nash on 07 October 2022, 10:18 https://devapps.diality.us/cru/HD-DEN-14000-1#c14150 This works, but convention has been to use a local byte ptr and increment between param memcpy calls (see other message handlers). Reply by Darren Cox on 10 October 2022, 10:34 > updated to use pointer. Reply by Sean Nash on 13 October 2022, 23:17 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 07 October 2022, 10:20 https://devapps.diality.us/cru/HD-DEN-14000-1#c14151 Should break out of loop here I think. Reply by Darren Cox on 10 October 2022, 10:35 > done. Reply by Sean Nash on 13 October 2022, 23:18 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 07 October 2022, 10:25 https://devapps.diality.us/cru/HD-DEN-14000-1#c14153 Convention is to ack when receiving one-shot (not broadcast) messages like this. Reply by Darren Cox on 10 October 2022, 10:35 > Comment removed, Ack kept. Reply by Sean Nash on 13 October 2022, 23:18 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 October 2022, 10:07 https://devapps.diality.us/cru/HD-DEN-14000-1#c14240 Can we assign request_type here at declaration instead of below? Reply by Sean Nash on 13 October 2022, 23:14 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 07 October 2022, 10:49 https://devapps.diality.us/cru/HD-DEN-14000-1#c14162 Convention is to put spaces around operators like = and <. Reply by Darren Cox on 10 October 2022, 10:36 > Updated. Reply by Sean Nash on 13 October 2022, 23:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 07 October 2022, 10:29 https://devapps.diality.us/cru/HD-DEN-14000-1#c14155 Should add a break from loop here. Reply by Darren Cox on 10 October 2022, 10:36 > Done. Reply by Sean Nash on 13 October 2022, 23:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 07 October 2022, 10:30 https://devapps.diality.us/cru/HD-DEN-14000-1#c14156 Convention is to put spaces around operators like = and <. Reply by Darren Cox on 10 October 2022, 10:36 > Updated. Reply by Sean Nash on 13 October 2022, 23:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 07 October 2022, 10:48 https://devapps.diality.us/cru/HD-DEN-14000-1#c14161 request_id is an enum, not a U32. Copy enum to a local U32, then memcpy the local U32. Probably same for other params below - please check. Reply by Darren Cox on 10 October 2022, 10:37 > Updated. Reply by Sean Nash on 13 October 2022, 23:20 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.h Revision Comment by Sean Nash on 12 October 2022, 10:03 https://devapps.diality.us/cru/HD-DEN-14000-1#c14235 Add // MSG_ID_... comment above prototype. Reply by Sean Nash on 13 October 2022, 23:11 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 07 October 2022, 10:12 https://devapps.diality.us/cru/HD-DEN-14000-1#c14149 Somewhere around line 450 there is a separator between normal and Dialin test support functions. These new functions are normal so should be above that separator. Looks like there's a few more normal functions down here too that should be moved. Reply by Sean Nash on 13 October 2022, 23:22 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/BloodFlow.c Revision Comment by Sean Nash on 07 October 2022, 09:37 https://devapps.diality.us/cru/HD-DEN-14000-1#c14142 Why don't we want to distinguish forward and reverse speeds with sign? Reply by Darren Cox on 10 October 2022, 10:55 > Rotor and motor turn in opposite directions. This ensures > true check of motor and rotor speed, regardless of direction. > Also, measMotorSpeed is absolute value in this function. > Direction check is done in a different function. Reply by Sean Nash on 13 October 2022, 23:28 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DialInFlow.c Revision Comment by Sean Nash on 07 October 2022, 09:43 https://devapps.diality.us/cru/HD-DEN-14000-1#c14144 Should add a comment for this (and maybe a #define). Looks like this value will prevail if measured RPM is zero so would expect infinite persist I guess - though shouldn't be calling this function when pump is stopped I would think. Should probably be 8 Fs (not 7). Reply by Darren Cox on 10 October 2022, 12:43 > Updated to define, added comment. Reply by Sean Nash on 13 October 2022, 23:27 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 07 October 2022, 09:58 https://devapps.diality.us/cru/HD-DEN-14000-1#c14145 Why absolute? If can't handle negative speeds, maybe should floor at zero instead? Reply by Darren Cox on 10 October 2022, 10:59 > Rotor and motor turn in opposite directions. This ensures > true check of motor and rotor speed, regardless of direction. > Also, measMotorSpeed is absolute value in this function. > Direction check is done in a different function. Reply by Sean Nash on 13 October 2022, 23:26 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DialOutFlow.c Revision Comment by Sean Nash on 07 October 2022, 10:02 https://devapps.diality.us/cru/HD-DEN-14000-1#c14146 Why absolute value? Reply by Darren Cox on 10 October 2022, 11:00 > Rotor and motor turn in opposite directions. This ensures > true check of motor and rotor speed, regardless of direction. > Also, measMotorSpeed is absolute value in this function. > Direction check is done in a different function. Reply by Sean Nash on 13 October 2022, 23:26 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: MsgDefs.h Revision Comment by Sean Nash on 06 October 2022, 10:46 https://devapps.diality.us/cru/HD-DEN-14000-1#c14133 Is this request for DG only? If so, rename enum to MSG_ID_REQUEST_DG_CPLD_STATUS. Reply by Darren Cox on 10 October 2022, 12:41 > Updated. Reply by Sean Nash on 13 October 2022, 23:29 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: HDDefs.h Revision Comment by Sean Nash on 07 October 2022, 09:35 https://devapps.diality.us/cru/HD-DEN-14000-1#c14141 Is this reject from HD or user or both? Comment suggests from HD only. Reply by Darren Cox on 07 October 2022, 15:04 > This enum is HD to UI only. Updated comments. Reply by Sean Nash on 14 October 2022, 17:50 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/OperationModes.c Revision Comment by Sean Nash on 12 October 2022, 09:59 https://devapps.diality.us/cru/HD-DEN-14000-1#c14231 Should say "private definitions". Reply by Sean Nash on 13 October 2022, 23:15 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 October 2022, 09:59 https://devapps.diality.us/cru/HD-DEN-14000-1#c14232 Remove extra blank line. Reply by Sean Nash on 13 October 2022, 23:10 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 October 2022, 10:00 https://devapps.diality.us/cru/HD-DEN-14000-1#c14233 Add a /// doxygen comment above this new declaration. Reply by Sean Nash on 13 October 2022, 23:10 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/OperationModes.h Revision Comment by Sean Nash on 12 October 2022, 09:58 https://devapps.diality.us/cru/HD-DEN-14000-1#c14230 Remove extra blank line. Reply by Sean Nash on 13 October 2022, 23:10 > RESOLVED in CODE WALKTHROUGH. --- ID: HD-DEN-14000-1 https://devapps.diality.us/cru/HD-DEN-14000-1 Title: HD-DEN-14000_SW Dev Sprint 80 Darren Statement of Objectives: State: Closed Summary: Author: Darren Cox Moderator: Darren Cox Reviewers: (2 active, 2 completed*) Sean Nash (*) wbracken (*) Dara Navaei Behrouz NematiPour