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

Added getArmAngleFromDistance method #75

Merged
merged 7 commits into from
Feb 7, 2024
Merged

Conversation

Rongrrz
Copy link
Contributor

@Rongrrz Rongrrz commented Feb 3, 2024

Why are we doing this?

Convert robot distance from speaker to angle the robot needs to be at

Asana task URL: https://app.asana.com/0/38541457243752/1206500303486160/f

Whats changing?

Added new method and changed names of some double properties to avoid confusion

Questions/notes for reviewers

How this was tested

  • unit tests added
  • tested on robot

@Rongrrz Rongrrz requested a review from a team as a code owner February 3, 2024 22:00
src/main/java/competition/subsystems/arm/ArmSubsystem.java Outdated Show resolved Hide resolved
@@ -225,6 +228,11 @@ public double convertShooterAngleToTicks(double angle) {
return 0;
}

public double getArmAngleFromDistance(double distance) {
Copy link
Contributor

Choose a reason for hiding this comment

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

⭐ ⭐ Consider writing a unit test for this logic to make sure you get a number you'd roughly expect (ie not a crazy negative value given a reasonable distance).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ One possible problem though. Right now, I've made it so that the test checks three values for the equation for with a big range of error. I'm not really sure if this is "future-proof" if the equation changes dramatically in the future.

@Rongrrz Rongrrz merged commit 5ea29c5 into main Feb 7, 2024
1 check passed
@Rongrrz Rongrrz deleted the getArmAngleFromDistance() branch February 7, 2024 02:17
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.

2 participants