Action file support by sservulo · Pull Request #311 · ros2/rosidl

sservulo · 2018-10-31T14:38:54Z

This PR closes #305. Added .action file parsing to rosidl_parser.
CMake preprocessing of .action files into services and messages is coming to this branch from sloretz/speficication-to-str.

hidmic

@sservulo I left some comments, nice work!

rosidl_parser/rosidl_parser/__init__.py

hidmic · 2018-10-31T15:49:22Z

rosidl_parser/rosidl_parser/__init__.py

+        lines[separator_indices[1] + 1:] + implicit_output)
+    feedback_msg = parse_message_string(
+        pkg_name, action_name + ACTION_FEEDBACK_MESSAGE_SUFFIX, message_string)
+    #----------------------------------------------------------------------------------------------


@sservulo Hmm, implicit fields may clash with explicitly given ones. Action generation in ROS1 would nest message definition as a field of a completely implicit message, see genaction.py sources. Alternatively, we could check for the presence of duplicate entries. @sloretz @jacobperron thoughts?

That's a good point. +1 to nesting the user defined message

Other issues were already addressed. @sloretz, this one will probably delay a bit cmake header typesupport on #302 and #303 due to the need to update location assumptions of nested msgs.

rosidl_parser/rosidl_parser/__init__.py

sservulo · 2018-11-01T20:53:48Z

Addressed review comments.

rosidl_parser/rosidl_parser/__init__.py

sloretz · 2018-11-01T23:07:35Z

rosidl_parser/rosidl_parser/__init__.py

+        action_name + ACTION_GOAL_MESSAGE_SUFFIX + SERVICE_REQUEST_MESSAGE_SUFFIX,
+        request_message_string)
+
+    implicit_output = ["bool accepted", "builtin_interfaces/Time stamp"]


@jacobperron @hidmic Will rcl_actions associate the service response by mapping goal ID to the request sequence number, or by having the higher level client library get the goal ID out of the message? If the former then these fields should be OK, if the latter then the stamp field should be action_msgs/GoalInfo so it includes a goal id too.

I think that'd be currently the case (using goal IDs I mean).

sloretz · 2018-11-01T23:12:29Z

rosidl_parser/rosidl_parser/__init__.py

+        action_name + ACTION_RESULT_MESSAGE_SUFFIX + SERVICE_REQUEST_MESSAGE_SUFFIX,
+        request_message_string)
+
+    implicit_output = ["int8 status"]


What's the status field for?

It's the goal status as defined in the proposal.

Whoops, I completely forgot about that

@jacobperron same question about rcl_actions using the sequence number vs goal id. If the latter then using action_msgs/GoalStatus here would get that.

Same as above.

sloretz · 2018-11-01T23:15:04Z

Besides a couple questions about the implicit fields, I think with a couple unit tests for parsing actions I think this could be merged, and then #310 could be re-targeted at master.

sservulo · 2018-11-06T00:04:43Z

Added tests to Action parser and included some fixes in implicit field collision logic. Note that the Action messages suffixes were changed in 3c4df0a to be compliant with message name regex.

hidmic · 2018-11-06T13:56:39Z

@sloretz @jacobperron I'd say we get this one in to unblock the rest. If the current set of implicit fields is later unfit (which I'm inclined to think but we may find out that it's not), we can change them.

jacobperron

Sorry for the late review.

I think the current set of implicit fields are okay, although we might consider using types defined in action_msgs so then any changes there are automatically reflected in the generated services/topics. I'm okay with merging this as is pending the one comment regarding the check for implicit field collisions.

jacobperron · 2018-11-06T18:10:47Z

rosidl_parser/rosidl_parser/__init__.py

+            # check for field name collision with action implicit parameters
+            # (e.g. 2 or more occurrences of a field_name contained in ACTION_IMPLICIT_FIELDS,
+            # including the implicit one)
+            if field_name in ACTION_IMPLICIT_FIELDS and action_fields[field_name] >= 1:


Does this mean that .msg or .srv specs containing an implicit action field will also raise a collision exception?
If so, maybe it would be better to pass in a list of invalid fields to the function instead of using ACTION_IMPLICIT_FIELDS directly.

@jacobperron It will trigger only if a field named with the same name as one implicit field is in the spec and there are at least two of them (replacing a ValueType error in this case), to make sure it triggers only when there's duplication, so if you have a field named uuid used in your msg definition it will not trigger. Should I go with the invalid fields route, then?

