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: rolling
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 <[email protected]>
@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.

@clalancette
Copy link
Contributor

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

@clalancette clalancette changed the base branch from iron to rolling November 5, 2024 18:12
@ros-discourse
Copy link

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

https://discourse.ros.org/t/next-client-library-wg-meeting-friday-22nd-november-2024/40703/1

@MichaelOrlov
Copy link

MichaelOrlov commented Nov 19, 2024

@jmachowinski I am a bit lost with the target use case and whether it is supposed to be a support for the recursive type declaration or just forward declaration in the same message definition file.

On today's PMC meeting you mentioned that you want to use it to define a list of ROS 2 parameters in the same ParameterValue.msg.
I am curious if it is supposed to be like just a forward declaration in the same file? like:

struct Foo; // Forward declaration
typedef sequence<Foo> FooSeq;
FooSeq ArrayOfFoo;
struct Foo {
  long value;
};

or recursive declaration like

struct Foo; // Forward declaration
typedef sequence<Foo> FooSeq;
struct Foo {
  long value;
  FooSeq chain; // Recursive
};

Could you please clarify?

@jmachowinski
Copy link
Author

Note, for now we implemented our own parameter loading,
but want to move back to 'pure' ros2 parameters in the long run.

A typical configuration in our system looks like this :

  lightbarriers:
    - pose:
        frame: conveyor_0/lightbarrier_0
        orientation:
          w: 0.707
          x: 0
          y: 0
          z: -0.707
        parent: conveyor_0
        position:
          x: 2.50
          y: 0.9
          z: 0.0555
      reflector_distance: 1.6
      update_frequency: 20
    - pose:
        frame: conveyor_0/lightbarrier_1
        orientation:
          w: 0.707
          x: 0
          y: 0
          z: -0.707
        parent: conveyor_0
        position:
          x: 2.25
          y: 0.9
          z: 0.0555
      reflector_distance: 1.6
      update_frequency: 20

Therefore we are interested in having support for arrays of complex nested structures.
One way of reaching this goal would be to modify the ParameterType and ParameterValue messages in this way :

# These types correspond to the value that is set in the ParameterValue message.

# Default value, which implies this is not a valid parameter.
uint8 PARAMETER_NOT_SET=0

uint8 PARAMETER_BOOL=1
uint8 PARAMETER_INTEGER=2
uint8 PARAMETER_DOUBLE=3
uint8 PARAMETER_STRING=4
uint8 PARAMETER_BYTE_ARRAY=5

uint8 PARAMETER_ARRAY=10
uint8 PARAMETER_STRUCT
# Used to determine which of the next *_value fields are set.
# ParameterType.PARAMETER_NOT_SET indicates that the parameter was not set
# (if gotten) or is uninitialized.
# Values are enumerated in `ParameterType.msg`.

# The type of this parameter, which corresponds to the appropriate field below.
uint8 type

# "Variant" style storage of the parameter value. Only the value corresponding
# the type field will have valid information.

# Boolean value, can be either true or false.
bool bool_value

# Integer value ranging from -9,223,372,036,854,775,808 to
# 9,223,372,036,854,775,807.
int64 integer_value

# A double precision floating point value following IEEE 754.
float64 double_value

# A textual value with no practical length limit.
string string_value

# An array of bytes, used for non-textual information.
byte[] byte_array_value

# If type is PARAMETER_STRUCT, this is the name of the entry in the struct
string struct_entry_name

# If type is PARAMETER_STRUCT this is an array of struct members
# If type is PARAMETER_ARRAY this will be an array of parameters
ParameterValue[] value_array_value

@MichaelOrlov to answer your question, the current idea would be to use a recursive approach.

I would like to point out, that I am not settled on this approach. If anyone has other ideas on how to support parameters as shown above, I would love to hear them !

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.

5 participants