-
Notifications
You must be signed in to change notification settings - Fork 126
Fix imu 2d transform #268
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
Open
efernandez
wants to merge
7
commits into
locusrobotics:devel
Choose a base branch
from
clearpathrobotics:fix-imu-2d-transform
base: devel
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Fix imu 2d transform #268
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
2cd0381
Add diffbot
efernandez 238dc89
Add IMU test
efernandez 02d2323
Fix IMU transform
efernandez d9e246b
Fix -Werror=sign-compare warnings
efernandez 75753e2
Include mutex header
efernandez 60a1328
Fix roslint issues
efernandez 0dbc4ad
Add missed xacro test depend
efernandez 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
<launch> | ||
<arg name="gui" default="false"/> | ||
|
||
<!-- Start world --> | ||
<include file="$(find gazebo_ros)/launch/empty_world.launch"> | ||
<arg name="gui" value="$(arg gui)"/> | ||
</include> | ||
|
||
<!-- Load diffbot model --> | ||
<param name="robot_description" | ||
command="$(find xacro)/xacro '$(find fuse_models)/test/diffbot.xacro'"/> | ||
|
||
<!-- Load diffbot config, e.g. PID gains --> | ||
<rosparam command="load" file="$(find fuse_models)/test/diffbot.yaml"/> | ||
|
||
<!-- Spawn diffbot robot in gazebo --> | ||
<node name="spawn_diffbot" pkg="gazebo_ros" type="spawn_model" | ||
args="-urdf -param robot_description -model diffbot -z 0.5"/> | ||
|
||
<!-- Load controller config --> | ||
<rosparam command="load" file="$(find fuse_models)/test/diffbot_controllers.yaml"/> | ||
|
||
<!-- Spawn controller --> | ||
<node name="controller_spawner" pkg="controller_manager" type="spawner" | ||
args="joint_state_controller | ||
diffbot_controller"/> | ||
|
||
<!-- Convert joint states to TF transforms --> | ||
<node name="robot_state_publisher" pkg="robot_state_publisher" type="robot_state_publisher"/> | ||
</launch> |
Oops, something went wrong.
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.
There are currently multiple target frames supported:
https://github.com/locusrobotics/fuse/blob/devel/fuse_models/include/fuse_models/parameters/imu_2d_params.h#L99-L101
...though I'm not entirely sure why. I would think that both the twist and acceleration would want to use the same target/body frame.
And it's been a long time since I thought about IMUs, but my recollection is that transforming the IMU pose is complicated. I need to review the imu_transformer package in detail. It was not clear to me how the orientation was being transformed.
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.
Yes, that's something I wanted to discuss in this PR and I forgot to mention it.
Right now there are three different target frames:
acceleration_target_frame
orientation_target_frame
twist_target_frame
However, the implementation in imu_transformer transforms everything in the IMU all at once with a single target frame.
I wonder if it really makes sense to have multiple target frames. 🤔
In that case, we could call the
imu_transformer
code three times and only take the components we're interested in, with each target frame. However, in that case, it'd be more efficient, and I believe still clean, to steal some of the code in imu_transformer to perform the same transformation for each component separately.I believe the twist is already done correctly, and probably the acceleration as well. The orientation is definitely incorrect.
Just let me know whether you want to keep the three target frames or not, and I'll make the changes mentioned above?
If we remove the existing target frames and create a singal
target_frame
, then we need some deprecation warnings, and set the newtarget_frame
with them. In that case, if the different target frames aren't the same, I'm not sure which one should be used. I've been usingtwist_target_frame
in this initial implementation.