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

Split irobot_create_toolbox #153

Merged
merged 6 commits into from
Feb 24, 2022
Merged

Split irobot_create_toolbox #153

merged 6 commits into from
Feb 24, 2022

Conversation

alsora
Copy link
Contributor

@alsora alsora commented Feb 17, 2022

Description

Huge, but relatively simple PR that reworks the irobot_create_toolbox package.
Relevant changes:

  • Nodes are moved to a new package named irobot_create_nodes, with the irobot_create_toolbox becoming the place where to define utilities and tools.
  • Register all irobot_create_nodes libraries as rclcpp components. This will allow eventually to dynamically load the nodes into systems and also allows to remove all the boilerplate main.cpp files replacing them with automatically generated ones.
  • Remove the declare_and_get_parameter utility and replace it with standard declare_parameter function. Moreover add a default argument to avoid crashes if a parameter is not provided in the yaml files.
  • Start populating the new irobot_create_toolbox package with stuff that was previously duplicated between ignition and classic code.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Smoke run using Gazebo classic and Gazebo ignition

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

@alsora alsora requested a review from justinIRBT as a code owner February 17, 2022 15:24
${dependencies}
)

rclcpp_components_register_node(motion_control_lib
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This CMake macro register the node as a component.
Essentially it does 2 things:

  • automatically generate a main executable with the provided name
  • allow to use dynamic composition, i.e. load multiple nodes in the same process through launch files (not using this feature yet)

};

// Data structure to hold the definitions related to bumper zones
const std::map<BumperZoneType, BumperZone> BUMPER_ZONES_MAP = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are examples of constants that previously were duplicated between the classic and ignition plugins.
eventually we should move all of them to files like this and also create common, reusable utilities that implement the sensor processing logic.


/// \brief Wrap angle between (-pi, pi]
template<typename T>
inline T WrapAngle(T angle) {return atan2(sin(angle), cos(angle));}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use angles::normalize_angle for this

template<typename T>
bool IsAngleBetween(T target, T angle1, T angle2)
{
T t = WrapAngle(target);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could probably use angles::shortest_angular_distance for this method


/// \brief Convert radians to degrees
template<typename T>
inline int Rad2Deg(T radians) {return radians / M_PI * 180;}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use angles::to_degrees instead (we already have an angles dependency elsewhere in the code). All the angles library suggestions for this file are just suggestions, not sure if this was code added for this review or just moved, in which case, don't worry about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I copied these math operations from the existing code.
I created a ticket to eventually use the angles library #157

@alsora alsora merged commit f1d9359 into main Feb 24, 2022
@alsora alsora deleted the asoragna/split-nodes-toolbox branch February 24, 2022 12:12
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.

2 participants