Add support for services and improve memory managment and error handling

[hoffmann-stefan]

Updated to include additional information

This PR adds services and clients to complete what we see as "minimum viable product" to start developing new products on top of it.

As our produts need to prove avaliability in automotive production environments I have paid attention to error handling and safe managment of native resources in the later commits of this PR.
No .NET developer wants to debug segmentation faults or simular issues in production.

Services

var service = node.CreateService<std_srvs.srv.SetBool, std_srvs.srv.SetBool_Request, std_srvs.srv.SetBool_Response>("test_dotnet_service_name", HandleRequest);

RCLdotnet.Spin(node);

// ...

private static void HandleRequest(std_srvs.srv.SetBool_Request request, std_srvs.srv.SetBool_Response response)
{
    Console.WriteLine($"Service got called with data {request.Data}");
    response.Success = true;
    response.Message = $"Got data {request.Data}";
}

Clients

var client = node.CreateClient<std_srvs.srv.SetBool, std_srvs.srv.SetBool_Request, std_srvs.srv.SetBool_Response>("test_dotnet_service_name");

// ...

var request = new std_srvs.srv.SetBool_Request();
request.Data = true;

// some other thread needs to spin the executer for this to work.
var response = await client.SendRequestAsync(request);
if (response.Success)
{
    // ...
}

Remove interfaces that expose internal implementation details

Most importent the IntPtr Handle property from ROS2.Interfaces.IDisposable
should not be public.
The interface ROS2.Interfaces.IDisposable was missleading as well, as there is System.IDisposable which most .NET developers think of if they see this interface name.

The INode, IPublisher and other interfaces dont't add any value over the Node class or the abstrct base classes without generic types in chase of Publisher and so on.
Nobody would be able to implement them anyway as they depend on internal implementation details.
Making these implementation details public in some way may be posible, but this would have to be carefully designed to still garantie internal consistency.

I try to follow the guidlines in the book Framework Design Guidlines wich lead to these changes. The book is from the team that implements the .NET standard library, so i think this is good advice. Part of an older edition of this book is availiable here: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/abstractions-abstract-types-and-interfaces

Made appropriate members internal as well and added some sealed and static modifiers.
This was done in preperation for adding SafeHandles to avoid memory leaks,
as this would change the implementation detail that was made public by the interfaces.

Use SafeHandles for memory managment

Now that the implementation details are hidden these commits use SaveHandles to manage the native recources. SafeHandles should be used instead of finalizers, see https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.safehandle?view=net-5.0#why-safehandle

In some places memory wouldn't get freed if exceptions are thrown before calling free.
In case of Node, Publisher, Subscription, Client and Service free was never called.

For messages there are still places where IntPtr is used, as it would complicate the generated code and didn't work well with arrays and collections. Code that uses message handles takes additional care to increase and decrease the reference counts of the SafeHandles.

As the nodes need to be freed after the publisher, subscibtions and so an there is an extra trick taken from the .NET crypto part of the standard library to garantie these constraints. See comments in the code for more detail.

Improve error handling

