ros2 / rclc

ROS Client Library for the C language.
Apache License 2.0
118 stars 42 forks source link

Data structures interfaces for multi-threaded executor #355

Closed JanStaschulat closed 1 year ago

JanStaschulat commented 1 year ago

The multi-threaded executor PR is not finished yet. I'l like to provide this feature also for the Iron-release. Therefore, I'd like to have the interface definitions already in Iron. Specifically, I just need a custom pointer in executor.h and executor_handle.h .

JanStaschulat commented 1 year ago

@mergifyio backport rolling iron

mergify[bot] commented 1 year ago

backport rolling iron

✅ Backports have been created

* [#362 Data structures interfaces for multi-threaded executor (backport #355)](https://github.com/ros2/rclc/pull/362) has been created for branch `rolling` * [#363 Data structures interfaces for multi-threaded executor (backport #355)](https://github.com/ros2/rclc/pull/363) has been created for branch `iron`
codecov-commenter commented 1 year ago

Codecov Report

Merging #355 (b84cdd4) into master (2648503) will decrease coverage by 0.08%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #355      +/-   ##
==========================================
- Coverage   69.20%   69.13%   -0.08%     
==========================================
  Files          16       16              
  Lines        2715     2715              
  Branches      765      765              
==========================================
- Hits         1879     1877       -2     
- Misses        450      451       +1     
- Partials      386      387       +1     

see 1 file with indirect coverage changes

gavanderhoorn commented 1 year ago

Having recently updated this package in my local build env to include this PR, I started running into build errors.

One of my platform headers already #defines a NONE, leading to problems with the value in the rclc_executor_type_t enum (here).

While I would agree with a statement along the lines of "that's not a very good idea to globally #define something called NONE", unfortunately I can't change my platform's header.

Would it perhaps be possible to prefix the names in the enum in executor.h, such that the chances of clashing with existing/platform #defines are reduced? Perhaps an EXECUTOR_ prefix?


Edit: the LET member in rclc_executor_semantics_t is also perhaps a candidate for a prefix, although I haven't yet ran into an issue with that.

JanStaschulat commented 1 year ago

@gavanderhoorn thanks for the comment. Sure I can change the default type name to something more descriptive non_initialized_executor? Same with LET semantics to LOGICAL_EXECUTION_TIME.

What do you think?

gavanderhoorn commented 1 year ago

Sure I can change the default type name to something more descriptive non_initialized_executor?

Would you not want that all upper case? Seeing as it's an enum member?

Oh, wait. My comment was about NONE (ie: the rclc_executor_type_t member), not about rclc_executor_type_t.

Same with LET semantics to LOGICAL_EXECUTION_TIME.

Yes, that seems much more descriptive and would have less chance of clashing.

JanStaschulat commented 1 year ago

Sure, I meant NON_INITIALIZED_EXECUTOR

gavanderhoorn commented 1 year ago

Seems like a good name.

Would it perhaps be an idea to prefix all members? SINGLE_THREADED and MULTI_THREADED are rather generic as well. Seeing as there is no enum scope in C11?

JanStaschulat commented 1 year ago

created an issue https://github.com/ros2/rclc/issues/378