Skip to content

Initial positions are set to 0 causing sudden movement#67

Open
daniel-kovari wants to merge 4 commits intoenactic:mainfrom
Kovari-Industries:main
Open

Initial positions are set to 0 causing sudden movement#67
daniel-kovari wants to merge 4 commits intoenactic:mainfrom
Kovari-Industries:main

Conversation

@daniel-kovari
Copy link
Copy Markdown

@daniel-kovari daniel-kovari commented Nov 8, 2025

Fixes #66

@daniel-kovari daniel-kovari changed the title Initial positions are set to 0 causing sudden movement #66 Initial positions are set to 0 causing sudden movement Nov 8, 2025
@kou kou requested a review from thomasonzhou November 10, 2025 08:02
@euyniy
Copy link
Copy Markdown
Contributor

euyniy commented Nov 19, 2025

Will test on the hardware!! Thanks

Copy link
Copy Markdown

@ggorjup ggorjup left a comment

Choose a reason for hiding this comment

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

I tested this on real hardware and it seems to work fine.

In case it's useful, I'm suggesting a couple of simplifications.

Comment thread openarm_hardware/src/v10_simple_hardware.cpp Outdated
Comment thread openarm_hardware/src/v10_simple_hardware.cpp Outdated
Comment thread openarm_hardware/src/v10_hardware.cpp Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the work!
Could v10_hardware.cpp go into a separate PR? This looks unrelated to the zero-position issue.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi, I cleaned up the PR. I moved the changes for gravity compensation onto another feature branch: https://github.com/Kovari-Industries/openarm_ros2/tree/feat/gravity-compensation. It is using a KDL chain and implying the urdf from the controller manager - HardwareInfo::original_xml. This works for me and makes the controls much smoother. If this is a valid implementation, I can put that up as a separate PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for splitting that out. It's much cleaner to review now.

Gravity compensation is something we're interested in as well, so please feel free to open it as a separate PR.
We can discuss the design (KDL chain, reading URDF via HardwareInfo::original_xml, etc.) over there.

"Setting current position...");

// Use read() to populate state arrays
read(rclcpp::Time(), rclcpp::Duration(0, 0));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What if CAN communication fails? Won't pos_states_ be left holding a stale or default value after read()?
Would it make sense to add a safety check?

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.

Initial positions are set to 0 causing sudden movement

4 participants