This is a list of all comments for DG-DEN-13834-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/DialysateFlow.c Revision Comment by Michael Garthwaite on 28 September 2022, 13:39 https://devapps.diality.us/cru/DG-DEN-13834-1#c14028 This file has been removed in commit 851ee57 and then added again in commit 256d5cb Reply by Dara Navaei on 28 September 2022, 16:36 > This was because I merged staging into my branch. I has been > removed again. Reply by Michael Garthwaite on 14 October 2022, 10:56 > RESOLVED in CODE WALK THROUGH. ---------------------------------------- File: firmware/App/Controllers/FlowSensors.c Revision Comment by Sean Nash on 04 October 2022, 08:24 https://devapps.diality.us/cru/DG-DEN-13834-1#c14093 Lets get this question answered. I don't see the harm in having a max flow. I can't imagine we're ever seeing that much flow, so not sure why this has to be commented out while we sort out its existence. Reply by Dara Navaei on 12 October 2022, 10:52 > This is part of the SRSs. This alarm is uncommented and is > running. Reply by Sean Nash on 12 October 2022, 16:40 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/FlowSensors.h Revision Comment by Sean Nash on 04 October 2022, 08:19 https://devapps.diality.us/cru/DG-DEN-13834-1#c14092 Remove extra blank line. Reply by Dara Navaei on 12 October 2022, 10:52 > Done. Reply by Sean Nash on 12 October 2022, 16:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 October 2022, 09:20 https://devapps.diality.us/cru/DG-DEN-13834-1#c14204 Align. Reply by Dara Navaei on 12 October 2022, 09:45 > Done. Reply by Sean Nash on 12 October 2022, 16:30 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ROPump.c Revision Comment by Darren Cox on 19 September 2022, 15:10 https://devapps.diality.us/cru/DG-DEN-13834-1#c13814 F32 needs 0.0F init value. Reply by Dara Navaei on 28 September 2022, 16:35 > Done. Reply by wbracken on 12 October 2022, 16:18 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 28 September 2022, 22:35 https://devapps.diality.us/cru/DG-DEN-13834-1#c14046 Not really an output. Reply by Dara Navaei on 12 October 2022, 10:53 > Done. Reply by wbracken on 12 October 2022, 16:18 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 October 2022, 08:36 https://devapps.diality.us/cru/DG-DEN-13834-1#c14096 POST does nothing? If always in progress, do we get stuck in this test? Reply by Dara Navaei on 12 October 2022, 10:46 > This is not called in the ModeInitPOST. Right now, it does > not do anything. Reply by Sean Nash on 12 October 2022, 16:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 20 September 2022, 17:47 https://devapps.diality.us/cru/DG-DEN-13834-1#c13837 No inputs. Reply by Dara Navaei on 28 September 2022, 16:46 > Done. Reply by wbracken on 12 October 2022, 16:21 > RESOLVED IN CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Heaters.c Revision Comment by wbracken on 20 September 2022, 17:41 https://devapps.diality.us/cru/DG-DEN-13834-1#c13835 Does this actually work? Reply by Dara Navaei on 28 September 2022, 16:53 > Yes, do you see any logic issues in the statement? Reply by wbracken on 12 October 2022, 16:23 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 20 September 2022, 17:43 https://devapps.diality.us/cru/DG-DEN-13834-1#c13836 Should 100.0 be a #define? Reply by Dara Navaei on 28 September 2022, 16:48 > Done. Reply by wbracken on 12 October 2022, 16:21 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 October 2022, 08:29 https://devapps.diality.us/cru/DG-DEN-13834-1#c14094 Can these declarations be moved to top of function? Reply by Dara Navaei on 12 October 2022, 10:51 > Done. Reply by Sean Nash on 12 October 2022, 16:39 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/UVReactors.c Revision Comment by Sean Nash on 12 October 2022, 09:33 https://devapps.diality.us/cru/DG-DEN-13834-1#c14208 Is this a start time (stamp) or a timer (count down/up)? Reply by Dara Navaei on 12 October 2022, 09:41 > No this was the variable that was keeping track of the time > that the reactor was on with no flow. This was to turn on the > reactor if the flow is recovered. Per the SRSs we fault if > the reactors are on with no flow. Reply by Sean Nash on 12 October 2022, 16:29 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 October 2022, 09:35 https://devapps.diality.us/cru/DG-DEN-13834-1#c14212 Either indent code block above or pull back the break to align. Reply by Dara Navaei on 12 October 2022, 09:46 > Done. Reply by Sean Nash on 12 October 2022, 16:28 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 October 2022, 09:34 https://devapps.diality.us/cru/DG-DEN-13834-1#c14211 No loop, so why can't vectorcast see the default? Reply by Dara Navaei on 12 October 2022, 09:40 > This function is called by the exec that loops through the > list of reactors. Reply by Sean Nash on 12 October 2022, 09:51 > But, since there is not loop, you can arrange default to be > reached in vectorcast - so no need to have #ifndef. Reply by Dara Navaei on 12 October 2022, 10:15 > Done. Reply by Sean Nash on 12 October 2022, 16:28 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeFill.c Revision Comment by Sean Nash on 12 October 2022, 09:43 https://devapps.diality.us/cru/DG-DEN-13834-1#c14221 #define name (or at least comment) should mention this is max deviation between redundant sensors. Reply by Dara Navaei on 12 October 2022, 09:58 > Done. Reply by Sean Nash on 12 October 2022, 16:26 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemComm.c Revision Comment by Sean Nash on 04 October 2022, 16:57 https://devapps.diality.us/cru/DG-DEN-13834-1#c14109 Why are we hiding this condition from Vectorcast? Reply by Dara Navaei on 12 October 2022, 10:40 > Please see the comments. Reply by Sean Nash on 12 October 2022, 11:21 > Remove #ifndef and remove the center condition (msgID <= > END_OF_MSG_IDS -1). Reply by Dara Navaei on 12 October 2022, 13:05 > Done. Reply by Sean Nash on 12 October 2022, 16:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Sean Nash on 04 October 2022, 17:00 https://devapps.diality.us/cru/DG-DEN-13834-1#c14110 Can this declaration be moved to top of function? Reply by Dara Navaei on 12 October 2022, 10:35 > Done. Reply by Sean Nash on 12 October 2022, 16:34 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 October 2022, 17:00 https://devapps.diality.us/cru/DG-DEN-13834-1#c14111 Why are we not enforcing payload length check? Reply by Dara Navaei on 17 October 2022, 11:25 > Done. Reply by Sean Nash on 18 October 2022, 11:17 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Darren Cox on 14 October 2022, 12:27 https://devapps.diality.us/cru/DG-DEN-13834-1#c14411 Removed for Phase 1B? Needs comment if so. Reply by Dara Navaei on 14 October 2022, 14:01 > I had added comments on top of the declaration of the > functions but I removed the schedule runs functions. Reply by Sean Nash on 18 October 2022, 11:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 October 2022, 17:02 https://devapps.diality.us/cru/DG-DEN-13834-1#c14112 If this function is not yet needed, should we remove or at least put a #ifdef PHASE_1B build flag around it? Otherwise this is dead code right now. Reply by Dara Navaei on 13 October 2022, 15:32 > Removed the code. Reply by Sean Nash on 18 October 2022, 11:17 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 October 2022, 17:04 https://devapps.diality.us/cru/DG-DEN-13834-1#c14113 Why are we not checking payload length? Reply by Dara Navaei on 17 October 2022, 11:22 > Done. Reply by Sean Nash on 18 October 2022, 11:16 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Darren Cox on 14 October 2022, 12:28 https://devapps.diality.us/cru/DG-DEN-13834-1#c14412 Removed for Phase 1B? Needs comment if so. Reply by Dara Navaei on 14 October 2022, 13:59 > I had added comments on top of the declaration of the > functions but I removed the schedule runs functions. Reply by Sean Nash on 18 October 2022, 11:38 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/DGCommon.h Revision Comment by Sean Nash on 04 October 2022, 17:17 https://devapps.diality.us/cru/DG-DEN-13834-1#c14115 Do we still need this? If so, can we make a s/w config instead? Reply by Dara Navaei on 12 October 2022, 13:04 > Removed the #define. Reply by Sean Nash on 12 October 2022, 16:31 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeFault.c Revision Comment by Sean Nash on 12 October 2022, 09:41 https://devapps.diality.us/cru/DG-DEN-13834-1#c14219 Warrants a comment. Reply by Dara Navaei on 12 October 2022, 10:01 > Done. Reply by Sean Nash on 12 October 2022, 16:26 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/AlarmMgmtSWFaults.h Revision Comment by Sean Nash on 04 October 2022, 16:51 https://devapps.diality.us/cru/DG-DEN-13834-1#c14106 This looks like 104 (not 106). And we should be noting every 5th enum anyway. Reply by Dara Navaei on 12 October 2022, 10:42 > Done. Reply by Sean Nash on 12 October 2022, 16:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 October 2022, 16:52 https://devapps.diality.us/cru/DG-DEN-13834-1#c14107 Is this 109? Reply by Dara Navaei on 12 October 2022, 10:42 > Done. Reply by Sean Nash on 12 October 2022, 16:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 28 September 2022, 10:52 https://devapps.diality.us/cru/DG-DEN-13834-1#c14019 Should be 111. Reply by Dara Navaei on 28 September 2022, 16:37 > That is 110. Reply by wbracken on 12 October 2022, 16:19 > RESOLVED IN CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Reservoirs.c Revision Comment by wbracken on 23 September 2022, 14:39 https://devapps.diality.us/cru/DG-DEN-13834-1#c13950 Update function header. Reply by Dara Navaei on 28 September 2022, 16:43 > Done. Reply by wbracken on 12 October 2022, 16:20 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 October 2022, 16:55 https://devapps.diality.us/cru/DG-DEN-13834-1#c14108 This comment should precede the if statement above. Reply by Dara Navaei on 12 October 2022, 10:41 > Done. Reply by Sean Nash on 12 October 2022, 16:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DrainPump.c Revision Comment by Sean Nash on 12 October 2022, 09:19 https://devapps.diality.us/cru/DG-DEN-13834-1#c14203 Indent break or put {} around default code like other cases. Reply by Dara Navaei on 12 October 2022, 09:46 > Done. Reply by Sean Nash on 12 October 2022, 16:30 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Fans.c Revision Comment by wbracken on 04 October 2022, 11:19 https://devapps.diality.us/cru/DG-DEN-13834-1#c14103 Remove isPOSTComplete. Reply by Dara Navaei on 12 October 2022, 10:43 > Done. Reply by wbracken on 12 October 2022, 16:18 > RESOLVED IN CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Pressures.c Revision Comment by Sean Nash on 12 October 2022, 09:28 https://devapps.diality.us/cru/DG-DEN-13834-1#c14206 Why 2 loops here? Reply by Dara Navaei on 12 October 2022, 09:43 > The first loop is used to convert the counts to pressure once > enough counts have been collected. The second loop is to > monitor the pressure sensors all the time. Reply by Sean Nash on 12 October 2022, 09:49 > I guess what I'm asking is - can loops be combined? Is > there any reason not to convert and monitor each sensor > before moving to next? Reply by Sean Nash on 12 October 2022, 16:30 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Thermistors.c Revision Comment by wbracken on 23 September 2022, 14:25 https://devapps.diality.us/cru/DG-DEN-13834-1#c13949 When will it be implemented? Reply by Dara Navaei on 28 September 2022, 16:37 > This implementation is not required right now. We are not > calibrating temperature sensors. Reply by Sean Nash on 04 October 2022, 08:39 > Can we remove "TODO" since implementation is not required > at this time. Re-phrase comment to something like "If temp > sensors require calibration, implement here." Reply by Dara Navaei on 12 October 2022, 10:55 > Done. Reply by wbracken on 12 October 2022, 16:19 > RESOLVED IN CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Voltages.c Revision Comment by Sean Nash on 12 October 2022, 09:41 https://devapps.diality.us/cru/DG-DEN-13834-1#c14217 Why aren't these being filtered like other FPGA voltages in getFpgaADC()? Reply by Dara Navaei on 12 October 2022, 10:12 > These are implemented like HD is implemented. Reply by Sean Nash on 12 October 2022, 16:27 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Voltages.h Revision Comment by Sean Nash on 12 October 2022, 09:36 https://devapps.diality.us/cru/DG-DEN-13834-1#c14213 1V is expected. Reply by Dara Navaei on 12 October 2022, 10:13 > Done. Reply by Sean Nash on 12 October 2022, 16:27 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 October 2022, 09:36 https://devapps.diality.us/cru/DG-DEN-13834-1#c14214 1.8V is expected. Reply by Dara Navaei on 12 October 2022, 10:14 > Done. Reply by Sean Nash on 12 October 2022, 16:27 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 October 2022, 09:36 https://devapps.diality.us/cru/DG-DEN-13834-1#c14215 0V is expected. Reply by Dara Navaei on 12 October 2022, 10:14 > Done. Reply by Sean Nash on 12 October 2022, 16:27 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/LoadCell.c Revision Comment by Sean Nash on 04 October 2022, 08:31 https://devapps.diality.us/cru/DG-DEN-13834-1#c14095 Remove blank line. Reply by Dara Navaei on 12 October 2022, 10:47 > Done. Reply by Sean Nash on 12 October 2022, 16:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 06 October 2022, 11:13 https://devapps.diality.us/cru/DG-DEN-13834-1#c14136 Why the change? Reply by Dara Navaei on 18 October 2023, 21:05 > RESOLVED IN CODE WALKTHROUGH Revision Comment by wbracken on 06 October 2022, 11:16 https://devapps.diality.us/cru/DG-DEN-13834-1#c14137 Should there be an alarm if loadCellID is invalid? Reply by Sean Nash on 12 October 2022, 09:24 > For handling of Dialin commands, we do not alarm. We want > Dialin support code to be nonintrusive and we will ignore any > Dialin command that isn't valid. Reply by Dara Navaei on 12 October 2022, 09:52 > For the overrides, if the sensor is not in range, the > command is just ignored. Reply by wbracken on 12 October 2022, 16:16 > RESOLVED IN CODE WALKTHROUGH. Revision Comment by wbracken on 06 October 2022, 11:18 https://devapps.diality.us/cru/DG-DEN-13834-1#c14138 Alarm for invalid loadCellID? Reply by Dara Navaei on 12 October 2022, 09:52 > For the overrides, if the sensor is not in range, the command > is just ignored. Reply by wbracken on 12 October 2022, 16:18 > RESOLVED IN CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Drivers/SafetyShutdown.c Revision Comment by Sean Nash on 04 October 2022, 11:29 https://devapps.diality.us/cru/DG-DEN-13834-1#c14104 Is this resolved now? Can we un-comment this code? Reply by Dara Navaei on 13 October 2022, 16:38 > Done. Reply by Sean Nash on 18 October 2022, 11:17 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/WatchdogMgmt.c Revision Comment by Sean Nash on 04 October 2022, 17:14 https://devapps.diality.us/cru/DG-DEN-13834-1#c14114 Is this resolved? Reply by Dara Navaei on 13 October 2022, 16:37 > They are uncommented. Reply by Sean Nash on 18 October 2022, 11:14 > RESOLVED in CODE WALKTHROUGH. Revision Comment by wbracken on 17 October 2022, 11:14 https://devapps.diality.us/cru/DG-DEN-13834-1#c14434 Should be 3.0F Reply by Dara Navaei on 17 October 2022, 11:26 > Done. Reply by wbracken on 17 October 2022, 13:16 > RESOLVED IN CODE WALKTHROUGH --- ID: DG-DEN-13834-1 https://devapps.diality.us/cru/DG-DEN-13834-1 Title: DG-DEN-13834_DG HD Dev HD DG Dvt Update Part 3 Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (2 active, 2 completed*) Sean Nash (*) wbracken (*) Michael Garthwaite Darren Cox