Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RCLC-Executor with new API and convenience functions #13

Merged
merged 75 commits into from Mar 16, 2020

Conversation

@JanStaschulat
Copy link
Contributor

JanStaschulat commented Feb 4, 2020

This repository contains two packages: The rclc-package with an RCLC-Executor providing a new C-API based on the RCL-layer. It supports the standard rclcpp-Executor semantics and provides additional features for deterministic and real-time execution:

  • sequential processing order
  • triggered execution
  • Logical Execution Time (LET) semantics for data synchronization.

Additionally, the rclc-package contains convenience functions to create RCL-objects to simplify setting up ROS2-applications in C.

Secondly, the package rclc_examples demonstrates how to use the RCLC-Executor and the convenience functions.

For this Executor the pull request 534 on ros2/rcl was already created in Nov. 2019. This pull request resolves the comments of that PR and provides the new feature of triggered execution. After the discussions in the ROS2 Real-Time Working Group (tag: wg-real-time) it was decided to create a new pull request for ros2/rclc.

BorjaOuterelo and others added 30 commits Sep 21, 2018
Exposes required interface functions using visibility macros.
Comments out not working test code in windows.
Disables test builds.
Release v0.0.1beta
Feature/crystal update
Update master.
Signed-off-by: Ralph Lange <ralph.lange@de.bosch.com>
Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
…criber, timer and node. Added executor_handle.

Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
…Updated documentation in README.md.

Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
…utor with rclc convenience functions. Updated both README.md

Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
… (data_availabe) needs to be saved across multiple calls to spin_some

Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
…1Hz r2=0,5Hz) one executor with two subscribers. Trigger function a) when both messages are received (AND logic) b) when either message is received (OR logic). With subscriptions_options.qos.depth=0 (register semantic at DDS queue) the output is a) message reveived with slower rate and intermediate message are lost b) all message are received

Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
…e new data from rcl . Tested with rclc_examples/let_executor.c

Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
…r => rclc_executor, filename changes *let_executor* => *executor*
Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
…on 'ALWAYS', that the callback is called for every invocation of spin_some or spin_period. This is not the case with the default trigger condition ANY, which only executes the registered callbacks, if new data for at least one handle is available. Solution: new trigger condition ALWAYS, which always returns true.

Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
…case.

Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
JanStaschulat and others added 8 commits Feb 10, 2020
- executor_spin_some_API
- pub_sub_example
- spin_some_sequential_execution

Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
- invocation_type

Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
- resolved bug in spin_some_sequential_execution
- update_wait_set

Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
- spin_period

Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
- semantics_RCLCPP
- semantics_LET

Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
- trigger_one
- trigger_any
- trigger_all
- trigger_always

Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
- removed package dependency of geometry_msgs in package.xml and CMakeList.txt

Signed-off-by: Staschulat Jan <jan.staschulat@de.bosch.com>
Dev: refactoring unit-tests for rclc executor
@JanStaschulat

This comment has been minimized.

Copy link
Contributor Author

JanStaschulat commented Feb 18, 2020

@carlossvg @wjwwood @dejanpan @fujitatomoya @jacobperron @Karsten1987 If there is anything missing to merge the pull request, please let me know.

@carlossvg

This comment has been minimized.

Copy link
Contributor

carlossvg commented Feb 18, 2020

@JanStaschulat Just some minor comments from my side:

  1. I would use a more descriptive name than RCLCPP_EXECUTOR. I understand that currently there is not a better choice to describe its semantics.
  2. It would be interesting to add an example specifically showing LET semantics benefits over rclcpp in terms of data determinism. For example showing some output differences when setting first "LET" semantics and then "RCLCPP_EXECUTOR" semantics.
  3. Also, could your provide some insights about how LET semantics would work in a multi-threaded context?
  4. Some typo fixes in the README: micro-ROS#17
Update README
@JanStaschulat

This comment has been minimized.

Copy link
Contributor Author

JanStaschulat commented Feb 19, 2020

@carlossvg Thanks for your remarks.

  1. Yes, it was also difficult for me to come up with a good name. What do you think about the following reasoning: Lets look at the characteristics of the different semantics:

    • LET: reading all available data from DDS at the beginning of spin_some(), processing all callbacks, publishing all data at the end (of the period) at once.
    • rclc Executor: checking for new data at DDS queue, fetch data of topic just before the corresponding callback is executed, data is published within the callback code.
      So I would propose to IMMEDIATE as a more descriptive name for the semantics of the rclc executor, as 1) data is fetched immediately before the corresponding callback is called and 2) data is immediately published, when publish() is called. (Synonym could also be INSTANTLY).
      @wjwwood What do you think? Do you prefer any other desciptive name for the current rclc semantics?
  2. There are two test cases: semantics_LET and semantics_RCLCPP showing the different semantics. The setup is:
    // configuration
    // - one publisher
    // - publishes integer topic X
    // - subscriber A
    // - subscribes to X
    // - publishes also on topic X
    // - subscriber B
    // - subscribes to X with DDS quality of service option: last-is-best
    // - evaluates data of topic X and copies it to global variable result_X
    // - evaluation in test case, what the value of result_X is.
    //
    // test setup
    // - publisher: publishes X=1
    // - sleep(100ms)
    // - spin_some()
    // - subscriber A : publishes X=2
    // - sleep(100ms)
    // - subscriber B :
    // expected test result for semantics RCLCPP:
    // => subscriber B receives X=2 (because it takes most recent data)
    // expected test result for semantics LET:
    // => subscriber B receives X=1 (because data is taken from DDS at the start of spin_some())

