Skip to content

Conversation

@FriedLongJohns
Copy link
Contributor

@brettle pls review
also @CoolSpy3 thanks!!

sobbing
sim is still broken
tests are still broken
also I probably broke some functionality somewhere
@FriedLongJohns FriedLongJohns requested a review from a team as a code owner January 15, 2025 01:16
sofiebudman and others added 8 commits February 7, 2025 17:30
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
timtogan and others added 7 commits December 21, 2025 16:47
Copy link
Member

@CoolSpy3 CoolSpy3 left a comment

Choose a reason for hiding this comment

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

Alright! Just a few more notes on how motors are handled. After this, I think I'll do another dive through the whole PR, look for any final bugs, apply some formatting fixes, and (barring any other major snags) this should be good to go! Home stretch now!

That being said, simulation is still probably in need of a pretty deep rewrite. I don't think that needs to be part of this PR though, I'd rather get polished 2025 code merged to master, and then sim can be tackled separately.

@timtogan
Copy link
Member

timtogan commented Jan 1, 2026

@CoolSpy3 Alr I am pretty sure I addresed pretty much all your comments if you could if you check everything that would be awesome :D

Copy link
Member

@CoolSpy3 CoolSpy3 left a comment

Choose a reason for hiding this comment

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

Awesome! I think all of the major stuff has been addressed. Just two more minor comments. I'll start re-reading everything for anything else that should be fixed before this merges.

Copy link
Member

@CoolSpy3 CoolSpy3 left a comment

Choose a reason for hiding this comment

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

Alright, here's the last batch of changes. After this, I might just run through and fix some formatting things, and then this should be good to merge!

Comment on lines +83 to +84
driveConfig = driveMotorType.createConfig();
turnConfig = turnMotorType.createConfig();
Copy link
Member

Choose a reason for hiding this comment

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

These don't appear to be used outside of the constructor. I'd bring them into the local scope so that they don't get used later.

FWIW, an alternative choice would be to keep them as-is and use them whenever the motors need to be configured (i.e. in the brake() and coast() functions), but that would overwrite any interim config changes, which would be annoying to debug.

Copy link
Member

@timtogan timtogan Jan 11, 2026

Choose a reason for hiding this comment

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

Aren't these being used in brake() and coast() already?

driveConfig.encoder
.positionConversionFactor(drivePositionFactor)
.velocityConversionFactor(driveVelocityFactor)
.quadratureAverageDepth(2);
Copy link
Member

Choose a reason for hiding this comment

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

Why are we setting this?

Copy link
Member

Choose a reason for hiding this comment

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

I mean this was added before I started working on this PR, so I am also not entirely sure why we are setting. I'll try to look into if there is a reason to set it.

@timtogan
Copy link
Member

Ok, I basically finished on this batch! Just need to get back to you on this: #107 (comment) I'll prob do it today or tommorow morning. (Also fyi the javadoc doesn't build rn only because ctre javadocs are down)

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.

8 participants