RobotecAI / ros2cs

A C# (.Net) implementation of ros2 client library (rcl), enabling communication between ros2 ecosystem and C#/.Net applications such as Unity3D
Apache License 2.0
89 stars 22 forks source link

Lifecycle improvements #47

Open Deric-W opened 1 year ago

Deric-W commented 1 year ago

Hello, some time ago I proposed some changes in #38 which I have implemented.

Changes

The following changes have been made besides the ones mentioned in the issue.

Disposing node primitives

While Node still has methods for removing primitives like publishers they are marked as internal and are called by the Dispose methods of the primitives. Furthermore, all dispose implementations are not thread safe which has been documented and can be changed if required.

Ros2cs class

The Ros2cs class has been split into Context which handles node creation and ManualExecutor which handles spinning. Executors are a concept fund in other rcl client libraries and allow for multithreading and other spin implementations. To prevent threading issues a node can have only one executor which handles all of its primitives.

Interfaces

Most of the rcl wrappers are only exposed behind an interface to allow wrappers (like the one used for Unity) to implement those interfaces which would help RobotecAI/ros2-for-unity#51.

Removal of C structs

To prevent issues with definitions becoming out of date and the GC moving memory at unexpected times most rcl structs are hidden behind an IntPtr and only accessed via C wrappers.

Removal of disposal status tracking

Whether a wrapper has been disposed is being detected by calling the *_is_valid rcl functions or checking whether the pointer has been set to NULL. This should prevent the status tracking from becoming out of sync with the rcl.

Guard Conditions

Guard Conditions have been implemented to allow ManualExecutor to be woken when a node requests removal. They are internal for now since they seem to be useless without access to wait sets whcih are internal too to prevent users from putting primitives into one which combined with an active executor can lead to thread safety problems.

This PR is pretty big and can break some existing code, if it is desireable I can try to break it into smaller PRs. The changes have been tested on Windows 10 and Ubuntu (using the changes from master) with Ros2 Humble. I was unable to get ros2 run to find the example package, which is why the examples have to be tested by you until I discover my error.

Deric-W commented 1 year ago

I have managed to test the examples and they seem to be working.

pijaro commented 1 year ago

Looking great, let us review it. Thanks for the PR :smiley:

pijaro commented 1 year ago

@Deric-W I started the review, but as you noticed, it is quite a lot of code :smiley:. Great job :+1:

This PR is pretty big and can break some existing code, if it is desireable I can try to break it into smaller PRs.

I don't know if this is possible because many things are tightly connected, but if you can - smaller PRs could help a lot here.

Deric-W commented 1 year ago

I can try to create a separate PR for allocating the C structs in native memory, but I dont know how it will handle the possibility of primitives being disposed while being waited on (which seems possible on master). This PR handles this case by notifying the executor (which manages the wait set) that a change occured and waiting until the change was handled (by regenerating the wait set, but the wait already ends after it is safe to assume that the wait set will be regenerated before the next use). I dont know how master handles this problem.

adamdbrw commented 1 year ago

@Deric-W this is a great contribution and I am looking forward to reviewing it. My apologies for the lengthy process, It has been quite a busy period for me.

Deric-W commented 1 year ago

While working on updating R2FU I noticed that currently GuardCondition and WaitSet use IContext.OnShutdown to automatically dispose themself while it is documented that this happens after all callbacks have been invoked, making it difficult to do things like stopping a background thread using this event. A possible fix is to handle them like nodes, which should change nothing besides the time at which they are disposed by the context.