-
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
Steps and inputmode #689
Conversation
... because otherwise, the effect of the callback is canceled by an assignment in the main update() loop, that calculates input_pos based on steps_, if we're in step/dir mode. - Fixes a steps_ underflow/wrapping bug
.. because circular_setpoints was broken beyond repair.
Defines and uses set_setpoint_to_estimate() reusable function Uses it in three places: - axis.cpp to avoid transient on startup - can_simple.cpp to avoid sudden movement upon mode change - odrive-interface.yaml just to make it executable via USB and UART Swaps set_input_pos() to set_input_pos_and_steps() so that we get the same behavior when changing input_pos over USB and UART as we get when we change it via CAN.
TL;DR. It works. I have tested the following things on this branch:
For axis0, the I have had these sudden movements upon startup with v0.5.1 as well, ca every second time, so it's nothing new. I've gotten so used to it that I almost didn't notice. I don't think it's related to this PR. So I restarted the experiment:
|
To make it match the new PR, which has some minor changes: odriverobotics/ODrive#689
To make it match the new PR, which has some minor changes: odriverobotics/ODrive#689
@@ -58,6 +58,7 @@ class Controller : public ODriveIntf::ControllerIntf { | |||
Controller* parent; | |||
void set_input_filter_bandwidth(float value) { input_filter_bandwidth = value; parent->update_filter_gains(); } | |||
void set_steps_per_circular_range(uint32_t value) { steps_per_circular_range = value > 0 ? value : steps_per_circular_range; } | |||
void set_control_mode(ControlMode value) { control_mode = value; parent->control_mode_updated(); } |
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.
steps_
with input pos.