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 steering system tests. Fix steering failure case #82

Closed
wants to merge 3 commits into from

Conversation

Dopp-IO
Copy link
Contributor

@Dopp-IO Dopp-IO commented May 1, 2024

Added steering test cases to cover all expected operating modes. Fixed case where if primary sensor was AWOL and secondary sensor was clamped, system would take 0 degree report from the error'd primary senesor.

@Dopp-IO Dopp-IO requested a review from RCMast3r May 1, 2024 02:25
@walkermburns
Copy link
Contributor

The code looks good to me. Probably should have asked about this more in depth earlier, but what is the expected behavior of this system?
It looks on first glance like it will always prioritize the upper sensor even if it is decided that the state of the encoder is marginal or the sensors exceed the divergence. Is that expected behavior?
Our upper sensor is definitely more precise, but not as accurate to the position of the wheels as the bottom sensor because of the steering slop, and I feel like we should prioritize the bottom sensor if we have decided that the encoder is marginal.

@Dopp-IO
Copy link
Contributor Author

Dopp-IO commented May 2, 2024

The purpose of the steering system as I see it is to gauge driver intent. The upper steering sensor is a better measure of intent than the lower sensor since there's no slop or u-joint. Even when the upper steering sensor is "marginal", the angle it measures will be accurate, just a little less precise. Even in this marginal state, it will significantly outperform the lower sensor.

@walkermburns
Copy link
Contributor

Yeah, that makes sense to me

@walkermburns walkermburns requested review from walkermburns and RCMast3r and removed request for RCMast3r and walkermburns May 2, 2024 02:43
Copy link
Collaborator

@RCMast3r RCMast3r left a comment

Choose a reason for hiding this comment

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

LGTM

@RCMast3r RCMast3r changed the base branch from master to v1.2_candidate May 27, 2024 04:53
@RCMast3r RCMast3r deleted the branch v1.2_candidate September 16, 2024 12:26
@RCMast3r RCMast3r closed this Sep 16, 2024
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