Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

35 create make determine waypoint command #39

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

Griffin9881
Copy link
Contributor

@Griffin9881 Griffin9881 commented Feb 15, 2025

Pull Request


Issue Number

Closes #35
Closes #36

Comments

@Griffin9881 Griffin9881 self-assigned this Feb 15, 2025
case L1Coral:
case L3Coral:
case L4Coral:
getCoralWaypoint(scoringSide);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this method exist?

@Override
public void initialize() {
List<PhotonPipelineResult> visionResults = vision.getResults();
double area = 0.0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will the area be used for?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The area variable isn't used for anything. It only determines the april tag with the smallest area.

Comment on lines 37 to 38
AcquisitionPositionSetpoint setpointLevel = OperatorState.getLastInput();
ScoringSide scoringSide = OperatorState.getScoringSide();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, I think we are assuming that the operator will need to press one of the scoring buttons before setting the waypoint. I would assume this means that we will need to disable the magic button until a scoring button is pressed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if we decide to change the scoring or cancel it altogether, we must ensure that the current instance of this command gets canceled and a new command replaces it.

case 6:
this.waypoint = scoringSide == ScoringSide.LEFT ? DriveWaypoints.LeftCoral22
: DriveWaypoints.RightCoral22;
this.isMirrored = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would set this boolean based on if it's greater than an April Tag judicial ID (i.e. this.isMirror = this.bestTag >= 17)

public static final double kMaxAccelerationMetersPerSecondSquared = 7;
public static final double kMaxAngularSpeedRadiansPerSecond = Math.PI;
public static final double kMaxAngularSpeedRadiansPerSecondSquared = Math.PI;

public static final double kPXController = 1.0;
public static final double kIXController = 0.35;
public static final double kDController = 0.0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this should be called kDXController to be consistent with the other variable names.

@@ -8,8 +18,8 @@ public final class VisionConstants {
public static final String kEnabled0Topic = "enable0";
public static final String kEnabled1Topic = "enable1";

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These variables should each have a k at the start of their names to remain consistent with the other variable names.

Saum27
Saum27 previously approved these changes Feb 19, 2025
case L1Coral:
case L3Coral:
case L4Coral:
this.waypoint = get134CoralWaypoint(scoringSide);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of passing in the scoringSide and performing the check for scoringSide within each case of the switch-case statement, we could pass in a boolean variable indicating whether the scoringSide is left or right and use that boolean to determine whether to use the right or left waypoint.


private Optional<DriveWaypoints> get134CoralWaypoint(ScoringSide scoringSide) {
switch (this.bestId) {
case 6:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look in branch 35-create-make-determine-waypoint-command-RG for HashMap implementation.

driveTrain.getState().Speeds.vxMetersPerSecond,
driveTrain.getState().Pose.getRotation()),
new GoalEndState(0, Rotation2d.kZero)))));
super.aButton.and(() -> determineWaypointCommand.getPose2d().isPresent()).whileTrue(
Copy link
Contributor

@ChasingCode34 ChasingCode34 Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the way this is working right now is that when the "A" button is first pressed, we check whether there is a waypoint present to go towards. However, we call the command after this check, so there will not be a value associated with the Optional when the check is performed. Though, my logic might be wrong.

Copy link
Contributor

@ChasingCode34 ChasingCode34 Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this check ensures the robot does not move until a waypoint is available. Perhaps, we can call driveToWaypoint every time the acquisition setpoint changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Magic Button make determine waypoint command
5 participants