This is a list of all comments for RO-LEAH-244-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Drivers/InternalADC.c Revision Comment by Vinayakam Mani on 04 November 2024, 09:11 https://devapps.diality.us/cru/RO-LEAH-244-1#c20767 This looks to be TD specific one. Reply by Dara Navaei on 05 November 2024, 10:04 > Removed them. Revision Comment by Vinayakam Mani on 04 November 2024, 09:11 https://devapps.diality.us/cru/RO-LEAH-244-1#c20768 This looks to be TD specific one. Reply by Dara Navaei on 05 November 2024, 10:04 > Removed them. Revision Comment by Vinayakam Mani on 04 November 2024, 09:11 https://devapps.diality.us/cru/RO-LEAH-244-1#c20769 This looks to be TD specific one. Reply by Dara Navaei on 05 November 2024, 10:04 > Removed them. ---------------------------------------- File: firmware/App/Drivers/InternalADC.h Revision Comment by Vinayakam Mani on 04 November 2024, 09:12 https://devapps.diality.us/cru/RO-LEAH-244-1#c20770 Looks BP are TD specific. Reply by Dara Navaei on 05 November 2024, 10:03 > Removed them. Revision Comment by Vinayakam Mani on 04 November 2024, 09:14 https://devapps.diality.us/cru/RO-LEAH-244-1#c20772 TD specific. Reply by Dara Navaei on 05 November 2024, 10:03 > Remove them. Revision Comment by Vinayakam Mani on 04 November 2024, 09:14 https://devapps.diality.us/cru/RO-LEAH-244-1#c20773 TD specific. ---------------------------------------- File: firmware/App/Modes/ModeFault.c Revision Comment by Vinayakam Mani on 04 November 2024, 09:17 https://devapps.diality.us/cru/RO-LEAH-244-1#c20774 ROFaultMode. Revision Comment by Sean Nash on 30 October 2024, 12:51 https://devapps.diality.us/cru/RO-LEAH-244-1#c20680 Need at least basic functionality (actuators to safe state) here. Reply by Dara Navaei on 05 November 2024, 09:47 > Done Reply by Sean Nash on 06 November 2024, 09:49 > Will resolve in next branch. ---------------------------------------- File: firmware/App/Modes/ModeFault.h Revision Comment by Vinayakam Mani on 04 November 2024, 09:17 https://devapps.diality.us/cru/RO-LEAH-244-1#c20775 RO FaultMode Reply by Dara Navaei on 05 November 2024, 09:47 > Done ---------------------------------------- File: firmware/App/Modes/ModeStandby.h Revision Comment by Sean Nash on 30 October 2024, 12:48 https://devapps.diality.us/cru/RO-LEAH-244-1#c20677 Why is #define name so long? Why not just _MODE_STANDBY_H_? Do we want to make a team decision to show the whole path in these names? I've been just doing the unit name w/o path. Reply by Dara Navaei on 02 November 2024, 16:39 > These are auto-generated that is why I left them. They are > now changed. ---------------------------------------- File: firmware/App/Services/AlarmMgmtRO.c Revision Comment by Vinayakam Mani on 04 November 2024, 09:19 https://devapps.diality.us/cru/RO-LEAH-244-1#c20776 RO alarm fault. Revision Comment by Vinayakam Mani on 04 November 2024, 09:20 https://devapps.diality.us/cru/RO-LEAH-244-1#c20777 Remove extra blank lines. Reply by Dara Navaei on 05 November 2024, 09:46 > Done Revision Comment by Vinayakam Mani on 04 November 2024, 09:22 https://devapps.diality.us/cru/RO-LEAH-244-1#c20778 Change all of these to RO fault in entire file. Reply by Dara Navaei on 05 November 2024, 09:45 > Done ---------------------------------------- File: firmware/App/Services/FpgaRO.c Revision Comment by Vinayakam Mani on 04 November 2024, 09:24 https://devapps.diality.us/cru/RO-LEAH-244-1#c20779 FpgaRO Reply by Dara Navaei on 05 November 2024, 09:23 > Done Revision Comment by Sean Nash on 30 October 2024, 13:01 https://devapps.diality.us/cru/RO-LEAH-244-1#c20683 There needs to be actuator (write) and sensor (read) register map structures defined here. Reply by Sean Nash on 06 November 2024, 09:48 > Will resolve in next branch. Revision Comment by Sean Nash on 30 October 2024, 13:02 https://devapps.diality.us/cru/RO-LEAH-244-1#c20684 Need to add this call to initialize common FPGA driver: initFPGA( (U08*)&fpgaHeader, (U08*)&fpgaSensorReadings, (U08*)&fpgaActuatorSetPoints, sizeof(FPGA_HEADER_T), sizeof(FPGA_SENSORS_T), sizeof(FPGA_ACTUATORS_T) ); Reply by Sean Nash on 06 November 2024, 09:48 > Will resolve in next branch. Revision Comment by Sean Nash on 30 October 2024, 13:02 https://devapps.diality.us/cru/RO-LEAH-244-1#c20685 Need to have memsets to zero register structures. Reply by Sean Nash on 06 November 2024, 09:47 > Will resolve in next branch. ---------------------------------------- File: firmware/App/Services/FpgaRO.h Revision Comment by Vinayakam Mani on 04 November 2024, 09:25 https://devapps.diality.us/cru/RO-LEAH-244-1#c20781 FpgaRO. change all TD references to RO. Reply by Dara Navaei on 05 November 2024, 09:23 > Done Revision Comment by Sean Nash on 30 October 2024, 12:59 https://devapps.diality.us/cru/RO-LEAH-244-1#c20682 Need bare minimum FPGA get/set functions to operate valves and pump. Reply by Sean Nash on 06 November 2024, 09:48 > Will add in next branch. ---------------------------------------- File: firmware/App/Services/Interrupts.c Revision Comment by Vinayakam Mani on 04 November 2024, 09:27 https://devapps.diality.us/cru/RO-LEAH-244-1#c20782 RO doesnt need these. Reply by Sean Nash on 06 November 2024, 09:35 > Removed. Revision Comment by Vinayakam Mani on 04 November 2024, 09:29 https://devapps.diality.us/cru/RO-LEAH-244-1#c20783 All these TD alarms should be RO ( entire file) Reply by Dara Navaei on 05 November 2024, 09:22 > Done Revision Comment by Vinayakam Mani on 04 November 2024, 09:31 https://devapps.diality.us/cru/RO-LEAH-244-1#c20784 looks RO not needing these. Reply by Dara Navaei on 05 November 2024, 09:20 > Done ---------------------------------------- File: firmware/App/Services/Messaging.c Revision Comment by Sean Nash on 30 October 2024, 11:33 https://devapps.diality.us/cru/RO-LEAH-244-1#c20658 Is this still needed? Reply by Dara Navaei on 04 November 2024, 08:46 > Removed it. Revision Comment by Sean Nash on 30 October 2024, 12:01 https://devapps.diality.us/cru/RO-LEAH-244-1#c20660 Comment says TD. Should be RO. Revision Comment by Sean Nash on 30 October 2024, 12:02 https://devapps.diality.us/cru/RO-LEAH-244-1#c20662 Remove extra blank line. Reply by Dara Navaei on 04 November 2024, 08:46 > Done Revision Comment by Sean Nash on 30 October 2024, 12:06 https://devapps.diality.us/cru/RO-LEAH-244-1#c20665 Needs to line up with the above array of message IDs. Should have same number of entries and in the same order. Reply by Sean Nash on 06 November 2024, 09:51 > Will resolve in next branch. Revision Comment by Sean Nash on 30 October 2024, 12:06 https://devapps.diality.us/cru/RO-LEAH-244-1#c20666 Remove extra blank line. Reply by Dara Navaei on 02 November 2024, 17:22 > Done Revision Comment by Sean Nash on 30 October 2024, 12:14 https://devapps.diality.us/cru/RO-LEAH-244-1#c20667 Remove this comment. Broadcast channel ok. Reply by Dara Navaei on 02 November 2024, 17:22 > Done Revision Comment by Sean Nash on 30 October 2024, 12:15 https://devapps.diality.us/cru/RO-LEAH-244-1#c20668 This should probably be moved to OperationModes.c and called by the generic message handling function that uses the function look-up table. Reply by Dara Navaei on 02 November 2024, 17:19 > In TD it is in the same file. Reply by Sean Nash on 05 November 2024, 10:38 > I haven't done op mode override handler in TD yet so I > don't think that is true. Reply by Sean Nash on 06 November 2024, 09:51 > Will resolve in next branch. ---------------------------------------- File: firmware/App/Services/Messaging.h Revision Comment by Sean Nash on 30 October 2024, 11:31 https://devapps.diality.us/cru/RO-LEAH-244-1#c20656 Shouldn't need a handler section since all handled messages should go through the above handleIncomingMessage() function. Reply by Dara Navaei on 04 November 2024, 08:55 > Done Revision Comment by Sean Nash on 30 October 2024, 11:30 https://devapps.diality.us/cru/RO-LEAH-244-1#c20655 This is not a test support function. Move up. Reply by Dara Navaei on 04 November 2024, 08:54 > Done ---------------------------------------- File: firmware/App/Services/MsgQueues.c Revision Comment by Sean Nash on 30 October 2024, 13:09 https://devapps.diality.us/cru/RO-LEAH-244-1#c20687 General question - should we (later) make this unit common to all f/w stacks? Reply by Dara Navaei on 02 November 2024, 16:23 > The more common code the better. We can discuss. Revision Comment by Sean Nash on 30 October 2024, 13:08 https://devapps.diality.us/cru/RO-LEAH-244-1#c20686 These alarms should be RO (not TD). Reply by Dara Navaei on 02 November 2024, 16:28 > Done ---------------------------------------- File: firmware/App/Services/SystemCommRO.c Revision Comment by Vinayakam Mani on 04 November 2024, 10:43 https://devapps.diality.us/cru/RO-LEAH-244-1#c20793 DD? Reply by Dara Navaei on 05 November 2024, 09:16 > Done Reply by Vinayakam Mani on 05 November 2024, 10:37 > Looks Macro still naming DG. Reply by Sean Nash on 06 November 2024, 09:34 > Fixed. Revision Comment by Vinayakam Mani on 04 November 2024, 10:44 https://devapps.diality.us/cru/RO-LEAH-244-1#c20794 HD -> TD. Reply by Dara Navaei on 05 November 2024, 09:14 > Done Revision Comment by Vinayakam Mani on 04 November 2024, 10:44 https://devapps.diality.us/cru/RO-LEAH-244-1#c20795 TD ->RO Reply by Dara Navaei on 05 November 2024, 09:13 > Done ---------------------------------------- File: firmware/App/Tasks/TaskGeneral.c Revision Comment by Vinayakam Mani on 04 November 2024, 10:46 https://devapps.diality.us/cru/RO-LEAH-244-1#c20798 No CPLD for RO? Reply by Dara Navaei on 05 November 2024, 09:11 > Correct. Removed the code. ---------------------------------------- File: firmware/source/sys_main.c Revision Comment by Sean Nash on 30 October 2024, 13:22 https://devapps.diality.us/cru/RO-LEAH-244-1#c20688 If not done already, we will need to make changes to some of this in HalCoGEN and there may be some of these function calls here that are not needed for RO. Reply by Dara Navaei on 02 November 2024, 16:21 > Where can I find the changes? Reply by Sean Nash on 06 November 2024, 09:46 > Will resolve in next branch. ---------------------------------------- File: AlarmMgmt.c Revision Comment by Sean Nash on 30 October 2024, 12:23 https://devapps.diality.us/cru/RO-LEAH-244-1#c20669 I guess technically we don't need the ';' for these macros. Delete ';' for all 3 stacks throughout unit. Reply by Dara Navaei on 02 November 2024, 17:14 > Done ---------------------------------------- File: CommBuffers.h Revision Comment by Sean Nash on 30 October 2024, 12:24 https://devapps.diality.us/cru/RO-LEAH-244-1#c20670 Why are you changing these? This is TD code. Reply by Dara Navaei on 02 November 2024, 17:13 > It was a mistake. Done. Revision Comment by Sean Nash on 30 October 2024, 12:25 https://devapps.diality.us/cru/RO-LEAH-244-1#c20671 Same. This is DD code. Leave this alone. Reply by Dara Navaei on 02 November 2024, 17:12 > Done Revision Comment by Sean Nash on 30 October 2024, 12:25 https://devapps.diality.us/cru/RO-LEAH-244-1#c20672 Ordering looks wrong here. Should be in order of priority/channel #. Reply by Sean Nash on 06 November 2024, 09:51 > Will resolve in next branch. ---------------------------------------- File: PersistentAlarm.h Revision Comment by Sean Nash on 04 November 2024, 09:25 https://devapps.diality.us/cru/RO-LEAH-244-1#c20780 = 0 on first enum. ---------------------------------------- File: SystemComm.c Revision Comment by Sean Nash on 30 October 2024, 12:31 https://devapps.diality.us/cru/RO-LEAH-244-1#c20673 Can we just remove the commented out code? Reply by Dara Navaei on 02 November 2024, 17:08 > Done ---------------------------------------- File: WatchdogMgmt.c Revision Comment by Sean Nash on 30 October 2024, 12:42 https://devapps.diality.us/cru/RO-LEAH-244-1#c20674 I disagree with this change. MODE_INIT is a TD mode. Reply by Dara Navaei on 02 November 2024, 17:06 > Done ---------------------------------------- File: AlarmDefs.h Revision Comment by Vinayakam Mani on 04 November 2024, 09:00 https://devapps.diality.us/cru/RO-LEAH-244-1#c20764 The fault should be false as it is not related to HD issue. Reply by Dara Navaei on 04 November 2024, 09:04 > Do you mean DD? Reply by Sean Nash on 04 November 2024, 09:13 > TD actually. Reply by Dara Navaei on 05 November 2024, 10:07 > Done Reply by Sean Nash on 06 November 2024, 09:40 > Fixed. Revision Comment by Sean Nash on 30 October 2024, 11:23 https://devapps.diality.us/cru/RO-LEAH-244-1#c20650 Change references to HD to TD. Reply by Dara Navaei on 04 November 2024, 09:06 > Done Revision Comment by Vinayakam Mani on 04 November 2024, 08:59 https://devapps.diality.us/cru/RO-LEAH-244-1#c20759 Mentioning as HD fault. May need to change as DD/RO fault.. DD and TD has same alarm. Need to see how to differentiate between those and RO alarms? Reply by Sean Nash on 06 November 2024, 09:45 > Referring to RO alarms as RO here. In properties table, RO > faults will be marked as DD faults (from TD perspective, they > kind of are DD faults). ---------------------------------------- File: MsgDefs.h Revision Comment by Sean Nash on 30 October 2024, 11:23 https://devapps.diality.us/cru/RO-LEAH-244-1#c20651 Remove extra blank line. Reply by Dara Navaei on 04 November 2024, 09:00 > Done ---------------------------------------- File: RODefs.h Revision Comment by Sean Nash on 30 October 2024, 11:24 https://devapps.diality.us/cru/RO-LEAH-244-1#c20652 These are TD modes. RO initially needs fault, service (empty for now), init (no POST tests for now), standby, and delivery RO water modes. Reply by Dara Navaei on 04 November 2024, 08:59 > Done Revision Comment by Sean Nash on 30 October 2024, 11:27 https://devapps.diality.us/cru/RO-LEAH-244-1#c20654 RO, not HD. Reply by Dara Navaei on 04 November 2024, 08:58 > Done Revision Comment by Sean Nash on 30 October 2024, 11:27 https://devapps.diality.us/cru/RO-LEAH-244-1#c20653 Remove all POST tests that look like HD POST tests for now. Since we're not doing any POST tests initially, we don't need to worry too much about this enum yet. Reply by Dara Navaei on 04 November 2024, 08:58 > Done --- ID: RO-LEAH-244-1 https://devapps.diality.us/cru/RO-LEAH-244-1 Title: RO-LEAH-244_FW RO Background Task Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (3 active, 2 completed*) Sean Nash (*) Vinayakam Mani (*) Tiffany Mejia Michael Garthwaite Behrouz NematiPour