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.

Sign up for GitHub

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

Jump to bottom

adding mount orientation to mount_control plugin #1297

Merged
merged 2 commits into from
Oct 25, 2019
Merged

adding mount orientation to mount_control plugin #1297

merged 2 commits into from
Oct 25, 2019

Conversation

dayjaby
Copy link
Contributor

@dayjaby dayjaby commented Aug 14, 2019

This PR publishes the gimbal orientation as geometry_msgs/Quaternion in the topic ~mount_control/orientation

TSC21
TSC21 requested changes Aug 14, 2019
View reviewed changes
Copy link
Member

@TSC21 TSC21 left a comment

Choose a reason for hiding this comment

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

Besides the comments, looks good. Could you show us a basic usage of this? Thanks

mavros_extras/src/plugins/mount_control.cpp
}

private:
ros::NodeHandle mount_nh;
ros::Subscriber command_sub;
ros::Publisher mount_orientation_pub;

Copy link
Member

Choose a reason for hiding this comment

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

please remove the extra line

Copy link
Member

Choose a reason for hiding this comment

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

Probably better to apply autoformatter?

mavros_extras/src/plugins/mount_control.cpp Show resolved Hide resolved
Jaeyoung-Lim
Jaeyoung-Lim reviewed Aug 15, 2019
View reviewed changes
mavros_extras/src/plugins/mount_control.cpp Outdated
*/
void handle_mount_orientation(const mavlink::mavlink_message_t *msg, mavlink::common::msg::MOUNT_ORIENTATION &o)
{
auto q = ftf::quaternion_from_rpy(Eigen::Vector3d(o.roll, o.pitch, o.yaw) * M_PI / 180.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a little confused on which frame this quaternion represents. The yaw is relative to the vehicle, but roll and pitch is in the global frame. Shouldn't it use yaw_absolute to be more consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think it's ok as it is, because it returns the same rpy values that you'd send with MAV_CMD_DO_MOUNT_CONTROL. In our usecase we have a gimbal with a yaw range of -320 to 320 degrees and we would like to get that value, no matter how the drone is positioned.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that the relative yaw values are more useful than the absolute value, but the frame you are publishing as orientation is not properly defined anywhere. What are the two frames orientation q is actually representing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reminder! According to mavlink description, the roll and pitch and yaw_absolute are according to global frame. roll, pitch and yaw would not be in the same frame. Maybe we can publish this similar to MountControl.msg: as roll, pitch, yaw, yaw_absolute in a MountOrientation.msg and not as quaternion?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dayjaby I am a little hesitant on having a new custom message definition for that, but I do think that would be less confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then let's publish roll, pitch and yaw_absolute as a quaternion. The relative yaw can be reconstructed if needed by subtracting the heading (/mavros/global_position/compass_hdg). This might be troublesome though, because PX4 only sends MOUNT_ORIENTATION.yaw but no MOUNT_ORIENTATION.yaw_absolute (see https://github.com/PX4/Firmware/blob/c75954fef8ec6a7c7e4f7a68b2e536c1b5b633ab/src/modules/mavlink/mavlink_messages.cpp#L4722). I will do a PR for that to set yaw_absolute as well. This doesn't matter for gimbals who construct complete MOUNT_ORIENTATION messages, but at least for the typhoon h480 in sitl.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh I didn't see that coming. Then I guess doing it as the initial implementation makes sense. I think we just need to be careful not to forget what the yaw means in that case.

@Jaeyoung-Lim
Copy link
Contributor

@dayjaby Any updates?

@dayjaby dayjaby requested a review from TSC21 September 9, 2019 08:05
TSC21
TSC21 requested changes Sep 9, 2019
View reviewed changes
Copy link
Member

@TSC21 TSC21 left a comment

Choose a reason for hiding this comment

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

You are adding things that are do not bellong to this PR. Please rebase your changes.

@dayjaby
Copy link
Contributor Author

dayjaby commented Sep 9, 2019

You are adding things that are do not bellong to this PR. Please rebase your changes.

Done.

@TSC21
Copy link
Member

TSC21 commented Sep 9, 2019

You are adding things that are do not bellong to this PR. Please rebase your changes.

Done.

Nope. Still isn't. Better you close this and create a new PR with a single and clean commit.

@vooon
Copy link
Member

vooon commented Sep 9, 2019

@TSC21 that is not required, it's possible to rebase that branch and then force-push it. Then PR will be updated accordingly.

@dayjaby dayjaby force-pushed the feature/mount_orientation branch 2 times, most recently from b2b5237 to d82cf35 Compare September 9, 2019 09:02
@dayjaby
Copy link
Contributor Author

dayjaby commented Sep 9, 2019

@TSC21 I am sorry! Been some time since doing PRs. Thanks a lot for your reviews and help.

@Jaeyoung-Lim Jaeyoung-Lim mentioned this pull request Sep 26, 2019
Merged
@Jaeyoung-Lim
Copy link
Contributor

Jaeyoung-Lim commented Sep 26, 2019 •
edited
Loading

@dayjaby Could you resolve the conflicts please?

@dayjaby
Copy link
Contributor Author

dayjaby commented Sep 26, 2019

@dayjaby Could you resolve the conflicts please?

Done.

@dayjaby dayjaby closed this Sep 26, 2019
@dayjaby dayjaby reopened this Sep 26, 2019
@TSC21 TSC21 force-pushed the master branch 3 times, most recently from 0759ae2 to 56687ac Compare October 5, 2019 20:56
@TSC21
Copy link
Member

TSC21 commented Oct 7, 2019

@dayjaby we need a new rebase. Sorry

@TSC21 TSC21 added this to the Version 0.33 milestone Oct 7, 2019
@vooon vooon modified the milestones: Version 0.33, Version 0.34 Oct 10, 2019
@dayjaby
Copy link
Contributor Author

dayjaby commented Oct 14, 2019

@TSC21 Done.

@TSC21
Copy link
Member

TSC21 commented Oct 14, 2019

@dayjaby Github is still complaining that the branch has conflicts...

@dayjaby
Copy link
Contributor Author

dayjaby commented Oct 15, 2019

@TSC21 "This branch has no conflicts with the base branch" or where can you see that?

@Jaeyoung-Lim
Copy link
Contributor

@TSC21 Can we merge this now?

@TSC21
Copy link
Member

TSC21 commented Oct 25, 2019

Yes. But I am having a weird problem on my side where it still states that the branch has conflicts. So I can't really merge. @vooon @mhkabir please do.

@vooon vooon merged commit c0a8474 into mavlink:master Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vooon vooon vooon left review comments

@Jaeyoung-Lim Jaeyoung-Lim Jaeyoung-Lim left review comments

@TSC21 TSC21 TSC21 approved these changes

Assignees
No one assigned
Labels
enhancement extras
Projects
None yet
Milestone
Version 1.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dayjaby @Jaeyoung-Lim @TSC21 @vooon