This is a list of all comments for DG-DEN-6200-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Services/AlarmMgmt.c Revision Comment by pmontazemi on 15 January 2021, 10:44 https://devapps.diality.us/cru/DG-DEN-6200-1#c7413 Where is alarmIsActive[] input in the code? Reply by qnguyen on 15 January 2021, 11:47 > Combined getAlarmId and isAlarmActive functions. Reply by pmontazemi on 18 January 2021, 10:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 15 January 2021, 10:04 https://devapps.diality.us/cru/DG-DEN-6200-1#c7396 Do you think this function should be moved to PersistentAlarm.c? Reply by qnguyen on 15 January 2021, 10:30 > I think this wrapper should stay with DG or HD, so they can > have their own implementation. Reply by Sean Nash on 15 January 2021, 10:34 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Sean Nash on 18 January 2021, 10:00 https://devapps.diality.us/cru/DG-DEN-6200-1#c7461 If we are using a single message to respond to multiple commands, should we include a parameter indicating which command we are responding to? Reply by qnguyen on 18 January 2021, 13:58 > Added command id to command response. Reply by Sean Nash on 20 January 2021, 10:39 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ConductivitySensors.c Revision Comment by Sean Nash on 14 January 2021, 15:51 https://devapps.diality.us/cru/DG-DEN-6200-1#c7375 This does not look compatible with changes to PersistentAlarm module. Does this compile? I see this everywhere. I suspect you changed all of these and just need to push main DG branch. Reply by qnguyen on 14 January 2021, 15:59 > On the DG side, I move this function into Alarm Management > module. I find it better than individually handle them one by > one and it still gives flexibility in case we need to handle > another persistent alarm differently. Reply by Sean Nash on 14 January 2021, 16:05 > Let's talk about it. If there's a good reason, fine. But > generally I'd like HD and DG to do similar things in the > same manner for consistency. Reply by Sean Nash on 15 January 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeFill.c Revision Comment by Sean Nash on 15 January 2021, 10:05 https://devapps.diality.us/cru/DG-DEN-6200-1#c7397 TODO? Reply by qnguyen on 15 January 2021, 10:32 > I think the flow rate out of range alarm should be handled by > the RO pump driver. Reply by Sean Nash on 15 January 2021, 10:34 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: PersistentAlarm.c Revision Comment by pmontazemi on 15 January 2021, 11:24 https://devapps.diality.us/cru/DG-DEN-6200-1#c7422 Remove extra spaces before = sign. Reply by qnguyen on 15 January 2021, 11:34 > Extra spaces is to keep all the variables align. > Removed extra space below to keep alignment. Reply by pmontazemi on 18 January 2021, 10:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 14 January 2021, 17:57 https://devapps.diality.us/cru/DG-DEN-6200-1#c7388 errorOccurredStartTime is confusing name considering how it's being used. Reply by qnguyen on 14 January 2021, 19:16 > Updated logic flow to make variable name easier to understand > and less confusing. Reply by Dara Navaei on 19 October 2023, 08:15 > RESOLVED in CODE WALKTHROUGH ---------------------------------------- File: PersistentAlarm.h Revision Comment by pmontazemi on 15 January 2021, 11:25 https://devapps.diality.us/cru/DG-DEN-6200-1#c7423 Why and where were these persistent alarm moved to? Reply by qnguyen on 15 January 2021, 11:32 > With the new design of persistent alarm, we no longer need to > keep a separate list of persistent alarm id. Therefore, these > have been deleted. Reply by pmontazemi on 18 January 2021, 10:40 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: HDDefs.h Revision Comment by Sean Nash on 14 January 2021, 15:31 https://devapps.diality.us/cru/DG-DEN-6200-1#c7368 I know these enums are in a header file with "HD" in the name and so the HD_ prefix may seem unnecessary, but I was putting HD_ and DG_ prefixes on mode state enums in case the HD and DG had one or more modes with the same name (like Standby) so there would not be a conflict if somebody wanted to include both HD and DG definitions in there module. Reply by qnguyen on 14 January 2021, 16:22 > Added HD_ prefix. Reply by Sean Nash on 15 January 2021, 10:37 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Reservoirs.c Revision Comment by Sean Nash on 18 January 2021, 09:55 https://devapps.diality.us/cru/DG-DEN-6200-1#c7460 What parameter is invalid? Reply by qnguyen on 18 January 2021, 13:55 > The dialysate temperature HD sent to DG as another message. Reply by Sean Nash on 20 January 2021, 10:41 > Maybe new reason like dialysate temperature not set? Reply by qnguyen on 20 January 2021, 12:37 > Function has been moved to heater module. > Target temperature has been added as part of the trimmer > heater start command. Reply by Sean Nash on 21 January 2021, 10:36 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Heaters.c Revision Comment by Sean Nash on 21 January 2021, 09:46 https://devapps.diality.us/cru/DG-DEN-6200-1#c7547 Heat disinfect may need this function to set target temp to 85°C. Reply by qnguyen on 21 January 2021, 10:19 > Reverted the deletion. Reply by Sean Nash on 21 January 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 January 2021, 09:44 https://devapps.diality.us/cru/DG-DEN-6200-1#c7546 How will DG control the trimmer heater (e.g. in heat disinfect mode) without this function? Trimmer heater not always started by HD command message. Reply by qnguyen on 21 January 2021, 10:19 > Reverted the deletion. Reply by Sean Nash on 21 January 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-6200-1 https://devapps.diality.us/cru/DG-DEN-6200-1 Title: DG-DEN-6200_HD Disposable Priming Dialyzer Side Statement of Objectives: State: Closed Summary: Author: qnguyen Reviewers: (0 active, 3 completed*) Sean Nash (*) Dara Navaei (*) pmontazemi (*)