Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: iron
Choose a base branch
from

Conversation

jmachowinski
Copy link

@jmachowinski jmachowinski commented Jun 1, 2023

Part of ros2/ros2#1445

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

@jmachowinski
Copy link
Author

Ping

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

@ros-discourse
Copy link

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

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 474 to 475
if 'default_value' in field:
del field['default_value']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

namespace rosidl_typesupport_introspection_cpp
{

template<>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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....

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

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
Copy link
Collaborator

emersonknapp commented Jun 22, 2023

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
Copy link
Author

jmachowinski commented Jun 23, 2023

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
Copy link

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
Copy link
Collaborator

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
Copy link
Author

jmachowinski commented Jul 13, 2023

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

@emersonknapp any progress ?

@jmachowinski
Copy link
Author

@emersonknapp ping ?

@ros-discourse
Copy link

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
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants