This is a list of all comments for LEAHI-TD-FIRMWARE-LDT-1886-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/Ejector.c Revision Comment by Sean Nash on 25 November 2025, 13:56 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25295 Should this be an enum at driver level? Driver get would return enum so controller doesn't have to figure out what it means. Revision Comment by Sean Nash on 25 November 2025, 13:45 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25292 Why are we bypassing driver here? Revision Comment by Vendor - TEL - Sameer Poyil on 11 December 2025, 10:10 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25733 Let all public functions come on top for all files Reply by Dara Navaei on 17 December 2025, 11:07 > Done Revision Comment by Sean Nash on 25 November 2025, 13:46 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25293 Why bypassing driver here? Reply by Dara Navaei on 25 November 2025, 16:01 > Took note that the FPGA interface is at the driver level. Revision Comment by Sean Nash on 25 November 2025, 13:23 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25283 Why is controller bypassing driver to get to fpga? ---------------------------------------- File: firmware/App/Services/FpgaTD.c Revision Comment by Sean Nash on 25 November 2025, 09:25 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25281 Seems backward. Why is inactive = TRUE? I would expect TRUE to indicate that the optical sensor is detecting home (retracted) position. Reply by Dara Navaei on 25 November 2025, 11:07 > According to the HDD, it is a '1' when inactive and becomes a > '0' when active. It becomes active when it reaches the > position. Reply by Sean Nash on 25 November 2025, 13:24 > Then your doxygen return comment looks backward. ---------------------------------------- File: firmware/App/Services/FpgaTD.h Revision Comment by Sean Nash on 25 November 2025, 13:48 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25294 Use U32 unless there is a good reason to use U08. Reply by Dara Navaei on 25 November 2025, 16:01 > Done ---------------------------------------- File: firmware/App/Drivers/EjectorMotor.c Revision Comment by Sean Nash on 15 December 2025, 14:23 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25814 Not sure how Doxygen will handle this comment between the /// and the const. I think we should remove it. Reply by Dara Navaei on 16 December 2025, 13:01 > Done Revision Comment by Sean Nash on 01 December 2025, 09:11 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25331 Should we initialize to inactive (and use #define)? Reply by Dara Navaei on 01 December 2025, 09:43 > Done ---------------------------------------- File: firmware/App/Controllers/Valves.c Revision Comment by Sean Nash on 02 December 2025, 09:16 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25391 Align = Reply by Dara Navaei on 02 December 2025, 09:50 > Done Revision Comment by Sean Nash on 18 December 2025, 14:54 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25947 These don't need to be initialized to zero. Reply by Dara Navaei on 18 December 2025, 15:08 > We tend to initialize the local variables as a standard > practice. Revision Comment by Vendor - TEL - Sameer Poyil on 11 December 2025, 10:14 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25734 let public function on top Reply by Dara Navaei on 17 December 2025, 11:06 > Done Revision Comment by Sean Nash on 16 December 2025, 14:13 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25857 Declarations should be at top of scope. Reply by Dara Navaei on 16 December 2025, 14:53 > Done Revision Comment by Sean Nash on 16 December 2025, 14:15 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25858 This if should have an else where we zero .valveOpsStartTime (in case it briefly got set but then failed this if before timeout). Reply by Dara Navaei on 16 December 2025, 14:55 > Done. Revision Comment by Sean Nash on 16 December 2025, 14:17 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25859 The else in this trinary looks wrong - doesn't round up to multiple of 8. Reply by Dara Navaei on 17 December 2025, 10:48 > Done ---------------------------------------- File: firmware/App/Drivers/RotaryValve.c Revision Comment by Vendor - TEL - Sameer Poyil on 11 December 2025, 10:15 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25736 move static function after public function Reply by Dara Navaei on 17 December 2025, 11:05 > Done Revision Comment by Sean Nash on 02 December 2025, 09:14 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25390 Should be 2 blank lines above/below test banner. Reply by Dara Navaei on 02 December 2025, 09:49 > Done ---------------------------------------- File: firmware/App/TDCommon.h Revision Comment by Sean Nash on 05 December 2025, 14:55 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25599 Should we comment this out before merging? Reply by Dara Navaei on 14 December 2025, 19:26 > Done ---------------------------------------- File: firmware/App/Controllers/Valves.h Revision Comment by Sean Nash on 16 December 2025, 14:13 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1#c25856 Is this temporary? Reply by Dara Navaei on 16 December 2025, 14:53 > No. It is permanent, I would like to know the value for the > homing. --- ID: LEAHI-TD-FIRMWARE-LDT-1886-1 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1886-1 Title: LEAHI-TD-FIRMWARE-LDT-1886_Blood Set Auto-Load and Auto-Eject - TD Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (8 active, 2 completed*) Sean Nash (*) Vendor - TEL - Sameer Poyil (*) Vendor - TEL - Arpita Srivastava Vendor - TEL - Jashwant Gantyada Vendor - TEL - Varshini Nagabooshanam Vinayakam Mani Raghu Kallala Michael Garthwaite Vendor - TEL - Sivvanarayana Kurapati Daniel Ho