This is a list of all comments for DG-DEN-13460-2. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/DialysateFlow.c Revision Comment by wbracken on 26 August 2022, 16:07 https://devapps.diality.us/cru/DG-DEN-13460-2#c13582 Remove comment. Reply by Dara Navaei on 31 August 2022, 14:38 > Done. Reply by wbracken on 22 September 2022, 15:36 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Controllers/ROPump.c Revision Comment by wbracken on 29 August 2022, 14:48 https://devapps.diality.us/cru/DG-DEN-13460-2#c13588 Are we moving away from initializing via definition? Reply by Dara Navaei on 18 October 2023, 21:19 > RESOLVED in CODE WALKTHROUGH Revision Comment by Darren Cox on 12 September 2022, 12:21 https://devapps.diality.us/cru/DG-DEN-13460-2#c13733 Use 0.0F Reply by Sean Nash on 16 September 2022, 10:37 > Probably a good idea for consistency, but not necessary. > Where we need to add the 'F' suffix to floating point > literals is when the literal will not be typed. Here, we are > assigning 0.0 to an F32 typed variable so it will be typed > properly. A #define may or may not be typed depending on how > it's used. If it's used in a mathematical expression, the > compiler will make it max precision (double) if we don't > explicitly add the 'F' suffix and that can cause some > problems with the precision in the math. Reply by Dara Navaei on 21 September 2022, 17:20 > Done. Reply by Darren Cox on 22 September 2022, 15:29 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Controllers/TemperatureSensors.c Revision Comment by wbracken on 29 August 2022, 14:56 https://devapps.diality.us/cru/DG-DEN-13460-2#c13589 Some values have "U"? Reply by Dara Navaei on 18 October 2023, 21:19 > RESOLVED in CODE WALKTHROUGH Revision Comment by Sean Nash on 20 September 2022, 08:57 https://devapps.diality.us/cru/DG-DEN-13460-2#c13828 Keep blank line. Reply by Dara Navaei on 21 September 2022, 17:23 > Done. Reply by Sean Nash on 22 September 2022, 15:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 20 September 2022, 10:16 https://devapps.diality.us/cru/DG-DEN-13460-2#c13830 Add blank line between declaration and code. Reply by Dara Navaei on 21 September 2022, 17:23 > Done. Reply by Sean Nash on 22 September 2022, 15:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 20 September 2022, 10:19 https://devapps.diality.us/cru/DG-DEN-13460-2#c13831 Can we add a comment explaining why these temp sensors are monitored and the rest are ignored? Reply by Dara Navaei on 21 September 2022, 17:26 > Done. Reply by Sean Nash on 22 September 2022, 15:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 20 September 2022, 10:22 https://devapps.diality.us/cru/DG-DEN-13460-2#c13833 Restore this blank line. Reply by Dara Navaei on 21 September 2022, 17:31 > Done. Reply by Sean Nash on 22 September 2022, 15:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 20 September 2022, 10:20 https://devapps.diality.us/cru/DG-DEN-13460-2#c13832 This is legal, but odd looking. Can we define this structure type at top of module and have a normal declaration for baroCoeffs here? Reply by Dara Navaei on 21 September 2022, 17:30 > Done. Reply by Sean Nash on 22 September 2022, 15:46 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Sean Nash on 21 September 2022, 10:13 https://devapps.diality.us/cru/DG-DEN-13460-2#c13840 Keep this blank line. Reply by Dara Navaei on 21 September 2022, 17:35 > Done. Reply by Sean Nash on 22 September 2022, 15:42 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ConductivitySensors.c Revision Comment by Sean Nash on 16 September 2022, 10:31 https://devapps.diality.us/cru/DG-DEN-13460-2#c13781 Remove blank line. Reply by Dara Navaei on 21 September 2022, 17:22 > Done. Reply by Sean Nash on 22 September 2022, 15:47 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeFlush.c Revision Comment by Darren Cox on 26 August 2022, 12:24 https://devapps.diality.us/cru/DG-DEN-13460-2#c13568 Remove todo in comment if it is resolved. Reply by Dara Navaei on 31 August 2022, 14:41 > Done. Reply by Darren Cox on 12 September 2022, 16:24 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Controllers/Voltages.c Revision Comment by Sean Nash on 21 September 2022, 09:09 https://devapps.diality.us/cru/DG-DEN-13460-2#c13838 This default is unreachable I think. Reply by Dara Navaei on 21 September 2022, 17:33 > Added the VectorCAST #define. Reply by Sean Nash on 22 September 2022, 15:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Darren Cox on 26 August 2022, 12:18 https://devapps.diality.us/cru/DG-DEN-13460-2#c13567 No comment to explain why it is all commented. Reply by Dara Navaei on 31 August 2022, 14:42 > This code is un-commented. Reply by Darren Cox on 12 September 2022, 16:24 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Drivers/CPLD.c Revision Comment by Sean Nash on 21 September 2022, 09:13 https://devapps.diality.us/cru/DG-DEN-13460-2#c13839 Should default trigger a s/w fault? Reply by Dara Navaei on 21 September 2022, 17:50 > Done. Reply by Sean Nash on 22 September 2022, 15:42 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/FlowSensors.c Revision Comment by Darren Cox on 12 September 2022, 12:16 https://devapps.diality.us/cru/DG-DEN-13460-2#c13732 Sensor misspelled (Sesnor). Change name to execFlowSensorMonitor. Reply by Dara Navaei on 21 September 2022, 17:19 > The typo has been fixed in DEN-13834. Reply by Darren Cox on 22 September 2022, 15:30 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: NVDataMgmt.c Revision Comment by Darren Cox on 26 August 2022, 13:40 https://devapps.diality.us/cru/DG-DEN-13460-2#c13570 When will it be uncommented? Reply by Dara Navaei on 31 August 2022, 14:39 > In in DEN-13834. Reply by Darren Cox on 12 September 2022, 16:23 > RESOLVED IN CODE WALKTHROUGH Revision Comment by wbracken on 29 August 2022, 15:42 https://devapps.diality.us/cru/DG-DEN-13460-2#c13591 Should this be in or not? Reply by Dara Navaei on 31 August 2022, 14:37 > Added TODO to un-comment this line of code. Reply by wbracken on 12 September 2022, 16:04 > RESOLVED IN CODE WALKTHROUGH Revision Comment by wbracken on 29 August 2022, 15:46 https://devapps.diality.us/cru/DG-DEN-13460-2#c13592 Should variables in if be reversed? Reply by Dara Navaei on 31 August 2022, 14:35 > "FAPI_CHECK_FSM_READY_BUSY" is a macro. Reply by wbracken on 31 August 2022, 16:21 > Line 1902 is the same check but reverses the order. Reply by Dara Navaei on 21 September 2022, 17:17 > Done. Reply by wbracken on 22 September 2022, 15:35 > RESOLVED IN CODE WALKTHROUGH Revision Comment by wbracken on 29 August 2022, 15:51 https://devapps.diality.us/cru/DG-DEN-13460-2#c13593 Should this be removed? Reply by Dara Navaei on 31 August 2022, 14:33 > Done. Reply by wbracken on 12 September 2022, 16:03 > RESOLVED IN CODE WALKTHROUGH Revision Comment by wbracken on 29 August 2022, 15:57 https://devapps.diality.us/cru/DG-DEN-13460-2#c13594 Set "status" to FALSE. Reply by Dara Navaei on 21 September 2022, 17:21 > Done. Reply by wbracken on 22 September 2022, 15:34 > RESOLVED IN CODE WALKTHROUGH ---------------------------------------- File: PersistentAlarm.c Revision Comment by Sean Nash on 21 September 2022, 10:33 https://devapps.diality.us/cru/DG-DEN-13460-2#c13842 Need a default and s/w fault for this switch statement. Reply by Dara Navaei on 21 September 2022, 17:37 > Done. Reply by Sean Nash on 22 September 2022, 15:42 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: Utilities.c Revision Comment by wbracken on 29 August 2022, 16:31 https://devapps.diality.us/cru/DG-DEN-13460-2#c13598 Should this function handle invalid hex characters? Reply by Dara Navaei on 21 September 2022, 17:17 > This function will be modified in DEN-13834. Reply by wbracken on 22 September 2022, 15:35 > RESOLVED IN CODE WALKTHROUGH Revision Comment by wbracken on 29 August 2022, 16:33 https://devapps.diality.us/cru/DG-DEN-13460-2#c13599 What is the value of SEMAPHORE_IN_USE_TIMEOUT_MS? The timeout appears to only be check when getSemaphore is called. Seems like you could acquire the semaphore and the process that acquired the semaphore could hold onto it for a time much longer than SEMAPHORE_IN_USE_TIMEOUT. Reply by Sean Nash on 21 September 2022, 10:44 > I think idea here is that we don't want any process to hold > onto semaphore for very long so it can't block other > processes for too long. If another process is calling this > function to try to access the protected resource, it will > check for timeout and alarm if semaphore has been held for > too long. Reply by Dara Navaei on 18 October 2023, 21:20 > RESOLVED in CODE WALKTHROUGH ---------------------------------------- File: Common.h Revision Comment by Darren Cox on 26 August 2022, 13:38 https://devapps.diality.us/cru/DG-DEN-13460-2#c13569 U64 implies Unsigned, but this is signed. Reply by Dara Navaei on 30 August 2022, 18:04 > Done. Reply by Darren Cox on 12 September 2022, 16:23 > RESOLVED IN CODE WALKTHROUGH --- ID: DG-DEN-13460-2 https://devapps.diality.us/cru/DG-DEN-13460-2 Title: DG-DEN-13460_DG HD Dev HD DG Dvt Update Part 2 Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (1 active, 3 completed*) Sean Nash (*) wbracken (*) Darren Cox (*) Michael Garthwaite