Exposing clock and adding timer features #124
Conversation
|
Hi @Chootin, did start the review today, but couldn't finish yet. |
|
@hoffmann-stefan No worries mate. Take your time, I'm in no great hurry. |
There was a problem hiding this comment.
This took some time, haven't finished everything yet, but wanted to post to avoid loosing the review comments. Haven't looked at the c code and the PInvoke definitions for example.
Could you rebase the branch on top of origin/main and force-push? I tested this myself, there is only one small merge conflict. It makes the history somewhat nicer to review as there are quite some merge commits.
I mostly concentrated on the public API for now using advice from the Framework Design Guidelines book and trying to be similar to rclcpp and rclpy.
Getting public API right the first time avoids having to break others when changing it later on.
Overall the implementation looks good 👍 But I am especially picky about public API and thread/memory-safty, so there are quite some comments ;)
I know this causes some work to address the review comments, so I would like to offer my support to implement them if it gets to much.
| } | ||
| } | ||
|
|
||
| public sealed class Clock |
There was a problem hiding this comment.
Framework Design Guidelines recommends not to use to generic names for types.
I'm wondering if we should use something like RclClock here.
There was a problem hiding this comment.
It's an interesting proposition, it does have the potential to eliminate confusion. I am finding I'm fully qualifying my usages of rcldotnet message types for this reason.
If we did make this change though, it would be a deviation from both rclcpp and rclpy. It would also be a deviation from other objects in rcldotnet like Client, Service, Subscription, etc.
There was a problem hiding this comment.
Yes, sometimes we have to choose if we follow .NET practices or if we are consistent with rclcpp and rclpy. There is no right answer in most of the cases, so it is quite subjective most of the times.
I don't have a strong opinion here, this was only a suggestion.
| } | ||
| } | ||
|
|
||
| public sealed class Timer |
There was a problem hiding this comment.
Framework Design Guidelines recommends not to use to generic names for types.
I'm wondering if we should use something like RclTimer here.
Chootin
force-pushed
the
tutina/upstream_pr1
branch
from
1c541a2
to
f1b4115
Compare
| } | ||
| } | ||
|
|
||
| public sealed class Clock |
There was a problem hiding this comment.
Yes, sometimes we have to choose if we follow .NET practices or if we are consistent with rclcpp and rclpy. There is no right answer in most of the cases, so it is quite subjective most of the times.
I don't have a strong opinion here, this was only a suggestion.
|
I downloaded your branch and tested your improved IDisposable implementation of the |
There was a problem hiding this comment.
Thanks @Chootin for the patience and the nice work 🎉
Merging this now as all review comments where addressed, the two open ones about naming classes are only suggestions, but given that there is already work done as a follow up let's keep it how it is now.
Looking forward to use this functionality in our codebase so that we can remove the workaround we had for timers 👍
This was the public API that was added/changed by this PR:
namespace ROS2
{
public enum ClockType
{
RosTime = 1,
SystemTime = 2,
SteadyTime = 3
}
public enum ClockChange
{
RosTimeNoChange = 1,
RosTimeActivated = 2,
RosTimeDeactivated = 3,
SystemTimeNoChange = 4
}
[StructLayout(LayoutKind.Sequential)]
public readonly struct TimeJump
{
public ClockChange ClockChange => throw null;
public TimeSpan Delta => throw null;
}
public delegate void PreJumpCallback();
public delegate void PostJumpCallback(TimeJump timeJump);
[StructLayout(LayoutKind.Sequential)]
public readonly struct JumpThreshold
{
public JumpThreshold(bool onClockChange, TimeSpan minForward, TimeSpan minBackward) {}
public bool OnClockChange => throw null;
public TimeSpan MinForward => throw null;
public TimeSpan MinBackward => throw null;
}
public class CallbackAlreadyRegisteredException : Exception
{
public CallbackAlreadyRegisteredException(string message) : base(message) {}
}
public sealed class Clock
{
public builtin_interfaces.msg.Time Now() => throw null;
public IDisposable CreateJumpCallback(JumpThreshold threshold, PreJumpCallback preJumpCallback, PostJumpCallback postJumpCallback) => throw null;
}
public partial class Node
{
public Clock Clock => throw null;
public IList<Timer> Timers => throw null;
public Timer CreateTimer(TimeSpan period, Action<TimeSpan> callback) => throw null;
}
public partial static class RCLdotnet
{
public static Clock CreateClock(ClockType type = ClockType.RosTime) => throw null;
}
public sealed class Timer
{
}
}
|
Cheers for your feedback @hoffmann-stefan - it has certainly made my work better! I'll get on to the next PR soon 👍. |
This PR exposes the clock to users of rcldotnet, removing the static clock reference and providing one on each node. This is important as later we will need a clock on each node for the purpose of implementing the use_sim_time parameter.
User facing duration objects have been substituded with TimeSpan in order to play nicely with other C# APIs.
rcldotnet_examples/RCLDotnetTalker.cs has been updated to demonstrate the use of the clock and timer features.
I also added RegisterNativeFunction to DllLoadUtils as I grew tired of making mistakes copy-pasting boilerplate code.