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

Steps and inputmode #689

Merged
merged 7 commits into from
May 31, 2022
Merged

Steps and inputmode #689

merged 7 commits into from
May 31, 2022

Conversation

madcowswe
Copy link
Member

  • Sync steps_ with input pos.
  • Trigger reset of input_pos and pos_setpoint to estimate when changing control mode into position control

tobbelobb and others added 6 commits April 7, 2022 15:10
... 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.
@madcowswe madcowswe requested review from Wetmelon and samuelsadok May 5, 2022 20:42
@CLAassistant
Copy link

CLAassistant commented May 5, 2022

CLA assistant check
All committers have signed the CLA.

@madcowswe madcowswe changed the base branch from master to devel May 5, 2022 20:42
@tobbelobb
Copy link
Contributor

tobbelobb commented May 13, 2022

TL;DR. It works.

I have tested the following things on this branch:

  • Compile and upload
  • Configured like this and then this
  • Checked input_pos on both axes immediately after startup.

For axis0, the input_pos was 0 as expected. For axis1 I found input_pos to be 0.94. As if the motor had been stepped 376 times forward during startup. (Steps per revolution is 400, and 376/400 = 0.94.) I heard that this rotation also happened physically. Immediately after its index search, one of the motors made a ~1 rotation movement.

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:

  • odrv0.reboot()
  • This time, both axes' input_pos were 0 after startup.
  • Ran both motors back and forth a couple of times, both by setting input_pos inside odrivetool and by sending step signals from the Duet. Both behaved as expected.
  • Put both motors in torque mode via CAN.
  • Turned both motors a few turns by hand.
  • Set both motors back into position mode via CAN. They both stood still after receiving the CAN command, and entered pos mode as expected.
  • Confirmed via odrivetool that input_pos of both motors had been updated.

tobbelobb added a commit to tobbelobb/hangprinter that referenced this pull request May 13, 2022
To make it match the new PR, which has some minor changes:
odriverobotics/ODrive#689
tobbelobb added a commit to tobbelobb/hangprinter that referenced this pull request May 13, 2022
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(); }
Copy link
Member

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.

Copy link
Collaborator

@Wetmelon Wetmelon May 23, 2022

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

@samuelsadok samuelsadok merged commit ae11bbc into devel May 31, 2022
@samuelsadok samuelsadok deleted the steps-and-inputmode branch May 31, 2022 05:43
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.

5 participants