fix: Generate correct code for nested arrays of own type #748

jmachowinski · 2023-06-01T18:30:06Z

Part of ros2/ros2#1445

This fixes the code generation for the case that you have a message
A :
A[] arrayOfA

jmachowinski · 2023-06-13T16:12:33Z

Ping

Note, this patch is a pre requirement for a patchset for structured parameter support.

ros-discourse · 2023-06-20T07:35:09Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/use-ros-message-in-parameter-interface/31269/19

emersonknapp

Are there other PRs or issues that this is related to? Some cross-links between them would help understand the context for this change

emersonknapp · 2023-06-21T15:56:14Z

rosidl_generator_type_description/rosidl_generator_type_description/__init__.py

+            if 'default_value' in field:
+                del field['default_value']


This is not important, just a thought for a one-liner over two

Suggested change

if 'default_value' in field:

del field['default_value']

field.pop('defaut_value', None)

emersonknapp · 2023-06-21T16:08:53Z

rosidl_typesupport_introspection_cpp/resource/msg__type_support.cpp.em

+namespace rosidl_typesupport_introspection_cpp
+{
+
+template<>


This is just a forward decl for a function defined lower down, right? If you could you add a comment to that effect? Is it not already declared in the .hpp.em header, and if so could we just include that instead? Why is the forward decl needed?

If we do need the forward declaration here - then I want to point out this expression

'::'.join([package_name] + list(interface_path.parents[0].parts))

Which is repeated many times in this file, and I think could be factored out into a variable at the top level block for easier reading and refactoring. Maybe factor out

message_namespace = '::'.join([package_name] + list(interface_path.parents[0].parts)) message_namespaced_name = '::'.join([message_namespace, message-structure.namespaced_type.name])

And that would really simplify some of the expressions farther down, make them more readable

We are declaring that a special instance of the template function (for the given template argument) is present, and will be defined later. If we don't do this, we will get a compile error. (Usage of template function before instantiation)

I refactored the rest of the code as suggested.

emersonknapp · 2023-06-21T16:12:46Z

rosidl_generator_c/resource/msg__struct.h.em

+// forward declare type
+struct @(idl_structure_type_to_c_typename(message.structure.namespaced_type));
+// Struct for a sequence of @(idl_structure_type_to_c_typename(message.structure.namespaced_type)).
+typedef struct @(idl_structure_type_sequence_to_c_typename(message.structure.namespaced_type))


I think I understand that you are putting the Sequence struct before the single struct, so that the single struct could contain as members a Sequence of itself, correct?

If so, maybe add that to the comment for future readers. Could even put it in a template comment @# just so it's only for people modifying the template in the future

Added a comment

emersonknapp · 2023-06-21T16:14:10Z

rosidl_generator_c/resource/full__description.c.em

 @[for itype_description in all_type_descriptions]@
+@[  if itype_description['type_name'] not in added_itype_names]@


Did you hit a case where there was a multiple of a type in here? all_type_descriptions should be a unique set to begin with

Yes, I ran into this problem.

//test gen rcl_interfaces/msg/ParameterValue {'type_name': 'rcl_interfaces/msg/ParameterValue', 'fields': [{'name': 'type', 'type': {'type_id': 3, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'bool_value', 'type': {'type_id': 15, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'integer_value', 'type': {'type_id': 8, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'double_value', 'type': {'type_id': 11, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'string_value', 'type': {'type_id': 17, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'byte_array_value', 'type': {'type_id': 160, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'bool_array_value', 'type': {'type_id': 159, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'integer_array_value', 'type': {'type_id': 152, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'double_array_value', 'type': {'type_id': 155, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'string_array_value', 'type': {'type_id': 161, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'value_array_value', 'type': {'type_id': 145, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': 'rcl_interfaces/msg/ParameterValue'}, 'default_value': ''}]}

static char rcl_interfaces__msg__ParameterValue__TYPE_NAME[] = "rcl_interfaces/msg/ParameterValue";
//test gen rcl_interfaces/msg/ParameterValue {'type_name': 'rcl_interfaces/msg/ParameterValue', 'fields': [{'name': 'type', 'type': {'type_id': 3, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'bool_value', 'type': {'type_id': 15, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'integer_value', 'type': {'type_id': 8, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'double_value', 'type': {'type_id': 11, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'string_value', 'type': {'type_id': 17, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'byte_array_value', 'type': {'type_id': 160, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'bool_array_value', 'type': {'type_id': 159, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'integer_array_value', 'type': {'type_id': 152, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'double_array_value', 'type': {'type_id': 155, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'string_array_value', 'type': {'type_id': 161, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': ''}, 'default_value': ''}, {'name': 'value_array_value', 'type': {'type_id': 145, 'capacity': 0, 'string_capacity': 0, 'nested_type_name': 'rcl_interfaces/msg/ParameterValue'}, 'default_value': ''}]}

static char rcl_interfaces__msg__ParameterValue__TYPE_NAME[] = "rcl_interfaces/msg/ParameterValue";

For an unknown reason, this is not an unique set....

Hm, I guess toplevel_msg['type_description'] and toplevel_msg['referenced_type_descriptions'] are equal in this special case ?

Oh, yeah - because you have

# ParameterValue.msg ParameterValue[] value_array_value

Then toplevel_msg['referenced_type_descriptions'] contains as one of its members the toplevel_msg['type_description']. So, there can only be this one possible case where any type gets defined twice, and that's if it is self-referencing. I think this change is fine, given that. No need to add a special-case code when you can detect it generically like this.

jmachowinski · 2023-06-22T14:34:09Z

Are there other PRs or issues that this is related to? Some cross-links between them would help understand the context for this change

This commit would be the follow up.
ros2/rcl_interfaces@d9731c3

The goal here is to support complex parameters.

This fixes the code generation for the case that you have a message A : A[] arrayOfA Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

emersonknapp · 2023-06-22T21:44:25Z

I don't see any real problem in this particular diff - but I'm curious if you've found other problems with introducing recursive message types. I imagine that this has unintended consequences in other parts of the codebase. Also, is supporting cyclic message definitions a change in the inherent design of ROS 2? Have you logged an issue somewhere to track the development of this feature? Probably https://github.com/ros2/ros2/issues would be a good place to track such a feature change.

jmachowinski · 2023-06-23T09:27:12Z

I don't see any real problem in this particular diff - but I'm curious if you've found other problems with introducing recursive message types. I imagine that this has unintended consequences in other parts of the codebase.

Nothing until now, but I haven't really started the code building up on this yet, as it would be pointless if this merge request would not go through.

Also, is supporting cyclic message definitions a change in the inherent design of ROS 2?

Hm, interesting point, until now I just thought it was a weakness in the implementation, but not by design.

Have you logged an issue somewhere to track the development of this feature? Probably https://github.com/ros2/ros2/issues would be a good place to track such a feature change.

I still haven't found out the 'correct' way how to contribute to ros...
According to https://docs.ros.org/en/iron/The-ROS2-Project/Contributing.html
one should contact Open Robotics as early as possible, but sadly it is not stated how.
From the rest of documentation it looked like discourse is the way to go.
I will go save and add a discourse, and an issue and link them.
Edit: If you open an issue, there is a hint, that general discussion should go to discourse.

Thanks for the review by the way.

ros-discourse · 2023-06-23T09:39:00Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/cyclic-message-definitions/32075/1

emersonknapp · 2023-06-26T20:28:43Z

If you open an issue, there is a hint, that general discussion should go to discourse.

"General discussion" should go to discourse, yes. But a specific feature/design that is being implemented via PRs, is what issue trackers are for. Sorry I stated it as a "probably" - given that this feature would span multiple repositories, then the issue should definitely go to ros2/ros2 metarepo. I've opened that issue here ros2/ros2#1445. Maybe it turns out the limitation is by design, in which case we can close it, but for now that's going to be better than discourse for tracking work.

Also, just as a general practice - if something is labeled as "fix" such as this PR, then it makes it much easier to future historians/implementers if there is a detailed bug ticket that describes the problem, that the PR then fixes. Just a generalized process tip going forward, not saying you did anything wrong here.

jmachowinski · 2023-07-13T12:54:23Z

So what is the way forward here ? We got no response at all for two weeks, judging from this, I would say it is not intentional that nested arrays do not work.

emersonknapp · 2023-07-18T18:45:32Z

There was some offline conversation about this last week, but we're still uncertain as to the use case, whether this should be supported. It was not specifically considered in the design either way originally, but seems like it opens a big can of complexity worms, the downstream effects are really uncertain.

This really seems like the sort of thing where a structured design proposal considering all effects and alternative approaches, highlighting what is actually trying to be accomplished would be the most valuable approach. The discussion is a bit scattered at this point, and I'm not seeing how self-referential nested types is helping to accomplish a real need that other approaches can't accomplish just as well.

Sorry for the slow progress - but this is unfortunately not a straightforward topic due to the potential downstream ramifications.

jmachowinski · 2023-07-21T11:53:48Z

The discussion is a bit scattered at this point, and I'm not seeing how self-referential nested types is helping to accomplish a real need that other approaches can't accomplish just as well.

My goal here is to have complex structured parameters. As rcl_interfaces::msg::ParameterDescriptor, rcl_interfaces::msg::ParameterValue etc are part of the rclcpp interface API, I can't extend the parameters without breaking the API.

Therefore the nicest way forward would be to use nested messages, as we would have full backward compatibility.

What are the potential downstream problems that you would expect ? (Only from this commit, not from the change to the parameters later on)

emersonknapp · 2023-07-21T20:44:57Z

I think I'm going to need @clalancette @tfoote opinion on this. Is complex nested ParameterDescriptor something that ROS 2 should support? That downstream decision is what necessitates nested types as a general feature, and therefore whether this PR should move forward.

jmachowinski · 2023-08-04T09:06:05Z

@emersonknapp any progress ?

jmachowinski · 2023-08-21T13:01:46Z

@emersonknapp ping ?

ros-discourse · 2023-09-08T15:05:05Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/simplifying-how-to-declare-parameters-in-ros-2/33272/5

jmachowinski · 2024-01-29T08:28:32Z

@emersonknapp What is the status of this ?
If possible, I would like to see this merged for jazzy if we decide to go ahead with it.

jmachowinski requested review from adityapande-1995, methylDragon, quarkytale and sloretz as code owners June 1, 2023 18:30

jmachowinski force-pushed the iron branch 3 times, most recently from b345117 to 045c0a2 Compare June 2, 2023 09:09

emersonknapp reviewed Jun 21, 2023

View reviewed changes

fix: Generate correct code for nested arrays of own type

28d2450

This fixes the code generation for the case that you have a message A : A[] arrayOfA Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

jmachowinski force-pushed the iron branch from 045c0a2 to 28d2450 Compare June 22, 2023 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Generate correct code for nested arrays of own type #748

fix: Generate correct code for nested arrays of own type #748

jmachowinski commented Jun 1, 2023 •

edited by emersonknapp

Loading

jmachowinski commented Jun 13, 2023

ros-discourse commented Jun 20, 2023

emersonknapp left a comment

emersonknapp Jun 21, 2023

jmachowinski Jun 22, 2023

emersonknapp Jun 21, 2023

jmachowinski Jun 22, 2023

emersonknapp Jun 21, 2023

jmachowinski Jun 22, 2023

emersonknapp Jun 21, 2023

jmachowinski Jun 22, 2023

jmachowinski Jun 22, 2023

emersonknapp Jun 22, 2023

jmachowinski commented Jun 22, 2023

emersonknapp commented Jun 22, 2023 •

edited

Loading

jmachowinski commented Jun 23, 2023 •

edited

Loading

ros-discourse commented Jun 23, 2023

emersonknapp commented Jun 26, 2023

jmachowinski commented Jul 13, 2023 •

edited

Loading

emersonknapp commented Jul 18, 2023

jmachowinski commented Jul 21, 2023

emersonknapp commented Jul 21, 2023

jmachowinski commented Aug 4, 2023

jmachowinski commented Aug 21, 2023

ros-discourse commented Sep 8, 2023

jmachowinski commented Jan 29, 2024

	if 'default_value' in field:
	del field['default_value']
	field.pop('defaut_value', None)

		@[for itype_description in all_type_descriptions]@
		@[ if itype_description['type_name'] not in added_itype_names]@

fix: Generate correct code for nested arrays of own type #748

Are you sure you want to change the base?

fix: Generate correct code for nested arrays of own type #748

Conversation

jmachowinski commented Jun 1, 2023 • edited by emersonknapp Loading

jmachowinski commented Jun 13, 2023

ros-discourse commented Jun 20, 2023

emersonknapp 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

jmachowinski commented Jun 22, 2023

emersonknapp commented Jun 22, 2023 • edited Loading

jmachowinski commented Jun 23, 2023 • edited Loading

ros-discourse commented Jun 23, 2023

emersonknapp commented Jun 26, 2023

jmachowinski commented Jul 13, 2023 • edited Loading

emersonknapp commented Jul 18, 2023

jmachowinski commented Jul 21, 2023

emersonknapp commented Jul 21, 2023

jmachowinski commented Aug 4, 2023

jmachowinski commented Aug 21, 2023

ros-discourse commented Sep 8, 2023

jmachowinski commented Jan 29, 2024

jmachowinski commented Jun 1, 2023 •

edited by emersonknapp

Loading

emersonknapp commented Jun 22, 2023 •

edited

Loading

jmachowinski commented Jun 23, 2023 •

edited

Loading

jmachowinski commented Jul 13, 2023 •

edited

Loading