This is a list of all comments for HD-DEN-8886-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Services/AlarmMgmt.c Revision Comment by Dara Navaei on 18 August 2021, 21:32 https://devapps.diality.us/cru/HD-DEN-8886-1#c10768 Missing doxygen comments. Reply by Sean Nash on 19 August 2021, 08:45 > Added. Reply by Dara Navaei on 19 August 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 09 August 2021, 10:20 https://devapps.diality.us/cru/HD-DEN-8886-1#c10614 Suggest define magic number 0. Reply by Sean Nash on 13 August 2021, 09:26 > Fixed. Reply by qnguyen on 13 August 2021, 10:39 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/FPGA.c Revision Comment by Dara Navaei on 18 August 2021, 21:34 https://devapps.diality.us/cru/HD-DEN-8886-1#c10769 If you merge staging into your branch, the temperatures driver converts all the temperature sensors and thermistors. Reply by Sean Nash on 19 August 2021, 08:43 > Changed to match your implementation. Reply by Dara Navaei on 19 August 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 18 August 2021, 21:37 https://devapps.diality.us/cru/HD-DEN-8886-1#c10770 Was it not decided to just get the FPGA counts and then do the conversion into the driver that calls the FPGA function? Reply by Sean Nash on 19 August 2021, 08:37 > Usually. But this function is called by AlarmMgmt (not > really a h/w driver) so I kept the conversion at FPGA level > to keep it low level. Reply by Dara Navaei on 19 August 2021, 10:32 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DialInFlow.c Revision Comment by qnguyen on 18 August 2021, 00:32 https://devapps.diality.us/cru/HD-DEN-8886-1#c10762 Fix alignment. Reply by Sean Nash on 19 August 2021, 08:48 > Fixed. Reply by qnguyen on 19 August 2021, 10:30 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DialOutFlow.c Revision Comment by Dara Navaei on 18 August 2021, 21:56 https://devapps.diality.us/cru/HD-DEN-8886-1#c10773 Please mention the usage of this include. Reply by Sean Nash on 19 August 2021, 08:29 > Done. Reply by Dara Navaei on 19 August 2021, 10:31 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/Dialysis.c Revision Comment by Dara Navaei on 16 August 2021, 16:33 https://devapps.diality.us/cru/HD-DEN-8886-1#c10756 Please add why you are using this include. Reply by Sean Nash on 17 August 2021, 11:20 > Done. Reply by Dara Navaei on 19 August 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/PresOccl.c Revision Comment by Dara Navaei on 16 August 2021, 14:38 https://devapps.diality.us/cru/HD-DEN-8886-1#c10754 Where do you convert the FPGA data to pressure? Reply by Sean Nash on 17 August 2021, 11:26 > Occlusion pressure readings are not converted to any units. > All thresholds are given in counts. Reply by Dara Navaei on 19 August 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Interrupts.c Revision Comment by Dara Navaei on 12 August 2021, 09:51 https://devapps.diality.us/cru/HD-DEN-8886-1#c10630 What is the plan with these software faults? Reply by Sean Nash on 13 August 2021, 09:19 > Fixed. Reply by Dara Navaei on 13 August 2021, 10:43 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/PresOccl.h Revision Comment by Dara Navaei on 18 August 2021, 21:56 https://devapps.diality.us/cru/HD-DEN-8886-1#c10772 Please add the PN and the manufacturer of the sensors. Reply by Sean Nash on 19 August 2021, 08:34 > Done. Reply by Dara Navaei on 19 August 2021, 10:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 16 August 2021, 14:40 https://devapps.diality.us/cru/HD-DEN-8886-1#c10755 Please add doxygen comments. Reply by Sean Nash on 17 August 2021, 11:25 > Done. Reply by Dara Navaei on 19 August 2021, 10:34 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Drivers/SafetyShutdown.c Revision Comment by Dara Navaei on 12 August 2021, 10:16 https://devapps.diality.us/cru/HD-DEN-8886-1#c10631 Are we planning to have #defines instead of the numbers? Reply by Sean Nash on 13 August 2021, 09:09 > Fixed. Reply by Dara Navaei on 13 August 2021, 10:42 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: Compatible.h Revision Comment by qnguyen on 18 August 2021, 00:41 https://devapps.diality.us/cru/HD-DEN-8886-1#c10764 File name mismatch. Reply by Sean Nash on 19 August 2021, 08:47 > Fixed. Reply by qnguyen on 19 August 2021, 10:30 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/Rinseback.c Revision Comment by Dara Navaei on 18 August 2021, 21:49 https://devapps.diality.us/cru/HD-DEN-8886-1#c10771 The brief does not match with what the function does. Reply by Sean Nash on 19 August 2021, 08:36 > Fixed. Reply by Dara Navaei on 19 August 2021, 10:32 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: TestSupport.c Revision Comment by qnguyen on 18 August 2021, 00:42 https://devapps.diality.us/cru/HD-DEN-8886-1#c10765 Should we include "TestSupport.h" for function declaration? Reply by Sean Nash on 19 August 2021, 08:46 > Common.h includes TestSupport.h. Reply by qnguyen on 19 August 2021, 10:30 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/AirTrap.c Revision Comment by Dara Navaei on 11 August 2021, 21:34 https://devapps.diality.us/cru/HD-DEN-8886-1#c10629 I do not see this variable in the function. Reply by Sean Nash on 13 August 2021, 09:21 > Variable is in the isAirTrapControlling() function. Reply by Dara Navaei on 13 August 2021, 10:43 > RESOLVED in CODE WALKTHROUGH. --- ID: HD-DEN-8886-1 https://devapps.diality.us/cru/HD-DEN-8886-1 Title: HD-DEN-8886_HD DG Dev Self Tests (2 of 2) Statement of Objectives: State: Closed Summary: Author: Sean Nash Moderator: Sean Nash Reviewers: (1 active, 2 completed*) qnguyen (*) Dara Navaei (*) pmontazemi