Add support for collections and other array/collections improvements

[hoffmann-stefan]

This PR was developed without knowing that @ooeygui / ms-iot (pr #87) did similar things. As this is our (as in Schiller Automatisierungstechnik GmbH) first open source contribution, there was some time between writing the code and now publishing and discussing this in the open ;)

Features added

Add support for arrays/collections of strings

test_msgs.msg.Arrays msg = new test_msgs.msg.Arrays();
msg.String_values[0] = "one";
msg.String_values[1] = "two";
msg.String_values[2] = "three";

Add support for collections (unbounded and bounded)

test_msgs.msg.UnboundedSequences msg = new test_msgs.msg.UnboundedSequences();
msg.Int32_values.Add(24);
msg.Int32_values.Add(25);
msg.Int32_values.Add(26);

test_msgs.msg.BoundedSequences msg = new test_msgs.msg.BoundedSequences();
msg.Int32_values.Add(24);
msg.Int32_values.Add(25);
msg.Int32_values.Add(26);

Add size constants for arrays and bounded sequences

test_msgs.msg.Arrays msg = new test_msgs.msg.Arrays();
msg.Byte_values = CalcualteExampleArrayWithLength(test_msgs.msg.Arrays.Byte_values_Length)

List<float> floats = GetFloats();
if (floats.Count <= test_msgs.msg.BoundedSequences.Float32_values_MaxCount)
{
    msg.Float32_values = floats;
}
else
{
    // handle error or slice collection...
}

Change mapped class for arrays

Ros arrays are better described with System.Array than with List<T>. For example, adding to an array doesn’t make sense as it would violate the length check on publish.

Initialize arrays with default values

In rclpy an rclcpp the default is to initialize all members, including arrays.

Add bounds checks for arrays and bounded sequences

Arrays of wrong size and bounded sequences that exceed the maximum length will throw an exception on publishing the message.

Added Tests for all new features

Nothing more to say ;)

Follow-up adding support for Services

I did continue with implementing services as well. Opend a draft PR #90 which is based on this one.

It will include a fix to get_error_string as well, which is the only feature that #87 has added which isn't included in here as far as I reviewed it.

[ooeygui]

Perhaps we should jump on a call and ensure that we're not doing duplicate work? We have other projects in flight including services, actions, and nested objects.

[esteve]

@hoffmann-stefan thank you so much for your contribution, I'll review this shortly.

[esteve]

Perhaps we should jump on a call and ensure that we're not doing duplicate work? We have other projects in flight including services, actions, and nested objects.

I'd prefer we stick to GitHub unless it's necessary, I like that the users of ros2-dotnet can see the direction of the project clearly and provide feedback openly. In any case, #90 looks like a more viable approach than #84, it's way more focused and reviewing it is definitely more feasible. So I'd rather merge #90 once it's ready. Moreover #84 duplicates work that is already done elsewhere (like the TF2 bindings done by @fmrico).

[shschaefer]

@esteve . I completely agree that we should keep this all in Github to ensure all feedback is public. @ooeygui was separating the PRs for you, to address your comment about duplicated work and the scope of the change. Now we have many overlapping dependencies. You have approved #87 which implements the same collection interfaces as this PR. There will be collision once the CI break is fixed if this is approved.

There is further work dependent upon integration of these contributions. It would be great to have some clear direction in terms of how you want us to resolve changes present in one set but not another, feedback on the current PRs as we delayed much refactoring and any other guidance you have for enhancing this important project.

[hoffmann-stefan]

We have other projects in flight including services, actions, and nested objects.

@ooeygui, @shschaefer: Do you have more changes in flight then the one published in #84?

Before starting to implement this PR and the folow-up one I looked at all public forks but didn't find yours at that time (mid of august). But maybe I missed it. After seeing your changes we stoped implementing new features.

From now on we should use public issues in this repo to coordinate which changes will be done and how they will be done.

I'd prefer we stick to GitHub unless it's necessary, I like that the users of ros2-dotnet can see the direction of the project clearly and provide feedback openly.

@esteve Would prefer this as well.

As we are planing to use this library in our products we will implement features as we go and need them. For now we can work with the features included in our branches.

Updating the description in #90 as well to include more information over there.

[shschaefer]

@hoffmann-stefan , thanks for your reply. As we did not see any activity in this repository, we went ahead and coalesced several different branches of work into one and exchanged emails with @esteve concerning our intent. At ROS World, we tried to make it clear to the ROS community that we would be investing in this project as it benefits all and enables many scenarios such as the MR showcase we demonstrated. @esteve has recently indicated that he did not like the combined PR, so we have started to break them apart.

I can appreciate your view of how our paths crossed. At the time of your PRs, there were already outstanding PRs on the same topic. That overlap is what I would like to handle as there are many other completed works behind it. We made many of the same changes as you have, just in different order. Actions are pending, logging support, better error handling and many other upgrades are next.

We have also stopped work on this project pending the resolution of these PRs as they will impact the nature of what follows. @ooeygui and I are excited that this project is receiving attention and more so that you will be building products and services based on it! We would like to support that in the most effective way while also being able to take similar dependencies in our products.

