This is a list of all comments for HD-DEN-4308-3. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/Valves.c Revision Comment by Sean Nash on 27 August 2020, 11:05 https://devapps.diality.us/cru/HD-DEN-4308-3#c4041 No copyright? Reply by Dara Navaei on 01 September 2020, 13:38 > Copyright will be automatically added in the Bamboo build. Reply by Sean Nash on 23 September 2020, 11:24 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 13:04 https://devapps.diality.us/cru/HD-DEN-4308-3#c4058 Make a separate #define for max delta counts +/- target position. Reply by Dara Navaei on 29 August 2020, 19:15 > Could you please be more specific? Reply by Sean Nash on 31 August 2020, 08:17 > You are using this step change #define for step size (as > name implies) but also for a maximum delta from your target > position. Though they share the same value at this time > (1000), they are two different things. Reply by Dara Navaei on 01 September 2020, 10:41 > I use #define MAX_DEVIATION_FROM_TARGET_IN_COUNTS for > deviation check Reply by Sean Nash on 23 September 2020, 11:20 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 15:25 https://devapps.diality.us/cru/HD-DEN-4308-3#c4062 This appears to be a homing step timeout - rename and use task interval to clarify timing. Reply by Dara Navaei on 29 August 2020, 19:05 > Done Reply by Sean Nash on 23 September 2020, 11:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 11:09 https://devapps.diality.us/cru/HD-DEN-4308-3#c4043 Make this based on task interval. Reply by Dara Navaei on 01 September 2020, 10:56 > Done Reply by Sean Nash on 23 September 2020, 11:23 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 08:53 https://devapps.diality.us/cru/HD-DEN-4308-3#c4727 Add more to comment. This is the 18th pin on HET port 1 that we're re-purposing for GPIO to actuate the air trap valve? Reply by Dara Navaei on 22 September 2020, 13:53 > Added more comment to explain that. Reply by Sean Nash on 23 September 2020, 11:13 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 18 September 2020, 08:08 https://devapps.diality.us/cru/HD-DEN-4308-3#c4716 Aligned doxygen comment. Reply by Dara Navaei on 22 September 2020, 11:09 > Done Reply by qnguyen on 23 September 2020, 10:48 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 11:11 https://devapps.diality.us/cru/HD-DEN-4308-3#c4044 Seems like idle and in transition states should come after homing states. Reply by Dara Navaei on 01 September 2020, 10:55 > Done Reply by Sean Nash on 23 September 2020, 11:23 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 09:14 https://devapps.diality.us/cru/HD-DEN-4308-3#c4737 Enums and structs should go in private definitions section above. They are not data. Reply by Dara Navaei on 22 September 2020, 14:01 > Done Reply by Sean Nash on 23 September 2020, 11:10 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 08:57 https://devapps.diality.us/cru/HD-DEN-4308-3#c4728 How is status different from mode above? Why do we need both? Reply by Dara Navaei on 22 September 2020, 13:54 > The status is used to check the bit status according to the > mode that we are in. For instance, if we are in bypass mode, > PID enabled must be 0 and bypass enable must be 1. PID > initialized will be always 1. Reply by Sean Nash on 23 September 2020, 11:13 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 18 September 2020, 07:58 https://devapps.diality.us/cru/HD-DEN-4308-3#c4714 Need doxygen comment. Reply by Dara Navaei on 22 September 2020, 11:06 > Done Reply by qnguyen on 23 September 2020, 10:50 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 08:58 https://devapps.diality.us/cru/HD-DEN-4308-3#c4729 Can we elaborate on direction in comments? Is clockwise moving towards energized edge? Reply by Dara Navaei on 22 September 2020, 13:57 > Done Reply by Sean Nash on 23 September 2020, 11:12 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 09:00 https://devapps.diality.us/cru/HD-DEN-4308-3#c4730 1-byte alignment does not appear to be necessary here. If you want to save a little space, 2-byte alignment would make sense. But since we're not serializing these records (e.g. for transmission over CAN), we don't need to pack these records at all. Reply by Dara Navaei on 02 October 2020, 09:18 > Removed them. Reply by Sean Nash on 05 October 2020, 16:11 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 September 2020, 08:25 https://devapps.diality.us/cru/HD-DEN-4308-3#c4396 Insert blank line between declarations and function code. Reply by Dara Navaei on 16 September 2020, 09:22 > Done Reply by Sean Nash on 22 September 2020, 09:11 > Blank line is in wrong place. Reply by Dara Navaei on 05 October 2020, 16:09 > Done Reply by Sean Nash on 05 October 2020, 16:10 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 09:05 https://devapps.diality.us/cru/HD-DEN-4308-3#c4731 Maybe elaborate a little more on comments. For instance, is this edge detection counter really couting edges? I suspect not. Reply by Dara Navaei on 02 October 2020, 09:42 > Improved the comment. Reply by Sean Nash on 05 October 2020, 16:11 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 09:05 https://devapps.diality.us/cru/HD-DEN-4308-3#c4732 Comments should say "self" not "set"? Reply by Dara Navaei on 22 September 2020, 13:58 > Changed to self. Reply by Sean Nash on 23 September 2020, 11:12 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 09:08 https://devapps.diality.us/cru/HD-DEN-4308-3#c4733 Comments need to be a little more verbose. I can't tell what these variables are for based on comments. Use /// on line above if you need more room. Reply by Dara Navaei on 22 September 2020, 13:59 > Done Reply by Sean Nash on 23 September 2020, 11:11 > Remove ///< if you use /// above. Reply by Dara Navaei on 28 September 2020, 08:47 > Done Reply by Sean Nash on 30 September 2020, 10:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 09:12 https://devapps.diality.us/cru/HD-DEN-4308-3#c4736 Why only initializing the state for each valve? Reply by Dara Navaei on 22 September 2020, 14:00 > This is the most important item to be initialized. The rest > will be initialized according to what state the valve is. For > instance, if homing is commanded, edge counter, number of > failed homings and other variables pertaining to homing will > be initialized first. Reply by Sean Nash on 23 September 2020, 11:10 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 September 2020, 09:09 https://devapps.diality.us/cru/HD-DEN-4308-3#c4821 Should the air trap valve be set to closed in init? Reply by Dara Navaei on 23 September 2020, 09:12 > Done Reply by Sean Nash on 23 September 2020, 10:52 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 09:16 https://devapps.diality.us/cru/HD-DEN-4308-3#c4738 check valve param is valid. Reply by Dara Navaei on 22 September 2020, 14:03 > Done Reply by Sean Nash on 23 September 2020, 11:09 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 11:17 https://devapps.diality.us/cru/HD-DEN-4308-3#c4046 Please make counters U32 generally. For this counter, maybe should be VALVE_T? Reply by Dara Navaei on 01 September 2020, 10:53 > Done Reply by Sean Nash on 23 September 2020, 11:22 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 11:13 https://devapps.diality.us/cru/HD-DEN-4308-3#c4045 Why not just use the state in the array of valve status directly? Reply by Dara Navaei on 01 September 2020, 13:50 > Done Reply by Sean Nash on 23 September 2020, 11:22 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 09:17 https://devapps.diality.us/cru/HD-DEN-4308-3#c4739 Check valve and position params are valid. Reply by Dara Navaei on 22 September 2020, 14:04 > Done Reply by Sean Nash on 23 September 2020, 11:09 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 11:21 https://devapps.diality.us/cru/HD-DEN-4308-3#c4048 Is this necessary? Reply by Dara Navaei on 29 August 2020, 19:20 > This is for testing only. I was concentrating on one valve at > the time. Reply by Sean Nash on 23 September 2020, 11:21 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 11:21 https://devapps.diality.us/cru/HD-DEN-4308-3#c4047 Can't you just pass in i here? Reply by Dara Navaei on 01 September 2020, 13:38 > Done Reply by Sean Nash on 23 September 2020, 11:20 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 22 September 2020, 15:21 https://devapps.diality.us/cru/HD-DEN-4308-3#c4795 Should there be an alarm for invalid valve? Reply by Dara Navaei on 28 September 2020, 09:05 > Done Reply by qnguyen on 30 September 2020, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 09:18 https://devapps.diality.us/cru/HD-DEN-4308-3#c4740 Check valve param is valid. Reply by Dara Navaei on 22 September 2020, 14:06 > Done Reply by Sean Nash on 23 September 2020, 11:09 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 18 September 2020, 08:04 https://devapps.diality.us/cru/HD-DEN-4308-3#c4715 Suggesting combining into one line (energized = state == STATE_OPEN). Reply by Dara Navaei on 22 September 2020, 11:08 > Do you mean to use the state enum instead of the boolean > variable that I defined? Reply by qnguyen on 22 September 2020, 15:17 > Rather than use the if statement to assign variable > energized to TRUE, i recommend to assign the if condition > directly to the variable energized. > So the whole if statement can be replaced by one line - > BOOL const energized = (STATE_OPEN == state). > Both has the same result, but the other way has less code. Reply by Dara Navaei on 23 September 2020, 10:04 > We thought about this. I think I would like to keep it > this way to be more readable. Reply by qnguyen on 23 September 2020, 10:50 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 09:23 https://devapps.diality.us/cru/HD-DEN-4308-3#c4741 I recommend we always reverse the order of == conditions so that if we accidentally use = instead of ==, the compiler will give an error. Reply by Dara Navaei on 22 September 2020, 11:08 > Done Reply by Sean Nash on 23 September 2020, 11:09 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 09:25 https://devapps.diality.us/cru/HD-DEN-4308-3#c4742 When will we un-comment the for loop? Reply by Dara Navaei on 22 September 2020, 14:07 > I uncommented the for loop. Reply by Sean Nash on 23 September 2020, 11:08 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 11:22 https://devapps.diality.us/cru/HD-DEN-4308-3#c4049 To be consistent with the many other state machines, we should just return state to executive and let executive assign new state to valvesStatus[ valve ].execState. Reply by Dara Navaei on 01 September 2020, 13:31 > Done Reply by Sean Nash on 23 September 2020, 11:20 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 11:25 https://devapps.diality.us/cru/HD-DEN-4308-3#c4050 Do we need to check whether valve has been homed? I thought we couldn't get to idle state unless homing is completed. Reply by Dara Navaei on 29 August 2020, 19:16 > If the number of homing was exceeded and we failed, we go > back to Idle, but the valve cannot go anywhere. It cannot > receive any command to go to any of the positions. When the > device turned on, we go straight to homing not started state > though. Reply by Sean Nash on 31 August 2020, 08:21 > I would think if you failed to home, you would go to homing > not started (not idle). Reply by Dara Navaei on 01 September 2020, 16:03 > Added a flag that is set to true if homing fails. This > way if homing failed, the state will go back to homing > not started and not idle. Reply by Sean Nash on 23 September 2020, 11:20 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 09:27 https://devapps.diality.us/cru/HD-DEN-4308-3#c4743 Why are we not assigning this directly in the switch statement? Why did we need the local state? Reply by Dara Navaei on 22 September 2020, 14:08 > Removed the local variable. Reply by Sean Nash on 23 September 2020, 11:08 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 11:42 https://devapps.diality.us/cru/HD-DEN-4308-3#c4051 Why do you want absolute value of positions here? And if we do want absolute value, you should use abs(). fabs() is for floating point values. Reply by Dara Navaei on 02 September 2020, 15:50 > Removed fabs and I am using abs instead. I also removed all > the absolute values that are not necessary. Reply by Sean Nash on 23 September 2020, 11:20 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 09:30 https://devapps.diality.us/cru/HD-DEN-4308-3#c4744 Resolve this TODO. Reply by Dara Navaei on 22 September 2020, 14:21 > Done Reply by Sean Nash on 23 September 2020, 11:08 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 21 September 2020, 10:58 https://devapps.diality.us/cru/HD-DEN-4308-3#c4718 Move state assignment outside of if else block since it get assigned to same value. Reply by Dara Navaei on 22 September 2020, 11:12 > Done Reply by qnguyen on 23 September 2020, 10:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 14:40 https://devapps.diality.us/cru/HD-DEN-4308-3#c4059 Comment above says you should subtract here - not add. Check direction on both - I think position B would be addition and C would be subtraction. Reply by Dara Navaei on 29 August 2020, 19:13 > Done Reply by Sean Nash on 23 September 2020, 11:22 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 13:27 https://devapps.diality.us/cru/HD-DEN-4308-3#c4759 I think this compound condition is not right. Self test should have to be complete (not or'd with homing request). Homing request can be or'd with door closed. Reply by Dara Navaei on 22 September 2020, 14:50 > Done Reply by Sean Nash on 23 September 2020, 11:05 > Add () to clarify order of precedence. Remove #ifdef on > door closed. Reply by Dara Navaei on 28 September 2020, 08:52 > Done Reply by Sean Nash on 30 September 2020, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 15:08 https://devapps.diality.us/cru/HD-DEN-4308-3#c4060 This assumes we will be at position zero when we get here. Maybe true if first homing, but if we get here from idle, it may not be the case. I think you should set this target to current position + step change. Reply by Dara Navaei on 29 August 2020, 19:09 > Done Reply by Sean Nash on 23 September 2020, 11:23 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 13:31 https://devapps.diality.us/cru/HD-DEN-4308-3#c4760 Should this only be incremented if major travel progress is not made? As it's coded, I think you're only giving the entire 1,000 step advancement 20 ms to complete. Reply by Dara Navaei on 22 September 2020, 14:25 > Added another elseif to increment it. Reply by Sean Nash on 23 September 2020, 11:05 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 15:22 https://devapps.diality.us/cru/HD-DEN-4308-3#c4061 Don't use fabs() on integers. I don't think you want absolute values of individual positions - just absolute value of the difference - and that might not even be right. The PID may overshoot your 1,000 count advancement target - sometimes by a lot if spring is helping to push. If we overshoot the target by more than max deviation, we should consider that as having reached the target (and then some) and proceed to next advancement target. So absolute value does not seem appropriate here even for the delta. Reply by Dara Navaei on 02 September 2020, 15:51 > Fixed the algorithm. Reply by Sean Nash on 23 September 2020, 11:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 21 September 2020, 11:07 https://devapps.diality.us/cru/HD-DEN-4308-3#c4719 Should there be an else to capture all other cases? Reply by Dara Navaei on 22 September 2020, 11:13 > There is implicitly an else which means if any of these > conditions are not true, stay where you are. Reply by qnguyen on 23 September 2020, 10:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 13:38 https://devapps.diality.us/cru/HD-DEN-4308-3#c4761 I think you should reverse the order of this subtraction and then remove the abs() in the condition below (like you did in FindEnergizedEdge above). Reply by Dara Navaei on 22 September 2020, 14:30 > Done Reply by Sean Nash on 23 September 2020, 11:04 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 13:49 https://devapps.diality.us/cru/HD-DEN-4308-3#c4764 Should this only be incremented if major travel progress is not being made? (same issue as function above) Reply by Dara Navaei on 22 September 2020, 14:30 > Added an elseif to check for this condition. Reply by Sean Nash on 23 September 2020, 11:03 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 13:48 https://devapps.diality.us/cru/HD-DEN-4308-3#c4763 You had a nice comment here in function above. Should have one here too. Reply by Dara Navaei on 22 September 2020, 14:32 > Done Reply by Sean Nash on 23 September 2020, 11:03 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 13:39 https://devapps.diality.us/cru/HD-DEN-4308-3#c4762 Blank line between declarations and code in any scope. Reply by Dara Navaei on 22 September 2020, 14:30 > Done Reply by Sean Nash on 23 September 2020, 11:04 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 15:35 https://devapps.diality.us/cru/HD-DEN-4308-3#c4063 Same comments as above re: absolute values. Reply by Dara Navaei on 02 September 2020, 15:51 > Fixed the algorithm. Reply by Dara Navaei on 19 October 2023, 08:26 > RESOLVED in CODE WALKTHROUGH Revision Comment by Sean Nash on 27 August 2020, 15:37 https://devapps.diality.us/cru/HD-DEN-4308-3#c4064 No absolute values here. We can assume energized edge is higher than de-energized edge. Reply by Dara Navaei on 02 September 2020, 15:51 > Done Reply by Sean Nash on 23 September 2020, 11:18 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 13:57 https://devapps.diality.us/cru/HD-DEN-4308-3#c4767 Shouldn't we increment fail count here? As it stands, seems like the counter is 1 fail behind. Reply by Dara Navaei on 22 September 2020, 14:33 > We are going back to homing not started and not more trails > is allowed. I don't think we need to increment here. Reply by Dara Navaei on 28 September 2020, 08:53 > Done Reply by Sean Nash on 30 September 2020, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 13:59 https://devapps.diality.us/cru/HD-DEN-4308-3#c4770 No abs() here either. Reply by Dara Navaei on 22 September 2020, 14:32 > Done Reply by Sean Nash on 23 September 2020, 10:58 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 15:41 https://devapps.diality.us/cru/HD-DEN-4308-3#c4065 Retries? Reply by Dara Navaei on 29 August 2020, 19:00 > This is the retry. In this condition, the number of retries > have exceeded and it will alarm. Reply by Sean Nash on 23 September 2020, 11:18 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 22 September 2020, 15:31 https://devapps.diality.us/cru/HD-DEN-4308-3#c4796 Combine into one line, it does not seem to exceed the word count limit yet. Reply by Dara Navaei on 23 September 2020, 09:15 > Done. I like them to be in multiple lines for better > readability though. Reply by qnguyen on 23 September 2020, 10:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 14:51 https://devapps.diality.us/cru/HD-DEN-4308-3#c4789 This condition warrants a comment since it is very similar to the first condition. Reply by Dara Navaei on 23 September 2020, 09:23 > Done Reply by Sean Nash on 23 September 2020, 10:56 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 15:45 https://devapps.diality.us/cru/HD-DEN-4308-3#c4067 Consider separating the two conditions here. Reply by Dara Navaei on 29 August 2020, 18:57 > Could you please be more specific? I need both those > conditions to be true to fault. Reply by Sean Nash on 31 August 2020, 08:41 > I think this would be simpler with the following structure: > if out of range > increment counter > if out of range for too long > fault > else > counter = 0 Reply by Dara Navaei on 01 September 2020, 10:36 > Done Reply by Sean Nash on 23 September 2020, 11:18 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 21 September 2020, 11:16 https://devapps.diality.us/cru/HD-DEN-4308-3#c4720 Suggest adding an else to capture other valve modes. Reply by Dara Navaei on 22 September 2020, 11:14 > I set the result variable to TRUE by default so if any of > these cases are not true, the variable is returned as TRUE. > So there is implicitly an else statement. Reply by qnguyen on 23 September 2020, 10:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 14:54 https://devapps.diality.us/cru/HD-DEN-4308-3#c4790 s/w fault is appropriate here. Reply by Dara Navaei on 23 September 2020, 09:19 > Done Reply by Sean Nash on 23 September 2020, 10:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 14:58 https://devapps.diality.us/cru/HD-DEN-4308-3#c4791 Should A/B/C positions be cast to S16? Reply by Dara Navaei on 23 September 2020, 09:18 > They are already S16 in the publish structure. Reply by Sean Nash on 23 September 2020, 10:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 14:59 https://devapps.diality.us/cru/HD-DEN-4308-3#c4792 Remove this test code or put it in a build switch. Reply by Dara Navaei on 02 October 2020, 16:36 > Removed this data publish. Reply by Sean Nash on 05 October 2020, 16:12 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 14:48 https://devapps.diality.us/cru/HD-DEN-4308-3#c4787 I think we should cap all of these target position assignments. 15,000 counts is intended to over-reach commanded position. So just blindly adding/subtracting 15,000 from current position is not what we want. If current position +/- the big step is beyond our commanded position, we just want to set our target to the commanded position. Only if target is less than commanded position do we want to take the entire step. Reply by Dara Navaei on 23 September 2020, 09:26 > This is done in the first if and else statements in this > function. 15000 steps is assigned and in this function if the > current position to the target position is less than 15000 > steps, it will assign the target to it. Reply by Sean Nash on 23 September 2020, 10:57 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 21 September 2020, 11:24 https://devapps.diality.us/cru/HD-DEN-4308-3#c4721 What happens when current position is valve not in position? Reply by Dara Navaei on 29 September 2020, 18:06 > If it is out of position, it will be faulted in the monitor > function. I also changed this to a switch statement to better > cover the cases. Reply by qnguyen on 30 September 2020, 10:34 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 15:02 https://devapps.diality.us/cru/HD-DEN-4308-3#c4793 s/w fault. Reply by Dara Navaei on 23 September 2020, 09:17 > Done Reply by Sean Nash on 23 September 2020, 10:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 21 September 2020, 11:26 https://devapps.diality.us/cru/HD-DEN-4308-3#c4722 Add two blank lines for separation from test support functions. Reply by Dara Navaei on 22 September 2020, 11:29 > Done Reply by qnguyen on 23 September 2020, 10:44 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Valves.h Revision Comment by Sean Nash on 27 August 2020, 11:05 https://devapps.diality.us/cru/HD-DEN-4308-3#c4042 No copyright? Reply by Dara Navaei on 01 September 2020, 10:56 > Copyright will be automatically added during the Bamboo > build. Reply by Sean Nash on 23 September 2020, 11:25 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 10:59 https://devapps.diality.us/cru/HD-DEN-4308-3#c4038 I think these are both reported positions (as comments infer) and commanded positions. Recommend making comments more broad to accommodate both uses. Reply by Dara Navaei on 01 September 2020, 11:04 > Done Reply by Sean Nash on 23 September 2020, 11:24 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 08:49 https://devapps.diality.us/cru/HD-DEN-4308-3#c4725 Only used in Valves.c, so why is it in the header. Bring into .c file and make it private. Reply by Dara Navaei on 22 September 2020, 11:36 > This enum is used by the clients to request any of the > positions. This enum is used in setValvePosition function. Reply by Sean Nash on 22 September 2020, 13:08 > Ah, sorry, I missed that. Reply by Sean Nash on 23 September 2020, 11:14 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 11:01 https://devapps.diality.us/cru/HD-DEN-4308-3#c4039 3-way valves. This enum does not cover all HD valves. Reply by Dara Navaei on 01 September 2020, 10:59 > Done Reply by Sean Nash on 23 September 2020, 11:24 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 11:02 https://devapps.diality.us/cru/HD-DEN-4308-3#c4040 Global public data is a no-no. Just pass one off these into broadcast function call. Reply by Dara Navaei on 01 September 2020, 10:57 > Done Reply by Sean Nash on 23 September 2020, 11:24 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 08:50 https://devapps.diality.us/cru/HD-DEN-4308-3#c4726 I know this is temporary test code, but global variables aren't allowed. Reply by Dara Navaei on 22 September 2020, 11:40 > Done Reply by Sean Nash on 23 September 2020, 11:14 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 September 2020, 08:57 https://devapps.diality.us/cru/HD-DEN-4308-3#c4820 What about a get function for air trap valve? Reply by Dara Navaei on 23 September 2020, 09:13 > Added a function to get the status of air trap valve. Reply by Sean Nash on 23 September 2020, 10:52 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Sean Nash on 27 August 2020, 10:54 https://devapps.diality.us/cru/HD-DEN-4308-3#c4036 I don't see any params in this function. Reply by Dara Navaei on 01 September 2020, 11:08 > Params have been removed since we use structures directly. > Removed the params. Reply by Sean Nash on 23 September 2020, 11:25 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 August 2020, 10:55 https://devapps.diality.us/cru/HD-DEN-4308-3#c4037 Where is valveData coming from? Reply by Dara Navaei on 01 September 2020, 11:05 > I was using the same public structure variable in valves.h. > Now I define local variables for both. Reply by Sean Nash on 23 September 2020, 11:24 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 September 2020, 08:22 https://devapps.diality.us/cru/HD-DEN-4308-3#c4395 Use U32. VALVE_T may not be 32 bits. Reply by Dara Navaei on 04 September 2020, 09:50 > Done Reply by Sean Nash on 23 September 2020, 11:17 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 21 September 2020, 13:34 https://devapps.diality.us/cru/HD-DEN-4308-3#c4723 Expand the macro. Reply by Dara Navaei on 22 September 2020, 11:33 > Done Reply by qnguyen on 23 September 2020, 10:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 22 September 2020, 15:40 https://devapps.diality.us/cru/HD-DEN-4308-3#c4797 Zero should be FALSE. Reply by Dara Navaei on 23 September 2020, 09:14 > Done Reply by qnguyen on 23 September 2020, 10:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 September 2020, 08:13 https://devapps.diality.us/cru/HD-DEN-4308-3#c4390 Suggest renaming variable to something meaningful like valveState. Also, change type to U32 as compiler may not consider an enum type like OPN_CLS_STATE_T as 32-bit and so memcpy may corrupt data beyond the enum. You can cast this valveState as a OPN_CLS_STATE_T when using in function call below. Reply by Dara Navaei on 04 September 2020, 12:37 > Done Reply by Sean Nash on 23 September 2020, 11:17 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 September 2020, 08:44 https://devapps.diality.us/cru/HD-DEN-4308-3#c4724 Looks like macro expanded, but need to clean it up a bit. Zeroes should be FALSE. Comments should be // instead of /* */. Reply by Dara Navaei on 22 September 2020, 11:35 > Done Reply by Sean Nash on 30 September 2020, 10:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 September 2020, 08:10 https://devapps.diality.us/cru/HD-DEN-4308-3#c4388 If payload is 2 U32s, why is memcpy below only copying one U32? Also, why is payload typed as VALVE_POSITION_T? So I see 3 different versions of what the payload is here. Reply by Dara Navaei on 04 September 2020, 12:36 > This function has been removed. Reply by Sean Nash on 23 September 2020, 11:17 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 September 2020, 08:21 https://devapps.diality.us/cru/HD-DEN-4308-3#c4394 Use U32 instead of enum type VALVE_POSITION_T to ensure memcpy will only copy to this variable. Reply by Dara Navaei on 04 September 2020, 12:36 > This function has been removed. Reply by Sean Nash on 23 September 2020, 11:17 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 05 October 2020, 14:57 https://devapps.diality.us/cru/HD-DEN-4308-3#c5175 The function name is not matched. Reply by Dara Navaei on 05 October 2020, 15:13 > Done Reply by qnguyen on 05 October 2020, 16:03 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/FPGA.c Revision Comment by qnguyen on 05 October 2020, 15:01 https://devapps.diality.us/cru/HD-DEN-4308-3#c5178 Remove "\n" to be consistent with other functions. Reply by Dara Navaei on 05 October 2020, 15:18 > Removed all the residual \n in the module. Reply by qnguyen on 05 October 2020, 16:02 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 05 October 2020, 14:59 https://devapps.diality.us/cru/HD-DEN-4308-3#c5176 Function name is not matched. Reply by Dara Navaei on 05 October 2020, 15:15 > Done Reply by qnguyen on 05 October 2020, 16:03 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 05 October 2020, 14:59 https://devapps.diality.us/cru/HD-DEN-4308-3#c5177 Function name is not matched. Reply by Dara Navaei on 05 October 2020, 15:15 > Done Reply by qnguyen on 05 October 2020, 16:02 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/source/sys_main.c Revision Comment by Sean Nash on 01 October 2020, 10:44 https://devapps.diality.us/cru/HD-DEN-4308-3#c5117 RTC and NV data already initialized above. Reply by Dara Navaei on 02 October 2020, 09:13 > Done Reply by Sean Nash on 05 October 2020, 16:08 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 01 October 2020, 10:44 https://devapps.diality.us/cru/HD-DEN-4308-3#c5118 Move valves to controllers section above. Reply by Dara Navaei on 02 October 2020, 09:13 > Done Reply by Sean Nash on 05 October 2020, 16:08 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/AlarmMgmt.h Revision Comment by pmontazemi on 30 September 2020, 15:27 https://devapps.diality.us/cru/HD-DEN-4308-3#c5038 What are 80 and 85 comments? Imagine it is my first day on the job, how would I know what these are? Reply by Dara Navaei on 02 October 2020, 09:15 > This is the number associated with that particular enum. It > is displayed every 5 counts. Reply by pmontazemi on 05 October 2020, 16:07 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/CommBuffers.c Revision Comment by pmontazemi on 30 September 2020, 15:28 https://devapps.diality.us/cru/HD-DEN-4308-3#c5039 I thought our C coding standard mandates spaces between [ ] and ( ) and their arguments? Reply by Dara Navaei on 02 October 2020, 09:14 > Done Reply by pmontazemi on 05 October 2020, 16:06 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeTreatmentParams.c Revision Comment by qnguyen on 05 October 2020, 09:14 https://devapps.diality.us/cru/HD-DEN-4308-3#c5139 Why does this else get commented out? Reply by Dara Navaei on 05 October 2020, 15:11 > I did this after a merge from master [~snash] could you > please respond? Reply by qnguyen on 05 October 2020, 16:13 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeTreatmentParams.h Revision Comment by qnguyen on 05 October 2020, 09:14 https://devapps.diality.us/cru/HD-DEN-4308-3#c5140 This should be public rather than private. Reply by Dara Navaei on 05 October 2020, 15:12 > Done Reply by qnguyen on 05 October 2020, 16:13 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemComm.c Revision Comment by Sean Nash on 15 September 2020, 09:16 https://devapps.diality.us/cru/HD-DEN-4308-3#c4701 It is not a blood trap. Please use air trap or bubble trap. Reply by Dara Navaei on 15 September 2020, 09:53 > Done Reply by Sean Nash on 23 September 2020, 11:15 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: MsgDefs.h Revision Comment by qnguyen on 21 September 2020, 10:54 https://devapps.diality.us/cru/HD-DEN-4308-3#c4717 Align doxygen comment. Reply by Dara Navaei on 22 September 2020, 11:10 > Will be addressed in Sprint28 branch. Reply by qnguyen on 23 September 2020, 10:50 > RESOLVED in CODE WALKTHROUGH. --- ID: HD-DEN-4308-3 https://devapps.diality.us/cru/HD-DEN-4308-3 Title: HD-DEN-4308_HD Valve Control (1 of 2 ) Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (0 active, 3 completed*) qnguyen (*) Sean Nash (*) pmontazemi (*)