Support for Actions #49
Comments
|
As described in public at ROS World 2021, it was our team's intent to work on this. Before checking in any code, it would be great to understand if anyone else is working on this, like your team @hoffmann-stefan. |
|
@shschaefer We are currently not working on this library as we wait which of the open PRs (#87/#84 or #89/#90) get chosen and how they are modified by review. Doing something now would result into huge merge conflicts. (Unless we absolutly need some bugfix or little feature for an internal project) After that we would take another look which features we need, Actions are certainly up on the list. So if no one else would implement them we sooner or later would need and implement them. But If someone else want's to do them first we won't complain. Certanly I would like to do a code review while the PR adding support for actions gets discussed. But before starting implementing Actions I would like to make sure that the following headlines from #90 are implemented/discussed in the main branch, to avoid touching nearly every other line in the actions code again when doing those afterwards:
|
|
Thanks @hoffmann-stefan . The work on Actions was near complete last fall before our overlapping PRs were submitted. I am simply dusting the code off and completing it. I have looked to see what the impact of changing Esteve's existing pattern which I tried to follow to what you have done. Most are trivial with the exception of using SafeHandles. There is a pile of additional work on error handling that exists in my PR and follow on work I have done to my variant of Services and with the Actions work. I will see how to fold that in. I am loathe to remove some of the interfaces as you have done as it creates more difficulty in using mocking frameworks and other interface driven patterns which are common in the C# community. Though I agree with not exposing internal implementation details, not have a top level interface or wrapper for certain functions can be problematic. |
|
@shschaefer Ok, thanks for the information and your work on this. I already thought that the interface change can be controversial. I realy like the arguments made in the book Framework Design Guidlines (.NET specific) about this. Though I know many use mocking frameworks (which need interfaces) to test theire code. I have not used a mocking framework yet, somehow got away with it or didn't have code structured that way that would make it nececary. Im not completly opossed to having interfaces here, but after reading and understanding the chaptor on this my default is not havig interfaces for this kind of library code. Also I sometimes watch the .NET Api Review streams, some time ago there was an intressting discussion on interfaces in the base class library, see video link in this comment: dotnet/runtime#45593 (comment). Though I see we may not have the same constraints as the .NET base class library. Maybe we should discuss this with an concrete method signitures how we can remove implementation details in interfaces without removing them completly (as compared to the main branch) or how we could add interfaces back in (compared to #90). Like should Online snippet of the chapter for interfaces: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/abstractions-abstract-types-and-interfaces |
|
@hoffmann-stefan , your observation is spot on that we are quite far away from the constraints of the BCL. However, it is also true that this is not service code where the IoC pattern has proven its worth and driven many of the interface based test patterns. I often find that the interface pattern allows me to move quickly, then refactor as the abstractions become clear. I have completed a first pass of Action support in the ms-iot fork, including minimal introduction of SafeHandles, added basic ROS logging support, etc... There is a good bit of work to be done to improve error handling and native memory safety. There is cost and complexity with using SafeHandles that I am working to digest. I am hopeful that we can get the main branch unstuck. Much of refactoring necessary to move Action support is mechanical. Would prefer to just get it right. |
|
@shschaefer, I looked at your current published code in the
Thoughts on the API of ActionClientsI'm not sure about the So I did an initial try on defining an API surface and doing some early method Porting stuff one direction or the other
I'm not sure why you are trying to port my changes over to your code, while the What are the things you don't like about my PR that you do that much work to Did I miss something that is in your branch (#84) that is not in my branch?
You did copy a lot of code line by line I did write initially without preserving I don't think you did this on purpose or wanted to do something bad, but it may I think most of the additional features should be easier to port on top of my @esteve: Maybe you have some comments here? Planning how work should continue on thisAs we need actions in the near future I can do the offer to implement the But there are other facts I may be missing (see section above), so I wanted to @shschaefer, @ooeygui: Did you already continue on not published branches? Have API ProposalGenerated Action Message TypesBut now back to my proposed API for actions: To support actions we need to generate additional interface types. This are the
These interfaces mimic the pattern used by the automatically generated messages. public interface IRosActionDefinition<TGoal, TResult, TFeedback>
where TGoal : IRosMessage, new()
where TResult : IRosMessage, new()
where TFeedback : IRosMessage, new()
{
// must be implemented on deriving types, gets called via reflection
// (static abstract interface members are not supported yet.)
// public static abstract IntPtr __GetTypeSupport();
// These methods and the used interfaces are a workaround as existential types are not (yet) supported in .NET/C#
// See https://github.com/dotnet/csharplang/issues/5556 for the issue of adding existential types to C#.
// public static abstract IRosActionSendGoalRequest<TGoal> __CreateSendGoalRequest();
// public static abstract SafeHandle __CreateSendGoalRequestHandle();
// public static abstract IRosActionSendGoalResponse __CreateSendGoalResponse();
// public static abstract SafeHandle __CreateSendGoalResponseHandle();
// public static abstract IRosActionGetResultRequest __CreateGetResultRequest();
// public static abstract SafeHandle __CreateGetResultRequestHandle();
// public static abstract IRosActionGetResultResponse<TResult> __CreateGetResultResponse();
// public static abstract SafeHandle __CreateGetResultResponseHandle();
// public static abstract IRosActionFeedbackMessage<TFeedback> __CreateFeedbackMessage();
// public static abstract SafeHandle __CreateFeedbackMessageHandle();
}
public interface IRosActionSendGoalRequest<TGoal> : IRosMessage
where TGoal : IRosMessage, new()
{
unique_identifier_msgs.msg.UUID Goal_id { get; set; } // Dependent on decision in https://github.com/ros2-dotnet/ros2_dotnet/issues/91.
TGoal Goal { get; set; }
}
public interface IRosActionSendGoalResponse : IRosMessage
{
bool Accepted { get; set; }
builtin_interfaces.msg.Time Stamp { get; set; }
}
public interface IRosActionGetResultRequest : IRosMessage
{
unique_identifier_msgs.msg.UUID Goal_id { get; set; }
}
public interface IRosActionGetResultResponse<TResult> : IRosMessage
where TResult : IRosMessage, new()
{
sbyte Status { get; set; }
TResult Result { get; set; }
}
public interface IRosActionFeedbackMessage<TFeedback> : IRosMessage
where TFeedback : IRosMessage, new()
{
unique_identifier_msgs.msg.UUID Goal_id { get; set; }
TFeedback Feedback { get; set; }
}Strictly speaking the interfaces may not even be needed, but they should make this nicer. public interface IRosActionDefinition<TGoal, TResult, TFeedback>
where TGoal : IRosMessage, new()
where TResult : IRosMessage, new()
where TFeedback : IRosMessage, new()
{
// Not part of the proposal, but an alternative considered.
// public static abstract IRosMessage CreateSendGoalRequest(UUID goal_id, TGoal goal);
// public static abstract void DeconstructSendGoalResponse(IRosMessage response, out bool accepted, out Time stamp);
// ...
}This are for example the generated classes for the public class Fibonacci_Goal : IRosMessage { /* ... */ }
public class Fibonacci_Result : IRosMessage { /* ... */ }
public class Fibonacci_Feedback : IRosMessage { /* ... */ }
public class Fibonacci_SendGoal_Request : IRosMessage, IRosActionSendGoalRequest<Fibonacci_Goal> { /* ... */ }
public class Fibonacci_SendGoal_Response : IRosMessage, IRosActionSendGoalResponse { /* ... */ }
public sealed class Fibonacci_SendGoal : IRosServiceDefinition<Fibonacci_SendGoal_Request, Fibonacci_SendGoal_Response> { /* ... */ }
public class Fibonacci_GetResult_Request : IRosMessage, IRosActionGetResultRequest { /* ... */ }
public class Fibonacci_GetResult_Response : IRosMessage, IRosActionGetResultResponse<Fibonacci_Result> { /* ... */ }
public sealed class Fibonacci_GetResult : IRosServiceDefinition<Fibonacci_GetResult_Request, Fibonacci_GetResult_Response> { /* ... */ }
public class Fibonacci_FeedbackMessage : IRosMessage, IRosActionFeedbackMessage<Fibonacci_Feedback> { /* ... */ }
public sealed class Fibonacci : IRosActionDefinition<Fibonacci_Goal, Fibonacci_Result, Fibonacci_Feedback> { /* ... */ }This is not part of the API but an internal implementation trick used already in internal static class ActionDefinitionStaticMemberCache<TAction, TGoal, TResult, TFeedback>
where TAction : IRosActionDefinition<TGoal, TResult, TFeedback>
where TGoal : IRosMessage, new()
where TResult : IRosMessage, new()
where TFeedback : IRosMessage, new()
{
static ActionDefinitionStaticMemberCache()
{
// Get pseudo static interface implementations via reflection.
}
// Call cached pseudo static interface implementation methods:
public static IntPtr GetTypeSupport() => throw null;
public static IRosActionSendGoalRequest<TGoal> CreateSendGoalRequest() => throw null;
public static SafeHandle CreateSendGoalRequestHandle() => throw null;
public static IRosActionSendGoalResponse CreateSendGoalResponse() => throw null;
public static SafeHandle CreateSendGoalResponseHandle() => throw null;
public static IRosActionGetResultRequest CreateGetResultRequest() => throw null;
public static SafeHandle CreateGetResultRequestHandle() => throw null;
public static IRosActionGetResultResponse<TResult> CreateGetResultResponse() => throw null;
public static SafeHandle CreateGetResultResponseHandle() => throw null;
public static IRosActionFeedbackMessage<TFeedback> CreateFeedbackMessage() => throw null;
public static SafeHandle CreateFeedbackMessageHandle() => throw null;
}Action ClientThis should be more or less a direct port of the python API here: https://github.dev/ros2/rclpy/blob/master/rclpy/rclpy/action/client.py API definitionpublic partial class Node
{
public ActionClient<TAction, TGoal, TResult, TFeedback> CreateActionClient<TAction, TGoal, TResult, TFeedback>(string actionName)
where TAction : IRosActionDefinition<TGoal, TResult, TFeedback>
where TGoal : IRosMessage, new()
where TResult : IRosMessage, new()
where TFeedback : IRosMessage, new()
{
IntPtr typesupport = ActionDefinitionStaticMemberCache<TAction, TGoal, TResult, TFeedback>.GetTypeSupport();
// ...
}
}
public enum ActionGoalStatus
{
// see definition here: https://github.com/ros2/rcl_interfaces/blob/master/action_msgs/msg/GoalStatus.msg
// <summary>
// Indicates status has not been properly set.
// </summary>
Unknown = 0,
// <summary>
// The goal has been accepted and is awaiting execution.
// </summary>
Accepted = 1,
// <summary>
// The goal is currently being executed by the action server.
// </summary>
Executing = 2,
// <summary>
// The client has requested that the goal be canceled and the action server has
// accepted the cancel request.
// </summary>
Canceling = 3,
// <summary>
// The goal was achieved successfully by the action server.
// </summary>
Succeeded = 4,
// <summary>
// The goal was canceled after an external request from an action client.
// </summary>
Canceled = 5,
// <summary>
// The goal was terminated by the action server without an external request.
// </summary>
Aborted = 6,
}
public abstract class ActionClientGoalHandle
{
// Only allow internal subclasses.
internal ActionClientGoalHandle()
{
}
public abstract Guid GoalId { get; }
public abstract bool Accepted { get; }
public abstract Time Stamp { get; }
public abstract ActionGoalStatus Status { get; }
}
public sealed class ActionClientGoalHandle<TAction, TGoal, TResult, TFeedback> : ActionClientGoalHandle
where TAction : IRosActionDefinition<TGoal, TResult, TFeedback>
where TGoal : IRosMessage, new()
where TResult : IRosMessage, new()
where TFeedback : IRosMessage, new()
{
// No public constructor.
internal ActionClientGoalHandle()
{
// ...
}
public override Guid GoalId { get; }
public override bool Accepted { get; }
public override Time Stamp { get; }
public override ActionGoalStatus Status { get; }
public async Task CancelGoalAsync() => throw null;
public async Task<TResult> GetResultAsync() => throw null;
}
public abstract class ActionClient
{
// Only allow internal subclasses.
internal ActionClient()
{
}
}
public sealed class ActionClient<TAction, TGoal, TResult, TFeedback> : ActionClient
where TAction : IRosActionDefinition<TGoal, TResult, TFeedback>
where TGoal : IRosMessage, new()
where TResult : IRosMessage, new()
where TFeedback : IRosMessage, new()
{
// No public constructor.
internal ActionClient(SafeActionClientHandle handle, Node node)
{
// ...
}
public bool ServerIsReady() => throw null;
// Add an optional CancellationToken parameter later on (useful in the service `Client.SendRequestAsync()` and all other public async methods as well)
// But this may be after initial implementation.
public Task<ActionClientGoalHandle<TAction, TGoal, TResult, TFeedback>> SendGoalAsync(TGoal goal) => throw null;
public Task<ActionClientGoalHandle<TAction, TGoal, TResult, TFeedback>> SendGoalAsync(TGoal goal, Action<TFeedback> feedbackCallback) => throw null;
}Usage Examplevar node = RCLDotnet.CreateNode("test_node");
var actionClient = node.CreateActionClient<Fibonacci, Fibonacci_Goal, Fibonacci_Result, Fibonacci_Feedback>("fibonacci_action");
var goalHandle = await actionClient.SendGoalAsync(
new Fibonacci_Goal() { Order = 10 },
(Fibonacci_Feedback feedback) => Console.WriteLine( "Feedback: " + string.Join(", ", feedback.Sequence)));
Fibonacci_Result actionResult = await goalHandle.GetResultAsync();
Console.WriteLine("Result: " + string.Join(", ", actionResult.Sequence));
// or
await goalHandle.CancelGoalAsync();Note: For this example to work there needs to be an Action ServiceLeft open for now, but should use the same overall handling of generic type arguments as client. |
|
Thanks @hoffmann-stefan , it is good to hear that your team needs to use this work. I have expressed since the first post here, the desire to avoid a repeat of PR#84-90 which I feel we are on course to do again. If you feel that my current work in a secondary staging branch that I indicated would include your code to resolve mechanical differences and ease upstream merging needs direct attribution to you, I am fine with doing so. As you requested, I am happy to step back and figure out a better way. It looks like the CI issue may be resolved soon and we can merge PR #90 then iterate, branch, etc... to better collaborate now that you may have time to also work on this. I have continued the implementation with the goal of getting it to a production-ready state. There are simply too many unhandled exception paths and other issues all the way down to use of the RCL libraries. I have resolved the generics issue similar to how you proposed hear to simply have Porting directionI have not questioned porting my code on top of PR #90. I would like to avoid another reimplementation of my work without attribution. I am directly offering to take your feedback and complete the work to upstream my contribution. Much of what you have proposed is inline with my working branch - post resolving the generics issue. As I noted in my branch, circular dependencies in compilation prevented the use of UUID and other base interfaces from within the rcldotnet_common branch. There were trivial fixes to that available. My first pass left this intact without disturbing the project structure. API FeedbackIn regards to library form, I tried to follow the RCLCPP Action implementation pattern instead of the Python pattern. And then recently to begin to make it more idiomatic C#. I have made my opinion on that known in feedback to PR #91. Early on I did not find it desirable to expose a goal handle to ActionClient(s). The RCL APIs do not separate feedback messages, nor does the RCLCPP impl, for example. But it is a simple lookup to forward messages tagged with Goal ID. I wonder what porting an existing node that relies on this would look like? Later, I found it simplifying for handling status and other callbacks. Your API definition does not reflect the status callback, but it is trivially added as much of the rest matches what I have implemented modulo naming, SendGoalRequestAsync vs. SendGoalAsync. I also found no reason to create a shadow enum for goal status. It is generated as action_msgs.msg.GoalStatus and can be cleanly serialized into and out of its status array. With the server, I have implemented the core pattern of a node with callbacks as shown in the RCLCPP tutorials. One each for handling goal requests, goal execution and cancellation. There is an ActionServerGoalHandle class which wraps impl details within the server. It is exported to the implementor for use in the execution handler. General feedbackWhen it comes to async and threading, there is a lot more to interaction with the underlying wait set implementation. Creating an async spin is something that ROS2 has tried to avoid with its use of executors and refactoring towards more general synchronization structures like wait sets and timers. I ran into many of these issues when attempting to complete a working build. |
|
@shschaefer: This took some time to write a response, but I hope the length of
For now we should be fine here, I did not start implementing this. Discussing Though I have to be honest, depending on when this will be finished (or at least I can't demand that you finish this in time for us. And I hope you understand me Thats why I made the offer in my last comment that I could implement this In the case that I implement this for our internal products I would continue
If this stays a secondary staging branch I don't mind. But for me that was more
Here we have different styles/priorities of working on such things. You wanted Not really knowing how you work lead me into believing that this was more than a
I did sketch the API based on When I looked at your code I was mainly concerned which APIs/functionality did
Thanks, I hope I didn't give to much feedback below in this posts ;) It's a lot Technical stuffGeneric parameters
How would The minimal solution I have found for this is like Cyclic references
Concerning the issue with cyclic references I didn't look that much into the
public interface IRosActionSendGoalRequest<TGoal> : IRosMessage
where TGoal : IRosMessage, new()
{
unique_identifier_msgs.msg.UUID Goal_id { get; set; } // Dependent on decision in https://github.com/ros2-dotnet/ros2_dotnet/issues/91.
TGoal Goal { get; set; }
}
edit from 2022-06-24: This proposed reflection cache does not work, see comment: #49 (comment) public interface IRosActionSendGoalRequest<TGoal> : IRosMessage
where TGoal : IRosMessage, new()
{
// NOTE why this can not be added and maybe provide short alternatives (Provide other types for UUID and Time, use newer language features...)
// NOTE to use RosActionSendGoalRequestReflectionCache instead
// unique_identifier_msgs.msg.UUID Goal_id { get; set; } // Dependent on decision in https://github.com/ros2-dotnet/ros2_dotnet/issues/91.
TGoal Goal { get; set; }
}
internal static class RosActionSendGoalRequestReflectionCache<TGoal>
where TGoal : IRosMessage, new()
{
static RosActionSendGoalRequestReflectionCache()
{
// Get Getter/Setter-Methods for properties not referenceable by the Interface via reflection and cache them.
}
// Call cached getter/Setter methods:
public static UUID GetGoalId(IRosActionSendGoalRequest<TGoal> sendGoalRequest) => throw null;
public static void SetGoalId(IRosActionSendGoalRequest<TGoal> sendGoalRequest, UUID value) => throw null;
}
Don't expose implementation details in the API
There is not only a naming difference between your
The user shouldn't know that there is an Python and C++ RCL APIs
I don't see that much difference between
Skimming over the the other action client and client goal handle classes doesn't
Idiomatic C# is way more than the naming conventions, #91 only talks about the Most of this is taking the Python and C++ method signatures and translating them Client goal handle and feedback callback
C++ has a goal handle as well ( std::shared_future<typename GoalHandle::SharedPtr>
async_send_goal(const Goal & goal, const SendGoalOptions & options = SendGoalOptions())This is not the job of the
I wonder if there exists a node that does handle feedback callbacks differently Status topic in the client
On the ROS2 design document this status topic is described as only for debugging
from http://design.ros2.org/articles/actions.html (may be out of date):
Status enum
I like enums better and would say this is the idiomatic way to expose this in Server API
Pretty much all I wrote about the API until now was without looking into the Async
Could you please describe this issues with async you have in more detail? At I'm not sure what you mean with ROS2 avoiding an async spin?
I would only provide the async versions for methods in rcldotnet for now, adding
To adopt this to idiomatic C# we would not only provide callbacks but also use // There needs to be another overload of the Service constructor that takes a async callback for this
public async Task<std.srv.Empty_Response> HandleSomeService(std.srv.Empty_Request request, CancellationToken cancellationToken)
{
Console.WriteLine("This gets executed in the thread that called `RCLDotnet.Spin(...)`");
// The cancellation token would "fire" if the executor does shut down for example on `CTRL-C`. So all running async callbacks would stop as soon as possible.
// some method that uses the builtin HttpClient https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.getasync?view=net-6.0
var httpResponse = await GetSomeHttpResponse(cancellationToken);
Console.WriteLine("This gets executed in the thread that called `RCLDotnet.Spin(...)` because of the SynchronizationContext " +
"(implicit thread local on the thread that starts the await).");
return new std.srv.Empty_Response();
}
|
|
@shschaefer: As written in the last post I did some investigation on how the action server APIs work and compared public abstract class ActionServerGoalHandle
{
// Only allow internal subclasses.
internal ActionServerGoalHandle()
{
}
public abstract Guid GoalId { get; }
public abstract bool IsActive { get; }
public abstract bool IsCanceling { get; }
public abstract bool IsExecuting { get; }
public abstract ActionGoalStatus Status { get; }
// In `rclpy` this calls the `executeCallback` after setting the state to executing.
// In `rclcpp` this does not call the `executeCallback` (as there is none) but sets the state for the explicit `AcceptAndDefer` `GoalResponse`.
public void Execute() => throw null;
internal abstract SafeActionServerGoalHandle Handle { get; }
}
public sealed class ActionServerGoalHandle<TAction, TGoal, TResult, TFeedback> : ActionServerGoalHandle
where TAction : IRosActionDefinition<TGoal, TResult, TFeedback>
where TGoal : IRosMessage, new()
where TResult : IRosMessage, new()
where TFeedback : IRosMessage, new()
{
// No public constructor.
internal ActionServerGoalHandle(
// ...
)
{
}
// `rclpy` uses the name `Request`, but the name from `rclcpp` `Goal` fits better.
public TGoal Goal { get; }
public override Guid GoalId => throw null;
public override bool IsActive => throw null;
public override bool IsCanceling=> throw null;
public override bool IsExecuting => throw null;
public override ActionGoalStatus Status => throw null;
public void PublishFeedback(TFeedback feedback) => throw null;
// Decide wich (or both?) of these methods should be exposed.
// "rclpy style"
public void Succeed() => throw null;
public void Abort() => throw null;
public void Canceled() => throw null;
// "rclcpp style"
public void Succeed(TResult result) => throw null;
public void Abort(TResult result) => throw null;
public void Canceled(TResult result) => throw null;
}
public enum GoalResponse
{
Default = 0, // Invalid value
Reject = 1,
// `rclpy` doesn't provide the option to defer the the execution (unless you override the `acceptCallback`)
Accept = 2,
// Alternative from `rclcpp`
AcceptAndExecute = 2,
AcceptAndDefer = 3, // Gives the option to call `GoalHandle.Execute()` later in the `acceptedCallback`.
}
public enum CancelResponse
{
Default = 0, // Invalid value
Reject = 1,
Accept = 2,
}
public abstract class ActionServer
{
// Only allow internal subclasses.
internal ActionServer()
{
}
internal abstract SafeActionServerHandle Handle { get; }
}
public sealed class ActionServer<TAction, TGoal, TResult, TFeedback> : ActionServer
where TAction : IRosActionDefinition<TGoal, TResult, TFeedback>
where TGoal : IRosMessage, new()
where TResult : IRosMessage, new()
where TFeedback : IRosMessage, new()
{
// No public constructor.
internal ActionServer(
// ...
)
{
}
}The main difference would be in creating the rclpy style callbacksApipublic partial sealed class Node
{
private static void DefaultAcceptedCallback(ActionServerGoalHandle goalHandle)
{
// This calls the provided `executeCallback` in an executor task (in `rclpy`)
goalHandle.Execute();
}
private static GoalResponse DefaultGoalCallback<TGoal>(TGoal goal)
{
return GoalResponse.Accept;
}
private static CancelResponse DefaultCancelCallback(ActionServerGoalHandle goalHandle)
{
return CancelResponse.Reject;
}
public ActionServer<TAction, TGoal, TResult, TFeedback> CreateActionServer<TAction, TGoal, TResult, TFeedback>(
string actionName,
Func<ActionServerGoalHandle<TAction, TGoal, TResult, TFeedback>, Task<TResult>> executeCallback,
Func<TGoal, GoalResponse> goalCallback = null,
Action<ActionServerGoalHandle<TAction, TGoal, TResult, TFeedback>> acceptedCallback = null,
Func<ActionServerGoalHandle<TAction, TGoal, TResult, TFeedback>, CancelResponse> cancelCallback = null
)
where TAction : IRosActionDefinition<TGoal, TResult, TFeedback>
where TGoal : IRosMessage, new()
where TResult : IRosMessage, new()
where TFeedback : IRosMessage, new()
{
if (goalCallback == null)
goalCallback = DefaultGoalCallback;
if (acceptedCallback == null)
acceptedCallback = DefaultAcceptedCallback;
if (cancelCallback == null)
cancelCallback = DefaultCancelCallback;
// ...
}
}Usageprivate void Main()
{
// ...
var actionServer = node.CreateActionServer<Fibonacci, Fibonacci_Goal, Fibonacci_Result, Fibonacci_Feedback>("fibonacci_action", ExecuteCallbackAsync);
}
private async Task<Fibonacci_Result> ExecuteCallbackAsync(ActionServerGoalHandle<Fibonacci, Fibonacci_Goal, Fibonacci_Result, Fibonacci_Feedback> goalHandle)
{
var feedbackMsg = new Fibonacci_Feedback();
feedbackMsg.Sequence = new List<int> { 0, 1 };
for (int i = 1; i < goalHandle.Goal.Order; i++)
{
if (goalHandle.IsCancelRequested)
{
var result = new Fibonacci_Result();
result.Sequence = feedbackMsg.Sequence;
// NOTE: canceled call and returning the result are separate.
goalHandle.Canceled();
return result;
}
feedbackMsg.Sequence.Add(feedbackMsg.Sequence[i] + feedbackMsg.Sequence[i - 1]);
goalHandle.PublishFeedback(feedbackMsg);
await Task.Delay(1000);
}
var result = new Fibonacci_Result();
result.Sequence = feedbackMsg.Sequence;
// NOTE: succeed call and returning the result are separate.
goalHandle.Succeed();
return result;
}rclcpp style callbacksMirrors Apipublic partial sealed class Node
{
private static GoalResponse DefaultGoalCallback<TGoal>(Guid goalId, TGoal goal)
{
return GoalResponse.AcceptAndExecute;
}
private static CancelResponse DefaultCancelCallback(ActionServerGoalHandle goalHandle)
{
return CancelResponse.Reject;
}
public ActionServer<TAction, TGoal, TResult, TFeedback> CreateActionServer<TAction, TGoal, TResult, TFeedback>(
string actionName,
// Make this an async callback? (`Func<GoalHandle, Task>`)
// Make this an async callback which returns the `TResult` (`Func<GoalHandle, Task<TResult>>`)? In that case this would be more or less an `executeCallback` from `rclpy`.
Action<ActionServerGoalHandle<TAction, TGoal, TResult, TFeedback>> acceptedCallback
// This has an additional `goalId` parameter compared to `rclpy`.
Func<Guid, TGoal, GoalResponse> goalCallback = null,
Func<ActionServerGoalHandle<TAction, TGoal, TResult, TFeedback>, CancelResponse> cancelCallback = null
)
where TAction : IRosActionDefinition<TGoal, TResult, TFeedback>
where TGoal : IRosMessage, new()
where TResult : IRosMessage, new()
where TFeedback : IRosMessage, new()
{
if (goalCallback == null)
goalCallback = DefaultGoalCallback;
if (cancelCallback == null)
cancelCallback = DefaultCancelCallback;
// ...
}
}Usageprivate void Main()
{
// ...
var actionServer = node.CreateActionServer<Fibonacci, Fibonacci_Goal, Fibonacci_Result, Fibonacci_Feedback>("fibonacci_action", AcceptCallback);
// This is the reason why I would like to provide an async `acceptCallback`
// version (later on) so that the user of the API doesn't need to do the
// dance to convert to async methods without getting this wrong. It also
// enables `rcldotnet` to handle exceptions in an consistent way with the
// other callbacks (which would need to be defined in some other
// discussion, log exception vs. shutdown the executor vs. make this
// configurable)
}
private void AcceptCallback(ActionServerGoalHandle<Fibonacci, Fibonacci_Goal, Fibonacci_Result, Fibonacci_Feedback> goalHandle)
{
var task = AcceptCallbackAsync(goalHandle);
_ = task; // can't be awaited here, waiting synchronously would cause a deadlock here. (In case of an "SingleThreadedExecutor")
}
private async Task AcceptCallbackAsync(ActionServerGoalHandle<Fibonacci, Fibonacci_Goal, Fibonacci_Result, Fibonacci_Feedback> goalHandle)
{
try
{
// User needs to handle logging exceptions or shuting down the node explicitly.
await AcceptCallbackAsyncInner(goalHandle)
}
catch (Exception ex)
{
// log this or shutdown the node
}
}
private async Task AcceptCallbackAsyncInner(ActionServerGoalHandle<Fibonacci, Fibonacci_Goal, Fibonacci_Result, Fibonacci_Feedback> goalHandle)
{
var feedbackMsg = new Fibonacci_Feedback();
feedbackMsg.Sequence = new List<int> { 0, 1 };
for (int i = 1; i < goalHandle.Goal.Order; i++)
{
if (goalHandle.IsCancelRequested)
{
var result = new Fibonacci_Result();
result.Sequence = feedbackMsg.Sequence;
// NOTE: canceled call and returning the result are combined.
goalHandle.Canceled(result);
}
feedbackMsg.Sequence.Add(feedbackMsg.Sequence[i] + feedbackMsg.Sequence[i - 1]);
goalHandle.PublishFeedback(feedbackMsg);
await Task.Delay(1000);
}
var result = new Fibonacci_Result();
result.Sequence = feedbackMsg.Sequence;
// NOTE: succeed call and returning the result are combined.
goalHandle.Succeed(result);
}Navigation 2
|
|
Hi @hoffmann-stefan , as I indicated earlier, I started bottoms up instead of tops down to understand the complexity of the action implementation as opposed to services and topics which are quite straightforward and less intertwined with the asynchronous behaviors of the underlying libraries. I started with the cpp libraries to keep the problem manageable before defining a desirable API surface. Through we are not far off when it comes to the API design, my prototyping revealed many things which you are discovering and assures me of what is possible. Since we are not commenting on my PR, but instead re-hashing the API defined there, I will begin with the most cornerstone API of the With enumerated values, my preference for idiomatic in structure but not naming shows itself. Trying to rename and redefine constants that are defined in header files like ACCEPT_AND_EXECUTE adds little value. Looking at a log coming from code invoked using this library, I would hope that the same constants are used in code. The addition of default or sentinel values, though semantically complete, are not valid. They will require effort to keep consistent in behavior and debugging. If we were to do it, I would prefer to see a name like INVALID_RESPONSE = 0 with It appears that you are starting to see how the use of different executors makes the asynchronous problem more complicated than just enabling async methods. We will need to be very explicit about how one would spin asynchronously with a single threaded executor and a seemingly benign wait. I do want to implement If you look at my example code in RCLDotNetActionServer.cs, it implements the exact code in your usage example except that it makes the goal and cancel callbacks explicit. The remainder are different patterns for handling errors and exceptions which should be standardized to clean up the code examples. I am partial to making the core API an idiomatic C# wrapper of the C++ action pattern. Additions to be more inline with the Python style seem either out of place or better served in layered helper classes. Additions to make the code more readable or simple to implement seem righteous, but having many overloads of the same functions to look like multiple API sets does not feel right. Let's prefer that layered approach. |
Sorry about that, this was a copy-paste error while translating this from the python code here (did copy the goal callback to the cancel one without noticing the different return value). I changed this in my post to avoid getting this wrong again later on. |
|
Some changes I found while implementing this (only
|
No description provided.
The text was updated successfully, but these errors were encountered: