This is a list of all comments for LEAHI-TD-FIRMWARE-LDT-1903-8. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/AirTrap.c Revision Comment by Sean Nash on 20 February 2026, 11:22 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27490 We've been using capital F. Let's be consistent. Revision Comment by Sean Nash on 26 February 2026, 22:49 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27587 Why two leading zeroes? Reply by Vendor - TEL - Sameer Poyil on 27 February 2026, 09:14 > fixed it in commit ID 71a6797 Revision Comment by Sean Nash on 20 February 2026, 11:24 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27491 We have a FRACTION_TO_PERCENT_FACTOR in Common.h that we can use when we need 100.0. Revision Comment by Sean Nash on 13 February 2026, 14:00 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27321 Remove extra blank line. Revision Comment by Sean Nash on 13 February 2026, 14:09 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27324 Why not for release? Revision Comment by Sean Nash on 13 February 2026, 14:03 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27322 Remove extra blank line. Revision Comment by Sean Nash on 02 March 2026, 09:50 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27673 This structure doesn't seem necessary - it's just a middle man. Why not just pass the #defines directly into the quadratic macro? Reply by Vendor - TEL - Sameer Poyil on 02 March 2026, 11:54 > fixed in latest code Revision Comment by Sean Nash on 13 February 2026, 14:10 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27325 Do we ever want to use the default (250)? Do we always want to use this venous pressure based power level? Reply by Vendor - TEL - Sameer Poyil on 13 February 2026, 15:46 > No, Yes for lowering event. > > Fill event uses PWM 150 always irrespective of Qb and Venous > pressure > only lower event used equation based PWM and that is in the > range of 60-250 , not default., based on venous pressure it > changes Revision Comment by Sean Nash on 27 February 2026, 13:28 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27654 duty misspelled. Reply by Vendor - TEL - Sameer Poyil on 02 March 2026, 07:36 > fixed Revision Comment by Sean Nash on 13 February 2026, 14:03 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27323 Remove extra blank line. Revision Comment by Sean Nash on 27 February 2026, 13:29 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27656 I think alarm should be handled inside the setAirPumpState() function. Reply by Vendor - TEL - Sameer Poyil on 02 March 2026, 07:37 > removed from here Revision Comment by Sean Nash on 27 February 2026, 13:30 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27657 Add blank line after declarations. Revision Comment by Sean Nash on 27 February 2026, 13:30 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27658 Alarm should be handled inside of setAirPumpState(). Revision Comment by Sean Nash on 27 February 2026, 13:31 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27659 Alarm should be handled inside of setAirPumpState(). Revision Comment by Sean Nash on 27 February 2026, 13:31 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27660 Alarm should be handled inside of setAirPumpState(). Revision Comment by Sean Nash on 27 February 2026, 13:31 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27661 Alarm should be handled inside of setAirPumpState(). Revision Comment by Sean Nash on 27 February 2026, 13:31 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27662 Alarm should be handled inside of setAirPumpState(). Revision Comment by Sean Nash on 27 February 2026, 13:32 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27663 Alarm should be handled inside of setAirPumpState(). Revision Comment by Sean Nash on 27 February 2026, 13:32 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27664 Add blank line after declarations. ---------------------------------------- File: firmware/App/Controllers/AirPump.c Revision Comment by Sean Nash on 27 February 2026, 13:26 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27653 Can we remove inclusion of fpga here? It shouldn't be needed. Reply by Vendor - TEL - Sameer Poyil on 02 March 2026, 07:42 > Fixed Revision Comment by Sean Nash on 23 February 2026, 12:52 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27538 Remove extra blank line. Reply by Vendor - TEL - Sameer Poyil on 24 February 2026, 08:14 > done Reply by Vendor - TEL - Sameer Poyil on 24 February 2026, 08:15 > done Revision Comment by Sean Nash on 20 February 2026, 11:31 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27494 In comment, specify units is % duty cycle (0..100%). Revision Comment by Sean Nash on 26 February 2026, 22:51 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27588 Add blank line after declarations. Reply by Vendor - TEL - Sameer Poyil on 27 February 2026, 09:17 > pushed new code Revision Comment by Sean Nash on 27 February 2026, 13:29 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27655 Should we have alarm here? Reply by Vendor - TEL - Sameer Poyil on 27 February 2026, 14:54 > We need new alarm . That should be included in SRS right ? > or shall i use SET_ALARM_WITH_2_U32_DATA( > ALARM_ID_TD_SOFTWARE_FAULT) ? Reply by Sean Nash on 02 March 2026, 09:52 > I think this would be a TD s/w fault (alarm already > define). Just need a new TD s/w fault ID in > AlarmMgmtSWFaults.h. Reply by Vendor - TEL - Sameer Poyil on 02 March 2026, 11:53 > fixed Reply by Vendor - TEL - Sameer Poyil on 02 March 2026, 11:53 > implemented Revision Comment by Vendor - TEL - Jashwant Gantyada on 25 February 2026, 11:49 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27573 I don't see it being used in this state. Do we need this? Reply by Vendor - TEL - Sameer Poyil on 25 February 2026, 12:31 > it is used for publish function in the bottom of this > function publishAirPumpData(); Revision Comment by Vendor - TEL - Jashwant Gantyada on 25 February 2026, 09:33 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27572 I don't think we need a start state as this state is doing nothing. It is just transitioning to the next state. We can just directly start from the next state. Reply by Vendor - TEL - Sameer Poyil on 25 February 2026, 12:29 > This is a default framework for all modules, even though > there is nothing right now, left it as it is. Reply by Vendor - TEL - Jashwant Gantyada on 26 February 2026, 09:13 > I am resolving it, but according to Sean, if we have a > state which is just asking it to go to the next state, we > might not need the state to be declared. Reply by Vendor - TEL - Sameer Poyil on 26 February 2026, 10:02 > Ya agreed. for the time being keeping it , we can later > remove it after feature development complete Revision Comment by Sean Nash on 26 February 2026, 22:52 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27589 I don't like controller bypassing driver to get to fpga access functions. There should be a get function in the driver that we can call here instead. Reply by Vendor - TEL - Sameer Poyil on 27 February 2026, 09:14 > Fixed Revision Comment by Sean Nash on 23 February 2026, 12:54 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27540 Put each condition in (). Also, should the setAirPumpState function be checking power is in range instead of here? Reply by Vendor - TEL - Sameer Poyil on 23 February 2026, 15:42 > If I don't check it here, Dialin get a true for even invalid > range. I need to pass the result immediately to user so that > user can take the action. Reply by Vendor - TEL - Sameer Poyil on 24 February 2026, 08:14 > done , but I need to check it here Reply by Sean Nash on 24 February 2026, 09:29 > setAirPumpState could return a BOOL (FALSE if range check > fails or state invalid) that you can set result to. Reply by Vendor - TEL - Sameer Poyil on 24 February 2026, 09:47 > Yes , right now its void , so that's why i didn't change > that . I will push new code ---------------------------------------- File: firmware/App/Drivers/GLXferPump.c Revision Comment by Vendor - TEL - Jashwant Gantyada on 25 February 2026, 09:22 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27568 I think we have to include a comment specifying what function is being used from this file. Reply by Vendor - TEL - Sameer Poyil on 25 February 2026, 12:32 > its a standard c library , for that no need to comment. using > ceil() function Reply by Vendor - TEL - Jashwant Gantyada on 26 February 2026, 09:15 > in other files when they use string and/or math headers > they had specified a comment describing which functions are > used from the headers. Revision Comment by Sean Nash on 23 February 2026, 12:53 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27539 You have these MIN/MAX definitions in both this unit and AirPump unit. Can we put these in header file for this unit and remove from AirPump unit? Revision Comment by Sean Nash on 20 February 2026, 11:05 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27484 Add space around () i.e. ..Scalar( F32 percentage ); Revision Comment by Sean Nash on 20 February 2026, 11:31 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27493 Should we specify that power level is now a duty cycle percentage (0..100%)? Revision Comment by Sean Nash on 20 February 2026, 11:20 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27489 Don't call function in function call. Set local var to scalar, then pass local var as param. Revision Comment by Sean Nash on 24 February 2026, 09:23 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27552 Inputs is none or FPGA. Revision Comment by Sean Nash on 20 February 2026, 11:07 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27485 ( F32 percentage ) { } Revision Comment by Sean Nash on 20 February 2026, 11:10 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27486 You can use RANGE macro in Common.h for this. Revision Comment by Sean Nash on 24 February 2026, 09:58 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27556 Use "F64" macro instead of double. Revision Comment by Vendor - TEL - Jashwant Gantyada on 25 February 2026, 09:25 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27571 I think we have to define macros for magic number. Reply by Vendor - TEL - Sameer Poyil on 26 February 2026, 06:18 > fixed Revision Comment by Sean Nash on 24 February 2026, 09:58 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27557 0.0F Revision Comment by Sean Nash on 24 February 2026, 09:59 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27559 I don't see why we need the extra precision here. I suspect a 64-bit division is expensive (s/w library implementation instead of co-processor) so I don't like to use doubles unless it is truly necessary. Revision Comment by Sean Nash on 24 February 2026, 09:59 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27558 Add blank line before return. ---------------------------------------- File: firmware/App/Drivers/GLXferPump.h Revision Comment by Vendor - TEL - Sameer Poyil on 21 February 2026, 19:10 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27523 capital F ---------------------------------------- File: firmware/App/TDCommon.h Revision Comment by Sean Nash on 23 February 2026, 12:47 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8#c27529 This looks like develop branch got merged into your branch. We should not be doing that. Always merge your branch to develop in that direction only. Reply by Vendor - TEL - Sameer Poyil on 23 February 2026, 18:15 > Yes, without checking the changes in this file, I pushed it. > It is OK when I merge the code. There is no option to correct > since my local branch shows 28.its auto generated and it will > take next version during the merge with staging. --- ID: LEAHI-TD-FIRMWARE-LDT-1903-8 https://devapps.diality.us/cru/LEAHI-TD-FIRMWARE-LDT-1903-8 Title: LEAHI-TD-FIRMWARE-LDT-1903_Air Trap Level Control - TD - 04: DEV - Feature Implementation Statement of Objectives: State: Closed Summary: Author: Vendor - TEL - Sameer Poyil Moderator: Vendor - TEL - Sameer Poyil Reviewers: (5 active, 0 completed*) Vendor - TEL - Arpita Srivastava Vendor - TEL - Jashwant Gantyada Sean Nash Raghu Kallala Dara Navaei