Implemented RCL global argument parsing.

[Chootin]

Also implemented methods for getting the name and namespace of nodes.

[Chootin]

I have, since this, done a lot more work on my fork:

• Made the clock available in .NET including setting the type
• Implemented parameter handling (this must be done for each language specific library as per https://github.com/ros2/rcl/blob/1260197bb53353ce91a129a47ba0271eea2033a2/rcl/include/rcl/node.h#L88)
• Implemented use_sim_time into Node.cs

All of this is currently merged into my dev branch at: https://github.com/Chootin/ros2_dotnet/tree/dev

[hoffmann-stefan]

@Chootin Thanks for your PR 👍

Currently I'm on vacation and getting things done at home, will look take a look at this when I'm back in office.

[Chootin]

@Chootin Thanks for your PR 👍

Currently I'm on vacation and getting things done at home, will look take a look at this when I'm back in office.

No worries mate, enjoy the vacation!

[hoffmann-stefan]

Thanks again for the PR, looks good overall 👍

Tested this as far on Ubuntu22.04 with humble locally and tried some remapping and validated it working with ros2 node/topic list

ros2 run rcldotnet_examples rcldotnet_talker --ros-args --remap __node:=my_talker --remap __ns:=/my_namespace --remap chatter:=my_chatter

[hoffmann-stefan]

I think we should use a get-only-computed property here instead of getter methods, but I can see arguments for using a method here as well. Have you considered properties instead?

  public string Name => NodeDelegates.native_rcl_get_name_handle(Handle);
  public string Namespace => NodeDelegates.native_rcl_get_namespace_handle(Handle);

[Chootin]

I agree. I think I will also include a FullyQualifiedNodeName in the same way as it is useful for the parameter handling code.

[hoffmann-stefan]

Returning a string directly from an PInvoke is a new pattern that isn't used anywhere yet in this code base.
After some research this seems problematic:

from https://www.mono-project.com/docs/advanced/pinvoke/

It’s easy to skim over memory management for most of Platform Invoke and marshaling, but for return values the CLI implements some default handling which must be considered.

The CLI runtime assumes that, under certain circumstances, the CLI runtime is responsible for freeing memory allocated by unmanaged code. Return values are one of those circumstances, causing the return value to be a memory boundary for control of memory (de)allocation.

The CLI assumes that all memory that is passed between the CLI/unmanaged code boundary is allocated via a common memory allocator. The developer does not get a choice in which memory allocator is used. For managed code, the Marshal.AllocCoTaskMem method can be used to allocate memory, Marshal.FreeCoTaskMem is used to free the memory allocated by Marshal.AllocCoTaskMem, and Marshal.ReAllocCoTaskMem is used to resize a memory region originally allocated by Marshal.AllocCoTaskMem.

Since classes are passed by reference, a pointer is returned, and the runtime assumes that it must free this memory to avoid a memory leak. The chain of events is thus:

1. Managed code invokes unmanaged function that returns a pointer to an unmanaged structure in unmanaged memory.
2. An instance of the appropriate managed class is instantiated, and the contents of the unmanaged memory is marshaled into the managed class.
3. The unmanaged memory is freed by the runtime “as if” by invoking Marshal.FreeCoTaskMem().

How is Marshal.AllocCoTaskMem, Marshal.ReAllocCoTaskMem, and Marshal.FreeCoTaskMem implemented? That’s platform-dependent. (So much for portable platform-dependent code.) Under Windows, the COM Task Memory allocator is used (via CoTaskMemAlloc(), CoTaskMemReAlloc(), and CoTaskMemFree()). Under Unix, the GLib memory functions g_malloc(), g_realloc(), and g_free() functions are used. Typically, these correspond to the ANSI C functions malloc(3), realloc(3), and free(3), but this is not necessarily the case as GLib can use different memory allocators; see g_mem_set_vtable() and g_mem_is_system_malloc() .

What do you do if you don’t want the runtime to free the memory? Don’t return a class. Instead, return an IntPtr (the moral equivalent of a C void* pointer), and then use the Marshal class methods to manipulate that pointer, such as Marshal.PtrToStructure, which works for both C# struct types and class types marked [StructLayout(LayoutKind.Sequential)].

My understanding of this is the following:
While this might work for linux as the runtime and the get_str_cpy use the same allocator, this will most certainly break on windows as then CoTaskMemFree() will get called on the pointer allocated by malloc().

So I would propose to use the method mentioned from the mono documentation and return an IntPtr instead and use Marshal.PtrToStringAnsi()
This method is used in the marshalling of the ROS messages as well.

Normally it should be fine to access the returned pointer directly after the call and pass it to Marshal.PtrToStringAnsi() as the livetime of the returned char* is documented to be valid as longe as the rcl_node_t is valid:

const char * native_rcl_get_name_handle(void *node_handle) {
  rcl_node_t *node = (rcl_node_t *)node_handle;

  // remove get_str_cpy
  return rcl_node_get_name(node);
}

public string GetName()
{
    IntPtr namePtr = NodeDelegates.native_rcl_get_name_handle(Handle);
    return Marshal.PtrToStringAnsi(namePtr);
}

But I think we should add additional SafeHandle.DangerousAddRef()/SafeHandle.DangerousRelease() around this to stay safe when some other thread does Dispose() the SafeNodeHandle (disposing nodes isn't implemented right now, but if we don't add this right now we will forget and only add it after a long memory management debugging session ;))

public string GetName()
{
    bool mustRelease = false;
    try
    {
        // This avoids accessing a invalid/freed pointer if some other thread disposes the SafeNodeHandle.
        Handle.DangerousAddRef(ref mustRelease);
        IntPtr namePtr = NodeDelegates.native_rcl_get_name_handle(Handle);
        return Marshal.PtrToStringAnsi(namePtr);
    }
    finally
    {
        if (mustRelease)
        {
            Handle.DangerousRelease();
        }
    }
}

But it's getting late here, so maybe I need to think about this again after some sleep :)

[Chootin]

Very insightful analysis. I had noticed the behaviour of freeing the memory from the managed side and assumed that simply returning a copy of the string was the best way to handle that at the time. I'll have to put some time into considering how this affects other code I have written after this initial pull request!

[Chootin]

I have implemented the requested changes, along with Node.FullyQualifiedName using some generics to cut down on duplicated code and provide an easy way to implement similar methods should they be required in the future.

[hoffmann-stefan]

Looks good, only one minor change left 👍

[hoffmann-stefan]

This method doesn't need to be generic:

Suggested change

	internal static string GetStringFromNativeDelegate<T>(RCLdotnetDelegates.NativeRCLGetStringType nativeDelegate, T safeHandle) where T : SafeHandle
	internal static string GetStringFromNativeDelegate(RCLdotnetDelegates.NativeRCLGetStringType nativeDelegate, SafeHandle safeHandle)

[Chootin]

Oh of course 👍

[Chootin]

Comment by @hoffmann-stefan addressed 👍

[hoffmann-stefan]

Thank you for your contribution 👍