PieceControl overall structure

[GalexY727]

BEFORE THIS WAS A DISCUSSION IT WAS AN ISSUE:

Scroll down to dive deeper and unearth this discussion

Issue: Subsystems as parameters rather than creating new objects

Make the handoff command accept parameters for the subsystems it requires rather than making new subsystems in its constructor. pass those in from the robot container.

[GalexY727]

@Oliver-Cushman are you fine with tackling this? 😗

[Oliver-Cushman]

yeah i got it. for now ill hold off from instantiating it in robotcontainer until the whole file is done.

[Jacob1010-h]

This should be complete with the last commit, 3554991; however, I think that we need to pass in the Commands, not the Subsystems, into the handoff. This will fit with our flowchart because the handoff does not need to communicate with the subsystems directly

[Jacob1010-h]

That can only be done when the method for that is implemented into the Commands themselves, so I'm moving that back into in progress

[PatribotsProgramming]

@Jacob1010-h:

I think that we need to pass in the Commands, not the Subsystems, into the handoff.

The handoff needs to run the commands in the subsystems though, no? If we want to do things like handOffpieceControl.shoot() and inside of that method would have

  intake.toShooter().andthen(waitSeconds()).andThen(shooter.stop().alongWith(pivot.reset()))

then we would need at least shooter, pivot, and intake as subsystems required inside of the handoffpieceControl.

[Jacob1010-h]

@Jacob1010-h:

I think that we need to pass in the Commands, not the Subsystems, into the handoff.

The handoff needs to run the commands in the subsystems though, no? If we want to do things like handOff_pieceControl_.shoot() and inside of that method would have

  intake.toShooter().andthen(waitSeconds()).andThen(shooter.stop().alongWith(pivot.reset()))

then we would need at least shooter, pivot, and intake as subsystems required inside of the handoff_pieceControl_.

This is an interesting discussion, and I'm glad what we are talking about. I had this discussion with Rudy in class today. If we were to use the subsystem methods inside of the handoff/piece control, that would cause the logic to be extended down into the handler, which really should only be controlling the interaction between the subsystems. If we were to only keep the Commands for the shooter (which needs to be done, if we were to extend the logic down), the logic flowchart would look like this:

The handoff should really only be controlling the interaction, not what each subsystem is doing as a response to those interactions.
Because of the simplified nature of the trap subsystems, you might not need a command file in the first place. However, that leaves the structure unbalanced. The next line of thinking might be, well, why would we need a shooter command? The answer is that it's some calculation and communication between multiple subsystems.

Of course, it's always possible to move the subsystem logic down to the handler; however, that file is responsible for more things overall and might get overwhelming to determine what this class is supposed to do. The goal of the command files is for the handler to be able to edit and control groups of subsystems at a time instead of each one individually.

Both work, but I'd love to hear everyone's thoughts.

[Oliver-Cushman]

I think it might be smarter to instantiate the ShooterCalc class for example, and then I can just modify the constructor in that class to create a new pivot and shooter.

[Jacob1010-h]

I think it might be smarter to instantiate the ShooterCalc class for example, and then I can just modify the constructor in that class to create a new pivot and shooter.

Do you think that the elevator command should exist as well?

[Oliver-Cushman]

Well I'm not sure how you want to handle the elevator considering it actually extends Command rather than just being a class composed of command methods like ShooterCalc.

[Jacob1010-h]

Well I'm not sure how you want to handle the elevator considering it actually extends Command rather than just being a class composed of command methods like ShooterCalc.

Most likely, that will be changed to the same format as ShooterCalc, depending on the outcome of this issue.
If we decide to keep it, it will change to adopt the functionality of the talking to the Claw and Elevator.

[Oliver-Cushman]

By the way, should handoff use prepareFireMovingCommand instead of prepareFireCommand? Because prepareFireCommand will just start preparation for the position it is given, and prepareFireMoveCommand repeatedly changes shooter speed and angle based on current swerve position. In my opinion this might be a better option because I don't think we want to stand still before we even start orienting and speeding up our shooter, and it would be better to already have it mostly ready by the time we are ready to shoot.

[Oliver-Cushman]

Well I'm not sure how you want to handle the elevator considering it actually extends Command rather than just being a class composed of command methods like ShooterCalc.

