Pre lehigh changes#68
Conversation
…12V ARGB systems.
afranchuk
left a comment
afranchuk
left a comment
There was a problem hiding this comment.
Overall, this is a pretty good cleanup. I didn't review the LED commits, assuming they were updated mostly as requested from the other PR. I just saw a few minor things that could use improvement.
There was a problem hiding this comment.
This isn't something to address now, but maybe something to think about for the future: it'd be very useful if we included a picture of each auto path in the repo, so that as autos change we can see and review by looking at the picture(s). Otherwise there's really no meaningful review process for these files. Or maybe part of the review process might involve loading them in path planner to view the animation (if the reviewer hasn't seen it before).
| try (/* Run simulation at a faster rate so PID gains behave more reasonably */ | ||
| /* use the measured time delta, get battery voltage from WPILib */ |
There was a problem hiding this comment.
Leave the comments above the try statement.
|
|
||
| /* Run simulation at a faster rate so PID gains behave more reasonably */ | ||
| /* use the measured time delta, get battery voltage from WPILib */ | ||
| try (/* Run simulation at a faster rate so PID gains behave more reasonably */ |
There was a problem hiding this comment.
I'm pretty sure this will result in the callback never being called? The Notifier.close() method will stop the periodic timer, join the thread, etc.
|
|
||
| public class Turret extends SpikeSystem<TurretIO.TurretIOInputs> { | ||
| public static final double TURRET_AIMING_TOLERANCE_DEGREES = 5.0; // degrees within which we consider the turret to be aimed at the target (+-) | ||
| public static final double TURRET_AIMING_TOLERANCE_DEGREES = 40.0; // degrees within which we consider the turret to be aimed at the target (+-) |
There was a problem hiding this comment.
This seems really extreme, and probably only works coincidentally as the turret continues to move to the target. If we're going to keep this, we should rename this constant to something more like TURRET_MOVING_TO_TARGET_TOLERANCE_DEGREES and a different comment alluding to the fact that we expect to reach the target by the time we shoot a piece of fuel?
It'd be even better if isAtTarget() was named e.g. willBeAtTarget(), and we took into account the turret velocity and the expected time it takes for a ball to get from the indexer to the shooter, since I expect this change was made to be able to shoot sooner, but I wouldn't want a more significant logical change such as that done right now since we want to maintain our current performance.
bjmcternan
left a comment
bjmcternan
left a comment
There was a problem hiding this comment.
These are too many changes to make in only a few days after a successful competition. I can't think we've properly tested this. If this was cut into smaller PRs than this would be an easier merge. As of now this is too much risk.
|
I'm going to close this PR for now. We can talk about it tomorrow but it is likely too late to make changes like this. |
|
Reopened to test during the 9:21 test match |
|
That last match looked good. We'll approve and merge |
5 participants
This pull request introduces new autonomous routines and paths for the robot, updates an existing autonomous routine to use the new paths, and makes supporting code changes. The most significant changes are the addition of new auto and path files, the update and renaming of a key path, and the removal of the
Elastic.javautility. Additionally, a new utility class for timing delays is introduced.Autonomous Routines and Paths:
1 Pass Outpost to Mid.autoand2 Pass Outpost to Mid.auto, which sequence robot commands and utilize the new "Pass 1" and "Pass 2" paths. (src/main/deploy/pathplanner/autos/1 Pass Outpost to Mid.auto,src/main/deploy/pathplanner/autos/2 Pass Outpost to Mid.auto) [1] [2]Pass 2 - Outpost to Mid.pathto support the second pass in the new autonomous routine. (src/main/deploy/pathplanner/paths/Pass 2 - Outpost to Mid.path)Path and Routine Updates:
Outpost Center Path + Return.pathtoPass 1 - Outpost to Mid.path, including changes to waypoints, rotation targets, constraint zones, and removal of event markers to better fit the new routines. (src/main/deploy/pathplanner/paths/Pass 1 - Outpost to Mid.path) [1] [2] [3]Outpost Center Score.autoroutine to use the new path and adjusted the shooting command for timing. (src/main/deploy/pathplanner/autos/Outpost Center Score.auto)Utility and Library Changes:
Elastic.javautility class, which provided notification and tab selection support for the Elastic dashboard. (src/main/java/frc/lib/Elastic.java)CountingDelayutility class for managing timed delays in robot code. (src/main/java/frc/lib/CountingDelay.java)Minor Cleanup:
FieldConstants.java. (src/main/java/frc/lib/FieldConstants.java)