This is a list of all comments for DG-DEN-4169-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/TemperatureSensors.c Revision Comment by Sean Nash on 31 July 2020, 10:13 https://devapps.diality.us/cru/DG-DEN-4169-1#c2954 Try to give a sense of time for this. I think Modes are calling the check function so that's on the General Task so that's every 50ms x 1000 = 50 seconds. I usually define these types of things as a # of ms divided by the appropriate task interval (in ms). Reply by qnguyen on 31 July 2020, 10:27 > Done. Without the sense of time, it is easier to make > mistakes. The intention here is 5 seconds rather than 50 > seconds. Reply by Sean Nash on 31 July 2020, 13:12 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 31 July 2020, 09:52 https://devapps.diality.us/cru/DG-DEN-4169-1#c2950 Why was the 2D array changed to a structure for the temperature sensors constants? Reply by qnguyen on 31 July 2020, 09:55 > The structure allows grouping of temperature sensor > properties in one place rather than spanning across multiple > 2D arrays. Reply by Dara Navaei on 31 July 2020, 13:09 > RESOLVED in CODE WALKTRHOUGH Revision Comment by Sean Nash on 31 July 2020, 10:21 https://devapps.diality.us/cru/DG-DEN-4169-1#c2960 Clearing should probably be a longer persistence than triggering threshold. Currently both are 50 sec. I think that's too long for triggering. Maybe ok for clearing. Reply by qnguyen on 31 July 2020, 10:35 > Fixed, the intention is 5 seconds for both rather than 50 > seconds. The SYS team will define the threshold later on. Reply by Sean Nash on 31 July 2020, 13:11 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 31 July 2020, 10:23 https://devapps.diality.us/cru/DG-DEN-4169-1#c2961 I generally prefer not using pow() for squaring. Simpler and more efficient to just multiply by itself instead. Reply by qnguyen on 31 July 2020, 10:43 > This is not squaring through, it is power of 2. Reply by Sean Nash on 31 July 2020, 13:11 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 31 July 2020, 10:26 https://devapps.diality.us/cru/DG-DEN-4169-1#c2962 Recommend using time windowed count here to narrow the time. Currently, it is enforcing a maximum # of instances over entire time system is on. Reply by qnguyen on 31 July 2020, 10:41 > The windowed count should be used for FPGA error. For this > one, the time is 40 ms (4 read and 10 ms per read) of > unchanged read count. Reply by Sean Nash on 31 July 2020, 13:11 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 31 July 2020, 10:34 https://devapps.diality.us/cru/DG-DEN-4169-1#c2964 Do we want to have some kind of persistence for this? Reply by qnguyen on 31 July 2020, 10:57 > Done. Reply by Sean Nash on 31 July 2020, 13:11 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 31 July 2020, 09:49 https://devapps.diality.us/cru/DG-DEN-4169-1#c2947 Why the 2D array of values have changed to a structure? Reply by qnguyen on 31 July 2020, 09:51 > The structure allows grouping of temperature sensor > properties in one place rather than spanning across multiple > 2D arrays. Reply by Dara Navaei on 31 July 2020, 13:10 > RESOLVED in CODE WALKTRHOUGH Revision Comment by pmontazemi on 31 July 2020, 09:38 https://devapps.diality.us/cru/DG-DEN-4169-1#c2942 Move all arguments and align them with the left "(" of start of argument list. Reply by qnguyen on 31 July 2020, 10:18 > Done Reply by pmontazemi on 31 July 2020, 13:06 > Align with "a" character Reply by qnguyen on 31 July 2020, 13:34 > Done Reply by pmontazemi on 03 August 2020, 09:06 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 31 July 2020, 09:38 https://devapps.diality.us/cru/DG-DEN-4169-1#c2943 Move all arguments and align them with the left "(" of start of argument list. Reply by qnguyen on 31 July 2020, 10:18 > Done Reply by pmontazemi on 03 August 2020, 09:04 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 31 July 2020, 09:38 https://devapps.diality.us/cru/DG-DEN-4169-1#c2944 Move all arguments and align them with the left "(" of start of argument list. Reply by qnguyen on 31 July 2020, 10:18 > Done Reply by pmontazemi on 03 August 2020, 09:05 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 31 July 2020, 09:39 https://devapps.diality.us/cru/DG-DEN-4169-1#c2945 Why was this functionality removed? Reply by qnguyen on 31 July 2020, 09:42 > We check the inlet water temperature only during Drain, Fill, > and Re-circulate modes. Reply by Dara Navaei on 31 July 2020, 09:51 > I think we want to check all the time in case the > temperature dropped to below the range for any reasons Reply by qnguyen on 31 July 2020, 10:15 > The water temperature will not cause an issue if we do > not produce dialysate through, because the water will > drain out later. Reply by Sean Nash on 31 July 2020, 10:40 > Update function header description to remove this > functionality. Reply by qnguyen on 31 July 2020, 13:33 > Done Reply by pmontazemi on 03 August 2020, 09:05 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Heaters.c Revision Comment by pmontazemi on 31 July 2020, 09:34 https://devapps.diality.us/cru/DG-DEN-4169-1#c2941 Move ");" to end of argument list (not on a separate line). Reply by qnguyen on 31 July 2020, 10:17 > Done Reply by pmontazemi on 31 July 2020, 13:02 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-4169-1 https://devapps.diality.us/cru/DG-DEN-4169-1 Title: DG-DEN-4169_DG Inlet Water Temperature Statement of Objectives: State: Closed Summary: Author: qnguyen Moderator: qnguyen Reviewers: (0 active, 3 completed*) Sean Nash (*) Dara Navaei (*) pmontazemi (*)