ros2_documentation icon indicating copy to clipboard operation
ros2_documentation copied to clipboard

Tutorial for writing a service with an asynchronous client node

Open santiago-tapia opened this issue 1 year ago • 6 comments

This PR is connected to another PR at example repository: Async Service Example.

This PR includes an intermediate tutorial to write an asynchronous client that sends a request inside a callback. The key feature about the example is that the node execution is not waiting in any where. It is just executing callbacks or spinning, all time.

Besides, it includes some experiments on executing the client in combination with a delayed server, those experiments are designed to help in the understanding of the concept of asynchronism.

santiago-tapia avatar Dec 14 '24 13:12 santiago-tapia

@kscottz, Thank very much for your comments.

I am going to try to solve the issues you mentioned. Just to confirm:

  • One sentence per line (without limit to line length?)
  • Replace ROS2 with ROS 2
  • Make an introduction on Asynchronism at the beginning of the tutorial.
  • Other fixes in concrete sentences in the comments above.
  • Improve sentences to make them shorter and more concise.

I was wondering if it could be interesting to make a "Concept" article about *asynchronism" and other related concepts. This way the explanations here could be shorter since the reader could find more details somewhere else.

It will take me a while because I have other urgent matters, but I am going to do it as soon as possible.

santiago-tapia avatar Dec 17 '24 09:12 santiago-tapia

One sentence per line (without limit to line length?)

Yep. Generally let your editor deal with line wrapping. Using this approach makes suggestions a whole lot easier on our side.

Replace ROS2 with ROS 2 Make an introduction on Asynchronism at the beginning of the tutorial. Other fixes in concrete sentences in the comments above. Improve sentences to make them shorter and more concise.

That sounds about right. Don't work too hard on shortening sentences, I can lend a hand with editing once we have the one sentence per line fixed. One other thing to keep in mind is that our linter is really picky. It tends to complain about trailing whitespace at the end of a line, so I would recommend making whitespace visible in your editor if you can. I can spot fix a few things but it really sucks if every line has an extra space or two at the end.

It will take me a while because I have other urgent matters, but I am going to do it as soon as possible.

No worries! It is the holidays. We really appreciate your contribution to the documentation!

I was wondering if it could be interesting to make a "Concept" article about *asynchronism" and other related concepts. This way the explanations here could be shorter since the reader could find more details somewhere else.

I would be open to it but I think it would help if we were all on the same page before you put a lot of effort into it. What I would recommend is that you file an issue and describe what you want to do with a detailed outline. We can discuss the outline before you sink a lot of effort into writing.

Thanks for being flexible about the idiosyncrasies in our docs contribution process.

kscottz avatar Dec 17 '24 18:12 kscottz

Hi again,

I have reviewed the whole tutorial according to the comments from @kscottz and @fujitatomoya.

I am afraid it must be very difficult to track the changes between my previous commit and this last commit because there are lots of changes. That is because I have written every sentence in a line, and, besides, I have reviewed almost every sentence to try to simplify the statements (just, please, remember that English is not my native language and that is not my fault...).

I have also reworked the introduction to say what the reader is going to do and to make a short explanation about asynchronism. That means I have moved some paragraphs and rewritten some others. I have also corrected some other minor details like code indentation. Of course I have replaced ROS2 with ROS 2.

This is my last commit, there is another commit but it is just a merge commit due to a fork synchronization, since this tutorial is completely new, it is not relevant.

Regards,

Santiago

santiago-tapia avatar Dec 30 '24 13:12 santiago-tapia

Hi again:

I have converted the plantuml to mermaid and removed the initial useless reference.

I would recommend including a note about the diagram format in the contributing file. I would also recommend saying that the documentation should be written using a line for each sentence even if this produces lines too long. 

Following our conversation on Delay Server: although it was not my original plan, I am going to make another PR to modify the minimal client service. In that PR, I will include the suggestions from @fujitatomoya about using a timer for sending async requests. 

But, I am afraid I have to insist on the convenience of this tutorial and the importance of the Delay Server. Just a note on its importance: the whole section "3 Run the asynchronous client" should be rewritten because it will be impossible to get that particular sequence of events.

santiago-tapia avatar Feb 02 '25 16:02 santiago-tapia

@santiago-tapia good catch on saying we should make our requests clear!

I added this clarification at your request.

Thanks for being patient.

kscottz avatar Feb 03 '25 23:02 kscottz

Hi again,

I have been looking for a precedent of sleeping the thread to emulate a running code that takes some time to complete. I was searching for it because I remember coming across this idea while reading an example in the ROS 2 documentation. And I found it:

Lifecycle Demo

    // Let's sleep for 2 seconds.
    // We emulate we are doing important
    // work in the activating phase.
    std::this_thread::sleep_for(2s);

That is exactly the same situation I try to emulate. I assume that this piece of code was reviewed and that using sleep_for was found acceptable to emulate a time-consuming process rather than being regarded as an anti-pattern.

That said, I was wondering about the status of this PR. Currently, its state is marked as "changes requested", but as far as I can tell, all requested changes have already been addressed. Could you clarify if there's anything else pending?

santiago-tapia avatar Feb 27 '25 16:02 santiago-tapia