2025 - draft #107
2025 - draft #107
Conversation
sim is still broken tests are still broken also I probably broke some functionality somewhere
There was a problem hiding this comment.
Some additional work probably has to be done here to make sure all types/methods are covered. At a glance, I don't see Faults implemented, but there are probably others. Given all the other changes, it's probably also worthwhile to see if Rev's fixed the crash when controller doesn't exist bug so we can get rid of this class too.
There was a problem hiding this comment.
how can I test this?
as far as I know, sim motors are automatically given controllers and I'm not sure how to remove them.
...or do I have to run the robot and rip out the wires while it's running?
There was a problem hiding this comment.
The basic test procedure is:
- Unplug all Spark Max/Flex devices from the test board
- Write some code that attempts to drive a Spark Max/Flex
- Deploy and Run
In the past, this caused the code to crash, which is why we created this class. Hopefully, they've fixed that.
There was a problem hiding this comment.
update: it appears to work without crash!
Unsure what I'd need to modify to remove this class, however. I'll take a look at it.
| System.err.println("heyy... lib199 doesn't have sim support sorri"); | ||
| // spark = MockSparkMax.createMockSparkMax(id, SparkLowLevel.MotorType.kBrushless); |
There was a problem hiding this comment.
Keep in mind that this will probably cause the calling code to throw a NullPointerException if run in sim. It might make more sense to print this warning and still initialize the spar normally. This applies to the other methods as well.
src/main/java/org/carlmontrobotics/lib199/swerve/SwerveModule.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/lib199/swerve/SwerveModule.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/lib199/swerve/SwerveModuleSim.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/lib199/swerve/SwerveModuleSim.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/lib199/MotorControllerFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/lib199/MotorControllerFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/lib199/SparkVelocityPIDController.java
Outdated
Show resolved
Hide resolved
#107 (comment) Co-authored-by: CoolSpy3 <[email protected]>
#107 (comment) Co-authored-by: CoolSpy3 <[email protected]>
1. duplicatestrategy to fail 2. no more CachedSparkMax 2. motorcontrollerfactory now configures sparks!! 3. motorErrors checks for null faults 4. swap from smartMotion to MAXmotion in mockedsparkmaxpidcontroller.java 5. cleaner builder pid configs in sparkvelocitypidcontroller.java 6. change from deprecated calculate(double, double) to calculateWithVelocities() in swervemodule and don't persist configs
src/main/java/org/carlmontrobotics/lib199/path/DifferentialDriveInterface.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/lib199/MotorControllerFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/lib199/MotorControllerFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/lib199/swerve/SwerveModule.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/lib199/swerve/SwerveModule.java
Outdated
Show resolved
Hide resolved
also turn on smartdashboard for debugging
|
Sim support... is back, perhaps. |
There was a problem hiding this comment.
Pretty good! Still a couple of minor changes. Also you still have to address a few of the above comments 👍
You should also start looking into updating the tests (it looks like 90% of the bugs are just imports). Here's the official migration guide to get you started.
|
|
||
| config.idleMode(IdleMode.kBrake); | ||
|
|
||
| config.voltageCompensation(12);//FIXME does this need to be different for different motors? |
There was a problem hiding this comment.
Nope, we always assume the nominal voltage is 12V.
| } | ||
| private static SparkFlexConfig baseSparkFlexConfig(MotorConfig motorConfig){ | ||
| //typical operating voltage: 12V. ( same as sparkMax ) | ||
| return (SparkFlexConfig) baseSparkConfig(motorConfig);//criminal casting usage |
There was a problem hiding this comment.
The comment kind of sums this up, but to clarify, this will always throw a ClassCastException.
| //what does this even supposed to do?? | ||
| public static SparkMax createDummySparkMax() { | ||
| return Mocks.mock(SparkMax.class, new REVLibErrorAnswer()); |
There was a problem hiding this comment.
This method was used to create a mocked spark max class when no actual spark was connected (See DummySparkMaxAnswer.java. Now that that class is deleted, I think this can just be removed.
| @@ -202,7 +215,10 @@ private static void reportSparkTemp(int port, CANSparkBase spark) { | |||
| System.err.println("Port " + port + " spark is operating at " + temp | |||
| + " degrees Celsius! It will be disabled until the robot code is restarted."); | |||
| } | |||
| spark.setSmartCurrentLimit(1); | |||
| spark.configure( | |||
| new SparkMaxConfig().smartCurrentLimit(1), | |||
There was a problem hiding this comment.
This will lead to a lot of object allocations if anything overheats. I recommend just pulling this out to a static variable.
| @@ -8,11 +8,15 @@ | |||
|
|
|||
| import org.mockito.internal.reporting.SmartPrinter; | |||
|
|
|||
| import com.ctre.phoenix6.signals.AbsoluteSensorRangeValue; | |||
| // import com.ctre.phoenix6.signals.AbsoluteSensorRangeValue; | |||
There was a problem hiding this comment.
No need to keep stuff like this
| // import com.ctre.phoenix6.signals.AbsoluteSensorRangeValue; |
| enum NEOType { | ||
| NEO(DCMotor.getNEO(1)), | ||
| NEO550(DCMotor.getNeo550(1)), | ||
| Vortex(DCMotor.getNeoVortex(1)), |
There was a problem hiding this comment.
| Vortex(DCMotor.getNeoVortex(1)), | |
| VORTEX(DCMotor.getNeoVortex(1)), |
| * @param countsPerRev the number of counts per revolution of this controller's built-in encoder. | ||
| */ | ||
| public MockSparkBase(int port, MotorType type, String name, int countsPerRev) { | ||
| public MockSparkBase(int port, MotorType type, String name, int countsPerRev, NEOType neoType) { |
There was a problem hiding this comment.
neoType should have an @param tag.
| if (neoType != NEOType.Vortex){ //WARNING can't initialize a sparkbase without an actual spark... | ||
| this.motor = new SparkMax(port,type); | ||
| this.spark = new SparkSim( | ||
| this.motor, | ||
| neoType.dcMotor | ||
| ); | ||
| } else { //only vortex uses sparkflex | ||
| this.motor = new SparkFlex(port,type); | ||
| this.spark = new SparkSim( | ||
| this.motor, | ||
| neoType.dcMotor | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this if condition is backwards
There was a problem hiding this comment.
Ah, sorry, I misread the above :P This is fine, I might swap the blocks and use == though so others don't make the same misreading.
| //simply drop all references for garbage collection (?) | ||
| absoluteEncoderImpl=null; | ||
| analogSensorImpl=null; | ||
| alternateEncoder=null; |
There was a problem hiding this comment.
Nope, we have to call the close methods so that the objects can cleanup their internal state before being destroyed. Garbage collection just wipes them.
Say what you will about C++, destructors are useful.
| private SparkAbsoluteEncoderSim absoluteEncoderImpl = null; | ||
| private SparkMaxAlternateEncoder alternateEncoder = null; | ||
| private SparkMaxAlternateEncoderSim alternateEncoderImpl = null; | ||
| private SparkAnalogSensor analogSensor = null; | ||
| private MockedEncoder analogSensorImpl = null; | ||
| private SparkAnalogSensorSim analogSensorImpl = null; |
There was a problem hiding this comment.
Don't bother swapping these for the sim classes. I agree that those are the right classes for interfacing with the new stuff, but here, they're being used as mock implementations for the classes, and they aren't designed for that.
@brettle pls review
also @CoolSpy3 thanks!!