Conversation
JAMthepersonj
requested review from
MikeKantrowitz,
armadilloz,
batchen1,
bengold22 and
noahheller
as code owners
|
this doesnt appear to be using the newly created class of TunablePidManager. Also why are we doing pid tuning in swerve debug mode and not tuning mode. this appears to not follow the conventions for pid tuning set by the hihi extender commit. I might be wrong and you should ask ben before doing any of the stuff I say but if he says then look at how the extender pid tuning is written. |
| this.poseEstimator = | ||
| new PoseEstimator( | ||
| frontLeft, frontRight, backLeft, backRight, apriltagIO, kinematics, getLastGyro()); | ||
| if(Constants.SWERVE_DEBUG){ |
There was a problem hiding this comment.
Shouldn't this be the constant TUNING_MODE?
There was a problem hiding this comment.
Yeah that's probably reasonable, we can enable/ disable this as a whole
|
|
||
| private final SparkMax driveMotor; | ||
| private final SparkBaseConfig driveConfig; | ||
| private final SparkMaxConfig driveConfig; |
There was a problem hiding this comment.
We shouldn't actually be keeping this around, we've seen it's better to only have local variable versions of the config objects.
| SmartShuffleboard.put("Drive", "BL ABS Pos", backLeft.getAbsPosition()); | ||
| SmartShuffleboard.put("Drive", "BR ABS Pos", backRight.getAbsPosition()); | ||
| } | ||
| if(Constants.SWERVE_DEBUG){ |
There was a problem hiding this comment.
Per Aditya's ideas, switch this to TUNING_MODE
| @Override | ||
| public void updateConfig( | ||
| double closedLoopRampRate, double secondaryCurrentLimit, int smartCurrentLimit) { | ||
| driveConfig |
There was a problem hiding this comment.
Don't re-use the instance variable, create a new instance of the config here
neo-Aditya
left a comment
neo-Aditya
left a comment
There was a problem hiding this comment.
Looks good, is this tested?
# Conflicts: # src/main/java/frc/robot/constants/GameConstants.java
|
yeah pull from main if possible |
5 participants
No description provided.