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

Orientation enum refactoring #341

Closed
wants to merge 9 commits into from

Conversation

TSC21
Copy link
Member

@TSC21 TSC21 commented Jul 16, 2015

This is a fix to was was pointed in #319 and ea8449c.

transform.header = range->header;
transform.child_frame_id = "fcu";
transform.header.frame_id = "fcu";
transform.header.stamp = uas->synchronise_stamp(dist_sen.time_boot_ms);
Copy link
Member

Choose a reason for hiding this comment

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

Use UAS::synchronized_header()

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes... forgot UAS::synchronized_header() has two args.

auto rpy = UAS::sensor_orientation_matching(static_cast<MAV_SENSOR_ORIENTATION>(dist_sen.orientation));
// how it can work, if rpy in degrees, not radians?
auto q = tf::createQuaternionFromRPY(rpy.x(), rpy.y(), rpy.z());
Copy link
Member

Choose a reason for hiding this comment

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

And angles still in degrees, while function i think expects radians.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ugly. Unneeded calculations at every call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well it will do the same thing if it converts to quaternions...

Copy link
Member Author

Choose a reason for hiding this comment

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

And it just does this once for each msg received.

Copy link
Member Author

Choose a reason for hiding this comment

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

For example Eigen::Vector3d v = Eigen::Vector3d(x, y, z) * DEG_TO_RAD; also does calculations on each call (and are exactly the same as the from_degrees() function). So I don't really see your point. You are forcing a problem where it doesn't exist.

Copy link
Member

Choose a reason for hiding this comment

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

make_orientation() needed to fill const array, so no extra calculations.

Copy link
Member

Choose a reason for hiding this comment

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

And quaternion are used in ros for all orientation/rotation things. I don't see any place where you need euler angles now. But i see where quaternion is usable.
Also to do rotation you steel need make quaternion and then apply it to Affine3d or other Q.

So no reason why store this data uncalculated and do calc at each call. That data is a constant, so just do precalculation.

}

return sensor_orientation[idx];
return Eigen::Vector3d(angles::from_degrees(sensor_orientation[idx].x()),
Copy link
Member Author

Choose a reason for hiding this comment

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

A thing I can do here is:

return
Eigen::Vector3d(sensor_orientation[idx].x(), sensor_orientation[idx].y(), sensor_orientation[idx].z()) * DEG_TO_RAD;

It's not so extensive and then I don't need angles.h in this 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.

Or since you want quaternions so bad, I can also do:

return UAS::quaternion_from_rpy(Eigen::Vector3d(sensor_orientation[idx].x(),
                                                sensor_orientation[idx].y(),
                                                sensor_orientation[idx].z()) * DEG_TO_RAD);

Better?

size_t idx = size_t(orientation);
if (idx >= sensor_orientation.size()) {
ROS_WARN_NAMED("uas", "SENSOR: wrong orintation index: %zu", idx);
return tf::Vector3();
return Eigen::Quaterniond();
Copy link
Member

Choose a reason for hiding this comment

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

I think it should return identity (1 0 0 0).

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

@vooon
Copy link
Member

vooon commented Jul 16, 2015

Please, stop working a sec. I'll merge to master.

@vooon
Copy link
Member

vooon commented Jul 16, 2015

Most of this work rebased and applied to master. Thanks.
(To complete with names easier make new PR).

@vooon vooon closed this Jul 16, 2015
@TSC21
Copy link
Member Author

TSC21 commented Jul 16, 2015

Most of this work rebased and applied to master. Thanks.
(To complete with names easier make new PR).

Well... ok

@TSC21 TSC21 deleted the orientation_enum_refact branch July 16, 2015 19:04
@vooon vooon added this to the Version 0.13 milestone Jul 21, 2015
@vooon vooon mentioned this pull request Jul 21, 2015
4 tasks
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.

None yet

2 participants