This is a list of all comments for DD-LEAH-220-2. Review Summary: No summary ---------------------------------------- File: firmware/App/Drivers/ConductivitySensors.c Revision Comment by Sean Nash on 16 September 2024, 16:31 https://devapps.diality.us/cru/DD-LEAH-220-2#c20232 I don't see Dialin override functions for read and error counter. Reply by Vinayakam Mani on 17 September 2024, 15:24 > Done. Revision Comment by Sean Nash on 16 September 2024, 17:12 https://devapps.diality.us/cru/DD-LEAH-220-2#c20251 Who will be calling this function? Is there any chance this function can be called more than once in a single task iteration? Is it possible to request a write at one address and then immediately request another write at another address? If so, I think the second write request will overwrite the first - you would need to add a write request queue that the write state machine could handle one job at a time. Reply by Vinayakam Mani on 17 September 2024, 15:25 > As discussed, will be evaluated once we get more details on > the sensor configuration/controls. Revision Comment by Sean Nash on 16 September 2024, 17:10 https://devapps.diality.us/cru/DD-LEAH-220-2#c20250 Should each conductivity sensor have its own state? It doesn't look like the state can be shared by all sensors. ---------------------------------------- File: firmware/App/Monitors/Conductivity.c Revision Comment by Sean Nash on 16 September 2024, 16:38 https://devapps.diality.us/cru/DD-LEAH-220-2#c20235 Should publish timer counter be initialized here? Reply by Vinayakam Mani on 17 September 2024, 15:30 > Done. Revision Comment by Sean Nash on 16 September 2024, 16:39 https://devapps.diality.us/cru/DD-LEAH-220-2#c20236 Use "publishes" instead of "advertises". Reply by Vinayakam Mani on 17 September 2024, 15:30 > Done. Revision Comment by Sean Nash on 16 September 2024, 16:41 https://devapps.diality.us/cru/DD-LEAH-220-2#c20237 Looks like "result = TRUE;" can be done just once above the if statement. Reply by Vinayakam Mani on 17 September 2024, 15:30 > Done. ---------------------------------------- File: firmware/App/Monitors/Pressure.c Revision Comment by Sean Nash on 16 September 2024, 16:43 https://devapps.diality.us/cru/DD-LEAH-220-2#c20240 I don't see Dialin override functions for these. Are you leaving these for Michael? Reply by Vinayakam Mani on 17 September 2024, 15:30 > Done. Revision Comment by Sean Nash on 16 September 2024, 16:44 https://devapps.diality.us/cru/DD-LEAH-220-2#c20241 Structures should be up in the definitions section - not in the private data section. Reply by Vinayakam Mani on 17 September 2024, 15:31 > Done. Revision Comment by Sean Nash on 16 September 2024, 16:47 https://devapps.diality.us/cru/DD-LEAH-220-2#c20244 Is there only 1 pressure sensor? Initialization looks wrong. Should the whole structure get initialized - maybe in init function? Reply by Vinayakam Mani on 17 September 2024, 15:31 > Done. Revision Comment by Sean Nash on 16 September 2024, 16:45 https://devapps.diality.us/cru/DD-LEAH-220-2#c20242 Use \\\<. Reply by Vinayakam Mani on 17 September 2024, 15:31 > Done. Revision Comment by Sean Nash on 16 September 2024, 16:45 https://devapps.diality.us/cru/DD-LEAH-220-2#c20243 Add a blank line between declarations and code. Reply by Vinayakam Mani on 17 September 2024, 15:31 > Done. Revision Comment by Sean Nash on 16 September 2024, 16:53 https://devapps.diality.us/cru/DD-LEAH-220-2#c20245 Remove "ed" from "filtered". Reply by Vinayakam Mani on 17 September 2024, 15:31 > Done. Revision Comment by Sean Nash on 16 September 2024, 16:54 https://devapps.diality.us/cru/DD-LEAH-220-2#c20246 Replace TD with DD. Reply by Vinayakam Mani on 17 September 2024, 15:31 > Done. ---------------------------------------- File: firmware/App/Drivers/PressureSensor.c Revision Comment by Sean Nash on 16 September 2024, 16:33 https://devapps.diality.us/cru/DD-LEAH-220-2#c20233 I don't see Dialin override functions for read and error counter. Reply by Vinayakam Mani on 17 September 2024, 15:29 > Done. ---------------------------------------- File: firmware/App/Drivers/PressureSensor.h Revision Comment by Sean Nash on 16 September 2024, 16:34 https://devapps.diality.us/cru/DD-LEAH-220-2#c20234 Switch order of first 2 enums so Hydraulics Outlet is first. Reply by Vinayakam Mani on 17 September 2024, 15:30 > Done. ---------------------------------------- File: firmware/App/Monitors/Pressure.h Revision Comment by Sean Nash on 16 September 2024, 16:42 https://devapps.diality.us/cru/DD-LEAH-220-2#c20238 I don't see a need for packing. All F32s. Reply by Vinayakam Mani on 17 September 2024, 15:32 > Done. Revision Comment by Sean Nash on 16 September 2024, 16:42 https://devapps.diality.us/cru/DD-LEAH-220-2#c20239 Add a blank line before test functions. Reply by Vinayakam Mani on 17 September 2024, 15:32 > Done. ---------------------------------------- File: firmware/App/Services/FpgaDD.c Revision Comment by Sean Nash on 16 September 2024, 16:58 https://devapps.diality.us/cru/DD-LEAH-220-2#c20247 Isn't bicarb bag de-scoped? Reply by Vinayakam Mani on 16 September 2024, 17:32 > Yes, Currently HDD listing this pressure sensor. Once HDD > cleanup is done, we may remove/comment the source as > required. ---------------------------------------- File: PersistentAlarm.h Revision Comment by Sean Nash on 16 September 2024, 17:02 https://devapps.diality.us/cru/DD-LEAH-220-2#c20249 Dara's build script should be updating these for us. Reply by Vinayakam Mani on 16 September 2024, 17:31 > This is just added for the sake of code review creation, the > actual changes were already merged in to staging. ---------------------------------------- File: MsgDefs.h Revision Comment by Sean Nash on 16 September 2024, 17:01 https://devapps.diality.us/cru/DD-LEAH-220-2#c20248 Where are the messages for conductivity and pressure overrides? Are you looking for Michael to add those? Reply by Vinayakam Mani on 17 September 2024, 15:32 > Done. --- ID: DD-LEAH-220-2 https://devapps.diality.us/cru/DD-LEAH-220-2 Title: DD-LEAH-220_FW DD Pressures Monitor Statement of Objectives: State: Closed Summary: Author: Vinayakam Mani Moderator: Vinayakam Mani Reviewers: (2 active, 2 completed*) Sean Nash (*) Dara Navaei (*) jpaguio Michael Garthwaite