This is a list of all comments for DIALIN-DEN-11750-1. Review Summary: No summary ---------------------------------------- File: dialin/dg/alarms.py Revision Comment by Michael Garthwaite on 22 February 2022, 16:08 https://devapps.diality.us/cru/DIALIN-DEN-11750-1#c12016 Don't think we should have a magic number of 500 for this considering this is done on the HD and DG side. Maybe ALARM_ID_MAX_ALARMS = 500 in alarm_defs.py? Reply by Dara Navaei on 23 February 2022, 11:15 > Done. Reply by Michael Garthwaite on 23 February 2022, 12:53 > RESOLVED. ---------------------------------------- File: dialin/dg/calibration_record.py Revision Comment by Michael Garthwaite on 22 February 2022, 16:21 https://devapps.diality.us/cru/DIALIN-DEN-11750-1#c12018 Is there a specific reason why we create our Observer object after the request has been called? Reply by Dara Navaei on 23 February 2022, 11:05 > This class is used to wait for the results to be received > from firmware. I fixed the order. Reply by Michael Garthwaite on 23 February 2022, 12:53 > RESOLVED. ---------------------------------------- File: dialin/dg/temperatures.py Revision Comment by Michael Garthwaite on 22 February 2022, 16:36 https://devapps.diality.us/cru/DIALIN-DEN-11750-1#c12021 Shouldn't the dictionary format for HD/DG sensors be the same? Do we need dictionaries within the dictionary? Reply by Dara Navaei on 23 February 2022, 11:24 > I made it like HD. Reply by Michael Garthwaite on 23 February 2022, 12:53 > RESOLVED. ---------------------------------------- File: dialin/hd/fans.py Revision Comment by Michael Garthwaite on 01 March 2022, 10:35 https://devapps.diality.us/cru/DIALIN-DEN-11750-1#c12156 I don't have an issue with temporary features or variables being merged into staging but could we not name them so vaguely? Ex: calc_rpm_start_time_removable or calc_rpm_start_time_temporary? Reply by Dara Navaei on 01 March 2022, 12:56 > This variable was very temporary. I deleted it. Reply by Michael Garthwaite on 01 March 2022, 13:06 > RESOLVED. ---------------------------------------- File: dialin/utils/excel_ops.py Revision Comment by Michael Garthwaite on 22 February 2022, 16:42 https://devapps.diality.us/cru/DIALIN-DEN-11750-1#c12022 Who and/or what are we trying to write protect against? Reply by Dara Navaei on 23 February 2022, 10:18 > The software configurations and calibration excel reports are > protected. They are protected because the format of the > reports do not have to change the scripts rely on them to > read the data back and send them to the firmware. The only > cells that are not protected are the cells that the users can > changed the values (i.e. the calibration values). This > feature is optional a report can be created that is not > protected. Reply by Michael Garthwaite on 23 February 2022, 10:55 > RESOLVED. ---------------------------------------- File: dialin/utils/nv_ops_utils.py Revision Comment by Michael Garthwaite on 22 February 2022, 16:18 https://devapps.diality.us/cru/DIALIN-DEN-11750-1#c12017 Please rename this observer class to something more specific. Ex: NVUtilsObserver. Generalized naming may cause problems with advanced usage of the API. Reply by Dara Navaei on 23 February 2022, 11:13 > Done. Reply by Michael Garthwaite on 23 February 2022, 12:55 > RESOLVED. ---------------------------------------- File: tests/hd_blood_leak_data.py Revision Comment by Michael Garthwaite on 22 February 2022, 16:47 https://devapps.diality.us/cru/DIALIN-DEN-11750-1#c12026 Please remove comments if they are no longer applicable to the test. Reply by Dara Navaei on 23 February 2022, 10:16 > This script is in the tests folder. The tests folder is not > controlled in the way the Dialin files are controlled so I > might go back and forth in between different tests. This is > not affecting any of the Dialin features. Reply by Michael Garthwaite on 23 February 2022, 10:23 > For files in the peter folder I understand. However, with > the repo scripts in this folder are typically shared with > cross-functional teams and are "forward facing." Reply by Dara Navaei on 23 February 2022, 11:16 > Agreed. I will clean them up gradually. Removed the > commented code. Reply by Michael Garthwaite on 23 February 2022, 12:55 > RESOLVED. ---------------------------------------- File: tests/peter/set_RTCs.py Revision Comment by Michael Garthwaite on 22 February 2022, 16:44 https://devapps.diality.us/cru/DIALIN-DEN-11750-1#c12023 Why is this test commented out but not the HD? Reply by Dara Navaei on 23 February 2022, 10:17 > This script is in the tests folder. The tests folder is not > controlled in the way the Dialin files are controlled so I > might go back and forth in between different tests. This is > not affecting any of the Dialin features. Last time it was > used to program the HD RTC clock. Reply by Michael Garthwaite on 23 February 2022, 10:55 > RESOLVED. ---------------------------------------- File: tests/peter/test_dg_records.py Revision Comment by Michael Garthwaite on 22 February 2022, 16:45 https://devapps.diality.us/cru/DIALIN-DEN-11750-1#c12024 Please remove comments if they are no longer applicable to the test. Reply by Dara Navaei on 23 February 2022, 10:16 > This script is in the tests folder. The tests folder is not > controlled in the way the Dialin files are controlled so I > might go back and forth in between different tests. This is > not affecting any of the Dialin features. Reply by Michael Garthwaite on 23 February 2022, 10:56 > RESOLVED. ---------------------------------------- File: tests/peter/test_hd_records.py Revision Comment by Michael Garthwaite on 22 February 2022, 16:46 https://devapps.diality.us/cru/DIALIN-DEN-11750-1#c12025 Please remove comments if they are no longer applicable to the test. Reply by Dara Navaei on 23 February 2022, 10:16 > This script is in the tests folder. The tests folder is not > controlled in the way the Dialin files are controlled so I > might go back and forth in between different tests. This is > not affecting any of the Dialin features. Reply by Michael Garthwaite on 23 February 2022, 10:56 > RESOLVED. ---------------------------------------- File: tests/test_hd_dg_fans.py Revision Comment by Michael Garthwaite on 22 February 2022, 16:49 https://devapps.diality.us/cru/DIALIN-DEN-11750-1#c12027 Please remove comments if they are no longer applicable to the test. Reply by Dara Navaei on 23 February 2022, 10:15 > This script is in the tests folder. The tests folder is not > controlled in the way the Dialin files are controlled so I > might go back and forth in between different tests. This is > not affecting any of the Dialin features. Reply by Michael Garthwaite on 23 February 2022, 10:57 > Please see comment in hd_blood_leak_data.py. Reply by Dara Navaei on 23 February 2022, 11:18 > Done. Reply by Michael Garthwaite on 23 February 2022, 12:55 > RESOLVED. ---------------------------------------- File: dialin/dg/ro_pump.py Revision Comment by Michael Garthwaite on 22 February 2022, 16:25 https://devapps.diality.us/cru/DIALIN-DEN-11750-1#c12019 self.measured_raw_flow_rate_my_little_pony to self.measured_raw_flow_rate_lpm. Otherwise publishing will not work. Reply by Dara Navaei on 23 February 2022, 10:28 > Fixed it. Reply by Michael Garthwaite on 23 February 2022, 10:55 > RESOLVED. --- ID: DIALIN-DEN-11750-1 https://devapps.diality.us/cru/DIALIN-DEN-11750-1 Title: DIALIN-DEN-11750_DG Dev Dialysate Temperature Control Tune Up Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (1 active, 2 completed*) Sean Nash (*) Michael Garthwaite (*) Behrouz NematiPour