BLlakers / 2024_Official

Other
2 stars 1 forks source link

Hanger Module Organization #35

Open dlezcan1 opened 3 months ago

dlezcan1 commented 3 months ago

This is a good start! Here's some suggestions

For the HangerModule class, can you implement the following:

For the Hanger class, do the following

dlezcan1 commented 3 months ago

Almost there @jforchheimer25. Some things that will cause issue with the command scheduler is with your debug controller bindings.

https://github.com/BLlakers/2024_Official/blob/e152885bc059b6e0f2e0a1f6c84e05fc82471b36/src/main/java/frc/robot/RobotContainer.java#L263-L287

Here, the Hanger submodule will cause a lock on the commands running and you won't be able to run each hanger arm at the same time. Thus, you would want to change it as an example:

debugController
        .leftBumper() // Left Hanger arm down
        .whileTrue(
            m_Hanger.leftHangerModule().runEnd(
                m_Hanger.leftHangerModule()::MoveHangDown,
                m_Hanger.leftHangerModule()::HangStop
             )
        );

This way, this won't cause for issues with the bindings.

dlezcan1 commented 3 months ago

Similarly, with the auto hang feature in the commands, it's missing the requirements needed to hold each of those hanger modules.

https://github.com/BLlakers/2024_Official/blob/e152885bc059b6e0f2e0a1f6c84e05fc82471b36/src/main/java/frc/robot/subsystems/Hanger.java#L77-L93

Here, we would need to change it so that we are running

leftHangerModule().runEnd(leftHangerModule()::MoveHangDown, leftHangerModule()::HangStop)

and similary with the rightHangerModule.

Finally, you would need to require the hanger subsystem, so finally, you would take the Command cmd and addRequirements(this) inside this function to ensure that the hanger doesn't run anymore code.

Same with the Hanger.RaiseHangAuto function.

dlezcan1 commented 3 months ago

Finally, I think this might be a better method for adding the data to SmartDashboard.

https://github.com/BLlakers/2024_Official/blob/e152885bc059b6e0f2e0a1f6c84e05fc82471b36/src/main/java/frc/robot/subsystems/Intake.java#L167-L170

This can be changed in DriveTrain and Hanger and I believe that would be best. Double check it looks alright with the simulation when you're done.

This should be updated: https://github.com/BLlakers/2024_Official/blob/e152885bc059b6e0f2e0a1f6c84e05fc82471b36/src/main/java/frc/robot/subsystems/Hanger.java#L96-L100

jforchheimer25 commented 3 months ago

Almost there @jforchheimer25. Some things that will cause issue with the command scheduler is with your debug controller bindings.

https://github.com/BLlakers/2024_Official/blob/e152885bc059b6e0f2e0a1f6c84e05fc82471b36/src/main/java/frc/robot/RobotContainer.java#L263-L287

Here, the Hanger submodule will cause a lock on the commands running and you won't be able to run each hanger arm at the same time. Thus, you would want to change it as an example:

debugController
        .leftBumper() // Left Hanger arm down
        .whileTrue(
            m_Hanger.leftHangerModule().runEnd(
                m_Hanger.leftHangerModule()::MoveHangDown,
                m_Hanger.leftHangerModule()::HangStop
             )
        );

This way, this won't cause for issues with the bindings.

Jared: Since this command is requiring the same sub? But if thats the case, isnt using the two modules twice requiring the same sub? (If i create the object twice does that get around the scheduler?)

dlezcan1 commented 3 months ago

That and you need to run it with the left/right hanger module subsystem, not the hanger subsystem.

jforchheimer25 commented 3 months ago

Im gonna commit in a sec, almost done with updates (probably at 11:45/12) Ill text you when I do Are you saying to add reqs within the parell for each also?

dlezcan1 commented 3 months ago

The only req you need to add with the parallel is the Hanger req. The others should run directly from the left/right hanger modules individually

jforchheimer25 commented 3 months ago

https://github.com/BLlakers/2024_Official/blob/e152885bc059b6e0f2e0a1f6c84e05fc82471b36/src/main/java/frc/robot/subsystems/Hanger.java#L90-L93) So your saying this is fine?

dlezcan1 commented 3 months ago

That part, but the commands in parallel need to be updated.

jforchheimer25 commented 3 months ago

Very important photo This is fine right

dlezcan1 commented 3 months ago

Should be fine.

jforchheimer25 commented 3 months ago

I committed. Go check it out: https://github.com/BLlakers/2024_Official/commit/a575ac568a2e76b5174d14fa24a329f5db8bd07b

dlezcan1 commented 3 months ago

Create a pull request into Dev and I'll review there.

jforchheimer25 commented 3 months ago

The PR was created.

dlezcan1 commented 2 months ago

@jforchheimer25 I reviewed. Comments need to be addressed.

dlezcan1 commented 2 months ago

Please address the comments on the PR first.