Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to Talon SRX for an integrated absolute encoder #208

Merged
merged 17 commits into from
Dec 4, 2024

Conversation

WispySparks
Copy link

First time working around the codebase, I went this route because I don't think you can really have a reference to a talon's sensor like you can with revlib. Not sure if this all looks right

@thenetworkgrinch
Copy link
Contributor

This shouldnt work, you didnt create the wrapper for a psuedo absolute encoder like we discussed.

It is a good attempt but I would not accept it in its current state. The other issue is you seem to forget that the absolute encoder is ALWAYS right, the absolute encoder can change the position, NOT the motor controller.

An acceptable trade off which i may consider is implementing both abstractions for motor controllers and absolute encoders in one class but i dont think that will work out well.

@thenetworkgrinch
Copy link
Contributor

You can rely on the TalonSRX sensor which is an attached absolute encoder. You just need to create a wrapper which stores the same motor object (NOT CREATE A NEW ONE) and exclusively polls the encoder position in the correct units.

@thenetworkgrinch
Copy link
Contributor

You're getting closer!! Good job!

@thenetworkgrinch
Copy link
Contributor

Last thing is, since this is a larger change than normal, please test this on an actual robot and let us know when you do! Once it is tested it can be merged. (Note: This does NOT need to be tested by you, just an actual robot)

@WispySparks
Copy link
Author

WispySparks commented May 20, 2024

Okay it might be a while until this is able to be tested but I'll let you know

@thenetworkgrinch
Copy link
Contributor

Any updates on this?

@WispySparks
Copy link
Author

Our swerve is in the process of being built over the summer, we still need to make a belly plate and wire stuff up but we only meet once a week. I'll let you know once we're testing stuff

@WispySparks WispySparks reopened this Jul 31, 2024
@thenetworkgrinch
Copy link
Contributor

Any updates in regards to testing? :D

@WispySparks
Copy link
Author

Still working through haha, at the point of tuning PID now I believe

@WispySparks
Copy link
Author

Currently we're doing a bandaid fix for selecting the current sensor into the TalonSRX because for some reason it's getting set to QuadEncoder instead of PulseWidth. I've looked through my pr'ed code and YAGSL code for a while but still haven't found the issue so we're doing this for now to get swerve up and running.

swerveDrive = parser.createSwerveDrive(SwerveK.maxRobotSpeed.in(MetersPerSecond), angleConversionFactor, driveConversionFactor);
for (var mod : swerveDrive.getModules()) { // Bandaid Solution
    var motor = (WPI_TalonSRX) mod.getAngleMotor().getMotor();
    motor.configSelectedFeedbackSensor(FeedbackDevice.PulseWidthEncodedPosition);
}

@thenetworkgrinch
Copy link
Contributor

Huh thats odd, you shouldnt need to, maybe you should make that checkc not use .getMotor instead check if its a wrapper type

@cmdada
Copy link

cmdada commented Nov 29, 2024

If you need a swerve bot with talons to test on I have access to am swerve with talon srxs on turning and would happily test code for it

@WispySparks
Copy link
Author

I'll be in the shop tomorrow testing that last change for the sensor being selected (I only did it in sim so far) and then this should be good to go.

@WispySparks
Copy link
Author

Everything looks good on my end, this should be good to merge.

@thenetworkgrinch
Copy link
Contributor

Almost i need to merge beta into dev then add a new function i recently added to the SwerveMotor.java

@thenetworkgrinch
Copy link
Contributor

Could you resolve the conflicts when you get a chance?

@thenetworkgrinch thenetworkgrinch merged commit 53c9d32 into BroncBotz3481:dev Dec 4, 2024
@thenetworkgrinch
Copy link
Contributor

Just in time for the 2025 release!

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.

3 participants