This is a list of all comments for DIALIN-DEN-9480-1. Review Summary: No summary ---------------------------------------- File: dialin/common/msg_ids.py Revision Comment by Sean Nash on 09 November 2021, 13:47 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11186 Where is 0xA044? Reply by Dara Navaei on 09 November 2021, 21:04 > It was not in my branch but I added it. Reply by Sean Nash on 11 November 2021, 09:46 > RESOLVED. ---------------------------------------- File: dialin/dg/heaters.py Revision Comment by Sean Nash on 09 November 2021, 13:55 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11190 Should efficiency be a float (0.0)? Reply by Dara Navaei on 09 November 2021, 21:01 > I made it a float but in Python it does not matter the data > types can be changed. Reply by Sean Nash on 10 November 2021, 08:50 > I think it still helps with readability. A 0.0 initialize > value indicates that the developer intends this to be a > float. Reply by Sean Nash on 11 November 2021, 09:45 > RESOLVED. Revision Comment by Sean Nash on 09 November 2021, 13:56 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11191 I don't see trimmer_heater_state being saved anywhere. If doing [0] tuple indexing here, just assign directly to class variables. Reply by Dara Navaei on 09 November 2021, 20:59 > I added the trimmer heaters state. Reply by Sean Nash on 11 November 2021, 09:45 > RESOLVED. ---------------------------------------- File: dialin/dg/rtc.py Revision Comment by Sean Nash on 09 November 2021, 14:00 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11193 This brief looks incomplete. Reply by Dara Navaei on 09 November 2021, 20:59 > Done. Reply by Sean Nash on 11 November 2021, 09:44 > RESOLVED. ---------------------------------------- File: dialin/dg/events.py Revision Comment by Sean Nash on 09 November 2021, 13:52 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11188 Shouldn't this be an else if in above if - else? Reply by Dara Navaei on 09 November 2021, 21:03 > I remember there was a case that was not covered so I made > this a separate to make sure it always is executed. Reply by Sean Nash on 10 November 2021, 08:48 > If separated like this, you are likely doing extra work > (assigning list_of_events in above if/else and then > potentially again in this if. Reply by Dara Navaei on 11 November 2021, 12:46 > This is considered as an optimization. I will implement > it in DEN-11114. Reply by Sean Nash on 11 November 2021, 13:54 > RESOLVED. ---------------------------------------- File: dialin/common/dg_defs.py Revision Comment by Sean Nash on 09 November 2021, 13:43 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11178 Should we have a NUM_OF_POST_STATES at the end of this enum like others? Reply by Dara Navaei on 10 November 2021, 09:53 > Done. Reply by Sean Nash on 11 November 2021, 09:47 > RESOLVED. Revision Comment by Sean Nash on 09 November 2021, 13:44 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11179 Should we have a NUM_OF_FLUSH_STATES at end of enum like others? Reply by Dara Navaei on 10 November 2021, 09:53 > Done. Reply by Sean Nash on 11 November 2021, 09:47 > RESOLVED. Revision Comment by Sean Nash on 09 November 2021, 13:45 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11181 Should we have a NUM_OF_HEAT_DISINFECT_UI_STATES at end of enum like others? Reply by Dara Navaei on 10 November 2021, 09:53 > Done. Reply by Sean Nash on 11 November 2021, 09:47 > RESOLVED. Revision Comment by Sean Nash on 09 November 2021, 13:45 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11182 Should we have a NUM_OF_CHEM_DISINFECT_STATES at end of enum like others? Reply by Dara Navaei on 10 November 2021, 09:53 > Done. Reply by Sean Nash on 11 November 2021, 09:47 > RESOLVED. Revision Comment by Sean Nash on 09 November 2021, 13:45 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11183 Should we have a NUM_OF_CHEM_DISINFECT_UI_STATES at end of enum like others? Reply by Dara Navaei on 10 November 2021, 09:52 > Done. Reply by Sean Nash on 11 November 2021, 09:46 > RESOLVED. Revision Comment by Sean Nash on 09 November 2021, 13:45 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11184 Should we have a NUM_OF_DG_EVENTS at end of enum? Reply by Dara Navaei on 10 November 2021, 09:45 > Done. Reply by Sean Nash on 11 November 2021, 09:46 > RESOLVED. Revision Comment by Sean Nash on 09 November 2021, 13:46 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11185 Should we have a NUM_OF_DG_EVENT_DATA_TYPES at end of enum? Reply by Dara Navaei on 10 November 2021, 09:46 > Done. Reply by Sean Nash on 11 November 2021, 09:46 > RESOLVED. ---------------------------------------- File: dialin/hd/rtc.py Revision Comment by hnguyen on 11 November 2021, 10:22 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11367 There are 2 returns in this function and why @return: None here? Reply by Dara Navaei on 11 November 2021, 11:11 > Fixed the return. Reply by hnguyen on 11 November 2021, 13:56 > RESOLVED in CODE WALKTHROUGH ---------------------------------------- File: dialin/dg/concentrate_pumps.py Revision Comment by Sean Nash on 09 November 2021, 13:49 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11187 Convention has been to do the [0] tuple indexing when assigned to class variable below. If doing the [0] tuple indexing here, we don't need these temporary variables - can assign directly to the class variables. Reply by Dara Navaei on 10 November 2021, 09:35 > Done. Reply by Sean Nash on 11 November 2021, 09:46 > RESOLVED. ---------------------------------------- File: dialin/dg/fans.py Revision Comment by Sean Nash on 09 November 2021, 13:54 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11189 If doing [0] tuple indexing here, just assign directly to class variable. Reply by Dara Navaei on 10 November 2021, 09:33 > Done. Reply by Sean Nash on 11 November 2021, 09:45 > RESOLVED. ---------------------------------------- File: dialin/hd/switches.py Revision Comment by Sean Nash on 09 November 2021, 14:04 https://devapps.diality.us/cru/DIALIN-DEN-9480-1#c11195 Should we add a NUM_OF_HD_SWITCHES to end of enum? Reply by Dara Navaei on 09 November 2021, 20:58 > I have not done this in Dialin. I do not know if it is > necessary. Reply by Sean Nash on 10 November 2021, 08:51 > I think it is useful to Dialin users to be able to easily > see how many things (switches in this case) are in the > list. They can probably get this from enum.length() call > too I guess, but I see many other enums in Dialin have the > NUM_OF_... to match the C code in f/w. Reply by Dara Navaei on 11 November 2021, 11:14 > It is true we should be consistent. I added the > NUM_OF_... at the end of the enum. Reply by Sean Nash on 11 November 2021, 13:53 > RESOLVED. --- ID: DIALIN-DEN-9480-1 https://devapps.diality.us/cru/DIALIN-DEN-9480-1 Title: DIALIN-DEN-9480_DG_DEV Dialysate Temperature Control Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (1 active, 2 completed*) Sean Nash (*) hnguyen (*) Behrouz NematiPour