fix: Generate correct code for nested arrays of own type

[jmachowinski]

Part of https://github.com/ros2/ros2/issues/1445

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

[jmachowinski]

Ping

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

[ros-discourse]

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

[jmachowinski]

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. https://github.com/ros2/rcl_interfaces/commit/d9731c388ef6c55e5dc1d534af4e5575c9221f64

The goal here is to support complex parameters.

[emersonknapp]

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]

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]

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]

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 https://github.com/ros2/ros2/issues/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]

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]

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]

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]

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]

@emersonknapp any progress ?

[jmachowinski]

@emersonknapp ping ?

[ros-discourse]

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]

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

[clalancette]

I'm going to retarget this one to rolling, and then we can discuss whether we actually want to take this.