service complete version commit #30

miyakoshi-dev · 2022-09-09T01:48:26Z

@adamdbrw
I have implemented the service.
When you have time, could you please check it for me?

miyakoshi-dev · 2022-09-09T02:04:07Z

Now that we have completed the implementation of the service, we have closed this draft PR and created a new PR.

miyakoshi-dev · 2022-09-09T02:06:04Z

I closed this by mistake, so I will reopen it.

adamdbrw · 2022-09-09T09:57:04Z

Thank you for the contribution, it would prove very valuable indeed!
We will review it with @pijaro.

adamdbrw · 2022-09-13T08:51:49Z

@pijaro if you could, please review today

pijaro · 2022-09-13T14:15:26Z

@miyakoshi-dev Thanks for the nice PR! I'm currently looking into the code. I will finish my review tomorrow 👍

pijaro · 2022-09-13T11:54:57Z

src/ros2cs/ros2cs_core/Client.cs

+    /// <summary> Service a message </summary>
+    /// <see cref="IService.SendAndRecv"/>
+    public IntPtr SendAndRecv(T msg)
+    ///public long SendAndRecv(T msg)


I believe we can remove unnecessary devel comments 👍

pijaro · 2022-09-13T12:09:58Z

src/ros2cs/ros2cs_core/Client.cs

+  /// <description> Services are created through INode.CreateClient </description>
+  public class Client<T>: IClient<T> where T : Message, new ()
+  {
+    public string Topic { get { return topic; } }


We could use some code formatting here 😃 Please group private/public and explicitly denote the accessibility of the members.

pijaro · 2022-09-13T12:51:35Z

src/ros2cs/ros2cs_core/Client.cs

+
+      /// send request
+      Utils.CheckReturnEnum(NativeRcl.rcl_send_request(ref serviceHandle, msgInternals.Handle, ref sequence_number));
+      System.Threading.Thread.Sleep(1000);


If this is a synchronous send and receive, then the 1sec wait is not enough for most use cases. Maybe some wait loop here?

Additionally, I think that async send and receive would make more sense - but this may be added later 👍

pijaro · 2022-09-13T12:54:28Z

src/ros2cs/ros2cs_core/Ros2cs.cs

@@ -231,6 +247,11 @@ public static void SpinOnce(List<INode> nodes, double timeoutSec = 0.1)
        {
          subscription.TakeMessage();
        }
+        ///WaitSet.Wait(global_context, allServices, timeoutSec);


Devel comment to remove

pijaro · 2022-09-13T13:04:10Z

src/ros2cs/ros2cs_core/interfaces/INode.cs

+    /// <description> Can only be called in an initialized Ros2cs state. </description>
+    /// <param name="topic"> Topic for the service. Naming restrictions of ros2 apply and violation results in an exception </param>
+    /// <param name="qos"> Quality of Service settings. Not passing this parameter will result in default settings </param>
+    /// <returns> Service for the topic, which can be used to service messages </returns>


Returns client (copy paste error 😃 )

pijaro · 2022-09-13T13:05:05Z

src/ros2cs/ros2cs_core/interfaces/INode.cs

+    /// <returns> Service for the topic, which can be used to service messages </returns>
+    Client<T> CreateClient<T>(string topic, QualityOfServiceProfile qos = null) where T : Message, new();
+
+    /// <summary> Remove a service </summary>


"Remove a client"

pijaro · 2022-09-13T13:06:32Z

src/ros2cs/ros2cs_core/native/NativeRcl.cs

@@ -179,6 +297,9 @@ internal static PublishType
        "rcl_publish"),
        typeof(PublishType));

+


Unnecessary lines

pijaro · 2022-09-13T13:16:50Z

src/ros2cs/rosidl_generator_cs/cmake/rosidl_generator_cs_generate_interfaces.cmake

-message(STATUS "Generating C# code for ROS interfaces ${_generated_msg_cs_files}")
-
-set(ros2_distro "$ENV{ROS_DISTRO}")
-if(ros2_distro STREQUAL "foxy" OR ros2_distro STREQUAL "galactic")


I'm not sure if that is still the case, but it was important for Humble setups

Did you try this PR with all supported ROS2 versions? (foxy, galactic, humble)

My quick test on humble failed to build. Most probably, this is one of the causes.

pijaro · 2022-09-14T14:41:04Z

src/ros2cs/rosidl_generator_cs/cmake/rosidl_generator_cs_generate_interfaces.cmake

@@ -223,15 +235,116 @@ foreach(_generated_msg_c_ts_file ${_generated_msg_c_ts_files})
    ${PROJECT_NAME}__rosidl_generator_c
  )

