This is a list of all comments for DG-DEN-9054-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/ConductivitySensors.c Revision Comment by Sean Nash on 27 July 2021, 16:16 https://devapps.diality.us/cru/DG-DEN-9054-1#c10469 I like to add a comment to right of system library includes explaining why they are needed (i.e. which math functions are you using in this module). Reply by Dara Navaei on 28 July 2021, 20:09 > Done. Reply by Sean Nash on 29 July 2021, 13:16 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DrainPump.c Revision Comment by qnguyen on 29 July 2021, 12:14 https://devapps.diality.us/cru/DG-DEN-9054-1#c10493 Should this function move to ModeRecirculate? It does not seem to contain any information related to drain pump. Reply by Dara Navaei on 29 July 2021, 16:25 > This function is called during POST to get all the pertaining > calibration data but since the modes do not have have POST > states, it was decided to use DrainPump.c to get the > calibration information. Reply by qnguyen on 30 July 2021, 10:37 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 29 July 2021, 12:13 https://devapps.diality.us/cru/DG-DEN-9054-1#c10492 What happens when calibration data is invalid? Reply by Dara Navaei on 29 July 2021, 16:26 > If the calibration data is invalid, the function will raise > an alarm. But the all the calibration records that fail CRC > test will be in benign values by NVDataMgmt.c so the benign > value is taken as the target flush volume. Reply by qnguyen on 30 July 2021, 10:37 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ROPump.c Revision Comment by Sean Nash on 27 July 2021, 17:04 https://devapps.diality.us/cru/DG-DEN-9054-1#c10471 I don't see this being initialized in init function. Reply by Dara Navaei on 28 July 2021, 20:07 > The calibration structures are populated during POST. The > initialization will be addressed in DEN-9480. Reply by Sean Nash on 02 August 2021, 10:32 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeRecirculate.c Revision Comment by Sean Nash on 26 July 2021, 10:31 https://devapps.diality.us/cru/DG-DEN-9054-1#c10448 Should we close both drain valves VRd1 and VRd2 here? Reply by Dara Navaei on 29 July 2021, 16:38 > Done. Reply by Sean Nash on 30 July 2021, 10:32 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/AlarmMgmt.h Revision Comment by qnguyen on 29 July 2021, 12:09 https://devapps.diality.us/cru/DG-DEN-9054-1#c10491 The comment is out-date and need to be updated. Reply by Dara Navaei on 29 July 2021, 16:33 > Done. Reply by qnguyen on 30 July 2021, 10:37 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Reservoirs.c Revision Comment by Sean Nash on 26 July 2021, 10:24 https://devapps.diality.us/cru/DG-DEN-9054-1#c10447 I think other fill volumes would come from HD, so we can remove this TODO. Reply by Dara Navaei on 28 July 2021, 20:10 > These fill volumes will come from non-volatile memory. I also > removed the TODOs. Reply by Sean Nash on 30 July 2021, 10:32 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by qnguyen on 29 July 2021, 12:05 https://devapps.diality.us/cru/DG-DEN-9054-1#c10489 This can fit in one line. Reply by Dara Navaei on 29 July 2021, 16:35 > Done. Reply by qnguyen on 30 July 2021, 10:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 03 August 2021, 09:33 https://devapps.diality.us/cru/DG-DEN-9054-1#c10588 Looks like the parameter is not being used. Reply by Dara Navaei on 03 August 2021, 09:43 > Done. Reply by qnguyen on 04 August 2021, 10:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 03 August 2021, 09:34 https://devapps.diality.us/cru/DG-DEN-9054-1#c10589 Unused parameter. Reply by Dara Navaei on 03 August 2021, 09:42 > Done. Reply by qnguyen on 04 August 2021, 10:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 29 July 2021, 12:05 https://devapps.diality.us/cru/DG-DEN-9054-1#c10490 Param name is not matched. Reply by Dara Navaei on 29 July 2021, 16:34 > Fixed it. Reply by qnguyen on 30 July 2021, 10:37 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeChemicalDisinfect.h Revision Comment by Sean Nash on 27 July 2021, 17:50 https://devapps.diality.us/cru/DG-DEN-9054-1#c10474 Where did this go? Reply by Dara Navaei on 28 July 2021, 20:03 > This structure was transferred DGDefs.h Reply by Sean Nash on 29 July 2021, 13:12 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: NVDataMgmt.c Revision Comment by Sean Nash on 26 July 2021, 10:10 https://devapps.diality.us/cru/DG-DEN-9054-1#c10443 I think these @param descriptions could fit in one line for a cleaner look. Reply by Dara Navaei on 29 July 2021, 16:40 > Done. Reply by Sean Nash on 30 July 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 03 August 2021, 09:27 https://devapps.diality.us/cru/DG-DEN-9054-1#c10587 maxBufferLength variable is not needed in this case and the comment is not matched the code. Suggest revert this change to line 2139. Reply by Dara Navaei on 03 August 2021, 09:48 > Fixed the maxBufferLength variable. Reply by qnguyen on 04 August 2021, 10:38 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: Accel.c Revision Comment by Sean Nash on 26 July 2021, 10:17 https://devapps.diality.us/cru/DG-DEN-9054-1#c10445 Please restore the vector length test. Reply by Dara Navaei on 28 July 2021, 20:44 > Done. Reply by Sean Nash on 29 July 2021, 13:21 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: NVDataMgmt.h Revision Comment by Sean Nash on 26 July 2021, 10:07 https://devapps.diality.us/cru/DG-DEN-9054-1#c10441 For consistency, can we put Dialin functions at end? Reply by Dara Navaei on 28 July 2021, 20:46 > Done. Reply by Sean Nash on 29 July 2021, 13:25 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: Common.h Revision Comment by Sean Nash on 26 July 2021, 10:12 https://devapps.diality.us/cru/DG-DEN-9054-1#c10444 How is this used? Reply by Dara Navaei on 28 July 2021, 20:45 > The default pump control mode is none. Reply by qnguyen on 29 July 2021, 11:34 > Should it be 0 and is the first one in the enumeration? Reply by Dara Navaei on 29 July 2021, 16:39 > I removed the enum. Reply by Sean Nash on 30 July 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/FPGA.c Revision Comment by Sean Nash on 30 July 2021, 10:55 https://devapps.diality.us/cru/DG-DEN-9054-1#c10562 Need function header. Reply by Dara Navaei on 30 July 2021, 13:48 > Done. Reply by Sean Nash on 02 August 2021, 10:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 26 July 2021, 10:20 https://devapps.diality.us/cru/DG-DEN-9054-1#c10446 The GPIO register is shared (e.g. see fluid leak in above function). So I think we should have separate functions for each bit that we use in the GPIO register. Also - why does the function name have the word Count in it? Reply by Dara Navaei on 29 July 2021, 09:07 > Done. Reply by Sean Nash on 29 July 2021, 13:21 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Switches.c Revision Comment by qnguyen on 29 July 2021, 12:23 https://devapps.diality.us/cru/DG-DEN-9054-1#c10495 Do we need to add copyrights or bamboo will do it? Reply by Dara Navaei on 29 July 2021, 16:19 > Bamboo will do the copyrights. It is not turned on right now > but it will automatically. Reply by qnguyen on 30 July 2021, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 July 2021, 10:53 https://devapps.diality.us/cru/DG-DEN-9054-1#c10561 Should we debounce switches before changing states? Check with Noe to see if FPGA is already debouncing these signals. Reply by Dara Navaei on 31 July 2021, 09:02 > The signal is debounced for 50ms. I added a debounce for a > longer time. Reply by Sean Nash on 02 August 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 July 2021, 17:09 https://devapps.diality.us/cru/DG-DEN-9054-1#c10472 So these & operators are going to yield a zero or a non-zero number that will be different for each switch (not a boolean). I think your intent is for these switch data to be an enum. So, for example, this one should be "... = ( ( fpgaRegister & CONCENTRATE_CAP_SWITCH_BIT_MASK ) != 0 ? OPEN : CLOSED ); Reply by Dara Navaei on 28 July 2021, 20:07 > Done. Reply by Sean Nash on 29 July 2021, 13:13 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 29 July 2021, 12:24 https://devapps.diality.us/cru/DG-DEN-9054-1#c10496 Need to add a check if switchId is in valid range. Reply by Dara Navaei on 29 July 2021, 16:19 > Done. Reply by qnguyen on 30 July 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 July 2021, 17:13 https://devapps.diality.us/cru/DG-DEN-9054-1#c10473 Put test functions at bottom of module (after usual banner). Reply by Dara Navaei on 28 July 2021, 20:06 > Done. Reply by Sean Nash on 29 July 2021, 13:12 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 29 July 2021, 13:13 https://devapps.diality.us/cru/DG-DEN-9054-1#c10518 Need to have 2 blank lines. Reply by Dara Navaei on 29 July 2021, 16:02 > Done. Reply by qnguyen on 30 July 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Switches.h Revision Comment by qnguyen on 29 July 2021, 12:20 https://devapps.diality.us/cru/DG-DEN-9054-1#c10494 Common has an enumeration (OPN_CLS_STATE_T) that is similar to this. Suggest using from common. Reply by Dara Navaei on 29 July 2021, 16:24 > Done. Reply by qnguyen on 30 July 2021, 10:36 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: HDDefs.h Revision Comment by Sean Nash on 26 July 2021, 09:58 https://devapps.diality.us/cru/DG-DEN-9054-1#c10440 Compatibility check should be before FPGA test at end where it was. We can't do it this early because UI may not have provided us with version/compatibility info yet. Reply by Dara Navaei on 28 July 2021, 20:54 > This is fixed in the latest commit. Reply by Sean Nash on 29 July 2021, 13:26 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-9054-1 https://devapps.diality.us/cru/DG-DEN-9054-1 Title: DG-DEN-9054_DG HD Switches Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (1 active, 2 completed*) qnguyen (*) Sean Nash (*) pmontazemi