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

Visual servoing #545

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

Conversation

Coolearthsky
Copy link
Contributor

No description provided.

Copy link
Member

@truher truher left a comment

Choose a reason for hiding this comment

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

hey, i'd like to get this merged, and generally i'd like this project to hurry up and get finished, but i have some comments i'd like you to see. is this the final code from today? it seems like you were doing things that i don't see here.

@@ -82,14 +83,14 @@ public void initialize() {
ySetpoint = m_swerve.getState().y();
thetaSetpoint = m_swerve.getState().theta();

Optional<Pose2d> opt = m_fieldRelativeGoal.get();
Optional<Pose2d> opt = m_fieldRelativeGoalSupplier.get();
Copy link
Member

Choose a reason for hiding this comment

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

it looks to me like this command only does the "coordinated profile" thing if it happens to run with a goal the first time it is "initialized". also, note that the coordination scaling changes the profiles, so if you press the button over and over, the profiles will get slower and slower. maybe take out all this initialization coordination stuff until the basic uncoordinated profile is working perfectly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured the whole calculation might take too long to run each cycle, however I could be wrong about that, so i'll change that, the profile does not get slower and slower due to the fact the scaling is undone in the end function.

Copy link
Member

Choose a reason for hiding this comment

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

tbh I would suggest leaving the coordination part out until the rest works perfectly.

Copy link
Member

Choose a reason for hiding this comment

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

if you want to keep it, definitely don't do the "undo at the end" thing. just don't overwrite the original.

Rotation2d bearing = new Rotation2d(
Math100.getMinDistance(measurement, fieldRelativeGoal.getRotation().getRadians()));
Math100.getMinDistance(measurement, m_fieldRelativeGoal.getRotation().getRadians()));
Copy link
Member

Choose a reason for hiding this comment

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

note the word "bearing" means "angle from point A to point B" but the expression here appears to return simply the angular component of the goal pose itself, which isn't the same thing. what do you mean this to mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -159,7 +161,7 @@ public void execute() {

@Override
public boolean isFinished() {
if (!m_fieldRelativeGoal.get().isPresent()) return true;
if (!m_fieldRelativeGoalSupplier.get().isPresent() && m_fieldRelativeGoal == null) return true;
Copy link
Member

Choose a reason for hiding this comment

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

don't you want the command to keep going even when it loses sight of the target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See other comment about null.

if (opt.isEmpty()) {
if (m_fieldRelativeGoal == null)
Copy link
Member

Choose a reason for hiding this comment

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

this logic seems to make the command end one cycle after the target is lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is mainly to prevent the case where when I press the button no note is identified having some random result. This will only happen if no object has ever been detected since the robot has been booted due to the fact it only is null upon initialization of the class and is changed upon the first initialization of the command given a pose is given.

Copy link
Member

Choose a reason for hiding this comment

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

hm, I think it's not doing what you think it's doing. maybe start with something simple that can work perfectly.

@@ -55,7 +56,7 @@ public DriveWithProfile2(
SwerveDriveSubsystem drivetrain,
HolonomicFieldRelativeController controller,
SwerveKinodynamics limits) {
m_fieldRelativeGoal = fieldRelativeGoal;
m_fieldRelativeGoalSupplier = fieldRelativeGoal;
m_swerve = drivetrain;
m_controller = controller;
m_limits = limits;
Copy link
Member

Choose a reason for hiding this comment

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

these profile limits are unattainably fast, aren't they? did you try making the profiles a lot slower, as i suggested? "first make it perfect, then make it fast"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

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

Successfully merging this pull request may close these issues.

2 participants