This is a list of all comments for HD-DEN-12847-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Modes/Dialysis.c Revision Comment by Sean Nash on 30 June 2022, 09:19 https://devapps.diality.us/cru/HD-DEN-12847-1#c13199 Use FALSE, not false. Reply by Darren Cox on 30 June 2022, 14:43 > Updated Reply by Sean Nash on 01 July 2022, 10:12 > Function no longer takes a boolean parameter. Reply by Sean Nash on 01 July 2022, 10:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 June 2022, 09:18 https://devapps.diality.us/cru/HD-DEN-12847-1#c13198 Use TRUE (not true). Reply by Darren Cox on 30 June 2022, 14:44 > Update Reply by Sean Nash on 01 July 2022, 10:13 > Function no longer takes a boolean parameter. Reply by Sean Nash on 01 July 2022, 10:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 June 2022, 09:44 https://devapps.diality.us/cru/HD-DEN-12847-1#c13207 Maybe change param name to final (i.e. final or start). Reply by Sean Nash on 01 July 2022, 09:53 > Issue is moot with re-factoring of function. Reply by Sean Nash on 01 July 2022, 10:29 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Michael Garthwaite on 28 June 2022, 10:09 https://devapps.diality.us/cru/HD-DEN-12847-1#c13178 This #define should be at the top of the file. Reply by Dara Navaei on 13 July 2022, 10:48 > Done. Reply by Michael Garthwaite on 13 July 2022, 16:23 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 June 2022, 09:22 https://devapps.diality.us/cru/HD-DEN-12847-1#c13200 It's not so much empty and full - neither is empty - it's more like start and final weights. Consider renaming these. Also, consider moving these declarations to top of module. Nothing wrong with them being here, but the style for this code base has been to declare everything at top of module. Reply by Sean Nash on 01 July 2022, 10:12 > Function re-factor makes this issue moot. Reply by Sean Nash on 01 July 2022, 10:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 June 2022, 09:41 https://devapps.diality.us/cru/HD-DEN-12847-1#c13206 I think we can handle both reservoirs with same code (no need for else). Probably just need to add a macro somewhere that converts a reservoir ID and primary/backup selection into a loadcell ID. Reply by Darren Cox on 30 June 2022, 14:46 > Added index variable and removed duplication of code. Reply by Sean Nash on 01 July 2022, 10:29 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 June 2022, 09:12 https://devapps.diality.us/cru/HD-DEN-12847-1#c13195 Is fabs() appropriate here? Reply by Darren Cox on 30 June 2022, 14:44 > No. Removed. Reply by Sean Nash on 01 July 2022, 10:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 June 2022, 09:33 https://devapps.diality.us/cru/HD-DEN-12847-1#c13201 Do an explicit condition here. i.e. if ( TRUE == reservoirFull ) Reply by Darren Cox on 30 June 2022, 14:47 > updated. Reply by Sean Nash on 01 July 2022, 10:12 > Function re-factor makes this issue moot. Reply by Sean Nash on 01 July 2022, 10:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 June 2022, 09:13 https://devapps.diality.us/cru/HD-DEN-12847-1#c13196 Is fabs() appropriate here? Reply by Darren Cox on 30 June 2022, 14:47 > No. Removed. Reply by Sean Nash on 01 July 2022, 10:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 June 2022, 09:39 https://devapps.diality.us/cru/HD-DEN-12847-1#c13205 I think this whole condition should be removed. If we are taking baseline reading for a reservoir, it is not time to do the drift check yet. Just need to save the readings here. Reply by Sean Nash on 01 July 2022, 10:10 > Function re-factor makes this issue moot. Reply by Sean Nash on 01 July 2022, 10:29 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 June 2022, 09:37 https://devapps.diality.us/cru/HD-DEN-12847-1#c13202 I think this alarm should be an HD fault for now (no recovery). Reply by Darren Cox on 30 June 2022, 14:18 > Per conversation with Sean: It is a DG Fault generated that > the HD is identifying. Reply by Sean Nash on 01 July 2022, 10:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 June 2022, 09:37 https://devapps.diality.us/cru/HD-DEN-12847-1#c13203 Remove blank line. Reply by Dara Navaei on 18 October 2023, 21:26 > RESOLVED in CODE WALKTHROUGH Revision Comment by Sean Nash on 30 June 2022, 09:37 https://devapps.diality.us/cru/HD-DEN-12847-1#c13204 Remove extra blank line. Reply by Dara Navaei on 13 July 2022, 10:47 > Done. Reply by Sean Nash on 13 July 2022, 16:24 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DGInterface.c Revision Comment by wbracken on 30 June 2022, 12:44 https://devapps.diality.us/cru/HD-DEN-12847-1#c13209 General comment: Should floating point values be of the format N.NF? What is the coding standard? My experience has been that it's best to always specify the "F". Reply by Sean Nash on 13 July 2022, 16:21 > General comment - nothing to be done. Generally only need F > suffix on #defines as they are not inherently typed. > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: AlarmDefs.h Revision Comment by Sean Nash on 30 June 2022, 10:01 https://devapps.diality.us/cru/HD-DEN-12847-1#c13208 This has the rank of a fault and I think it should be an HD fault - so set the T/F properties same as the alarm above (FPGA clock speed check). Reply by Darren Cox on 30 June 2022, 14:13 > Leave as is per conversation w Sean. Reported by HD, but > actually a DG generated problem. Reply by Sean Nash on 01 July 2022, 10:29 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/SyringePump.c Revision Comment by Sean Nash on 13 July 2022, 10:25 https://devapps.diality.us/cru/HD-DEN-12847-1#c13289 Remove blank line. Reply by Dara Navaei on 13 July 2022, 10:46 > Done. Reply by Sean Nash on 13 July 2022, 16:25 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/Rinseback.c Revision Comment by Sean Nash on 13 July 2022, 10:31 https://devapps.diality.us/cru/HD-DEN-12847-1#c13290 I don't think this does what we want anymore. Need to revisit VC issue. Reply by Dara Navaei on 13 July 2022, 10:46 > Done. Reply by Sean Nash on 13 July 2022, 16:22 > RESOLVED in CODE WALKTHROUGH. --- ID: HD-DEN-12847-1 https://devapps.diality.us/cru/HD-DEN-12847-1 Title: HD-DEN-12847_SW Dev Sprint 71 Darren Statement of Objectives: State: Closed Summary: Author: Darren Cox Moderator: Darren Cox Reviewers: (3 active, 2 completed*) Sean Nash (*) Dara Navaei (*) Michael Garthwaite wbracken Dong Nguyen