set up subsystem interfaces #39
Conversation
added some methods and motor controllers to Lift, Climber, and Intake
added some methods and motor controllers to Lift, Climber, and Intake
lhmcgann
left a comment
lhmcgann
left a comment
There was a problem hiding this comment.
Great work on doing all of this! It's a great place to start and work from as we begin to develop our subsystems' functions. Go ahead and review the requested changes, and then let me know again once you have your next "draft" done and ready for me to look at. The next step(s) after that will be writing JUnit tests to make sure all of the methods do the correct things, then actually implementing the methods according to what the tests expect them to do. We'll also be creating Commands (like for the climber process) and tying them to buttons in OI, figuring out how the user/driver will interface with the functions. We'll also need to experiment w/ and research some sensors, as well as talk to Design, to determine which sensors are best for which functions and where they should be placed, etc.
| * returns current motor value | ||
| */ | ||
| public double getIntake(); | ||
|
|
There was a problem hiding this comment.
change name of this method to getIntakeSpeed() because getIntake() makes it seems like you are returning the whole intake object which isn't happening :)
|
|
||
| /** | ||
| * Uses (insert sensor here) to detect the distance above the ground | ||
| */ |
There was a problem hiding this comment.
clarify comment a little bit: not exactly dist above ground, but rather how high up/at what position the carriage of the lift is (could be measuring from top or btm so don't need to specify reference frame yet)
| public double getLift() { | ||
| return liftMotors.get(); | ||
| } | ||
|
|
There was a problem hiding this comment.
same w/ getIntake: change this to getLiftSpeed() or getMotorValue() bc may not necessarily be spd (could be voltage --> something to check in documentation of WPI_TalonSRX)
| public void goToBar() { | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
instead of having these four methods, I would suggest having one general goToPosition(double height) method. Then this method can be used for all cases and a parameter can be which specific height. The parameter doesn't have to be a straight double, although it could be. I would suggest looking into enums, possibly (you can look at Kevin's code in Autonomous.java for some reference too). You can create a new enum class essentially, called LiftPositions maybe, and then values can be BAR, SCALE, SWITCH, GROUND, etc, each key w/ a specified number value associated with it. Could also just use SD keys too, but enums could be good to explore.
| @@ -1,57 +0,0 @@ | |||
| package org.usfirst.frc.team199.Robot2018.autonomous; | |||
There was a problem hiding this comment.
I don't know why it shows these two classes being deleted, but they shouldn't be, so if you could check to make sure they're still there that would be good 👍
| public static SpeedController intakeMotors; | ||
| public static SpeedController liftMotors; | ||
| public static SpeedController climberMotors; | ||
|
|
There was a problem hiding this comment.
Can you make these singular? i.e. intakeMotor instead of intakeMotors? Ik it's a detail but kind of confusing being plural. I also don't think they need to be speed controllers, you can just make them WPI_TalonSRX directly.
| // here. Call these from Commands. | ||
| private final SpeedController climberMotors = RobotMap.climberMotors; | ||
|
|
There was a problem hiding this comment.
Yeah all of these references can be WPI_TalonSRX's as well.
| public void goUp() { | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
A better name might be climb() ?
| goUp(); | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
I'm a little confused what this method is meant to do. What I would have guessed a runClimber() method would do would just set the climberMotor to the speed parameter, so pretty basic. I see what you're trying to do: the whole climbing process right? Calling all of the individual steps together to make the full process is more of what happens in a Command (i.e. in the execute method), so I would wait for that and not put that here. Maybe just make it a simple runClimber(double speed) method as described above.
| public boolean detectCube() { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
I might change the method name detectCube() to something like seesCube() to make the difference between this method and the one below (hasCube) a little clearer. In the end this method is something that might become obsolete because the driver will just look through the camera and see it, but good to have here in case we are able to automate enough :)
added some methods and motor controllers to Lift, Intake, and Climber
|
Okay, I fixed everything you mentioned. For setting the lift position, I have an enum for the standard positions and then a double for the offset. It seems like the simplest option. I'm not sure what's going on with the Auto classes. They're still there in the autonomous package as far as I can tell |
|
Looks good! Thanks for getting all of those changes done. Next up is implementing everything (and combining climber and lift bc that happened on Saturday at the end). 👍 |
2 participants
added some methods and motor controllers to Lift, Climber, and Intake