export internal subscriptions of feedback and status #3

iuhilnehc-ynos · 2021-03-16T02:06:50Z

For internal review.

Signed-off-by: Chen Lihui [email protected]

Signed-off-by: Andrea Sorbini <[email protected]>

iuhilnehc-ynos · 2021-03-16T02:09:39Z

@fujitatomoya @Barry-Xu-2018

Could you help me review this?

Barry-Xu-2018 · 2021-03-16T08:03:29Z

rcl_action/include/rcl_action/action_client.h

+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | No
+ * Thread-Safe        | No


If only get pointer, is it thread-safe ?

It's not thread-safe (almost all of the api in this files are not thread-safe), so the outside caller need to add a lock for it.

My mistake. It's not thread-safe.

right, in rcl there is not mutex lock currently. i think that this is because of difficulty and maintenance cost since mutex introduces complication especially in C implementation.

Barry-Xu-2018 · 2021-03-17T08:19:29Z

LGTM

fujitatomoya

probably unit test will be required as well.

fujitatomoya · 2021-03-22T00:33:44Z

rcl_action/include/rcl_action/action_client.h

+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | No
+ * Thread-Safe        | No


right, in rcl there is not mutex lock currently. i think that this is because of difficulty and maintenance cost since mutex introduces complication especially in C implementation.

fujitatomoya · 2021-03-22T00:34:34Z

rcl_action/include/rcl_action/action_client.h

