This is a list of all comments for DG-DEN-12931-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/ConductivitySensors.c Revision Comment by Sean Nash on 22 July 2022, 10:31 https://devapps.diality.us/cru/DG-DEN-12931-1#c13324 Keep this blank line. Reply by Dara Navaei on 03 August 2022, 15:51 > Done. Reply by Sean Nash on 04 August 2022, 15:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 July 2022, 10:33 https://devapps.diality.us/cru/DG-DEN-12931-1#c13326 This assignment should be done above at declaration. Move calStatus declaration above result. Reply by Dara Navaei on 03 August 2022, 15:50 > Done. We usually define the state at the top so for > consistency, I defined the state first. Reply by Sean Nash on 04 August 2022, 15:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 July 2022, 10:36 https://devapps.diality.us/cru/DG-DEN-12931-1#c13327 Blank line between declarations and code. Reply by Dara Navaei on 03 August 2022, 15:48 > Done. Reply by Sean Nash on 04 August 2022, 15:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 July 2022, 10:37 https://devapps.diality.us/cru/DG-DEN-12931-1#c13328 Should be s/w fault. Add TODO or trigger s/w fault now. Reply by Dara Navaei on 03 August 2022, 15:47 > Done. Reply by Sean Nash on 04 August 2022, 15:46 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Heaters.c Revision Comment by Sean Nash on 23 July 2022, 13:34 https://devapps.diality.us/cru/DG-DEN-12931-1#c13332 Add blank line between declarations and code. Reply by Sean Nash on 04 August 2022, 15:54 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 July 2022, 13:35 https://devapps.diality.us/cru/DG-DEN-12931-1#c13333 You can just use 0.0F here. Reply by Dara Navaei on 03 August 2022, 15:44 > Done. Reply by Sean Nash on 04 August 2022, 15:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 July 2022, 13:36 https://devapps.diality.us/cru/DG-DEN-12931-1#c13334 Keep this blank line. Reply by Dara Navaei on 03 August 2022, 15:43 > Done. Reply by Sean Nash on 04 August 2022, 15:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 July 2022, 13:37 https://devapps.diality.us/cru/DG-DEN-12931-1#c13335 Why are these commented out. I thought these were fixed now. Reply by Dara Navaei on 03 August 2022, 15:42 > These have been fixed in DEN-13460. Reply by Sean Nash on 04 August 2022, 15:44 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/TemperatureSensors.c Revision Comment by Sean Nash on 23 July 2022, 13:46 https://devapps.diality.us/cru/DG-DEN-12931-1#c13339 Keep this blank line. Reply by Dara Navaei on 03 August 2022, 15:36 > Done. Reply by Sean Nash on 04 August 2022, 15:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 July 2022, 13:49 https://devapps.diality.us/cru/DG-DEN-12931-1#c13340 DVT software configuration? Reply by Dara Navaei on 03 August 2022, 15:35 > This function covers both DVT and V3 and depending on the > status it pick the conversion coefficients. Reply by Sean Nash on 04 August 2022, 15:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 July 2022, 13:50 https://devapps.diality.us/cru/DG-DEN-12931-1#c13341 Add comments like others. Reply by Dara Navaei on 04 August 2022, 15:52 > Will address in DEN-13460. Reply by Sean Nash on 04 August 2022, 15:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 July 2022, 13:50 https://devapps.diality.us/cru/DG-DEN-12931-1#c13342 Add blank line between declarations and code. Reply by Dara Navaei on 03 August 2022, 15:34 > Done. Reply by Sean Nash on 04 August 2022, 15:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 July 2022, 13:51 https://devapps.diality.us/cru/DG-DEN-12931-1#c13343 Keep this blank line. Reply by Dara Navaei on 03 August 2022, 15:33 > Done. Reply by Sean Nash on 04 August 2022, 15:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 July 2022, 13:51 https://devapps.diality.us/cru/DG-DEN-12931-1#c13344 Magic number. Reply by Dara Navaei on 03 August 2022, 15:32 > This is a temporary number and has been removed in DEN-13460. Reply by Sean Nash on 04 August 2022, 15:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 July 2022, 13:52 https://devapps.diality.us/cru/DG-DEN-12931-1#c13345 Keep this blank line. Reply by Dara Navaei on 03 August 2022, 15:31 > Done. Reply by Sean Nash on 04 August 2022, 15:38 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/FPGA.c Revision Comment by Sean Nash on 23 July 2022, 14:35 https://devapps.diality.us/cru/DG-DEN-12931-1#c13350 No point in initializing here - gets set by exec function. Reply by Dara Navaei on 03 August 2022, 14:42 > Done. Reply by Sean Nash on 04 August 2022, 15:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 July 2022, 14:36 https://devapps.diality.us/cru/DG-DEN-12931-1#c13351 Is this logic right? Looks like it should be || instead of && and it should be == instead of != DG_MODE_INIT. Reply by Dara Navaei on 03 August 2022, 14:41 > Done. Reply by Sean Nash on 04 August 2022, 15:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 July 2022, 14:41 https://devapps.diality.us/cru/DG-DEN-12931-1#c13352 If these comment outs are not permanent, add a TODO. Otherwise remove the commented out code. Reply by Dara Navaei on 03 August 2022, 14:39 > Done. Reply by Sean Nash on 04 August 2022, 15:52 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 01 August 2022, 14:46 https://devapps.diality.us/cru/DG-DEN-12931-1#c13363 Remove comment at end of line? sam for 710/822. Reply by Dara Navaei on 03 August 2022, 14:33 > Done. Reply by Dara Navaei on 04 August 2022, 15:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 01 August 2022, 14:51 https://devapps.diality.us/cru/DG-DEN-12931-1#c13364 Remove comment at end of line. Is fpgaReadByteSize replacing sizeof(DG_FPGA_SENSORS_T)? Reply by Dara Navaei on 03 August 2022, 14:33 > Removed it. Reply by Dara Navaei on 04 August 2022, 15:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 July 2022, 14:43 https://devapps.diality.us/cru/DG-DEN-12931-1#c13353 Is it really true that all function below this line are DVT? Reply by Dara Navaei on 03 August 2022, 14:39 > Removed the banner. Reply by Sean Nash on 04 August 2022, 15:23 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Reservoirs.c Revision Comment by Sean Nash on 23 July 2022, 14:44 https://devapps.diality.us/cru/DG-DEN-12931-1#c13354 TODO should be all CAPS. Reply by Dara Navaei on 03 August 2022, 14:38 > Done. Reply by Sean Nash on 04 August 2022, 15:23 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Michael Garthwaite on 04 August 2022, 13:54 https://devapps.diality.us/cru/DG-DEN-12931-1#c13428 Does the broadcast need to be updated with the new struct fields? Reply by Dara Navaei on 04 August 2022, 15:05 > Will address in DEN-13460. Reply by Michael Garthwaite on 04 August 2022, 15:05 > RESOLVED CODE IN WALKTHROUGH. Revision Comment by Sean Nash on 23 July 2022, 14:46 https://devapps.diality.us/cru/DG-DEN-12931-1#c13355 Should 0.5 be a #define? Reply by Dara Navaei on 03 August 2022, 14:37 > Done Reply by Sean Nash on 04 August 2022, 15:23 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ConcentratePumps.c Revision Comment by Sean Nash on 22 July 2022, 09:00 https://devapps.diality.us/cru/DG-DEN-12931-1#c13317 Define disable (even if not used) so it is clear which bit position controls enable/disable. Reply by Dara Navaei on 03 August 2022, 17:03 > This was defined in DEN-13460. Reply by Sean Nash on 04 August 2022, 15:50 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 July 2022, 09:01 https://devapps.diality.us/cru/DG-DEN-12931-1#c13318 This blank line should stay - style is to have blank lines between declarations and code. Reply by Dara Navaei on 03 August 2022, 16:42 > Done. Reply by Sean Nash on 04 August 2022, 15:49 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 July 2022, 09:04 https://devapps.diality.us/cru/DG-DEN-12931-1#c13319 What is the point in having these locals? Why not just use the concentratePumps fields directly? Reply by Dara Navaei on 03 August 2022, 16:40 > Each of these functions are used twice. Reply by Sean Nash on 04 August 2022, 15:49 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 01 August 2022, 10:44 https://devapps.diality.us/cru/DG-DEN-12931-1#c13361 Does the monitorPumpSpeed function generate the alarm Reply by Dara Navaei on 18 October 2023, 21:21 > RESOLVED in CODE WALKTHROUGH Revision Comment by Sean Nash on 22 July 2022, 10:09 https://devapps.diality.us/cru/DG-DEN-12931-1#c13321 Prefer normal if else for calling functions. I see these throughout. Reply by Dara Navaei on 03 August 2022, 16:34 > Done. Reply by Sean Nash on 04 August 2022, 15:50 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 July 2022, 10:15 https://devapps.diality.us/cru/DG-DEN-12931-1#c13322 Blank line between declarations and code. Reply by Dara Navaei on 03 August 2022, 16:31 > Done. Reply by Sean Nash on 04 August 2022, 15:49 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 July 2022, 10:17 https://devapps.diality.us/cru/DG-DEN-12931-1#c13323 Do either of these 2 if statements need an else? Reply by Dara Navaei on 03 August 2022, 15:58 > We do not update the variables if the if they do not meet the > conditions. Reply by Sean Nash on 04 August 2022, 15:55 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeFill.c Revision Comment by Sean Nash on 23 July 2022, 14:25 https://devapps.diality.us/cru/DG-DEN-12931-1#c13347 Fix alignment. Reply by Dara Navaei on 04 August 2022, 15:38 > Done. Reply by Sean Nash on 04 August 2022, 15:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 July 2022, 14:26 https://devapps.diality.us/cru/DG-DEN-12931-1#c13348 Fix alignment. Reply by Dara Navaei on 03 August 2022, 15:29 > Done. Reply by Sean Nash on 04 August 2022, 15:36 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ROPump.c Revision Comment by Sean Nash on 23 July 2022, 13:42 https://devapps.diality.us/cru/DG-DEN-12931-1#c13337 Fix alignment. Reply by Dara Navaei on 03 August 2022, 15:40 > Done. Reply by Sean Nash on 04 August 2022, 15:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 July 2022, 13:45 https://devapps.diality.us/cru/DG-DEN-12931-1#c13338 Flow rate is F32 so change 0 to 0.0F. Reply by Dara Navaei on 03 August 2022, 15:37 > Done. Reply by Sean Nash on 04 August 2022, 15:43 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeService.c Revision Comment by Sean Nash on 23 July 2022, 14:29 https://devapps.diality.us/cru/DG-DEN-12931-1#c13349 Is this a TODO? Reply by Dara Navaei on 03 August 2022, 15:26 > Done. Reply by Sean Nash on 04 August 2022, 15:36 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeFault.c Revision Comment by Sean Nash on 23 July 2022, 14:24 https://devapps.diality.us/cru/DG-DEN-12931-1#c13346 Should go to NV data POST state regardless of RTC pass/fail. Still need to get calibration and system records read. Reply by Dara Navaei on 03 August 2022, 15:30 > This issue will be addressed in DEN-13460. Reply by Sean Nash on 04 August 2022, 15:38 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-12931-1 https://devapps.diality.us/cru/DG-DEN-12931-1 Title: DG-DEN-12931_DG HD Dev HD DG Dvt Update Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (4 active, 0 completed*) Sean Nash Michael Garthwaite wbracken Dong Nguyen