-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Steps and inputmode #689
Merged
Merged
Steps and inputmode #689
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
ae262e4
Sets steps_ in CANSimple::set_input_pos_callback
tobbelobb d272fbe
Puts fmodf_pos back into the steps_ assignment
tobbelobb 58a6567
Stands still upon switching to position mode via can
tobbelobb c5017f4
Making circular_setpoints optional in step/dir mode
tobbelobb fa144cf
Makes behavior consistent across interfaces
tobbelobb 6444197
make input mode change trigger consistent with all sources
madcowswe 4ca99a6
fix setter logic
madcowswe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sets the input position to the current position estimate every time the control mode is switched. Is this important for some use cases?
I could imagine cases where a user wants to seamlessly transition from velocity control to position control with a preconfigured setpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, seems like a good "Law of Least Surprise" thing anyway. Switching from vel to pos control could result in the motor suddenly moving, which isn't good for safety. Unless the user explicitly wants that behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The seamless transition into a standstill position mode, and the safety it provides are both important features for the Hangprinter use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going from not-position control into position control: setting the setpoint to the estimate is fine here. I would say it's better to do this because it's safer, and less surprising like Paul said. The person in your case will have to set the pos setpoint after changing mode, which I think is acceptable.