Technolib try outs (DO NOT MERGE JUST YET!) #155
Conversation
|
@Yinn333 take a look at aece80a and 168d7e2 for the commits that show some of the "Hey, you don't have to create an unending stream of BlahSbusystemDoThingCommand classes" alternative(s). I think you'll have the most experience with that stuff. (@1198159 if you're not neck deep in college crap, I'd value your perspective, too) |
kevinfrei
force-pushed
the
technolib-try-outs
branch
4 times, most recently
from
f5b4b72
to
e883b74
Compare
kevinfrei
force-pushed
the
migrate-to-90
branch
from
101d94c
to
fc62887
Compare
kevinfrei
force-pushed
the
technolib-try-outs
branch
from
e883b74
to
ae636c1
Compare
There was a problem hiding this comment.
I like the fact that you got rid of getInstance. I'm not a fan of singletons. However, I cannot find the code reference for CommandScheduler, so my review is very limited
There was a problem hiding this comment.
Yeah, the javadoc comments are all you're going to find. I've been slowly trying to get more stuff documented, but it's just me typing when that's what I feel like doing 🤣
There was a problem hiding this comment.
BTW: You can find the source (with javadoc comments, as they are) for the custom TechnoLib branch I'm working with here: https://github.com/technototes/TechnoLib/blob/cleanup/RobotLibrary/src/main/java/com/technototes/library/command/CommandScheduler.java
| .lineToLinearHeading(RIGHT_MOVE.toPose()) | ||
| .build(), | ||
| public static final TrajectoryPath | ||
| TELESTART_TO_FORWARD_MOVE = TrajectoryPath.lineToLinearHeading(TELESTART, FORWARD_MOVE), |
There was a problem hiding this comment.
It's definitely cleaner, but it's less flexible. I need to make sure that there are examples that use the TrajectoryBuilder directly, as you can string multiple trajectories together (though it's probably all gotta change if/when we eventually migrate to RoadRunner 1.0)
| @@ -18,25 +14,25 @@ public AutoLeftCycleLeft(Robot r) { | |||
| AutoConstants.Left.START_TO_E_JUNCTION | |||
| ) | |||
| .alongWith( | |||
| new LiftHighJunctionCommand(r.liftSubsystem), | |||
| new ClawOpenCommand(r.clawSubsystem), | |||
| r.liftSubsystem.highCommand, | |||
There was a problem hiding this comment.
I'm not sure if I like this change or not. On one hand, no need to use new objects. But on the other hand, The hierarchy is broken, I think. A highCommand belongs to the liftSubsystem doesn't roll on the tongue as nicely as a highCommand uses the liftSubsystem.
There was a problem hiding this comment.
This is the primary change that I'm not strictly happy with. I sort of like the idea of making "single subsystem" commands nested classes, but I was able to eliminate most of them by enabling the methodRef stuff. The only place the commands come up now are in autonomous command groups. This is probably worth chatting with the students about (Particularly our veteran programmers, as both of them were involved with some of this stuff last year)
| @@ -60,40 +50,40 @@ private void AssignNamedControllerButton() { | |||
| } | |||
|
|
|||
| public void bindClawControls() { | |||
| clawOpenButton.whenPressed(new ClawOpenCommand(robot.clawSubsystem)); | |||
| clawCloseButton.whenReleased(new ClawCloseCommand(robot.clawSubsystem)); | |||
| clawOpenButton.whenPressed(robot.clawSubsystem.openCommand); | |||
There was a problem hiding this comment.
Why now we have three ways to do this? through a dot object, a colon function, and simple required command? What's the difference?
There was a problem hiding this comment.
My primary goal is to simplify the work of manipulating the robot through code. The vast majority of commands are 25-30 lines files, that have precisely 2 lines of real code (addRequirements(subsys) in the constructor, and then subsys.doTheThing() in the execute method) There are a few different ways we could structure things to minimize the amount of copy-pasta. I think the library, as it's currently authored, enables lots of different ways to accomplish what we're talking about. Perhaps removing all the CommandButton overloads, and instead encouraging a static class of commands for each subsystem that are consistent in their usage would be easier to follow? I'll try a minor change and modify this PR with a different possibility (after I've diagnosed a null pointer exception, and maybe added some better logging to help tracking them down easier in the future)
kevinfrei
force-pushed
the
technolib-try-outs
branch
from
5d23284
to
30b0aad
Compare
I've made a bunch of modifications to TechnoLib in a branch (, and then tried them out in the PowerPlay codebase.
@Yinn333 @jachninja (& @1198159 ) what do you think? I feel like not having to create all the commands as full classes is a big improvement. The commands that we do have to create, I'd rather create them as nested classes in the subsystems (slightly more work that the SimpleRequiredCommand<>'s you'll see in the LiftSubsystem, but not an entire new file)
Here's the PR for the TechnoLib changes (You won't be able to build without a nest clone of TechnoLib, since I didn't create a release for JitPack of the TechnoLib changes)