This is a list of all comments for DIALIN-1. Review Summary: No summary General Comment by Behrouz NematiPour on 05 December 2019, 10:50 https://devapps.diality.us/cru/DIALIN-1#c477 In general I'm not a fan of having parameter name when passing a value to it. I know some languages like old lady VisualBasic had it, but it's confusing and also give the ability to reorder the functions parameters when passing values which I believe is not good and is making the code hard to understand and read. As you used a very good naming for the values it shows which parameter is used. Reply by pmontazemi on 16 December 2019, 08:03 > RESOLVED. Reply by Behrouz NematiPour on 30 December 2019, 10:38 > RESOLVED General Comment by Behrouz NematiPour on 05 December 2019, 10:53 https://devapps.diality.us/cru/DIALIN-1#c478 There are so many hex values roaming around the code which has not been defined in one place to be able to refer to. Please collect them in one place with a line of description. Or even if it is referring to the "X:\Engineering\Denali\06- Software Design\Messaging\Message List.xlsx" (which I found some there) please make a note to refer to that one if group of them can be read in that excel sheet. Reply by pmontazemi on 16 December 2019, 08:03 > RESOLVED. Reply by Behrouz NematiPour on 30 December 2019, 10:38 > RESOLVED General Comment by Behrouz NematiPour on 05 December 2019, 11:55 https://devapps.diality.us/cru/DIALIN-1#c485 Is it our python coding standard to have an empty line at the begin of each if block ? I think that make code longer and hard to follow.... Reply by pmontazemi on 16 December 2019, 08:02 > RESOLVED. Reply by Behrouz NematiPour on 30 December 2019, 10:38 > RESOLVED General Comment by lbaloa on 05 December 2019, 13:24 https://devapps.diality.us/cru/DIALIN-1#c490 1) If you are referring to keyword arguments, it is allowed per our code standards and it is a standard in python. 2) Documentation for the Messaging list is being done in the excel file that you are referring. You will find that channel IDs are located DenaliChannels class. We need to do some touch ups in the excel document 3) Having a line after each if blocks is part of pep8. Reply by Behrouz NematiPour on 10 December 2019, 16:37 > I don't like if that's pep8 (python coding style) or what > ever We need to have a subset of it for ourselves. and it's > OK. Reply by pmontazemi on 12 December 2019, 15:31 > Behrouz, > If you are suggesting a change wrt PEP8 coding standard, > that is fine. But, you will also need to make that change > to our Python coding standard, which is currently an exact > copy of the PEP8 coding standard for Python. What is the > decision? Reply by pmontazemi on 13 December 2019, 13:28 > DECISION: will leave as is for now. Reply by pmontazemi on 13 December 2019, 13:29 > RESOLVED. Reply by Behrouz NematiPour on 30 December 2019, 10:35 > Doesn't let me to click complete seem like I need to > put a reply here. > So : > The interpretation of PEP8 were not correct and Leo > suppose to fix it. > > RESOLVED Reply by pmontazemi on 16 December 2019, 08:02 > RESOLVED. Reply by Behrouz NematiPour on 30 December 2019, 10:38 > RESOLVED ---------------------------------------- File: DialityCoreCanProtocol.py Revision Comment by Behrouz NematiPour on 05 December 2019, 12:00 https://devapps.diality.us/cru/DIALIN-1#c486 OK. I found what I want. So why it has not been used for the other hex values ? Reply by lbaloa on 05 December 2019, 13:13 > These hex values were assigned by Sean. I have an idea why? > He is making sure that the number used is the one bit move to > the left. Done. Please resolve. Reply by Dara Navaei on 19 October 2023, 13:21 > RESOLVED Revision Comment by Sean Nash on 26 November 2019, 13:21 https://devapps.diality.us/cru/DIALIN-1#c362 Missing channel for CAN ID 0x200 (UI sync broadcast). Reply by lbaloa on 27 November 2019, 09:43 > Done Reply by Sean Nash on 11 December 2019, 13:22 > RESOLVED Revision Comment by Sean Nash on 26 November 2019, 13:28 https://devapps.diality.us/cru/DIALIN-1#c367 A lot of magic numbers throughout the python code. What is the python equivalent of a #define? Can we create named constants for these values? Reply by lbaloa on 27 November 2019, 09:38 > Done Reply by Sean Nash on 11 December 2019, 13:21 > RESOLVED Revision Comment by pmontazemi on 11 December 2019, 14:32 https://devapps.diality.us/cru/DIALIN-1#c551 Extra line Reply by lbaloa on 12 December 2019, 12:54 > Done Reply by pmontazemi on 12 December 2019, 15:03 > RESOLVED. Revision Comment by pmontazemi on 11 December 2019, 14:32 https://devapps.diality.us/cru/DIALIN-1#c552 Extra line Reply by lbaloa on 12 December 2019, 12:54 > Done Reply by pmontazemi on 12 December 2019, 15:03 > RESOLVED. Revision Comment by pmontazemi on 11 December 2019, 14:33 https://devapps.diality.us/cru/DIALIN-1#c553 Extra line Reply by lbaloa on 12 December 2019, 12:55 > Done Reply by pmontazemi on 12 December 2019, 15:03 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 13:02 https://devapps.diality.us/cru/DIALIN-1#c358 """ Stop listening to the can interface """ Reply by lbaloa on 26 November 2019, 13:28 > Yes, we will stop listening to the can interface. This is > the destructor of the class. Once we are done, it is game > over. Also made the thread a daemon so it will autoclose when > the program exits. Reply by pmontazemi on 06 December 2019, 10:28 > All I was eluding to was to put the header comment to the > method in one line instead of three. Reply by pmontazemi on 11 December 2019, 14:33 > RESOLVED. Revision Comment by Behrouz NematiPour on 05 December 2019, 11:38 https://devapps.diality.us/cru/DIALIN-1#c484 I'm a little confused! Regarding our structure the length of the data in a header should not be more than 3 (without CRC). Or you are getting CRC as part of the data and then take it out maybe? I can't find the section CRC took out. MsgId : 2bytes | A5| MsgId |Len| Data |CRC|..padding..| And please make these values defined in one place(class) and use variables instead of the values. . . Reply by lbaloa on 05 December 2019, 13:18 > There is no CRC implemented yet. Len can go all the way to > 255. Each message is broken in 8-byte packets. Reply by Behrouz NematiPour on 05 December 2019, 13:23 > Its place should be reserved then and must not be used for > data. > So when it has been implemented only a simple change will > be required. Reply by lbaloa on 13 December 2019, 14:30 > Replace with PAYLOAD_LENGTH_FIRST_PACKET setup to 3. > Please resolve Reply by Behrouz NematiPour on 17 December 2019, 11:19 > RESOLVED Revision Comment by pmontazemi on 13 December 2019, 13:33 https://devapps.diality.us/cru/DIALIN-1#c594 4 to become a CONSTANT defined somewhere else. Reply by lbaloa on 13 December 2019, 14:29 > Done Reply by pmontazemi on 16 December 2019, 08:04 > RESOLVED. Revision Comment by pmontazemi on 11 December 2019, 14:33 https://devapps.diality.us/cru/DIALIN-1#c554 Indentation misaligned. Reply by lbaloa on 12 December 2019, 12:58 > Done Reply by pmontazemi on 12 December 2019, 15:04 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 13:03 https://devapps.diality.us/cru/DIALIN-1#c359 Not a big fan of sleep() in general. Reply by lbaloa on 26 November 2019, 13:29 > We need to sleep, otherwise, we wouldn't let other tasks run. Reply by Sean Nash on 26 November 2019, 13:34 > I'm ok with sleep generally, but not sure it's appropriate > here as it is currently implemented. > Here, it appears we're pacing ourselves at no more than 1000 > (packets or messages?) per second. Maybe ok over time or > maybe not - what if we're publishing more than 1000 packets > per second when all is said and done? But even if we're not > publishing that many packets, I imagine data may come in > spurts that will exceed this pace. Is there a buffer holding > received packets until this loop "receives" them? If so, how > deep is the buffer? If not, I fear we may drop some packets. Reply by pmontazemi on 13 December 2019, 13:32 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 13:04 https://devapps.diality.us/cru/DIALIN-1#c361 What about if not? Reply by lbaloa on 26 November 2019, 13:22 > If it is not, it will be ignored. Denali message must be > multiple of 8 bytes Reply by Sean Nash on 26 November 2019, 13:27 > I wouldn't say it's ignored. I would say it's already a > multiple of 8 in the "else" case - so no need to pad it. Reply by pmontazemi on 06 December 2019, 10:26 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 13:04 https://devapps.diality.us/cru/DIALIN-1#c360 With the number of arguments, one if and elif might not be enough to handle all cases. Reply by lbaloa on 27 November 2019, 09:44 > I would need to explain logic. let's meet Reply by pmontazemi on 13 December 2019, 13:29 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 16:08 https://devapps.diality.us/cru/DIALIN-1#c373 Is the payload length constrained by its definition or it can accidentally become negative? Reply by lbaloa on 27 November 2019, 08:52 > payload is constrainted by definition as unsigned number from > 0 to 255. Reply by pmontazemi on 06 December 2019, 10:26 > RESOLVED. ---------------------------------------- File: HemodialysisDevice.py Revision Comment by pmontazemi on 26 November 2019, 16:09 https://devapps.diality.us/cru/DIALIN-1#c374 How is the message Timeout!!!! related to received_message being equal to None? Reply by pmontazemi on 11 December 2019, 14:36 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 16:10 https://devapps.diality.us/cru/DIALIN-1#c375 How is the message Timeout!!!! related to received_message being equal to None? Reply by pmontazemi on 11 December 2019, 14:36 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 16:10 https://devapps.diality.us/cru/DIALIN-1#c377 Shouldn't this be rst? Reply by Sean Nash on 03 December 2019, 13:29 > reset is the integer value passed into the method. rst is > the integer value converted to a byte array to be > concatenated into the message. Reply by pmontazemi on 06 December 2019, 10:25 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 16:10 https://devapps.diality.us/cru/DIALIN-1#c376 How is the message Timeout!!!! related to received_message being equal to None? Reply by pmontazemi on 11 December 2019, 14:36 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 16:11 https://devapps.diality.us/cru/DIALIN-1#c378 How is the message Timeout!!!! related to received_message being equal to None? Reply by pmontazemi on 11 December 2019, 14:36 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 16:11 https://devapps.diality.us/cru/DIALIN-1#c379 How is the message Timeout!!!! related to received_message being equal to None? Reply by pmontazemi on 11 December 2019, 14:36 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 16:11 https://devapps.diality.us/cru/DIALIN-1#c380 How is the message Timeout!!!! related to received_message being equal to None? Reply by pmontazemi on 11 December 2019, 14:35 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 16:11 https://devapps.diality.us/cru/DIALIN-1#c381 How is the message Timeout!!!! related to received_message being equal to None? Reply by pmontazemi on 11 December 2019, 14:35 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 16:11 https://devapps.diality.us/cru/DIALIN-1#c382 How is the message Timeout!!!! related to received_message being equal to None? Reply by pmontazemi on 11 December 2019, 14:35 > RESOLVED. Revision Comment by Behrouz NematiPour on 05 December 2019, 10:58 https://devapps.diality.us/cru/DIALIN-1#c480 How this message has been split in section? How next person will now the structure? Reply by Sean Nash on 10 December 2019, 13:11 > Message structure defined in our Messages spreadsheet and > eventually in our SDDs. Reply by Sean Nash on 17 December 2019, 08:22 > Created constants for field positions. Reply by Behrouz NematiPour on 17 December 2019, 12:24 > RESOLVED Revision Comment by pmontazemi on 26 November 2019, 16:11 https://devapps.diality.us/cru/DIALIN-1#c383 How is the message Timeout!!!! related to received_message being equal to None? Reply by pmontazemi on 11 December 2019, 14:35 > RESOLVED. Revision Comment by Behrouz NematiPour on 05 December 2019, 11:04 https://devapps.diality.us/cru/DIALIN-1#c481 Please Change the Cargo variable name to payload... Reply by Sean Nash on 10 December 2019, 13:01 > Done. Reply by Behrouz NematiPour on 17 December 2019, 12:22 > Two cargo(s) > resolved for now Reply by Behrouz NematiPour on 17 December 2019, 12:22 > RESOLVED Revision Comment by Behrouz NematiPour on 05 December 2019, 11:05 https://devapps.diality.us/cru/DIALIN-1#c483 What is the 5th element? It has been mentioned every where with no definition. Reply by Sean Nash on 10 December 2019, 13:06 > The 5th byte is where the payload starts. I copied this from > some code Leo had - but I agree we should clean this up a > bit. Reply by Sean Nash on 17 December 2019, 08:28 > Used constant. Reply by Behrouz NematiPour on 17 December 2019, 12:20 > RESOLVED Revision Comment by Behrouz NematiPour on 05 December 2019, 11:05 https://devapps.diality.us/cru/DIALIN-1#c482 Please Change the Cargo variable name to payload... Reply by Sean Nash on 10 December 2019, 13:09 > Done. Reply by Behrouz NematiPour on 17 December 2019, 12:21 > funny that there were two cargo(s). > One has been changed only. > resolve for now. Reply by Behrouz NematiPour on 17 December 2019, 12:21 > RESOLVED Revision Comment by pmontazemi on 26 November 2019, 16:11 https://devapps.diality.us/cru/DIALIN-1#c384 How is the message Timeout!!!! related to received_message being equal to None? Reply by pmontazemi on 11 December 2019, 14:35 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 16:11 https://devapps.diality.us/cru/DIALIN-1#c385 How is the message Timeout!!!! related to received_message being equal to None? Reply by Sean Nash on 10 December 2019, 13:14 > If the send method returns None, that means it sent the > message and waited 1 second and got no response. Reply by pmontazemi on 11 December 2019, 14:35 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 16:12 https://devapps.diality.us/cru/DIALIN-1#c386 How is the message Timeout!!!! related to received_message being equal to None? Reply by Sean Nash on 03 December 2019, 13:27 > If we haven't received a reply (indicated by None) within 1 > second of transmit of command, then we timed out. Reply by pmontazemi on 06 December 2019, 10:16 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 16:12 https://devapps.diality.us/cru/DIALIN-1#c387 Can blood flow rate variable be negative by accident? If so, that case needs to be handled. Reply by Sean Nash on 03 December 2019, 13:25 > Logic here is determining zero vs. non-zero only. Sign is > not considered here one way or the other. Reply by pmontazemi on 06 December 2019, 10:16 > RESOLVED. ---------------------------------------- File: Miscellaneous/HD_TestScript.py Revision Comment by pmontazemi on 26 November 2019, 13:01 https://devapps.diality.us/cru/DIALIN-1#c355 Not a big fan of sleeps in general. Reply by lbaloa on 26 November 2019, 13:24 > This is just test code. Any suggestion? Reply by pmontazemi on 11 December 2019, 14:37 > Sometimes Python needs a brief sleep(0.001) or sleep(0.01) > to do certain things like SCI/UART/LIN or handle HW > interrupts. Without these, Python crashes or throws in a > timeout error or warning of some sort. Anywhere else where > there is no need to have a sleep, there should be none. Reply by pmontazemi on 13 December 2019, 13:35 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 13:00 https://devapps.diality.us/cru/DIALIN-1#c352 Make 2000 a centralized parameter at the top of the file or class. Reply by pmontazemi on 26 December 2019, 11:28 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 13:01 https://devapps.diality.us/cru/DIALIN-1#c354 How are we handling negative values in case they are generated by mistake? Reply by pmontazemi on 13 December 2019, 13:35 > Blood flow can be negative, hence, it is handled. Reply by pmontazemi on 13 December 2019, 13:36 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 13:00 https://devapps.diality.us/cru/DIALIN-1#c353 Make 2000 a centralized parameter at the top of the file or class. Reply by Sean Nash on 03 December 2019, 13:31 > This is just an example test script using the HD library. I > imagined a tester wanting to set a specific interval and I > don't imagine testers will be creating constants or enums for > their code. Reply by pmontazemi on 06 December 2019, 10:28 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 13:01 https://devapps.diality.us/cru/DIALIN-1#c356 How are we handling negative values in case they are generated by mistake? Reply by Sean Nash on 03 December 2019, 13:34 > Negative values are allowed for flow rates. Negative > indicates reverse direction. I have to assume tester knows > what they're commanding. Reply by pmontazemi on 06 December 2019, 10:29 > RESOLVED. Revision Comment by pmontazemi on 26 November 2019, 13:01 https://devapps.diality.us/cru/DIALIN-1#c357 Make 200 a centralized parameter at the top of the file or class. Reply by pmontazemi on 06 December 2019, 10:29 > RESOLVED. --- ID: DIALIN-1 https://devapps.diality.us/cru/DIALIN-1 Title: Dialin Review Statement of Objectives: State: Closed Summary: Author: lbaloa Moderator: lbaloa Reviewers: (0 active, 4 completed*) Sean Nash (*) Dara Navaei (*) Behrouz NematiPour (*) pmontazemi (*)