As you indicated, if there are additional plans that you have, let's open up issues for each in the repository and set out a publicly available roadmap showing the work and its responsible parties.

[esteve]

@hoffmann-stefan , thanks for your reply. As we did not see any activity in this repository, we went ahead and coalesced several different branches of work into one and exchanged emails with @esteve concerning our intent.

That's not how I remember it, and I'd dare to say not what happened. I've exchanged emails with @ooeygui for over a year, and as I understood, ms-iot's fork would get merged gradually into ros2-dotnet, but after a year, there was no action from ms-iot until just a few days before ROSWorld. I even added @ooeygui as one of the maintainers in the hopes that you'd start submitting changes from your fork.

At ROS World, we tried to make it clear to the ROS community that we would be investing in this project as it benefits all and enables many scenarios such as the MR showcase we demonstrated.

From the talk I gave at ROSCon in 2018, I already talked about ros2-dotnet supporting the Hololens. @fmrico showcased an rviz-like app running onboard the Hololens (https://github.com/IntelligentRoboticsLabs/ARViz), and recorded multiple videos (https://www.youtube.com/watch?v=lQXtoK3w5X8, https://www.youtube.com/watch?v=mGTKNB-Iog0, https://www.youtube.com/watch?v=QVhvxE6DuYM, https://www.youtube.com/watch?v=Av-UpGzqmOc)

@esteve has recently indicated that he did not like the combined PR, so we have started to break them apart.

Not that I don't like it, it's just that it's objectively not feasible to review a 3K+ diff. And trying to get that merged, with even changing the license, is not the best way to approach an opensource community.

[shschaefer]

@esteve I can understand your frustrations concerning the license. Had we known this was an issue, we would not have done so. @ooeygui specifically asked me about this before I made the change. It is not "an approach to the opensource community". Our code is offered freely. As it was done separately, it felt appropriate to use a copyright not in your name. We did not change the existing license. We simply added a new open license for the files we added. We are happy to change that to the Apache license if that is what you would prefer.

In regards to the support of the Hololens, our work is intended to update support for the latest Hololens 2 and to bring ROS2 forward to the new application model of HL2 as it is incompatible with the existing code from @fmrico and earlier efforts. We are not attempting to reduce the importance of your work. It will always stand as your recordings show. Our work for the Mixed Reality Toolkit is to build platform components that could be used in ARViz should you choose to update it. We are not building an AR application to compete with ARViz.

My question stands as to how you would like to merge the overlapping code. We will ensure that the license issue is resolved to your satisfaction, representing the work that has been done and will be in the future. One example solution is to add an attribution block to the README or another location to ensure that Stefan and other's contributions are noted.

Using the scope of change as a metric, PR #90 is over 2500 lines. The services code requires a large amount of boilerplate that accompanies the core of the work. No matter which PR is accepted, the scope is significant if we simply look at LOCs. The action support code will require some boilerplate for P/Invokes and other areas as well.

I can understand separating TF changes, collections, services, etc... Which is what @ooeygui was in process of doing at your request. Please let us all know how you would like to proceed. We will hold off any further changes until the repository converges.

We continue to support the principles of ROS2 for .Net and look forward to contributing to your project in the future.

[esteve]

We are happy to change that to the Apache license if that is what you would prefer.

I asked you about that, but the response I got was that Microsoft would only release as MIT. If you guys can change your contributions to have the same license as the rest of ros2-dotnet (which is the same that the core of ROS2 uses), that'd be very much appreciated, thanks. Maintaining code with multiple licenses is a nightmare, we had that experience in ROS1 and that's why we chose to use Apache and require contributions to use the same license.

Using the scope of change as a metric, PR #90 is over 2500 lines. The services code requires a large amount of boilerplate that accompanies the core of the work. No matter which PR is accepted, the scope is significant if we simply look at LOCs. The action support code will require some boilerplate for P/Invokes and other areas as well.

90 depends on #89, so it includes changes from both PRs so the actual diff is smaller, but in any case, both have a clear focus. Instead #84 includes many features, some of them unrelated from each other, so it's all or nothing, features can't be merged one by one.

I can understand separating TF changes, collections, services, etc... Which is what @ooeygui was in process of doing at your request. Please let us all know how you would like to proceed. We will hold off any further changes until the repository converges.

I understood that @ooeygui was working on the build issues (as per https://github.com/ros2-dotnet/ros2_dotnet/pull/87#issuecomment-961327901), I've approved #87 pending the build fixes, but I don't know what's the progress status on that.

Anyway, let's better continue this conversation in another ticket, this is taking away the focus from @hoffmann-stefan 's contribution.

[hoffmann-stefan]

Rebased on top of master. @esteve or @ooeygui: Could you please press the button to run CI?

[hoffmann-stefan]

@esteve, @ooeygui, @shschaefer: Since CI is now passing in this PR and in #90 how is the plan about reviewing these PRs? Just wanted to do a reminder/ping here.

[hoffmann-stefan]

@esteve thanks for merging!

One question: did you intentionally squash all commits into one when merging? All previous PRs seem to be merged with merge commits.