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

fuse -> ROS 2 fuse_models: Linting #315

Merged

Conversation

methylDragon
Copy link
Collaborator

See: #276
Depends on: #314

Linting
Migrates .h to .hpp
Pinging @svwilliams for visibility.

Note: There's an open question about what to do about test_unicycle_2d.cpp and test_sensor_proc.cpp's proprietary copyright licenses (to fix ament_copyright)

@methylDragon methylDragon force-pushed the rolling-models-linting branch 2 times, most recently from 9379686 to cf7a114 Compare January 28, 2023 09:51
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Copy link
Collaborator Author

methylDragon commented Jan 28, 2023

Note @svwilliams : Two files had to get excluded from linting: e62784b

This is because they featured copyright docstrings with proprietary copyrights.

https://github.com/methylDragon/fuse/blob/rolling-models-linting/fuse_models/test/test_sensor_proc.cpp

/***************************************************************************
 * Copyright (C) 2019 Clearpath Robotics. All rights reserved.
 * Unauthorized copying of this file, via any medium, is strictly prohibited
 * Proprietary and confidential
 ***************************************************************************/

https://github.com/methylDragon/fuse/blob/rolling-models-linting/fuse_models/test/test_unicycle_2d.cpp

/***************************************************************************
 * Copyright (C) 2017 Locus Robotics. All rights reserved.
 * Unauthorized copying of this file, via any medium, is strictly prohibited
 * Proprietary and confidential
 ***************************************************************************/

Signed-off-by: methylDragon <methylDragon@gmail.com>
Copy link
Collaborator

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

All tests in fuse_publishers and fuse_models pass for me locally. When these comments are addressed LGTM

* 1. Creates relative or absolute orientation and constraints. If the \p differential parameter is
* set to false (the default), the orientation measurement will be treated as an absolute
* constraint. If it is set to true, consecutive measurements will be used to generate relative
* orientation constraints. 2. Creates 2D velocity variables and constraints.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit, newline to keep list structure.

* This sensor subscribes to a nav_msgs::msg::Odometry topic and:
* 1. Creates relative or absolute pose variables and constraints. If the \p differential parameter
* is set to false (the default), the measurement will be treated as an absolute constraint. If it
* is set to true, consecutive measurements will be used to generate relative pose constraints. 2.
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment about new line to keep list structure.

* velocity (array with x at index 0, y at index 1) 3 : vel_yaw1 - First
* yaw velocity 4 : acc_linear1 - First linear acceleration (array with x
* at index 0, y at index 1) 5 : position2 - Second position (array with x
* at index 0, y at index 1) 6 : yaw2 - Second yaw 7 : vel_linear2 -
Copy link
Collaborator

Choose a reason for hiding this comment

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

Newlines to preserve list structure here

// if (jacobians[i])
// {
// Eigen::Map<fuse_core::Matrix<double, 8, ParameterDims::GetDim(i)>> jacobian(jacobians[i]);
// jacobian.applyOnTheLeft(-A_); } }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Newlines to preserve code example here

Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon merged commit a6e6866 into locusrobotics:rolling Jan 30, 2023
@methylDragon methylDragon deleted the rolling-models-linting branch January 30, 2023 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants