This is a list of all comments for DG-DEN-11750-1. Review Summary: No summary General Comment by Michael Garthwaite on 22 February 2022, 15:57 https://devapps.diality.us/cru/DG-DEN-11750-1#c12014 *Notices the 170 files that need to be reviewed* https://c.tenor.com/RXMxqsRKEn0AAAAC/this-is-fine.gif General Comment by Dara Navaei on 22 February 2022, 16:01 https://devapps.diality.us/cru/DG-DEN-11750-1#c12015 Most of the files are VectorCAST that has been brought back from Code Clinic. Look at the files but they are auto generated so they are mostly passing through. ---------------------------------------- File: firmware/App/Controllers/Heaters.c Revision Comment by Sean Nash on 02 March 2022, 08:49 https://devapps.diality.us/cru/DG-DEN-11750-1#c12203 Seems like this should be an alarm. Reply by Dara Navaei on 02 March 2022, 10:22 > I will implement it in DEN-12224. Reply by Sean Nash on 03 March 2022, 13:34 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Sean Nash on 25 February 2022, 10:56 https://devapps.diality.us/cru/DG-DEN-11750-1#c12084 Should we check that payloadLength is at least 12 bytes (sizeof(U32) x 3)? Reply by Dara Navaei on 02 March 2022, 13:10 > Done. Reply by Sean Nash on 03 March 2022, 13:43 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ConductivitySensors.c Revision Comment by Michael Garthwaite on 22 February 2022, 17:06 https://devapps.diality.us/cru/DG-DEN-11750-1#c12029 All the new timer counters no longer are initialized to 0. I understand that they get set in their respective init()'s to the timer counter, but was this intentional? Reply by Dara Navaei on 25 February 2022, 18:31 > Yes, if they are initialized in init() which they should be, > they don't need to be initialized here. This puts this static > variables in the uninitialized section of the memory. Reply by Michael Garthwaite on 03 March 2022, 13:17 > RESOVLED IN CODE WALKTHROUGH. Reply by Dara Navaei on 18 October 2023, 21:31 > RESOLVED in CODE WALKTHROUGH Revision Comment by Michael Garthwaite on 22 February 2022, 17:12 https://devapps.diality.us/cru/DG-DEN-11750-1#c12030 If the DG is broadcasting these raw values, shouldn't Dialin be sync'd as well to handle these new fields? Reply by Dara Navaei on 25 February 2022, 18:30 > This might have been updated in the different Dialin branch. Reply by Michael Garthwaite on 03 March 2022, 13:22 > RESOVLED IN CODE WALKTHROUGH. Reply by Dara Navaei on 18 October 2023, 21:31 > RESOLVED in CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Controllers/Fans.c Revision Comment by Michael Garthwaite on 22 February 2022, 17:16 https://devapps.diality.us/cru/DG-DEN-11750-1#c12031 Could we move these fields outside of the for loop? They dont need to be initialized NUM_OF_FANS_NAMES times. Reply by Dara Navaei on 25 February 2022, 18:29 > Good catch, thanks. Reply by Michael Garthwaite on 03 March 2022, 13:15 > RESOVLED IN CODE WALKTHROUGH. Reply by Dara Navaei on 19 October 2023, 09:30 > RESOLVED in CODE WALKTHROUGH Revision Comment by Sean Nash on 02 March 2022, 10:50 https://devapps.diality.us/cru/DG-DEN-11750-1#c12206 Should this happen in the previous if scope to simplify? Reply by Dara Navaei on 02 March 2022, 15:02 > Done. Reply by Sean Nash on 03 March 2022, 13:34 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/LoadCell.c Revision Comment by Michael Garthwaite on 24 February 2022, 15:17 https://devapps.diality.us/cru/DG-DEN-11750-1#c12071 I think this alarm should be not commented out. Reply by Dara Navaei on 25 February 2022, 18:26 > The alarm is not commented out currently, this might be due > to an old commit. Reply by Michael Garthwaite on 03 March 2022, 13:15 > RESOVLED IN CODE WALKTHROUGH. Reply by Dara Navaei on 18 October 2023, 21:32 > RESOLVED in CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Modes/ModeChemicalDisinfect.c Revision Comment by Sean Nash on 01 March 2022, 10:49 https://devapps.diality.us/cru/DG-DEN-11750-1#c12159 ? Reply by Dara Navaei on 02 March 2022, 08:05 > This will be addressed once we were working on deciding the > number of rinses. Reply by Sean Nash on 03 March 2022, 13:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 01 March 2022, 10:49 https://devapps.diality.us/cru/DG-DEN-11750-1#c12160 Any reason why this is still a TODO? Reply by Dara Navaei on 02 March 2022, 08:05 > This will be addressed once we were working on deciding the > number of rinses. Reply by Sean Nash on 03 March 2022, 13:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 01 March 2022, 10:50 https://devapps.diality.us/cru/DG-DEN-11750-1#c12161 Concentrate pumps are available, right? Reply by Dara Navaei on 02 March 2022, 08:05 > This will be addressed once we were working on deciding the > number of rinses. Reply by Sean Nash on 03 March 2022, 13:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 01 March 2022, 10:51 https://devapps.diality.us/cru/DG-DEN-11750-1#c12162 Any reason why this is still a TODO? Reply by Dara Navaei on 02 March 2022, 08:04 > This will be addressed once we were working on deciding the > number of rinses. Reply by Sean Nash on 03 March 2022, 13:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 01 March 2022, 10:51 https://devapps.diality.us/cru/DG-DEN-11750-1#c12163 ? Reply by Dara Navaei on 02 March 2022, 08:02 > Done. Reply by Sean Nash on 03 March 2022, 13:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 01 March 2022, 10:52 https://devapps.diality.us/cru/DG-DEN-11750-1#c12164 Any reason why this declaration isn't at top of function? Reply by Dara Navaei on 02 March 2022, 08:00 > Done. Reply by Sean Nash on 03 March 2022, 13:37 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 01 March 2022, 10:52 https://devapps.diality.us/cru/DG-DEN-11750-1#c12165 Remove blank line. Reply by Dara Navaei on 02 March 2022, 07:57 > Done. Reply by Sean Nash on 03 March 2022, 13:40 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeHeatDisinfect.c Revision Comment by Michael Garthwaite on 24 February 2022, 15:33 https://devapps.diality.us/cru/DG-DEN-11750-1#c12073 Do we need an if for an always true condition? does this branch statement need to be uncommented? Reply by Dara Navaei on 25 February 2022, 18:25 > This was commented for testing. I un-commented it. Reply by Michael Garthwaite on 03 March 2022, 13:20 > RESOVLED IN CODE WALKTHROUGH. Reply by Dara Navaei on 18 October 2023, 21:32 > RESOLVED in CODE WALKTHROUGH Revision Comment by Sean Nash on 25 February 2022, 14:24 https://devapps.diality.us/cru/DG-DEN-11750-1#c12087 Looks like this needs to be addressed. Reply by Dara Navaei on 25 February 2022, 18:12 > Done. Reply by Sean Nash on 03 March 2022, 13:42 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Reservoirs.c Revision Comment by Sean Nash on 25 February 2022, 10:58 https://devapps.diality.us/cru/DG-DEN-11750-1#c12085 rpm should be in (). Reply by Dara Navaei on 25 February 2022, 18:13 > This macro has been removed. Reply by Sean Nash on 03 March 2022, 13:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Michael Garthwaite on 24 February 2022, 15:36 https://devapps.diality.us/cru/DG-DEN-11750-1#c12074 Does this need to be memset when we can initialize it with 0'd values above? (line 92) Reply by Dara Navaei on 25 February 2022, 18:24 > You cannot run a function there. Also, the init function is > the proper place to initialize the value. Reply by Michael Garthwaite on 03 March 2022, 13:20 > RESOVLED IN CODE WALKTHROUGH. Reply by Dara Navaei on 18 October 2023, 21:33 > RESOLVED in CODE WALKTHROUGH Revision Comment by Sean Nash on 25 February 2022, 11:02 https://devapps.diality.us/cru/DG-DEN-11750-1#c12086 Magic #. Reply by Dara Navaei on 25 February 2022, 18:12 > Done. Reply by Sean Nash on 03 March 2022, 13:43 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DialysateFlow.c Revision Comment by Michael Garthwaite on 22 February 2022, 17:05 https://devapps.diality.us/cru/DG-DEN-11750-1#c12028 We should probably keep the = 0; Reply by Dara Navaei on 25 February 2022, 18:33 > The variable is initialized in the init(). Reply by Michael Garthwaite on 03 March 2022, 13:18 > RESOVLED IN CODE WALKTHROUGH. Reply by Dara Navaei on 18 October 2023, 21:33 > RESOLVED in CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Controllers/TemperatureSensors.c Revision Comment by Sean Nash on 01 March 2022, 11:03 https://devapps.diality.us/cru/DG-DEN-11750-1#c12168 Do we need to fix this? Reply by Dara Navaei on 02 March 2022, 07:57 > I will test this in DEN-12224. Reply by Sean Nash on 03 March 2022, 13:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/UVReactors.c Revision Comment by Michael Garthwaite on 24 February 2022, 15:25 https://devapps.diality.us/cru/DG-DEN-11750-1#c12072 Shouldn't the override check be its own get function like other overidden properties? Ex: getUVReactorHealth() ? Reply by Sean Nash on 01 March 2022, 10:57 > This doesn't look like a get - status is being set. I don't > think we should be setting .ovData here at all - should only > be set via an override cmd message. I think .data should > always be set here (no if). Reply by Dara Navaei on 02 March 2022, 11:16 > You are right. I removed the code. Reply by Michael Garthwaite on 03 March 2022, 13:21 > RESOVLED IN CODE WALKTHROUGH. Reply by Dara Navaei on 18 October 2023, 21:34 > RESOLVED in CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Services/AlarmMgmt.c Revision Comment by Sean Nash on 02 March 2022, 10:33 https://devapps.diality.us/cru/DG-DEN-11750-1#c12205 Who's right? Reply by Dara Navaei on 02 March 2022, 13:13 > I brought the BOOL back. Reply by Sean Nash on 03 March 2022, 13:34 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: NVDataMgmt.c Revision Comment by Sean Nash on 25 February 2022, 10:42 https://devapps.diality.us/cru/DG-DEN-11750-1#c12083 There are 2 functions in this module with this name. Reply by Dara Navaei on 26 February 2022, 12:15 > Yes that is true. One is specifically looking at the HD > software config and the other is checking the DG software > config. I can have one function that covers both but it has > to have #ifdef for for HD and DG. Reply by Sean Nash on 01 March 2022, 10:42 > Yes, I think we should do that Reply by Dara Navaei on 03 March 2022, 13:46 > I will address them in DEN-12224. Reply by Sean Nash on 03 March 2022, 13:47 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: NVDataMgmt.h Revision Comment by Sean Nash on 25 February 2022, 08:52 https://devapps.diality.us/cru/DG-DEN-11750-1#c12081 Add #ifndef _RELEASE_ around these definitions and all other build switch related code. Reply by Dara Navaei on 25 February 2022, 18:15 > Done. Reply by Sean Nash on 03 March 2022, 13:46 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: NVDataMgmtDGRecords.h Revision Comment by Sean Nash on 25 February 2022, 08:51 https://devapps.diality.us/cru/DG-DEN-11750-1#c12079 Add #ifndef _RELEASE_ around this enum. Reply by Dara Navaei on 26 February 2022, 12:21 > Done. Reply by Sean Nash on 03 March 2022, 13:47 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 25 February 2022, 08:51 https://devapps.diality.us/cru/DG-DEN-11750-1#c12080 Add #ifndef _RELEASE_ around this structure. Reply by Dara Navaei on 25 February 2022, 18:16 > Done. Reply by Sean Nash on 03 March 2022, 13:47 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: NVDataMgmtHDRecords.h Revision Comment by Sean Nash on 25 February 2022, 08:49 https://devapps.diality.us/cru/DG-DEN-11750-1#c12077 I think there should be a #ifndef _RELEASE_ around these build switches. Reply by Dara Navaei on 25 February 2022, 18:22 > Done. Reply by Sean Nash on 03 March 2022, 13:48 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 25 February 2022, 08:50 https://devapps.diality.us/cru/DG-DEN-11750-1#c12078 Should be #ifndef _RELEASE_ around this structure. Reply by Dara Navaei on 25 February 2022, 18:22 > Done. Reply by Sean Nash on 03 March 2022, 13:48 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: Common.h Revision Comment by Sean Nash on 25 February 2022, 08:56 https://devapps.diality.us/cru/DG-DEN-11750-1#c12082 Are these really common? Can they be moved to sys_selftest.c? Reply by Sean Nash on 03 March 2022, 13:51 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: Compatible.h Revision Comment by Sean Nash on 25 February 2022, 08:46 https://devapps.diality.us/cru/DG-DEN-11750-1#c12075 Dara, I keep seeing this change in every code review - Compatibility.h is deleted and Compatible.h is created. Why do we keep seeing this? Reply by Dara Navaei on 25 February 2022, 18:23 > Yes because the staging is branched form master that is very > old. At the end of this code review I will update the master > branch of common. Reply by Sean Nash on 03 March 2022, 13:48 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-11750-1 https://devapps.diality.us/cru/DG-DEN-11750-1 Title: DG-DEN-11750_DG Dev Dialysate Temperature Control Tune Up Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (2 active, 2 completed*) Sean Nash (*) Michael Garthwaite (*) hnguyen Behrouz NematiPour