Thanks for the clarification. I think it is fine to leave it as-is.

jacobperron · 2018-11-06T18:18:26Z

rosidl_parser/test/test_parse_action_string.py

+
+import pytest
+
+# from rosidl_parser import InvalidServiceSpecification


nit: remove comment

jacobperron · 2018-11-06T19:37:56Z

CI:

Linux
Linux-aarch64
macOS
Windows

hidmic · 2018-11-07T12:37:41Z

@jacobperron @sservulo looks like this one's ready to go!

dirk-thomas · 2018-11-07T15:12:31Z

rosidl_parser/rosidl_parser/__init__.py

+
+
+def parse_action_string(pkg_name, action_name, action_string):
+    action_blocks = action_string.split(ACTION_REQUEST_RESPONSE_SEPARATOR)


This logic is incorrect. It e.g. even matches a --- in a comment. Please use the same logic as for splitting the request and response in a service.

You're right. I had not considered the possibility of a separator in comments. So it's either above's logic using slices or re.split('^---$', action_string, flags=re.MULTILINE).

Fixed using re w/ multiline. @dirk-thomas Is this approach OK for you or do you have a strong preference for the slice logic?

sservulo added 6 commits October 25, 2018 14:31

Updated rosidl_parser to support action files

631735a

Added action services message suffixes to rosidl_parser

e1486cb

Added support for action files in rosidl_generator and typesuppport

1583167

Updated rosidl to support action files

6688d60

Cleanup in rosidl_generator

4d00a7b

Updated implicit parameter types in action file parser

a7e63b5

sservulo requested review from hidmic, jacobperron and sloretz October 31, 2018 14:38

tfoote added the in progress Actively being worked on (Kanban column) label Oct 31, 2018

hidmic requested changes Oct 31, 2018

View reviewed changes

sloretz reviewed Oct 31, 2018

View reviewed changes

rosidl_parser/rosidl_parser/__init__.py Outdated Show resolved Hide resolved

rosidl_parser/rosidl_parser/__init__.py Outdated Show resolved Hide resolved

rosidl_parser/rosidl_parser/__init__.py Outdated Show resolved Hide resolved

sservulo added 2 commits November 1, 2018 17:39

Addressed review comments to rosidl_parser action support

e1118ae

Added check for duplicate field names in messages in rosidl_parser

3b7a695

sloretz reviewed Nov 1, 2018

View reviewed changes

rosidl_parser/rosidl_parser/__init__.py Outdated Show resolved Hide resolved

sloretz reviewed Nov 1, 2018

View reviewed changes

sloretz mentioned this pull request Nov 1, 2018

Generate messages and services from actions #310

Merged

sservulo added 2 commits November 5, 2018 20:22

Updated Action messages suffixes to comply with message name regex

3c4df0a

Added tests and fixed lint issues on rosidl action parser

2d429cc

hidmic approved these changes Nov 6, 2018

View reviewed changes

jacobperron reviewed Nov 6, 2018

View reviewed changes

Addressed review comments pt2

f78bb93

hidmic added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 7, 2018

Fixed action messages name pattern

c759f54

dirk-thomas reviewed Nov 7, 2018

View reviewed changes

Fix to action parser action block separator logic

d8a3efc

jacobperron approved these changes Nov 7, 2018

View reviewed changes

sservulo merged commit 82a6a5f into master Nov 7, 2018

tfoote removed the in review Waiting for review (Kanban column) label Nov 7, 2018

sservulo deleted the sservulo/305-parse_action_file branch November 8, 2018 18:31


		import pytest

		# from rosidl_parser import InvalidServiceSpecification



		def parse_action_string(pkg_name, action_name, action_string):
		action_blocks = action_string.split(ACTION_REQUEST_RESPONSE_SEPARATOR)

Conversation

sservulo commented Oct 31, 2018

Uh oh!

hidmic left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sservulo Nov 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sservulo commented Nov 1, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloretz commented Nov 1, 2018

Uh oh!

sservulo commented Nov 6, 2018

Uh oh!

hidmic commented Nov 6, 2018

Uh oh!

jacobperron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacobperron commented Nov 6, 2018

Uh oh!

hidmic commented Nov 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic Nov 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sservulo Nov 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hidmic left a comment •

edited

Loading

sservulo Nov 1, 2018 •

edited

Loading

hidmic commented Nov 7, 2018 •

edited

Loading

hidmic Nov 7, 2018 •

edited

Loading

sservulo Nov 7, 2018 •

edited

Loading