From ba85bd10b982a201b05ede987313487101158b63 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Wed, 28 Sep 2022 14:52:54 -0700 Subject: [PATCH 1/3] Test action with same name as nested members I.e. try to generate code for Nested.action that contains members of type Nested.msg. This commit will not compile as-is, demonstrating a bug in the Java code generator. Signed-off-by: Jacob Perron --- test_rosidl_generator_java/CMakeLists.txt | 1 + test_rosidl_generator_java/action/Nested.action | 5 +++++ 2 files changed, 6 insertions(+) create mode 100644 test_rosidl_generator_java/action/Nested.action diff --git a/test_rosidl_generator_java/CMakeLists.txt b/test_rosidl_generator_java/CMakeLists.txt index 62f104ae..e45b7d75 100644 --- a/test_rosidl_generator_java/CMakeLists.txt +++ b/test_rosidl_generator_java/CMakeLists.txt @@ -20,6 +20,7 @@ if(BUILD_TESTING) ${test_interface_files_MSG_FILES} ${test_interface_files_SRV_FILES} action/BasicTypes.action + action/Nested.action SKIP_INSTALL ) diff --git a/test_rosidl_generator_java/action/Nested.action b/test_rosidl_generator_java/action/Nested.action new file mode 100644 index 00000000..813a0795 --- /dev/null +++ b/test_rosidl_generator_java/action/Nested.action @@ -0,0 +1,5 @@ +Nested nested_value +--- +Nested nested_value +--- +Nested nested_value From 51635712b47240a1815012e4e0cdf97a1ac3b855 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Wed, 28 Sep 2022 14:55:38 -0700 Subject: [PATCH 2/3] Fix code generation for nested types with same name If we have an action that has a nested message and both use the same basename, then the Java compiler cannot resolve the two different types. The solution is to fully qualify the action name wherever it is used. This change also splits the 'marker_interfaces' variable into two variables, 'imports' and 'implements', in an effort to simplify the code templates. Signed-off-by: Jacob Perron --- rosidl_generator_java/resource/action.java.em | 48 +++++++++++-------- rosidl_generator_java/resource/idl.java.em | 3 +- rosidl_generator_java/resource/msg.java.em | 5 +- rosidl_generator_java/resource/srv.java.em | 3 +- 4 files changed, 33 insertions(+), 26 deletions(-) diff --git a/rosidl_generator_java/resource/action.java.em b/rosidl_generator_java/resource/action.java.em index f5e33b2b..50d6cb91 100644 --- a/rosidl_generator_java/resource/action.java.em +++ b/rosidl_generator_java/resource/action.java.em @@ -10,6 +10,7 @@ from rosidl_cmake import expand_template namespaces = action.namespaced_type.namespaces type_name = action.namespaced_type.name +fully_qualified_type_name = '.'.join(namespaces + [type_name]) goal_type_name = action.goal.structure.namespaced_type.name result_type_name = action.result.structure.namespaced_type.name feedback_type_name = action.feedback.structure.namespaced_type.name @@ -25,7 +26,8 @@ data = { } data.update({ 'message': action.goal, - 'marker_interfaces': [f'org.ros2.rcljava.interfaces.GoalDefinition<{type_name}>'], + 'imports': ['org.ros2.rcljava.interfaces.GoalDefinition'], + 'implements': [f'GoalDefinition<{fully_qualified_type_name}>'], }) output_file = os.path.join(output_dir, *namespaces[1:], goal_type_name + '.java') expand_template( @@ -36,7 +38,8 @@ expand_template( data.update({ 'message': action.result, - 'marker_interfaces': [f'org.ros2.rcljava.interfaces.ResultDefinition<{type_name}>'], + 'imports': ['org.ros2.rcljava.interfaces.ResultDefinition'], + 'implements': [f'ResultDefinition<{fully_qualified_type_name}>'], }) output_file = os.path.join(output_dir, *namespaces[1:], result_type_name + '.java') expand_template( @@ -47,7 +50,8 @@ expand_template( data.update({ 'message': action.feedback, - 'marker_interfaces': [f'org.ros2.rcljava.interfaces.FeedbackDefinition<{type_name}>'], + 'imports': ['org.ros2.rcljava.interfaces.FeedbackDefinition'], + 'implements': [f'FeedbackDefinition<{fully_qualified_type_name}>'], }) output_file = os.path.join(output_dir, *namespaces[1:], feedback_type_name + '.java') expand_template( @@ -58,7 +62,8 @@ expand_template( data.update({ 'message': action.feedback_message, - 'marker_interfaces': [], + 'imports': [], + 'implements': [], }) output_file = os.path.join(output_dir, *namespaces[1:], feedback_message_type_name + '.java') expand_template( @@ -67,7 +72,8 @@ expand_template( output_file, template_basepath=template_basepath) -del data['marker_interfaces'] +del data['imports'] +del data['implements'] del data['message'] data.update({'service': action.send_goal_service}) output_file = os.path.join(output_dir, *namespaces[1:], send_goal_type_name + '.java') @@ -109,13 +115,13 @@ import @(action_import); public class @(type_name) implements ActionDefinition { - public static class SendGoalRequest extends @(type_name)_SendGoal_Request implements GoalRequestDefinition<@(type_name)> { + public static class SendGoalRequest extends @(type_name)_SendGoal_Request implements GoalRequestDefinition<@(fully_qualified_type_name)> { public List getGoalUuid() { return super.getGoalId().getUuidAsList(); } } - public static class SendGoalResponse extends @(type_name)_SendGoal_Response implements GoalResponseDefinition<@(type_name)> { + public static class SendGoalResponse extends @(type_name)_SendGoal_Response implements GoalResponseDefinition<@(fully_qualified_type_name)> { public void accept(boolean accepted) { super.setAccepted(accepted); } @@ -128,14 +134,14 @@ public class @(type_name) implements ActionDefinition { } } - public static class GetResultRequest extends @(type_name)_GetResult_Request implements ResultRequestDefinition<@(type_name)> { + public static class GetResultRequest extends @(type_name)_GetResult_Request implements ResultRequestDefinition<@(fully_qualified_type_name)> { public List getGoalUuid() { return super.getGoalId().getUuidAsList(); } } - public static class GetResultResponse extends @(type_name)_GetResult_Response implements ResultResponseDefinition<@(type_name)> { - public void setResult(ResultDefinition<@(type_name)> result) { + public static class GetResultResponse extends @(type_name)_GetResult_Response implements ResultResponseDefinition<@(fully_qualified_type_name)> { + public void setResult(ResultDefinition<@(fully_qualified_type_name)> result) { super.setResult((@(type_name)_Result)result); } public void setGoalStatus(byte status) { @@ -143,8 +149,8 @@ public class @(type_name) implements ActionDefinition { } } - public static class FeedbackMessage extends @(type_name)_FeedbackMessage implements FeedbackMessageDefinition<@(type_name)> { - public void setFeedback(FeedbackDefinition<@(type_name)> feedback) { + public static class FeedbackMessage extends @(type_name)_FeedbackMessage implements FeedbackMessageDefinition<@(fully_qualified_type_name)> { + public void setFeedback(FeedbackDefinition<@(fully_qualified_type_name)> feedback) { super.setFeedback((@(type_name)_Feedback) feedback); } public void setGoalUuid(List goalUuid) { @@ -152,39 +158,39 @@ public class @(type_name) implements ActionDefinition { } } - public Class> getSendGoalRequestType() { + public Class> getSendGoalRequestType() { return SendGoalRequest.class; } - public Class> getSendGoalResponseType() { + public Class> getSendGoalResponseType() { return SendGoalResponse.class; } - public Class> getGetResultRequestType() { + public Class> getGetResultRequestType() { return GetResultRequest.class; } - public Class> getGetResultResponseType() { + public Class> getGetResultResponseType() { return GetResultResponse.class; } - public Class> getResultType() { + public Class> getResultType() { return @(type_name)_Result.class; } - public Class> getFeedbackType() { + public Class> getFeedbackType() { return @(type_name)_Feedback.class; } - public Class> getFeedbackMessageType() { + public Class> getFeedbackMessageType() { return FeedbackMessage.class; } - private static final Logger logger = LoggerFactory.getLogger(@(type_name).class); + private static final Logger logger = LoggerFactory.getLogger(@(fully_qualified_type_name).class); static { try { - JNIUtils.loadTypesupport(@(type_name).class); + JNIUtils.loadTypesupport(@(fully_qualified_type_name).class); } catch (UnsatisfiedLinkError ule) { logger.error("Native code library failed to load.\n" + ule); System.exit(1); diff --git a/rosidl_generator_java/resource/idl.java.em b/rosidl_generator_java/resource/idl.java.em index 4714151d..c228a959 100644 --- a/rosidl_generator_java/resource/idl.java.em +++ b/rosidl_generator_java/resource/idl.java.em @@ -29,7 +29,8 @@ data = { 'interface_path': interface_path, 'output_dir': output_dir, 'template_basepath': template_basepath, - 'marker_interfaces': [], + 'imports': [], + 'implements': [], } for message in content.get_elements_of_type(Message): diff --git a/rosidl_generator_java/resource/msg.java.em b/rosidl_generator_java/resource/msg.java.em index f5b62f2e..1721581c 100644 --- a/rosidl_generator_java/resource/msg.java.em +++ b/rosidl_generator_java/resource/msg.java.em @@ -17,8 +17,7 @@ from rosidl_parser.definition import BoundedSequence from rosidl_parser.definition import NamespacedType type_name = message.structure.namespaced_type.name -interfaces_implemented = ['MessageDefinition'] -interfaces_implemented.extend(f"{t.rsplit('.', 1)[1]}" for t in marker_interfaces) +interfaces_implemented = ['MessageDefinition'] + implements interfaces_implemented = ', '.join(interfaces_implemented) message_imports = [ @@ -29,7 +28,7 @@ message_imports = [ 'org.slf4j.Logger', 'org.slf4j.LoggerFactory', ] -message_imports.extend(f"{t.split('<', 1)[0]}" for t in marker_interfaces) +message_imports.extend(imports) }@ @[for message_import in message_imports]@ import @(message_import); diff --git a/rosidl_generator_java/resource/srv.java.em b/rosidl_generator_java/resource/srv.java.em index a238ecfd..7a248e99 100644 --- a/rosidl_generator_java/resource/srv.java.em +++ b/rosidl_generator_java/resource/srv.java.em @@ -18,7 +18,8 @@ data = { 'interface_path': interface_path, 'output_dir': output_dir, 'template_basepath': template_basepath, - 'marker_interfaces': [], + 'imports': [], + 'implements': [], } data.update({'message': service.request_message}) output_file = os.path.join(output_dir, *namespaces[1:], request_type_name + '.java') From 0509f1c2985d0e8723323157566db3fa78669b24 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Wed, 28 Sep 2022 15:10:05 -0700 Subject: [PATCH 3/3] Add unit test for Nested.action Signed-off-by: Jacob Perron --- .../org/ros2/generator/InterfacesTest.java | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test_rosidl_generator_java/src/test/java/org/ros2/generator/InterfacesTest.java b/test_rosidl_generator_java/src/test/java/org/ros2/generator/InterfacesTest.java index 274ee604..f50f5d3a 100644 --- a/test_rosidl_generator_java/src/test/java/org/ros2/generator/InterfacesTest.java +++ b/test_rosidl_generator_java/src/test/java/org/ros2/generator/InterfacesTest.java @@ -1187,4 +1187,34 @@ public final void testBasicTypesAction() { assertEquals(expectedInt643, basicTypesResult.getInt64Value()); } + @Test + public final void testNestedAction() { + test_rosidl_generator_java.action.Nested_Goal nestedGoal = + new test_rosidl_generator_java.action.Nested_Goal(); + test_rosidl_generator_java.action.Nested_Feedback nestedFeedback = + new test_rosidl_generator_java.action.Nested_Feedback(); + test_rosidl_generator_java.action.Nested_Result nestedResult = + new test_rosidl_generator_java.action.Nested_Result(); + + // Set goal field + int expectedInt32Goal = 123; + nestedGoal.getNestedValue().getBasicTypesValue().setInt32Value(expectedInt32Goal); + + // Set feedback field + int expectedInt32Feedback = 231; + nestedFeedback.getNestedValue().getBasicTypesValue().setInt32Value(expectedInt32Feedback); + + // Set result field + int expectedInt32Result = 132; + nestedResult.getNestedValue().getBasicTypesValue().setInt32Value(expectedInt32Result); + + // Get fields + assertEquals( + expectedInt32Goal, nestedGoal.getNestedValue().getBasicTypesValue().getInt32Value()); + assertEquals( + expectedInt32Feedback, nestedFeedback.getNestedValue().getBasicTypesValue().getInt32Value()); + assertEquals( + expectedInt32Result, nestedResult.getNestedValue().getBasicTypesValue().getInt32Value()); + } + }