team401 / high-key-2024

Other
2 stars 0 forks source link

Auto + Alignment #23

Closed linglejack06 closed 1 month ago

linglejack06 commented 2 months ago
linglejack06 commented 2 months ago

@jkleiber do you know why my alignment command isn't working? the auto just tried to point it directly to the left. Is there an issue with the target pose I'm setting?

jkleiber commented 2 months ago

@linglejack06 if you could post some advantage scope plots of the key variables that would be helpful in figuring out the root cause

linglejack06 commented 1 month ago

@jkleiber i think ive got the issue fixed, when i merged main into the code something went wrong. So i reset to before merging and the robot aligns how it should. Path is oddly flipped though starting at blue speaker even though my pathplanner shows it at red speaker, any clue why?

jkleiber commented 1 month ago

@linglejack06 this probably has to do with your path planner configuration in the drivetrain subsystem. Also check your sim settings that you're on the alliance you think you're on

linglejack06 commented 1 month ago

@jkleiber @aidnem could one of you take a look at the auto-alignment (housed in the drivetrain getAutoRotaion method) and the teleop auto-alignment (housed in AlignToTarget command)? The approach I used for teleop helps isolate some logic away from the drivetrain. All that is left for auto now is adding named commands for shooting and intaking, I think.

linglejack06 commented 1 month ago

@jkleiber i have added a isDriveAligned Method to interface with scoring for auto. But i have a quick question in regard to teleop. I took the route of seperating alignment logic from drive, so its housed in a command that sends goal speeds to drive. With this route, calling is drive aligned adds unnecessary steps (it finds the target heading again and compares it again). These are unecessary because the command will automatically end when the drive is aligned due to the logic inside. Could we look into setting up a sequential command like this: align to target -> shoot at target? By using a sequential command, we could avoid calling the isDriveAligned method in teleop, as the scoring subsystem would only run once the alignment command finishes (it's aligned).

jkleiber commented 1 month ago

@linglejack06 could we keep alignment logic in drive rather than putting it in commands?

The reason I don't like to put control logic like this in commands is because then you run into this exact question about auto vs teleop. Ideally the subsystem will manage its own state on if it's aligned or not and can report its status to whatever commands need to know the status

linglejack06 commented 1 month ago

Yeah I can refactor that

aidnem commented 1 month ago

@linglejack06 @jkleiber could we consider using alignState to enable/disable alignment instead of just using alignTarget.NONE? This would allow masher to select an alignTarget without forcing robot to align, until the driver actually pulls the trigger (or presses the button) to start aligning. I feel that this would be an easier solution for the driver, and this is the way it was done on main bot 2024.

jkleiber commented 1 month ago

@aidnem that sounds reasonable to me

linglejack06 commented 1 month ago

@aidnem yeah I can easily add that. I would say we have the driver pull a trigger to align and continue holding, maybe on release of trigger the alignment stops? What do you think of that idea? @jkleiber

aidnem commented 1 month ago

If I recall correctly that's how it worked last year as well, I can ask Oliver tonight to make sure

linglejack06 commented 1 month ago

look into field centric facing angle swerve request for alignment

linglejack06 commented 1 month ago

@jkleiber ive switched to using the FieldCentricFacingAngle swerve request. no difference that i can tell, but check it out and see which one you like better? @aidnem too

linglejack06 commented 1 month ago

@jkleiber merged main in, needs reapproval

linglejack06 commented 1 month ago

@jkleiber @aidenm issue fixed just need approval to merge

aidnem commented 1 month ago

@linglejack06 did you ever add a check that will disable idling when we are trying to align aren't aligned yet? My fear is that the robot will only be able to align while moving because if the sticks are at zero it'll start forcing idle even if there's still error in rotation.

linglejack06 commented 1 month ago

I'll add that check soon

linglejack06 commented 1 month ago

@aidnem check is added

linglejack06 commented 1 month ago

@jkleiber @aidnem alignment works well. Haven't tested auto yet. do we want to wait for merge until auto is tested or go ahead and merge -> tuning has been moved to 14-calibratte-cameras branch as it depended on vision, auto will depend on it too. My opinion is we merge this and then auto tuning is done in 14-calibrate-cameras like we did alignment

jkleiber commented 1 month ago

We should merge this and follow your suggestion @linglejack06

linglejack06 commented 1 month ago

it is, only thing changed in code tested that isnt here is pid gains and i had to reverse direction of speaker rotation by subtracting pi from it (that is in the 14-calibrate-cameras branch)

On Tue, Oct 15, 2024 at 9:38 AM Justin Kleiber @.***> wrote:

@.**** approved this pull request.

approved. I am assuming this is the code that was tested yesterday

— Reply to this email directly, view it on GitHub https://github.com/team401/high-key-2024/pull/23#pullrequestreview-2369382077, or unsubscribe https://github.com/notifications/unsubscribe-auth/AZ7J5PWJJJC6YY5KB6XTWLTZ3ULGHAVCNFSM6AAAAABPEMZA6SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNRZGM4DEMBXG4 . You are receiving this because you were mentioned.Message ID: @.***>

jkleiber commented 1 month ago

@linglejack06 I skimmed over this branch vs the calibrate cameras branch. You're good to merge this since that other branch is serving as the integration branch. Hopefully merging this to main will make the final review of the integration branch easier