search

eshsrobotics / 2019bot

Code for the 2019 FRC Competition
1 stars 0 forks source link

Suggestions for Arm code #1

Open TheThirdOne opened 5 years ago

TheThirdOne commented 5 years ago

I took a look at d5826a957961fc113dfccfd29d0f7899bafceb65 and have a few concerns and suggestions.

For Potentiometer code which is not currently in use:

Both Line 55 and Line 77 essentially scale a voltage (0-5V) to an arbitrary range. There is no reason to do this in two steps; that needlessly adds complexity and a possibility for bugs. Either setup AnalogPotentiometer with appropriate values or scale on your own an leave AnalogPotentiometer with scale=1 and offset=0 as in Line 54.

Line 78 is either incorrect or the description of RobotMap.MIN_MOTOR_POWER is. A value of u close to 0 does not mean the motor isn't moving, it means it is close to moving negatively at max speed. See line Line 86

About adjust motor:

Line 145 can just be if (direction > 0). Math.signum isn't changing anything.

Is there a reason Line 147 and Line 161 are different?

If not I would suggest putting that if outside the other.

In arm(OI):

Line 217 - Line 225 could be written as:

double h = oi.leftJoystick.getRawButton(3)?0.3:(oi.rightJoystick.getRawButton(3)?-1:0);

This uses the Java's only ternary operator. Its a significantly more concise way of writing it, but that is a stylistic concern.

General comments:

There is a lot of commenting out. When changes are drastic enough that you need to comment that much, it makes sense to utilize git as a backup so you can delete with confidence. This can be done using a new branch or just with commits if you feel confident enough in your git skills.

uakotaobi commented 5 years ago

Some comments (I'll leave the pull request assigned to @dispencermoore to fix):

  1. Re: AnalogPotentiometer scaling:

I think the idea I had there was that I'd measure the pot with the default values coming out of wpilibj (SCALE_FACTOR = 1, OFFSET = 0), get the real range of the pot, and then change the scale factor and offset so that the "right" values were coming out of wpilibj -- namely, angles in degrees. But we had a very unreliable potentiometer at the time and it did not have anything close to a linear response curve, so I shrugged my shoulders, wrote off SCALE_FACTOR and OFFSET as a loss, and then used linear interpolation to scale the potentiometer readings myself.

Then, later, we got a better (read: more linear) potentiometer and I put the SCALE_FACTOR and OFFSET back. I guess I should have taken out the interpolation on line 77. (You'll notice that the comments I had for the scaling constants were unfinished.)

I agree that if we're just using pot.get() directly and we're not scaling its value beyond the interval [0, 1], then further interpolation of pot.get() on line 77 is unnecessary and can be taken out.

  1. Re: MIN_MOTOR_POWER:

setElbowMotorSpeedBasedOnElbowPotentiometer() is just a test function and will be thrown away pretty soon. Its job was to turn a test motor in response to manual twisting of a pot, just so we could get the basic mechanics down, as I'm rather new at AnalogPotentiometer programming. I acknowledge that there was no comment at the top of the function to indicate this to the reader.

The actual way we need to set the speed of the elbow motors ~us~ is by using a PID algorithm, and that's not written yet. (We've agreed this week that it's not an immediate priority if manual arm control is not too unwieldy.) By the time we get to it--if we do get to it--I think MIN_MOTOR_POWER will be used for its intended meaning.

  1. Re: Math.signum:

Yeah, good point. Same thing goes for line 159.

  1. Re: ELBOW_INCREMENT_INTERVAL_MILLISECONDS vs. minUpdateIntervalMilliseconds:

...Because I forgot to change line 161. And line 176. (I of course intended the code to work independently of the elbow motor.)

  1. Re: Ternary operators:

Yeah, you could write it in one line. It's up to @dispencermoore, but stylistically, inlining long expressions like that is not my own preference.

  1. Re: Commenting out:

Acknowledged. I was going to kill the dead code in the next commit, but I usually wait until the live code is tested before I do that. In this case, I had to leave the lab before that testing happened.

Thank you for the code review -- more eyes make for quicker work. There are more changes to come.