+/// Add a goal uuid.
+/**
+ * This function is to add a goal uuid to the map of rcl_action_client_t
+ * and then set content filtered topic again.


Suggested change

* and then set content filtered topic again.

* and then try to set content filtered topic if it is supported.

fujitatomoya · 2021-03-22T00:36:07Z

rcl_action/include/rcl_action/action_client.h

+ *
+ * \param[in] action_client handle to the client that will take the goal response
+ * \param[in] uuid pointer to a uuid which length is 16
+ * \return `RCL_RET_OK` if the response was taken successfully, or


Suggested change

* \return `RCL_RET_OK` if the response was taken successfully, or

* \return `RCL_RET_OK` if success on setting a goal uuid, or

fujitatomoya · 2021-03-22T00:37:14Z

rcl_action/include/rcl_action/action_client.h

+/// Remove a goal uuid.
+/**
+ * This function is to remove a goal uuid from the map of rcl_action_client_t
+ * and then set content filtered topic again.


Suggested change

* and then set content filtered topic again.

* and then try to reset content filtered topic if it is supported.

fujitatomoya · 2021-03-22T00:37:56Z

rcl_action/include/rcl_action/action_client.h

+ *
+ * \param[in] action_client handle to the client that will take the goal response
+ * \param[in] uuid pointer to a uuid which length is 16
+ * \return `RCL_RET_OK` if the response was taken successfully, or


Suggested change

* \return `RCL_RET_OK` if the response was taken successfully, or

* \return `RCL_RET_OK` if success on removing a goal uuid, or

fujitatomoya · 2021-03-22T01:14:40Z

rcl_action/src/rcl_action/action_client.c

@@ -171,6 +176,27 @@ _rcl_action_client_fini_impl(
    goto fail; \
  }

+size_t hash_map_uuid_hash_func(const void * uuid)


just a comment, i think it is okay just to use djb2 hash function here.

fujitatomoya · 2021-03-22T01:17:30Z

rcl_action/src/rcl_action/action_client.c

@@ -222,6 +248,16 @@ rcl_action_client_init(
  SUBSCRIPTION_INIT(feedback);
  SUBSCRIPTION_INIT(status);

+  // Initialize goal_uuids map
+  rcutils_ret_t rcutils_ret = rcutils_hash_map_init(


RCL_RET_FROM_RCUTIL_RET should be used?

I thought that before, but the macro(RCL_RET_FROM_RCUTIL_RET) definition is at https://github.com/ros2/rcl/blob/0ad11506e4a2e7db0925df48e113f66f16db0651/rcl/src/rcl/logging_rosout.c#L46
. OK, I'll move this macro into https://github.com/ros2/rcl/blob/0ad11506e4a2e7db0925df48e113f66f16db0651/rcl/include/rcl/macros.h

yeah, i do not have strong preference here, so either way you go, i am good to go with it.

fujitatomoya · 2021-03-22T01:20:29Z

rcl_action/src/rcl_action/action_client.c

+#define UUID_FMT \
+  "%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X"
+
+#define UUID_FMT_ARGS(uuid) \
+  uuid[0], \
+  uuid[1], \
+  uuid[2], \
+  uuid[3], \
+  uuid[4], \
+  uuid[5], \
+  uuid[6], \
+  uuid[7], \
+  uuid[8], \
+  uuid[9], \
+  uuid[10], \
+  uuid[11], \
+  uuid[12], \
+  uuid[13], \
+  uuid[14], \
+  uuid[15]


probably we would want to move these into rcl_action/include/rcl_action/types.h?

fujitatomoya · 2021-03-22T02:37:34Z

rcl_action/src/rcl_action/action_client.c

+  allocator.deallocate(feedback_filter, allocator.state);
+  feedback_filter = feedback_filter_update;
+
+  for (i = 0; i < size; ++i) {


Does this even work for multiple action clients request the goal?

client-A requests goal-1-A.

client-B requests goal-2-B.

client-A requests goal-3-A.

client-B requests goal-4-B.

as far as i see, example on client-A. it will try to do filtering 1 & 2 which are goal-1-A and goal-2-B on the server side list because client-A/B cannot know the array size on server side? so in this case, we would need probably like a wildcard filtering expression?

Does this even work for multiple action clients request the goal?

Yes, it does.

cannot know the array size on server side?

Sorry? I feel that client-A/B does not need to know the array size on server side.
the feedback and status subscribers are on client side, and they know the array size of goal_id.

Client A request goal-1-A with UUID-1-A

it will set feedback filter with (goal_id.uuid=UUID-1-A) and status filter with (status_list[0].goal_info.goal_id.uuid=UUID-1-A)

Client B request goal-2-B with UUID-2-B

it will set feedback filter with (goal_id.uuid=UUID-2-B) and status filter with (status_list[0].goal_info.goal_id.uuid=UUID-2-B)

Client A request goal-3-A with UUID-3-A ( if goal-1-A is still processed )

it will set feedback filter with (goal_id.uuid=UUID-1-A or goal_id.uuid=UUID-3-A) and status filter with (status_list[0].goal_info.goal_id.uuid=UUID-1-A or status_list[1].goal_info.goal_id.uuid=UUID-3-A or status_list[0].goal_info.goal_id.uuid=UUID-3-A or status_list[1].goal_info.goal_id.uuid=UUID-1-A)

Client B request goal-4-B with UUID-4-B

it will set feedback filter with (goal_id.uuid=UUID-2-B or goal_id.uuid=UUID-4-B) and status filter with (status_list[0].goal_info.goal_id.uuid=UUID-2-B or status_list[1].goal_info.goal_id.uuid=UUID-4-B or status_list[0].goal_info.goal_id.uuid=UUID-4-B or status_list[1].goal_info.goal_id.uuid=UUID-2-B)

What do you think?

so in this case, we would need probably like a wildcard filtering expression?

Sorry, I can't find it to support array wildcard filtering expression? So I add the commend for the https://github.com/iuhilnehc-ynos/rclcpp/blob/4ec21c2414c3c8677445a8bcc8cf9b654cbc65e4/rclcpp/src/rclcpp/time_source.cpp#L126-L133

After reconsideration, yes, it has the issues there. I'll find a way to support this wildcard filtering expression for array.

sorry to be late to get back to this. yeah, the problem is with index of status_list, semantic of index is different between writer and reader.

as far as i can see from https://community.rti.com/static/documentation/connext-dds/5.2.3/doc/manuals/connext_dds/html_files/RTI_ConnextDDS_CoreLibraries_UsersManual/Content/UsersManual/SQL_Filter_Expression_Notation.htm#topics_3596570972_161086, it does not seem to support the wildcard for array nor sequence.

It seems there only exist two ways:

ask RTI to support this kind of feature. (Actually, the "&hex" for UUID is also an extension for the content filtered topic on rti_connext, other DDS implementation might not support this feature.)

change action message type (goal request) to transfer node name, but it will change the original action design. (One design needs to update another design, is it OK to go for it? If so, do you think we need to update the action design first, and then go back to finish this CFT feature.)

Do you have any suggestions?

ask RTI to support this kind of feature. (Actually, the "&hex" for UUID is also an extension for the content filtered topic on rti_connext, other DDS implementation might not support this feature.)

probably 1st step is just ask their opinion is good. with some background about CFT feature, could you create question
on https://github.com/rticommunity/rmw_connextdds/issues?

change action message type (goal request) to transfer node name, but it will change the original action design. (One design needs to update another design, is it OK to go for it? If so, do you think we need to update the action design first, and then go back to finish this CFT feature.)

probably we can skip status topic at this time, only feedback. i would say, CFT is still really good feature for user subscription, parameter events and action feedback topic. so let's finish that 1st. then we can come back to this topic for more additional enhancement with CFT requesting action status message modification. how about that?

could you create question
on https://github.com/rticommunity/rmw_connextdds/issues?

I have opened a feature request on ros2/rmw_connextdds#11.

how about that?

Thanks for your understanding.

@iuhilnehc-ynos

how about we put this status topic enhancement as in T.B.D but we can improve feedback? we will leave the description as T.B.D or W.I.P in official PR? how does it sound?

* make rcl_lifecycle_com_interface optional Signed-off-by: Karsten Knese <[email protected]> * only disable services, not transition event publisher Signed-off-by: Karsten Knese <[email protected]> * flag if com interface is enabled Signed-off-by: Karsten Knese <[email protected]> * use options struct Signed-off-by: Karsten Knese <[email protected]> * attach allocator to state machine struct Signed-off-by: Karsten Knese <[email protected]> * validate allocator before using in fini Signed-off-by: Karsten Knese <[email protected]> * set allocator in default options Signed-off-by: Knese Karsten <[email protected]> * Update rcl_lifecycle/include/rcl_lifecycle/rcl_lifecycle.h Co-authored-by: Chris Lalancette <[email protected]> * remove allocator check in fini Signed-off-by: Karsten Knese <[email protected]> Co-authored-by: Chris Lalancette <[email protected]>

Signed-off-by: Chris Lalancette <[email protected]>

Signed-off-by: Chen Lihui <[email protected]>

iuhilnehc-ynos · 2021-03-24T06:06:04Z

Close it. (already pushed into official PR)

Don't expect RCL_RET_TIMEOUT to set an error string (ros2#900)

fc3f93b

Signed-off-by: Andrea Sorbini <[email protected]>

Barry-Xu-2018 reviewed Mar 16, 2021

View reviewed changes

2.6.0

0ad1150

fujitatomoya reviewed Mar 22, 2021

View reviewed changes

Karsten1987 and others added 4 commits March 23, 2021 11:26

Changelog.

6feb291

Signed-off-by: Chris Lalancette <[email protected]>

3.0.0

2987f9f

to support a feature of content filtered topic

34d0802

Signed-off-by: Chen Lihui <[email protected]>

iuhilnehc-ynos force-pushed the topic-content_filtered_topic-action branch from 0d6a26d to 34d0802 Compare March 24, 2021 05:54

iuhilnehc-ynos closed this Mar 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

export internal subscriptions of feedback and status #3

export internal subscriptions of feedback and status #3

iuhilnehc-ynos commented Mar 16, 2021

iuhilnehc-ynos commented Mar 16, 2021

Barry-Xu-2018 Mar 16, 2021

iuhilnehc-ynos Mar 16, 2021 •

edited

Loading

Barry-Xu-2018 Mar 17, 2021

fujitatomoya Mar 22, 2021

Barry-Xu-2018 commented Mar 17, 2021

fujitatomoya left a comment

fujitatomoya Mar 22, 2021

fujitatomoya Mar 22, 2021

fujitatomoya Mar 22, 2021

fujitatomoya Mar 22, 2021

fujitatomoya Mar 22, 2021

fujitatomoya Mar 22, 2021

fujitatomoya Mar 22, 2021

iuhilnehc-ynos Mar 22, 2021

fujitatomoya Mar 23, 2021

fujitatomoya Mar 22, 2021

fujitatomoya Mar 22, 2021

iuhilnehc-ynos Mar 22, 2021

iuhilnehc-ynos Mar 22, 2021

fujitatomoya Mar 22, 2021

fujitatomoya Mar 23, 2021

iuhilnehc-ynos Mar 23, 2021

fujitatomoya Mar 23, 2021

iuhilnehc-ynos Mar 23, 2021

fujitatomoya Mar 23, 2021

iuhilnehc-ynos commented Mar 24, 2021

	* and then set content filtered topic again.
	* and then try to set content filtered topic if it is supported.

	* \return `RCL_RET_OK` if the response was taken successfully, or
	* \return `RCL_RET_OK` if success on setting a goal uuid, or

	* and then set content filtered topic again.
	* and then try to reset content filtered topic if it is supported.

	* \return `RCL_RET_OK` if the response was taken successfully, or
	* \return `RCL_RET_OK` if success on removing a goal uuid, or

export internal subscriptions of feedback and status #3

export internal subscriptions of feedback and status #3

Conversation

iuhilnehc-ynos commented Mar 16, 2021

iuhilnehc-ynos commented Mar 16, 2021

Choose a reason for hiding this comment

iuhilnehc-ynos Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Barry-Xu-2018 commented Mar 17, 2021

fujitatomoya left a 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

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

Choose a reason for hiding this comment

iuhilnehc-ynos commented Mar 24, 2021

iuhilnehc-ynos Mar 16, 2021 •

edited

Loading