This is a list of all comments for HD-DEN-14170-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/SyringePump.c Revision Comment by wbracken on 03 November 2022, 13:41 https://devapps.diality.us/cru/HD-DEN-14170-1#c14731 0.0F Reply by Darren Cox on 04 November 2022, 15:26 > Per Sean - "No harm in adding "F" suffix, but not necessary > for assignment to a float variable". Will leave as is for > float vars. Reply by wbracken on 07 November 2022, 10:26 > RESOLVED IN CODE WALKTHROUGH Revision Comment by Sean Nash on 04 November 2022, 11:25 https://devapps.diality.us/cru/HD-DEN-14170-1#c14760 I think this should probably be reset elsewhere. Init function and on exit of retract maybe? Reply by Sean Nash on 04 November 2022, 16:23 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 November 2022, 11:28 https://devapps.diality.us/cru/HD-DEN-14170-1#c14761 I would think this alarm would be triggered generally by driver because current mode said we shouldn't see syringe yet. Can we just set stopPump to TRUE here and nothing else? Reply by Darren Cox on 04 November 2022, 15:22 > Per our discussion, this is the proper place to set the > alarm. But will set stopPump = TRUE and add ELSE condition > for move and check move conditions. Reply by Sean Nash on 04 November 2022, 16:22 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 November 2022, 11:57 https://devapps.diality.us/cru/HD-DEN-14170-1#c14764 Ok to calculate here (this is called before UI needs it I assume). I was expecting this to be pre-calculated on exit of treatment params mode or on entry to pre-treatment mode when we have what we need to do the calculation. Reply by Darren Cox on 04 November 2022, 14:27 > I attempted to do this earlier in the process, but > getTreatmentParameterF32() caused a fault when called too > early (treat params not ready?).. So I kept it here after > determining it was before UI needed it. Reply by Sean Nash on 04 November 2022, 15:51 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 November 2022, 16:21 https://devapps.diality.us/cru/HD-DEN-14170-1#c14783 I think the else can end right after ramp. Ok to do checks I would think. Reply by Darren Cox on 08 November 2022, 12:17 > Updated else only with rampSyringePump(). Reply by Sean Nash on 08 November 2022, 12:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 03 November 2022, 13:42 https://devapps.diality.us/cru/HD-DEN-14170-1#c14733 0.0F Reply by Darren Cox on 04 November 2022, 15:24 > Per Sean - "No harm in adding "F" suffix, but not necessary > for assignment to a float variable". Will leave as is for > float vars. Reply by wbracken on 07 November 2022, 10:25 > RESOLVED IN CODE WLAKTHROUGH Revision Comment by Sean Nash on 04 November 2022, 11:32 https://devapps.diality.us/cru/HD-DEN-14170-1#c14762 We probably don't need this flag at this point, but should we reset this flag on exit of retract (after treatment done)? Reply by Darren Cox on 04 November 2022, 14:41 > It is reset on Retract / Home completion. Once the Seek > occurs, it is no longer valid so I reset it here on Seek > completion.SelfTest.c uses this flag in progression from > Retract to Preload to Seek to Prime. . Reply by Sean Nash on 04 November 2022, 16:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 03 November 2022, 13:42 https://devapps.diality.us/cru/HD-DEN-14170-1#c14734 0.0F Reply by Sean Nash on 04 November 2022, 11:36 > No harm in adding "F" suffix, but not necessary for > assignment to a float variable - assignment statement is > inherently typed. We need the "F" suffix where typing is not > assured (e.g. #define where we don't know how it will be > used). Reply by wbracken on 04 November 2022, 12:09 > Thought we had decided that the standard would be to add > the F? Reply by Sean Nash on 04 November 2022, 16:25 > If we want to do that for consistency, that's fine but > let's handle that as a bulk update and going forward kind > of thing rather than keep commenting individually on code > reviews. Reply by wbracken on 07 November 2022, 10:25 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Modes/SelfTests.c Revision Comment by Sean Nash on 04 November 2022, 11:16 https://devapps.diality.us/cru/HD-DEN-14170-1#c14758 Probably could just be if ( TRUE == isSyringePumpPreLoaded() ). Reply by Sean Nash on 04 November 2022, 16:23 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Behrouz NematiPour on 05 November 2022, 12:12 https://devapps.diality.us/cru/HD-DEN-14170-1#c14790 I know all the FW code is written in this style but it is always interesting to me, 1 - why don't just say {code} if ( isSyringePumpPreLoaded() ) {code} the function is a boolean why compare to TRUE to get a bool expression? if it was returning an int it made sense to have something like this: {code} if ( isSyringePumpPreLoaded() != 0 ) {code} Which is still unnecessary in some cases. 2 - even if we want to use TRUE, why to put it at the beginning of the expression and don't put it at the end, like {code} if ( isSyringePumpPreLoaded() == TRUE ) {code} So the first thing developer sees would be the important part which is the function not always a TRUE or FALSE. Reply by Sean Nash on 08 November 2022, 08:43 > It is not technically a boolean - it is a BOOL which is our > implementation of a boolean (TRUE = 1, FALSE = 0). > If we don't explicitly compare a BOOL to TRUE, compiler will > consider any non-zero value to be TRUE. So if memory is > overwritten/corrupted to a random non-zero value, it would > satisfy "if ( isSyringePumpPreLoaded() )" and s.w would do > the wrong thing. > For extra safety, we prefer to compare to TRUE (1) or FALSE > (0) depending on what would be safer. > For example, in this case we are deciding whether to seek > plunger or alarm. We only seek plunger if the syringe pump > was pre-loaded. It is safer to check pre-loaded flag is TRUE > (1) to seek and ANY OTHER VALUE would alarm. > Also, when using the "==" compare operator, we will put the > literal first. It looks awkward, but if we accidently use > the "=" operator by mistake the compiler will give us a > syntax error which saves us from having a tricky bug to fix > later. Reply by Behrouz NematiPour on 08 November 2022, 10:06 > That was one the best explanations I have ever gotten about > one of my confusions. > I should have asked this a long time ago. > Thanks a lot, Sean. > > RESOLVED in CODE WALKTHROUGH. --- ID: HD-DEN-14170-1 https://devapps.diality.us/cru/HD-DEN-14170-1 Title: HD-DEN-14170_SW Dev Sprint 82 Darren Statement of Objectives: State: Closed Summary: Author: Darren Cox Moderator: Darren Cox Reviewers: (2 active, 3 completed*) Sean Nash (*) wbracken (*) Behrouz NematiPour (*) Michael Garthwaite Dara Navaei