-  set(ros2_distro "$ENV{ROS_DISTRO}")
+  rosidl_target_interfaces(${_target_name}


rosidl_target_interfaces is deprecated since Humble: https://docs.ros.org/en/humble/Releases/Release-Humble-Hawksbill.html#deprecation-of-rosidl-target-interfaces

It is a good idea to keep humble and up in check.

pijaro · 2022-09-14T14:42:02Z

src/ros2cs/rosidl_generator_cs/cmake/rosidl_generator_cs_generate_interfaces.cmake


-  if(ros2_distro STREQUAL "humble" OR ros2_distro STREQUAL "rolling")
-    rosidl_get_typesupport_target(c_typesupport_target "${PROJECT_NAME}" "rosidl_typesupport_c")


Use new API for humble and up (https://docs.ros.org/en/humble/Releases/Release-Humble-Hawksbill.html#deprecation-of-rosidl-target-interfaces)

pijaro · 2022-09-14T14:47:25Z

I've added some feedback 👍 Thanks again for such an important PR 👍

I also did a quick test on galactic and humble (u20) - it worked for galactic but failed for humble. It would be nice to keep the compatibility between ROS2 releases as well.

miyakoshi-dev · 2022-09-14T23:22:41Z

Thank you for your review.
I will reflect your comments when I have time.
Currently, it only supports galactic, so humble has not been verified.

miyakoshi-dev · 2022-09-28T04:40:06Z

Addressed non-Humble related review comments.

・Improve typing of service response ・remove unused references to example_interfaces (2) Finished checking with humble/galactic under ubuntu/windows environment

miyakoshi-dev · 2022-11-01T04:14:50Z

@pijaro @adamdbrw
hello
It's been a while, but now it supports humble.
In addition to humble support, it supports the PR I received on my github.
I have confirmed that the service works on humble/galactic on ubuntu/windows.

pijaro · 2023-01-23T12:20:49Z

I will close this PR since the work is being continued on #36 by @Deric-W 👍
@miyakoshi-dev let's continue our work there 🏅

service complete version commit

b01a111

miyakoshi-dev closed this Sep 9, 2022

miyakoshi-dev reopened this Sep 9, 2022

adamdbrw requested review from pijaro and adamdbrw September 9, 2022 09:56

adamdbrw mentioned this pull request Sep 13, 2022

Add support for services #1

Closed

pijaro reviewed Sep 14, 2022

View reviewed changes

Modified by review comments

f7a8b2d

miyakoshi-dev added 3 commits October 14, 2022 11:07

Supported hamble. (confirmed only in the windows environment)

3f964c2

Supported hamble again. (confirmed only in the windows environment)

67b6435

(1) Corresponding to the following pull request

03f949d

・Improve typing of service response ・remove unused references to example_interfaces (2) Finished checking with humble/galactic under ubuntu/windows environment

Replace with local variable

52a820f

Deric-W mentioned this pull request Dec 14, 2022

further work on service PR #36

Merged

Deric-W mentioned this pull request Dec 21, 2022

add Service and Client methods RobotecAI/ros2-for-unity#58

Merged

pijaro closed this Jan 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

service complete version commit #30

service complete version commit #30

miyakoshi-dev commented Sep 9, 2022

miyakoshi-dev commented Sep 9, 2022

miyakoshi-dev commented Sep 9, 2022

adamdbrw commented Sep 9, 2022

adamdbrw commented Sep 13, 2022

pijaro commented Sep 13, 2022

pijaro Sep 13, 2022

pijaro Sep 13, 2022

pijaro Sep 13, 2022

pijaro Sep 13, 2022

pijaro Sep 13, 2022

pijaro Sep 13, 2022

pijaro Sep 13, 2022

pijaro Sep 13, 2022

pijaro Sep 14, 2022

pijaro Sep 14, 2022

pijaro commented Sep 14, 2022

miyakoshi-dev commented Sep 14, 2022

miyakoshi-dev commented Sep 28, 2022

miyakoshi-dev commented Nov 1, 2022

pijaro commented Jan 23, 2023

		@@ -179,6 +297,9 @@ internal static PublishType
		"rcl_publish"),
		typeof(PublishType));


		if(ros2_distro STREQUAL "humble" OR ros2_distro STREQUAL "rolling")
		rosidl_get_typesupport_target(c_typesupport_target "${PROJECT_NAME}" "rosidl_typesupport_c")

service complete version commit #30

service complete version commit #30

Conversation

miyakoshi-dev commented Sep 9, 2022

miyakoshi-dev commented Sep 9, 2022

miyakoshi-dev commented Sep 9, 2022

adamdbrw commented Sep 9, 2022

adamdbrw commented Sep 13, 2022

pijaro commented Sep 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pijaro commented Sep 14, 2022

miyakoshi-dev commented Sep 14, 2022

miyakoshi-dev commented Sep 28, 2022

miyakoshi-dev commented Nov 1, 2022

pijaro commented Jan 23, 2023