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

Added humble branch #1

Merged
merged 6 commits into from
Oct 17, 2022
Merged

Added humble branch #1

merged 6 commits into from
Oct 17, 2022

Conversation

routiful
Copy link
Contributor

@routiful routiful commented Oct 16, 2022

This framework is pretty excited! I have tested this on ROS 2 humble ant it works well!
Since ROS 2 API was changed, however, it is needed to modify some files.
I would expect someone want to run this on humble. I added comments for what I changed.

  1. 2f877f5
    rclcpp/client.hpp/SharedFuture() was deprecated.
    So I added a conversion.

  2. 5a427e7
    rclcpp/node_interfaces/node_base/get_callback_group() wasn't supported on humble.
    I added deprecated log on that function and deleted a declared function in cpp.

  3. 2c8c77d
    rclcpp/publisher/publisher() was changed. This function hasn't be permitted overriding.
    I deleted an override option in cluster_node_publisher.cpp.

Thank you for sharing this project :) and hope to see you in ROSCon 2022.

Copy link
Member

@huchijwk huchijwk left a comment

Choose a reason for hiding this comment

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

Many thanks for your contribution. :)

  1. From the Humble, get_callback_group() is replaced with for_each_callback_group(). Please refer to inline comments and Add in callback_groups_for_each. ros2/rclcpp#1723.
  2. Please use a commit message format similar to the previous commits.
    • module or category: blan blah (all lower case)
    • ex) foros: delete override keyward from publisher()

see you at ROSCon

foros/include/akit/failover/foros/cluster_node.hpp Outdated Show resolved Hide resolved
foros/src/cluster_node.cpp Show resolved Hide resolved
Taehun Lim added 3 commits October 17, 2022 01:55
Signed-off-by: Taehun Lim <thlim@robotis.com>
Signed-off-by: Taehun Lim <thlim@robotis.com>
Signed-off-by: Taehun Lim <thlim@robotis.com>
Taehun Lim added 2 commits October 17, 2022 02:21
Signed-off-by: Taehun Lim <thlim@robotis.com>
Signed-off-by: Taehun Lim <thlim@robotis.com>
@routiful
Copy link
Contributor Author

  1. From the Humble, get_callback_group() is replaced with for_each_callback_group(). Please refer to inline comments and ros2/rclcpp#1723.

Appreciated your comments. I deleted get_callback_group() and added for_each_callback_group() with inline comments to a hpp file. Also, I imported an implementation about `for_each_callback_group() to a cpp file.

  1. Please use a commit message format similar to the previous commits.

    • module or category: blan blah (all lower case)
    • ex) foros: delete override keyward from publisher()

I changed every commits that I did. I hope it is correct and helpful.

Copy link
Member

@huchijwk huchijwk left a comment

Choose a reason for hiding this comment

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

Almost done, please align coding style. Fortunately clang format file ('.clang-format') is already included in the project.

There are two methods to check the coding style prior to PR. I did not make a contribution guide and CI workflow yet. sorry. :"-(

Method 1. if you are vscode user, set file for C_Cpp: Clang_format_path in the preference . And re-format documents you modified.
image

Method 2. In the foros top directory,
find . -name *.hpp ! -name foros.hpp -o -name *.cpp | xargs clang-format -i -style=file

(method 1 + auto formatting is higly recommended)

Eventually, you can see below changes.

diff --git a/foros/include/akit/failover/foros/cluster_node.hpp b/foros/include/akit/failover/foros/cluster_node.hpp
index 2838634..0fbc6ee 100644
--- a/foros/include/akit/failover/foros/cluster_node.hpp
+++ b/foros/include/akit/failover/foros/cluster_node.hpp
@@ -135,7 +135,8 @@ class ClusterNode : public std::enable_shared_from_this<ClusterNode>,
       rclcpp::CallbackGroupType group_type,
       bool automatically_add_to_executor_with_node = true);
 
-  /// Iterate over the callback groups in the node, calling func on each valid one.
+  /// Iterate over the callback groups in the node, calling func on each valid
+  /// one.
   /**
    * From Humble, get_callback_groups() is replaced with this method.
    * https://github.com/ros2/rclcpp/pull/1723
@@ -144,7 +145,8 @@ class ClusterNode : public std::enable_shared_from_this<ClusterNode>,
    */
   CLUSTER_NODE_PUBLIC
   void for_each_callback_group(
-    const rclcpp::node_interfaces::NodeBaseInterface::CallbackGroupFunction & func);
+      const rclcpp::node_interfaces::NodeBaseInterface::CallbackGroupFunction
+          &func);
 
   /// Create a Publisher.
   /**
diff --git a/foros/src/cluster_node.cpp b/foros/src/cluster_node.cpp
index a9dc831..ce4ba8d 100644
--- a/foros/src/cluster_node.cpp
+++ b/foros/src/cluster_node.cpp
@@ -120,7 +120,8 @@ rclcpp::CallbackGroup::SharedPtr ClusterNode::create_callback_group(
 }
 
 void ClusterNode::for_each_callback_group(
-    const rclcpp::node_interfaces::NodeBaseInterface::CallbackGroupFunction & func) {
+    const rclcpp::node_interfaces::NodeBaseInterface::CallbackGroupFunction
+        &func) {
   node_base_->for_each_callback_group(func);
 }

Signed-off-by: Taehun Lim <thlim@robotis.com>
@routiful
Copy link
Contributor Author

Thank you for details! I checked coding style using clang :)
Furthermore, I ran colcon_test to confirm whether every test are passed. As a result all tests are cleanly passed.

Copy link
Member

@huchijwk huchijwk left a comment

Choose a reason for hiding this comment

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

LGTM

@huchijwk huchijwk merged commit fb76bfc into 42dot:humble Oct 17, 2022
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