This adds error handling with exceptions to each pInvoke which returns an error code (if I didn't miss something).
In .NET exceptions should be prefered over return values, unless there are some good reasons to go otherwise.

This adds RCLExceptionHelper.CreateFromReturnValue(...) to create exceptions including the return code and the rcl_error_string.
RCLExceptionHelper.CheckReturnValue(...) can be used to avoid writing a condition if the compiler doesn't need the explicit throw for control flow analysis. This is usefull in precondition checks.

[esteve]

@hoffmann-stefan thanks! I prefer this PR much more than the one submitted by @ooeygui / Microsoft since it's more focused and doesn't change the license. Let me know when it's ready for review and I'll do it.

[hoffmann-stefan]

@esteve: I updted the PR description with more details. If youre done reviewing the other PR without major issues you can continue with this one. I don't plan on adding more code in this PR except to address something from a review.

[esteve]

@hoffmann-stefan sorry for the late response, I was hoping we could get #86 fixed soon, but then holiday season and covid struck. Overall, changes look good to me, but I'd like to get the CI fixed as soon as possible so we can properly test this PR and the other one you submitted.

Would your team be interested in helping with the CI issues? There seems to be a conflict with newer versions of ROS GitHub action and ros2-dotnet.

[hoffmann-stefan]

@hoffmann-stefan sorry for the late response, I was hoping we could get #86 fixed soon, but then holiday season and covid struck.

@esteve No worries, thanks for looking at the PRs. Seems like #86 got some progress by @ooeygui since you wrote this comment. In the short term we need to hold some deadlines so we don't realy have time to look into something new. Most of the team uses linux and the other ones (like me) use WSL2 so no one has a working Windows ROS setup or experience right now (seems like the windows side of the CI has problems as far as I looked). So I can't promise something right now.

[hoffmann-stefan]

Rebased on top of master.
@esteve or @ooeygui: Could you please press the button to run CI? Or do you first want to get #89 green?

[tank104]

I would really like to be able to use Services, do you think this PR will get in soon?

[hoffmann-stefan]

@tank104:

I pinged the maintainers back in may about the review of the PRs, but no word yet.

To use these features you don't need the PRs to be merged though, you can checkout the branch from the PR and use that one instead in your workspace. In doing so you could give feedback if this worked for you and maybe start a review of the PRs to help get them merged.

[esteve]

@hoffmann-stefan

I pinged the maintainers back in may about the review of the PRs, but no word yet.

I'm really sorry, I completely missed your message. I'll have a look at it and #89 now, but to be honest, I'll most likely just merge it, the changes look fine overall

[esteve]

Please don't change the NOTICE file, it's fine to add your copyright in the individual files, though.

[hoffmann-stefan]

Didn't know about the implications of changching this file.
Did some quick googling and found this if others wonder as well: https://infra.apache.org/licensing-howto.html

Removed the change and pushed again.

[esteve]

Changes look fine overall, only one change request.

@ooeygui I like this PR because it's self contained. Thanks for the work done in the other services PR. If you're ok with this PR, once the change request is addressed, we can merge it. Can you have a look at it? Thanks.

[hoffmann-stefan]

Rebased on top of master and adressed @esteve's change request

Before merging please notice the comment over at #89 (comment) to not loose the logical commits for this PR by an accidental squash merge (if there is a reason for squash merge I am fine with it too).

[hoffmann-stefan]

Seems like CI for Windows Desktop broke, but this happend after merging #89 into master as well: https://github.com/ros2-dotnet/ros2_dotnet/actions/runs/3436780831/jobs/5730712425
Back then #89 and this PR before rebase and the lite update to NOTICE did pass as well.
Seems there is something broken for Windows Desktop independend on the changes in this repository?

Would have to look into this some further, but would be glad if someone has some pointers for resolving this as I don't currently have a local Windows ROS setup to reproduce...

[hoffmann-stefan]

Found the issue with CI

• There was a break in ament_core that was caused by CMake 3.24.
  • Support new target export template introduced with CMake 3.24 ament/ament_cmake#395
• The github action windows runner images updated to newer CMake version, but the binaries used in CI didn't include the fix from the PR to ament_cmake above
• I created a PR to setup_ros that updates the windows binary releases to include the fix: Update to latest binary ros2 windows distributions ros-tooling/setup-ros#548

Once the PR to setup_ros is merged and tagged we can use this here. Will update and clean up this PR then, maybe even first creating a PR that fixes CI and then rebasing this PR.

[hoffmann-stefan]

rebased on top of #96 (which is now included) to check ci again.

[hoffmann-stefan]

@esteve:

Took some time, but now your change request is resolved and CI is fixed again, mostly due to find the time to debug windows CI.

Here are the changes since your last review (see the small notices about force pushes to this PR): https://github.com/ros2-dotnet/ros2_dotnet/compare/79a338d8c04e8240e651dd31b274bfa75aa8d15b..2068e8b0d2b055cfe4051b26d450508524ef940d

Created a separate PR for testing the CI fixes on master, this PR is now rebased on top of it: #96

Could you take a look again and approve/merge if it's OK? (maybe #96 or directly this one as it includes the one commit)

Before merging please notice the comment over at #89 (comment) to not loose the logical commits for this PR by an accidental squash merge (if there is a reason for squash merge I am fine with it too).