Most likely, that will be changed to the same format as ShooterCalc, depending on the outcome of this issue. If we decide to keep it, it will change to adopt the functionality of the talking to the Claw and Elevator.

I personally think it would be better to make it another file composed of commands, but you should have that conversation with Rudy as I don't know his full idea of what the class is.

[Jacob1010-h]

By the way, should handoff use prepareFireMovingCommand instead of prepareFireCommand? Because prepareFireCommand will just start preparation for the position it is given, and prepareFireMoveCommand repeatedly changes shooter speed and angle based on current swerve position. In my opinion this might be a better option because I don't think we want to stand still before we even start orienting and speeding up our shooter, and it would be better to already have it mostly ready by the time we are ready to shoot.

I agree, ill make that change now for #27

[Jacob1010-h]

Well I'm not sure how you want to handle the elevator considering it actually extends Command rather than just being a class composed of command methods like ShooterCalc.

Most likely, that will be changed to the same format as ShooterCalc, depending on the outcome of this issue. If we decide to keep it, it will change to adopt the functionality of the talking to the Claw and Elevator.

I think it would be better to make it another file composed of commands, but you should have that conversation with Rudy as I don't know his full idea of the class.

I was able to talk with Rudy about it today, and he said that he would start converting the file soon. I appreciate the advice.
However, we will wait to see what more people think about removing the elevator command before we jump to whether the file should be rewritten.

[GalexY727]

I believe that the current state of ElevatorCommand and maybe even pieceControl contents could all be contained within pieceControl, as the elevator is just one part of controlling the piece, much like the shooter is another part. In the same way we tell the shooter to shoot, we can tell the elevator to lift and place inside of pieceControl. I don't mind that movement being it's own Command file for simplicity, as then we can just say

public Command placeTrap() {
  return new ElevatorCommand(ElevatorConstants.TRAP_POSITION);
}

and inside of ElevatorCommand there is an isFinished checking if the elevator is up to where it should be and if it is it runs claw.outake() on exit. This is the (arguably) cleaner alternative to decorating a Commands.run() with an end method and isFinished condition.
The alternative to this which I would still consider acceptable would remove the file ElevatorCommand entirely and set PieceControl.placeTrap() to be

PieceControl.java:
public Command placeTrap() {
  return elevator.getPlacementCommand(ElevatorConstants.TRAP_POSITION)
    .andThen(claw.outtake());
}
. . .
Elevator.java:
public Command getPlacementCommand(double position) {
  return runOnce(() -> motor.setTargetPosition(position))
    .andThen(Commands.waitUntil(this::getAtDesiredPosition);
}

[GalexY727]

Back to the topic of this issue: It is smart to keep all subsystems in RobotContainer for a couple of reasons, but it's easiest to say that we need it because if we want to log things inside of the subsystem it needs to be constructed inside of RobotContainer, as monologue looks through all subsystems in the defined container which you define in Robot::robotInit.

This is starting to stray away from the root issue of subsystems being passed into the constructor of whatever file is meant to manage them, even if they aren't yet used outside of the file. Should a sister issue be created, or should this be converted to a discusion?

[RudyG252]

I agree, I think that Elevator Command going back to being a file that extends command and just moves the arm to the needed position and then placing with the claw is the cleanest solution (though we do need an outtake command for the claw, which is already made so it's chill_

[GalexY727]

Do you have a preference for the first and second solution that I mentioned above? I think that the second one might be nicer, but that's just because I like to have as minimal files in the commands folder as possible, which isn't necessarily a good or bad practice, just a preference of abstraction.

[RudyG252]

I think we can change this to a discussion on the general strucuture of the code and piece manager file, and that that discussion shouldn't necessarily be finalized until we at least have a bot, figure out any issues with our structure, and figure out what works the best

[Jacob1010-h]

I'd like to jump in on this, I don't mind the removal of the elevator class/command, however I guess I'm still confused on the structure that you are suggesting. Do you mean to remove elevator and shooter calc entirely? Or maybe to rename the handler to placement.java? Or if this is just a concern about the commands file, we could just move the commands to the subsystem folder that they control. i would love to talk with you about this at lunch. @GalexY727 @RudyG252 @Oliver-Cushman

[Jacob1010-h]

This was already resolved and ended up becoming a situation that is simular to the tldaw