changes to do to dev #15
Conversation
…d under to disable
…ings in limelight (still not working)
…es smaller for the swerve
…e module classes to KazaSwerve and SdsSwerve Classes in RobotMap
Swerve sds
NoamRosenberg08
changed the title
| * | ||
| */ | ||
| public static double modulo(double x, double y) { | ||
| public static double modulo(double x, double y) { |
There was a problem hiding this comment.
I know it is already on dev, but I didn't get a chance to write it before on a CR.
GBMath.modulo() is not a clear name, if I will ask someone who didn't hear about it (maybe a team member in a few years), what does GBMath.modulo() do, he won't be able to tell.
The bare minimum is to have clear docs about it, because, what is the "correct modulo result"?
The extra step is to find a better name. I will say, I don't have one right now, but at least improve the docs.
(Or you can simply not use this function, there are other solutions)
|
|
||
| private Battery battery; | ||
|
|
||
| private static final int LEN_OF_AVG = 10; |
There was a problem hiding this comment.
maybe something like, AVG_FILTER_SIZE?
| import edu.greenblitz.pegasus.RobotMap; | ||
| import edu.wpi.first.wpilibj.RobotController; | ||
|
|
||
| public class Battery extends GBSubsystem { |
There was a problem hiding this comment.
Tell me if I missing something, but why is it a subsystem? The only functions it has only use static data.
(another nitpick: why are the changes to the battery on the path-planner CR?)
There was a problem hiding this comment.
it's a subsystem because a command is running on it, and about the nitpick, it's because i made the merge from dev -> path-planner to be safer but apparently i needed to do this the other way
There was a problem hiding this comment.
A command doesn't have to run on a subsystem.
And yes, before merging to dev, you need to merge dev to your branch
|
|
||
| public class Battery extends GBSubsystem { | ||
|
|
||
| private double currentVoltage; |
There was a problem hiding this comment.
even if I am missing something, what can access this variable?
There was a problem hiding this comment.
we don't need it. now it's gone
| SmartDashboard.putNumber("bl abs encoder", blAbsEncoder); | ||
| SmartDashboard.putNumber("fr abs encoder", frAbsEncoder); | ||
| SmartDashboard.putNumber("fl abs encoder", flAbsEncoder); | ||
| // SmartDashboard.putNumber("error", Math.toDegrees(falconEncoder - brAbsEncoder) %); |
| import edu.greenblitz.pegasus.utils.PIDObject; | ||
| import edu.wpi.first.math.kinematics.SwerveModuleState; | ||
|
|
||
| public class DecoyModule implements SwerveModule{ |
There was a problem hiding this comment.
What is the purpose of this class?
There was a problem hiding this comment.
To be able to run some of swerve chassis functions with less then 4 modules
| @Override | ||
| public void rotateToAngle(double angle) { | ||
|
|
||
| double diff = GBMath.modulo(angle - getModuleAngle(), 2 * Math.PI); |
There was a problem hiding this comment.
Talking about lines 79-81:
I must say, even with the knowledge of what the GBMath.modulo function does, the flow here is unclear.
I think that the flow here should be split better using if and maybe using more variables to define calculations that are part of the big calculation.
There was a problem hiding this comment.
Can also be fixed with better doc
| */ | ||
| @Override | ||
| public void rotateToAngle(double angleInRads) { | ||
| double diff = GBMath.modulo(angleInRads - getModuleAngle(), 2 * Math.PI); |
There was a problem hiding this comment.
Same here like the KazaSwerve
There was a problem hiding this comment.
Can also be fixed with better doc
pose estimator and rotate to angle by pose estimator
NoamRosenberg08
changed the title
No description provided.