This is a list of all comments for DIALIN-DEN-5980-1. Review Summary: No summary ---------------------------------------- File: dialin/dg/calibration.py Revision Comment by plucia on 12 February 2021, 12:12 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8102 This function should be deleted and its usages should be replaced with struct.calcsize instead. For example: {code} >>> struct.calcsize(" I looked at struct's bytes size method. I made this function > because the byte size should not be assumed in anyways, > otherwise, the data will not be mapped correctly to the > firmware structures. Reply by plucia on 24 March 2021, 10:58 > Not sure I follow - Call me when you have a second so we > can discuss Reply by Dara Navaei on 28 March 2021, 10:49 > I changed the location this function to nv_ops_utils.py > and I changed it use the calcsize method. Reply by plucia on 01 April 2021, 09:14 > RESOLVED Revision Comment by plucia on 12 February 2021, 12:13 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8103 This function should have many parameters for all the dg calibration data. That way, you can remove the hardcoded numbers This function should be split up into multiple functions each with a single responsibility. This will improve readability and maintainability. Reply by Dara Navaei on 23 March 2021, 22:01 > This part was used for testing only. I removed it. Do you > mean the hardcoded calibration values? Reply by plucia on 24 March 2021, 09:48 > RESOLVED ---------------------------------------- File: dialin/ui/crc.py Revision Comment by Sean Nash on 17 February 2021, 10:19 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8178 I think this should be moved to either CAN.py (in protocols folder) or in your new utils.py. Reply by Dara Navaei on 23 March 2021, 21:23 > Transferred it to nv_ops_utils module. Reply by Sean Nash on 25 March 2021, 11:04 > RESOLVED. Revision Comment by plucia on 12 February 2021, 17:09 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8106 This should be added to protocols/CAN.py Reply by Dara Navaei on 23 March 2021, 21:58 > I removed this function and added it to nv_ops_utils.py since > crc_16 is only used in NV operations. Reply by plucia on 24 March 2021, 09:53 > RESOLVED ---------------------------------------- File: dialin/dg/system_record.py Revision Comment by plucia on 12 February 2021, 11:25 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8091 The current_message, total_messages, length, and system record variables should be published Reply by Dara Navaei on 23 March 2021, 22:09 > They are added to the publish decorator. Reply by plucia on 24 March 2021, 16:25 > Thanks, are you sure received_msg_length should be > published? I'm not finding that defined anywhere Reply by Dara Navaei on 24 March 2021, 20:56 > I thought I refactored it. The variable length is > refactored to received_ms_length. Reply by Dara Navaei on 29 March 2021, 23:37 > Fixed all the modules. Reply by plucia on 01 April 2021, 09:15 > RESOLVED Revision Comment by plucia on 12 February 2021, 11:53 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8099 This _handler_dg_system_record will be called in a separate thread. If get_dg_system_record is called and clears self.raw_sys_record while you're parsing new data, your data would get corrupted Therefore, it would be best to reduce the scope of self.raw_sys_record where possible. 1. Line 147: Update self.raw_sys_record to a local variable (i.e. raw_sys_record without "self") 2. Line 151: Pass raw_sys_record into _update_dg_system_record_from_fw 3. Return the parsed dg_system_record at the end of _update_dg_system_record_from_fw. Assign it to self.raw_sys_record on line 151. 4. Refactor (rename) _update_dg_system_record_from_fw to _parse_system_record_from_dg Reply by Dara Navaei on 29 March 2021, 23:39 > So I added a flag the prevents another request while a > request to get data from firmware is in progress. Reply by plucia on 01 April 2021, 09:20 > RESOLVED Revision Comment by plucia on 12 February 2021, 12:03 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8100 self.DG_SYSTEM_RECORD is not a constant so it should be all lowercase instead of all caps (PEP8) Reply by Dara Navaei on 23 March 2021, 22:05 > I made it a lower case name. Reply by plucia on 24 March 2021, 09:50 > RESOLVED Revision Comment by plucia on 12 February 2021, 11:28 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8092 This function should be split up into functions that each have a single responsibility Reply by Dara Navaei on 23 March 2021, 22:05 > I split the function into smaller pieces. Reply by plucia on 24 March 2021, 09:54 > RESOLVED ---------------------------------------- File: tests/test_dg_system_record.py Revision Comment by plucia on 12 February 2021, 12:17 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8105 While they are not mission critical, let's make an effort to keep these test functions clean by removing unused code. Reply by Dara Navaei on 23 March 2021, 22:00 > Will be removed. Reply by plucia on 24 March 2021, 09:53 > RESOLVED ---------------------------------------- File: dialin/dg/service_record.py Revision Comment by plucia on 12 February 2021, 11:41 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8093 This function should be split up into separate functions that have a single responsibility Reply by Dara Navaei on 23 March 2021, 22:08 > I split the function. Reply by plucia on 24 March 2021, 09:52 > RESOLVED Revision Comment by plucia on 12 February 2021, 11:42 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8094 This is a good place to use the logger (e.g. self.logger.debug(...)) instead of print Reply by Dara Navaei on 23 March 2021, 22:07 > I removed that particular print statement, but I am using the > logger. Reply by plucia on 24 March 2021, 09:47 > RESOLVED ---------------------------------------- File: tests/test_dg_service_record.py Revision Comment by plucia on 12 February 2021, 12:15 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8104 While they are not mission critical, let's make an effort to keep these test functions clean by removing unused code. Reply by Dara Navaei on 28 March 2021, 10:39 > Done. Most of these test files will not be needed soon. Reply by plucia on 01 April 2021, 09:20 > RESOLVED ---------------------------------------- File: dialin/dg/calibration_record.py Revision Comment by Sean Nash on 17 February 2021, 09:49 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8164 Add comment for this. Reply by Dara Navaei on 23 March 2021, 21:55 > Done. Reply by Sean Nash on 25 March 2021, 10:55 > RESOLVED. Revision Comment by Sean Nash on 17 February 2021, 09:49 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8165 Clarify in seconds. Reply by Dara Navaei on 23 March 2021, 21:55 > Done. Reply by Sean Nash on 25 March 2021, 10:57 > RESOLVED. Revision Comment by Sean Nash on 17 February 2021, 09:54 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8167 Message length? Payload length? Reply by Dara Navaei on 23 March 2021, 21:52 > I renamed it. Reply by Sean Nash on 25 March 2021, 10:57 > RESOLVED. ---------------------------------------- File: dialin/dg/scheduled_runs_record.py Revision Comment by Sean Nash on 17 February 2021, 10:07 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8170 Consider renaming all of the NV record modules/classes with record_ or nv_record_ prefix or maybe putting them in a sub-folder of dg so they are easily identified for their general purpose. Reply by Dara Navaei on 23 March 2021, 21:50 > Done. Reply by Sean Nash on 25 March 2021, 10:57 > RESOLVED. ---------------------------------------- File: dialin/utils/utils.py Revision Comment by Sean Nash on 17 February 2021, 09:52 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8166 Consider renaming this module/class to clarify these utility functions are for non-volatile memory support. Reply by Dara Navaei on 23 March 2021, 21:53 > This file and its class has been renamed to better reflect > that it is used for non-volatile operations. Reply by Sean Nash on 25 March 2021, 11:04 > RESOLVED. ---------------------------------------- File: dialin/dg/accelerometer.py Revision Comment by Sean Nash on 17 February 2021, 09:32 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8163 This function won't work now right? Instead of sending msg to DG, should update accel section of DG cal record dictionary and call external function to send whole cal record to DG. Should also call external function to read whole cal record from DG first and then need a get function to pull the accel section from DG cal record dictionary. Reply by Dara Navaei on 23 March 2021, 21:57 > It will no longer work. I removed the function. Reply by Sean Nash on 25 March 2021, 10:59 > RESOLVED. ---------------------------------------- File: dialin/hd/accelerometer.py Revision Comment by Sean Nash on 17 February 2021, 10:13 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8171 Same comment as with DG accelerometer cal function. Reply by Dara Navaei on 23 March 2021, 21:35 > I removed the function. Reply by Sean Nash on 25 March 2021, 10:58 > RESOLVED. ---------------------------------------- File: dialin/hd/blood_flow.py Revision Comment by Sean Nash on 17 February 2021, 10:13 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8172 Same comment as DG accelerometer cal function. Reply by Dara Navaei on 23 March 2021, 21:33 > I removed the function. Reply by Sean Nash on 25 March 2021, 10:59 > RESOLVED. ---------------------------------------- File: dialin/hd/calibration_record.py Revision Comment by Sean Nash on 17 February 2021, 10:15 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8174 Comment to clarify purpose. Reply by Dara Navaei on 23 March 2021, 21:31 > Done. Reply by Sean Nash on 25 March 2021, 10:59 > RESOLVED. Revision Comment by Sean Nash on 17 February 2021, 10:15 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8175 Clarify in seconds. Reply by Dara Navaei on 23 March 2021, 21:28 > Done Reply by Sean Nash on 25 March 2021, 11:03 > RESOLVED. Revision Comment by Sean Nash on 17 February 2021, 10:15 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8176 Length of what? Reply by Dara Navaei on 23 March 2021, 21:28 > It is the length of the received message. I renamed it. Reply by Sean Nash on 25 March 2021, 11:03 > RESOLVED. ---------------------------------------- File: dialin/hd/dialysate_inlet_flow.py Revision Comment by Sean Nash on 17 February 2021, 10:14 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8173 Same comment as DG accelerometer cal function. Reply by Dara Navaei on 23 March 2021, 21:32 > I removed the function. Reply by Sean Nash on 25 March 2021, 11:03 > RESOLVED. ---------------------------------------- File: dialin/hd/hemodialysis_device.py Revision Comment by Sean Nash on 17 February 2021, 10:16 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8177 Do these get/set functions still work? Do they need updating or are they obsolete? Reply by Dara Navaei on 23 March 2021, 21:26 > I removed them. Reply by Sean Nash on 25 March 2021, 11:03 > RESOLVED. ---------------------------------------- File: dialin/hd/service_record.py Revision Comment by plucia on 01 April 2021, 13:16 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8932 This should be received_msg_length Reply by Dara Navaei on 01 April 2021, 14:52 > Done. Reply by plucia on 01 April 2021, 14:56 > RESOLVED ---------------------------------------- File: dialin/hd/system_record.py Revision Comment by plucia on 01 April 2021, 13:16 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8931 This should be received_msg_length Reply by Dara Navaei on 01 April 2021, 14:52 > Done. Reply by plucia on 01 April 2021, 14:56 > RESOLVED ---------------------------------------- File: dialin/protocols/CAN.py Revision Comment by plucia on 01 April 2021, 09:24 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8900 It looks like all instances of length were renamed to received_msg_length in 551e0a9620126efd158e93c45e9ed84ee6ec85fb Reply by Dara Navaei on 01 April 2021, 12:57 > Done. Reply by plucia on 01 April 2021, 14:58 > RESOLVED Revision Comment by plucia on 01 April 2021, 09:25 https://devapps.diality.us/cru/DIALIN-DEN-5980-1#c8903 This should remain unchanged Reply by Dara Navaei on 01 April 2021, 12:56 > Done. Reply by plucia on 01 April 2021, 12:57 > RESOLVED --- ID: DIALIN-DEN-5980-1 https://devapps.diality.us/cru/DIALIN-DEN-5980-1 Title: DIALIN-DEN-5980_Non-volatile Data Management 2 of 2 Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (1 active, 2 completed*) Sean Nash (*) plucia (*) pmontazemi