Do you think to have this as an explicit example in the rclc_examples directory?

  1. The current implementation supports only the single threaded use-case. In a next step, we want to support the multi-threaded use-case as well. That means to extend the API of the RCLC Executor with a rclc_publish() function and some additional messages for synchronization of processes

3.1 rclc_publish() function:

To be used in the callback like this:
callback(){
// some code
rclc_publish(&sub, &msg);
}

This function will just add the topic and the message into a queue, which is managed by the RCLC Executor. At initialization, the user must specify the length of this queue and the period, when all the collected messages shall be published.

rclc_executor {
unsigned int max_pub_size;
void * pub_queue;
}
rclc_executor_configure_LET_publisher(max_size, period_ms);
rclc_executor__LET_publish();
rclc_executor_publish(rcl_subscription_t *; void *);

Please refer to Figure 3 in rclc documentation
The assumption is that each process will run in a different thread and has a fixed period. That is, every process is periodically executed, in embedded context that is typically 1ms, 5ms, 10ms, 20ms, 100ms time slices.

By the way, this time-driven paradigm is also applicable for robotics applications, as the sensor (e.g. laser) publishes data periodically and drives with this the entire (data-driven) pipeline.

  • At initialization of the LET-Executor, the publish-queue is configured with the maximum number of elements and the period, how often to publish.
  • Within a callback the rclc_publish() function will put the data into a pub_queue.
  • Every 'period' all data in the pub_queue will be published by rclc_executor_LET_publish()

This completes the semantics for LET

  • reading data at the beginning of the period (already implemented)
  • publishing data at the end of the period

3.2 Synchronization between threads (processes)

  • There needs to be a synchronization mechanism between the different threads to make sure that publishing is atomic. e.g. that publishing the data from the 1ms process occurs completely before any other process reads this data.
  • option A) one thread for each executor: use a dedicated "pub-ready" message which then is used as trigger condition for the other processes.
  • option B) one executor with multiple threads (processes): data publishing is orchestrated by executor
    This needs to be worked out in more detail, but the principle is to make sure that communication between threads is synchronized.
  1. merged pull request.
@ros-discourse

This comment has been minimized.

Copy link

ros-discourse commented Feb 21, 2020

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-02-20/12901/1

@JanStaschulat

This comment has been minimized.

Copy link
Contributor Author

JanStaschulat commented Mar 2, 2020

@carlossvg @wjwwood @dejanpan @Karsten1987 What is the status of your discussion for merging this pull request?

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Mar 2, 2020

I'm happy to get out of you guys way and let you merge things on this repository without my review. I simply don't have the bandwidth to provide reviews for another client library on top of rclcpp and rclpy, sorry. :(

If you guys need permissions or someone else on @ros2/team wants to help with reviews let me know.

@Karsten1987

This comment has been minimized.

Copy link

Karsten1987 commented Mar 3, 2020

I think it would make sense to transfer the ownership of this repo to the realtime working group similar to how rosbag2 and friends are managed and reviewed by the tooling working group.
We can create a github team which has the appropriate rights to review and merge PRs on this repo.

Thoughts?

@ralph-lange

This comment has been minimized.

Copy link
Contributor

ralph-lange commented Mar 3, 2020

We are happy to take over the ownership, possibly together with a partner not directly involved the micro-ROS development to have a second (unbiased) view on the API.
@wjwwood, @carlossvg Is there anybody from the Real-Time WG to volunteer? Otherwise, I would start with a PR to transfer the ownership to Bosch and eProsima for the time being, to be able to review our PRs mutually.

@carlossvg

This comment has been minimized.

Copy link
Contributor

carlossvg commented Mar 3, 2020

@ralph-lange It makes sense to me that Bosch and eProsima take over the ownership as they are the main entities involved. Anyway, we can comment this in the next RTWG meeting in case there is someone else interested.

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Mar 3, 2020

We can leave the repository in this organization. I can just setup a team for you guys so you can administrate it yourself and add whoever you want to it. I can also help you if you'd like to move it to another org, but I think leaving it here is best for visibility and discovery personally.

Just let me know who and I'll set it up!

@ralph-lange

This comment has been minimized.

Copy link
Contributor

ralph-lange commented Mar 4, 2020

If no one else from the Real-time WG is eager to take ownership of rclc in today's meeting, then please make @JanStaschulat and @julibert owners of it.

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Mar 4, 2020

@ralph-lange I've added @JanStaschulat and @julibert to the @ros2/rclc team. I can add others too, if needed. If you all would let me know when you've accepted the invitations I can update your permissions afterwards.

@JanStaschulat

This comment has been minimized.

Copy link
Contributor Author

JanStaschulat commented Mar 5, 2020

@wjwwood Accepted the invitation.

@ralph-lange

This comment has been minimized.

Copy link
Contributor

ralph-lange commented Mar 5, 2020

@wjwwood Jan and Julian both accepted the invitation, you may make them owners now.

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Mar 5, 2020

You both now have the ⚡️! Thanks for helping with this package.

@JanStaschulat JanStaschulat requested a review from julibert Mar 6, 2020
@JanStaschulat JanStaschulat merged commit 40e92b5 into ros2:master Mar 16, 2020
@ros-discourse

This comment has been minimized.

Copy link

ros-discourse commented Mar 20, 2020

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-03-18/13313/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.