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

chore: move position correction code to lib #8

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

eduardacoppo
Copy link
Contributor

@eduardacoppo eduardacoppo commented Jan 23, 2025

Depends on:

What is this PR for

[sc-67374]
This PR moves the position correction code here

How I did it

Moved the code that was previously in drivers/orogen/nmea0183 and made some improvements in the code.

Results, How I tested

Unit tests

Checklist

  • Assign yourself in the PR
  • Write unit tests (when relevant)
  • Run rubocop and rake tests locally

and make some improvements in the code.
src/AIS.cpp Outdated Show resolved Hide resolved
src/AIS.cpp Show resolved Hide resolved
src/AIS.cpp Outdated Show resolved Hide resolved
src/AIS.cpp Outdated Show resolved Hide resolved
src/AIS.cpp Outdated Show resolved Hide resolved
src/AIS.cpp Outdated Show resolved Hide resolved
use std::string instead of constexpr char and fix tests
src/AIS.hpp Outdated Show resolved Hide resolved
src/AIS.cpp Outdated Show resolved Hide resolved
src/AIS.cpp Outdated Show resolved Hide resolved
src/AIS.cpp Outdated Show resolved Hide resolved
src/AIS.cpp Outdated Show resolved Hide resolved
src/AIS.cpp Outdated Show resolved Hide resolved
test/test_AIS.cpp Outdated Show resolved Hide resolved
and log the message directly instead of saving it to a variable
and fix wrong documentation about it
src/AIS.cpp Outdated Show resolved Hide resolved
and avoid using null values in converter
Copy link
Member

@doudou doudou left a comment

Choose a reason for hiding this comment

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

src/AIS.cpp Outdated Show resolved Hide resolved
src/AIS.cpp Outdated Show resolved Hide resolved
src/AIS.cpp Outdated Show resolved Hide resolved
floating point types are already implicitly converted into double, if necessary
pass utm_converter as const& and write const& instead of const <TYPE>&, for consistency.
Pass position as std::optional
because they're expected behaviors, we shouldn't treat them as errors
@@ -129,3 +136,113 @@ ais_base::VoyageInformation AIS::getVoyageInformation(ais::message_05 const& mes
info.destination = message.get_destination();
return info;
}

std::pair<Eigen::Quaterniond, ais_base::PositionCorrectionStatus> vesselToWorldOrientation(
base::Angle const& yaw,
Copy link
Contributor

Choose a reason for hiding this comment

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

what is yaw? give it a more descriptive name

Copy link
Contributor Author

@eduardacoppo eduardacoppo Jan 30, 2025

Choose a reason for hiding this comment

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

Yaw is the rotation around the vertical axis in a 3d space... this is already a well-established concept. What would a more descriptive name be? Do you have any suggestions?

Choose a reason for hiding this comment

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

He meant the yaw of what with regards to what frame?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did he? Well, I think the name of the function already gives the context we need. But because there's no documentation for this function, I could:

  1. add the prefix vessel_ to all the function variables.
  2. add a documentation for the standalone functions.

I personally prefer the second option.

Choose a reason for hiding this comment

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

I think that the function name does not really tell what it does.

struct CourseOverGround {
    base::Angle const& direction;
    double norm;
};

std::pair<Eigen::Quaterniond, ais_base::PositionCorrectionStatus> selectVesselHeadingSource(
    base::Angle const& compass,
    CourseOverGround const& course_over_ground
)

I think the above is better and no useless comments (as @param some_snake_case_name The some snake case name ) are needed.
Documentation is necessary when the code is not sufficiently expressive, and rewriting function signature in plain english is not documentation for me and I think it is just noisy. It is not a complaint about you specifically, I see this practice in other team members too. Just using this opportunity to raise this discussion

src/AIS.cpp Outdated Show resolved Hide resolved
src/AIS.cpp Outdated Show resolved Hide resolved
@eduardacoppo eduardacoppo requested a review from jhonasiv January 30, 2025 18:43
@@ -129,3 +136,113 @@ ais_base::VoyageInformation AIS::getVoyageInformation(ais::message_05 const& mes
info.destination = message.get_destination();
return info;
}

std::pair<Eigen::Quaterniond, ais_base::PositionCorrectionStatus> vesselToWorldOrientation(
base::Angle const& yaw,

Choose a reason for hiding this comment

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

I think that the function name does not really tell what it does.

struct CourseOverGround {
    base::Angle const& direction;
    double norm;
};

std::pair<Eigen::Quaterniond, ais_base::PositionCorrectionStatus> selectVesselHeadingSource(
    base::Angle const& compass,
    CourseOverGround const& course_over_ground
)

I think the above is better and no useless comments (as @param some_snake_case_name The some snake case name ) are needed.
Documentation is necessary when the code is not sufficiently expressive, and rewriting function signature in plain english is not documentation for me and I think it is just noisy. It is not a complaint about you specifically, I see this practice in other team members too. Just using this opportunity to raise this discussion

gps_base::UTMConverter const& utm_converter)
{
base::samples::RigidBodyState vessel2world_UTM;
vessel2world_UTM.position = sensor2world_UTM.position + sensor2vessel_in_world_pos;

Choose a reason for hiding this comment

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

It is late and I am tired, so I can be very wrong but, shouldn't it be a minus? So it become vessel2sensor_in_world_pos

Suggested change
vessel2world_UTM.position = sensor2world_UTM.position + sensor2vessel_in_world_pos;
vessel2world_UTM.position = sensor2world_UTM.position + (-1 * sensor2vessel_in_world_pos);

also I think the naming is a bit confusing, maybe place all math inside a single method would be less confusing

// vessel and sensor orientation are the same 
auto vessel2sensor_pos = -sensor2vessel_pos;
vessel2world.position = sensor2world.position + sensor2world_ori * vessel2sensor_pos; 

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