This is a list of all comments for HD-DEN-1404-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/DialOutFlow.c Revision Comment by Sean Nash on 15 January 2020, 10:21 https://devapps.diality.us/cru/HD-DEN-1404-1#c1033 I know I named this, but I'm regretting it. Outlet pump is not controlled per flow sensor like the others. I think DialOutUF or DialOutLoadCells or DialOutVolume would be better. Reply by lbaloa on 15 January 2020, 13:47 > This change implies a lot of changes in vectorcast, even > breaking most test. I believe the fact that we have a > dialout and dialin is good enough to indicate which pump we > are referring to. I suggest leaving it as is. Reply by lbaloa on 17 January 2020, 11:57 > As discussed, this activity can be post-pone. Reply by Sean Nash on 17 January 2020, 14:12 > RESOLVED in CODE WALKTHROUGH Revision Comment by Dara Navaei on 15 January 2020, 14:08 https://devapps.diality.us/cru/HD-DEN-1404-1#c1053 Why are these variables volatile? Reply by lbaloa on 16 January 2020, 08:58 > Because they are accessed from different interrupts > functions. Reply by Dara Navaei on 16 January 2020, 10:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 15 January 2020, 10:10 https://devapps.diality.us/cru/HD-DEN-1404-1#c1028 Are these variables part of the module or temporary engineering code? Reply by lbaloa on 16 January 2020, 08:57 > Done, part of simulator, #define was used Reply by Sean Nash on 16 January 2020, 10:20 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 16 January 2020, 09:37 https://devapps.diality.us/cru/HD-DEN-1404-1#c1094 Where's newState @param? Reply by lbaloa on 16 January 2020, 09:52 > Done Reply by Sean Nash on 16 January 2020, 10:12 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 15 January 2020, 14:10 https://devapps.diality.us/cru/HD-DEN-1404-1#c1054 Why not just go to switch directly and if there is no change to the current state, exit? Reply by lbaloa on 16 January 2020, 08:57 > Because, when you switch, they might be initialization code. > If you are in pause and mistakenly you switch to pause, you > don't want to run initialization routines. Reply by Dara Navaei on 16 January 2020, 10:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 15 January 2020, 10:00 https://devapps.diality.us/cru/HD-DEN-1404-1#c1022 Should have static (private) function to handle each of these states. Reply by lbaloa on 16 January 2020, 08:54 > State change is relatively simple. Handles are created in > the state machine. Reply by Sean Nash on 17 January 2020, 14:14 > RESOLVED in CODE WALKTHROUGH Revision Comment by Sean Nash on 16 January 2020, 09:39 https://devapps.diality.us/cru/HD-DEN-1404-1#c1095 comment should say "to stop" Reply by lbaloa on 16 January 2020, 09:53 > Done Reply by Sean Nash on 16 January 2020, 10:11 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 15 January 2020, 10:02 https://devapps.diality.us/cru/HD-DEN-1404-1#c1023 I don't see this param in function - cut & paste error? Reply by lbaloa on 16 January 2020, 08:54 > Done Reply by Sean Nash on 16 January 2020, 10:22 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 16 January 2020, 09:52 https://devapps.diality.us/cru/HD-DEN-1404-1#c1101 So this function will be called at a set time interval? What is that interval? Should probably mention it in the function brief as a requirement for the function caller. Reply by lbaloa on 16 January 2020, 09:57 > Done Reply by Sean Nash on 16 January 2020, 10:11 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 15 January 2020, 10:06 https://devapps.diality.us/cru/HD-DEN-1404-1#c1026 This function returns a boolean. Reply by lbaloa on 16 January 2020, 08:53 > Done Reply by Sean Nash on 16 January 2020, 10:21 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 15 January 2020, 10:03 https://devapps.diality.us/cru/HD-DEN-1404-1#c1024 There is a similar #define in common.h. Use that one. Reply by lbaloa on 16 January 2020, 08:52 > Done Reply by Sean Nash on 16 January 2020, 10:22 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 15 January 2020, 10:07 https://devapps.diality.us/cru/HD-DEN-1404-1#c1027 I would prefer to see all of these definitions moved to the top of the module. Reply by lbaloa on 16 January 2020, 08:50 > Done. Reply by Sean Nash on 16 January 2020, 10:20 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 15 January 2020, 10:06 https://devapps.diality.us/cru/HD-DEN-1404-1#c1025 Move this (and any other definitions that may be useful for other modules) to common.h. Reply by lbaloa on 16 January 2020, 08:50 > Done Reply by Sean Nash on 16 January 2020, 10:21 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 15 January 2020, 10:11 https://devapps.diality.us/cru/HD-DEN-1404-1#c1029 If you're casting these to floats, why not just type your parameters to float so you get what you want? Reply by lbaloa on 16 January 2020, 08:49 > I'm reducing unnecessary bytes in communication. Reply by Sean Nash on 16 January 2020, 08:58 > I believe most if not all of these will be floats for the > caller of this function. You're forcing caller to cast to > U16 and then you're casting them back. Reply by lbaloa on 16 January 2020, 09:44 > Yes Reply by Sean Nash on 16 January 2020, 10:20 > With understanding that these are from prescription > settings. RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 15 January 2020, 14:13 https://devapps.diality.us/cru/HD-DEN-1404-1#c1055 Please leave space in paranthesis Revision Comment by Sean Nash on 16 January 2020, 10:19 https://devapps.diality.us/cru/HD-DEN-1404-1#c1111 Put simulator code in #define. Reply by lbaloa on 17 January 2020, 11:55 > Done Reply by Sean Nash on 17 January 2020, 14:10 > RESOLVED in CODE WALKTHROUGH Revision Comment by Sean Nash on 15 January 2020, 10:12 https://devapps.diality.us/cru/HD-DEN-1404-1#c1030 I've been putting module initialize functions first throughout HD code. For consistency, please move to top. Reply by lbaloa on 16 January 2020, 09:43 > Done Reply by Sean Nash on 16 January 2020, 10:16 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 15 January 2020, 14:13 https://devapps.diality.us/cru/HD-DEN-1404-1#c1056 Please leave space in paranthesis Reply by lbaloa on 16 January 2020, 08:47 > Done Reply by Dara Navaei on 16 January 2020, 10:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 15 January 2020, 14:14 https://devapps.diality.us/cru/HD-DEN-1404-1#c1057 Please leave space in paranthesis Reply by lbaloa on 16 January 2020, 08:45 > Done Reply by Dara Navaei on 16 January 2020, 10:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 15 January 2020, 14:15 https://devapps.diality.us/cru/HD-DEN-1404-1#c1058 Please leave space in paranthesis Reply by lbaloa on 16 January 2020, 08:44 > Done Reply by Dara Navaei on 16 January 2020, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 16 January 2020, 09:58 https://devapps.diality.us/cru/HD-DEN-1404-1#c1105 These should be sensors values we can override with dialin. Use the test macros for these to facilitate that. Reply by lbaloa on 17 January 2020, 11:56 > Done. See DialoutFlow.h Reply by Sean Nash on 17 January 2020, 14:11 > RESOLVED in CODE WALKTHROUGH Revision Comment by Dara Navaei on 15 January 2020, 14:16 https://devapps.diality.us/cru/HD-DEN-1404-1#c1059 I think you should move the #defines to the top Reply by lbaloa on 16 January 2020, 08:43 > Done Reply by Dara Navaei on 16 January 2020, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 15 January 2020, 10:14 https://devapps.diality.us/cru/HD-DEN-1404-1#c1031 Put test code in a #ifdef blocks so it's clearer that it's test code and it's easier to turn on/off during development and it's easier to find and remove later. Reply by lbaloa on 16 January 2020, 08:32 > Done! Reply by Sean Nash on 16 January 2020, 10:15 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 15 January 2020, 14:17 https://devapps.diality.us/cru/HD-DEN-1404-1#c1060 You have two magic numbers, I think we need #define for them Reply by lbaloa on 16 January 2020, 08:40 > Done Reply by Dara Navaei on 16 January 2020, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 15 January 2020, 14:30 https://devapps.diality.us/cru/HD-DEN-1404-1#c1064 semicolon missing Reply by lbaloa on 16 January 2020, 08:33 > Done Reply by Sean Nash on 16 January 2020, 09:08 > I think the macro does the ';' so you don't need one. Reply by lbaloa on 16 January 2020, 09:41 > True, but for consistency, I put it Reply by Dara Navaei on 16 January 2020, 10:37 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 15 January 2020, 14:32 https://devapps.diality.us/cru/HD-DEN-1404-1#c1065 I would probably use a #define for the 0.0 Reply by lbaloa on 16 January 2020, 08:36 > Done Reply by Dara Navaei on 16 January 2020, 10:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 15 January 2020, 14:33 https://devapps.diality.us/cru/HD-DEN-1404-1#c1066 Semicolon is missing Reply by lbaloa on 16 January 2020, 08:36 > Done Reply by Dara Navaei on 16 January 2020, 10:30 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 15 January 2020, 10:17 https://devapps.diality.us/cru/HD-DEN-1404-1#c1032 Should be more data published (rotor speed, motor speed, motor controller speed, motor controller current) like other pumps. Reply by lbaloa on 15 January 2020, 13:49 > I suggest implementing them when they are needed. Reply by Sean Nash on 16 January 2020, 09:10 > Sapna will need them as soon as you close out the SDLC > sub-tasks and the story goes to her. Reply by lbaloa on 17 January 2020, 11:58 > Done. More to come. Reply by Sean Nash on 17 January 2020, 14:13 > RESOLVED in CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Common.h Revision Comment by Sean Nash on 15 January 2020, 09:54 https://devapps.diality.us/cru/HD-DEN-1404-1#c1021 This macro appears to be duplicate of INC_WRAP above. Reply by lbaloa on 16 January 2020, 09:06 > I believe INC_WRAP is more flexible. It mostly used in > digital filter implementation. This should be gone once the > digital filter are moved to the once in the module. Reply by Sean Nash on 16 January 2020, 10:26 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/PIControllers.c Revision Comment by Sean Nash on 15 January 2020, 09:49 https://devapps.diality.us/cru/HD-DEN-1404-1#c1020 I think this subtraction is backward. If range reduced control signal to maximum, windupError will be negative here and will end up making errorSum larger. Reply by lbaloa on 16 January 2020, 09:03 > Yeap. Done Reply by Sean Nash on 16 January 2020, 10:28 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/FIRFilters.c Revision Comment by Sean Nash on 16 January 2020, 08:51 https://devapps.diality.us/cru/HD-DEN-1404-1#c1081 Why U16? Seems like float would be the most versatile type. Reply by lbaloa on 16 January 2020, 09:48 > True, my original idea is not to create big math operation. > I thought of signal as unsigned, most of them are. I think > we could change it, but not in this round. I created this > module for DG. When I get back to using it, we can move it > to float. Reply by lbaloa on 17 January 2020, 11:57 > As discussed, this activity can be post-pone Reply by Sean Nash on 17 January 2020, 14:14 > RESOLVED in CODE WALKTHROUGH --- ID: HD-DEN-1404-1 https://devapps.diality.us/cru/HD-DEN-1404-1 Title: HD-DEN-1404_HD Dialysate Outlet Pump/Drive, Flow Sensor, Control Statement of Objectives: State: Closed Summary: Author: lbaloa Moderator: lbaloa Reviewers: (1 active, 3 completed*) Sean Nash (*) Dara Navaei (*) pmontazemi (*) Behrouz NematiPour