Skip to content

Conversation

@JanStaschulat
Copy link
Contributor

@JanStaschulat JanStaschulat commented Jun 12, 2023

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
Copy link
Contributor Author

@Mergifyio backport rolling iron

@mergify
Copy link
Contributor

mergify bot commented Jun 12, 2023

backport rolling iron

✅ Backports have been created

Details

@JanStaschulat JanStaschulat mentioned this pull request Jun 12, 2023
20 tasks
@JanStaschulat JanStaschulat requested a review from pablogs9 June 12, 2023 07:11
@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2023

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

Signed-off-by: Jan Staschulat <[email protected]>
Signed-off-by: Jan Staschulat <[email protected]>
@JanStaschulat JanStaschulat merged commit 4b8ef7b into master Jun 12, 2023
@JanStaschulat JanStaschulat deleted the feature/add-interface-for-real-time-executor branch June 12, 2023 09:21
mergify bot pushed a commit that referenced this pull request Jun 12, 2023
* added custom pointer in data structures

Signed-off-by: Jan Staschulat <[email protected]>

* added executor type

Signed-off-by: Jan Staschulat <[email protected]>

* renamed struct

Signed-off-by: Jan Staschulat <[email protected]>

---------

Signed-off-by: Jan Staschulat <[email protected]>
(cherry picked from commit 4b8ef7b)
mergify bot pushed a commit that referenced this pull request Jun 12, 2023
* added custom pointer in data structures

Signed-off-by: Jan Staschulat <[email protected]>

* added executor type

Signed-off-by: Jan Staschulat <[email protected]>

* renamed struct

Signed-off-by: Jan Staschulat <[email protected]>

---------

Signed-off-by: Jan Staschulat <[email protected]>
(cherry picked from commit 4b8ef7b)
JanStaschulat added a commit that referenced this pull request Jun 14, 2023
#362)

* Data structures interfaces for multi-threaded executor (#355)

* added custom pointer in data structures

Signed-off-by: Jan Staschulat <[email protected]>

* added executor type

Signed-off-by: Jan Staschulat <[email protected]>

* renamed struct

Signed-off-by: Jan Staschulat <[email protected]>

---------

Signed-off-by: Jan Staschulat <[email protected]>
(cherry picked from commit 4b8ef7b)

* inserted a space to trigger ci-pipeline

Signed-off-by: Jan Staschulat <[email protected]>

* removed white space - retrigger ci pipeline

Signed-off-by: Jan Staschulat <[email protected]>

* updated actions/checkout to version v3

Signed-off-by: Jan Staschulat <[email protected]>

---------

Signed-off-by: Jan Staschulat <[email protected]>
Co-authored-by: Jan Staschulat <[email protected]>
JanStaschulat added a commit that referenced this pull request Jun 14, 2023
#363)

* Data structures interfaces for multi-threaded executor (#355)

* added custom pointer in data structures

Signed-off-by: Jan Staschulat <[email protected]>

* added executor type

Signed-off-by: Jan Staschulat <[email protected]>

* renamed struct

Signed-off-by: Jan Staschulat <[email protected]>

---------

Signed-off-by: Jan Staschulat <[email protected]>
(cherry picked from commit 4b8ef7b)

* inserted a space to trigger ci-pipeline

Signed-off-by: Jan Staschulat <[email protected]>

* removed white space - retrigger ci pipeline

Signed-off-by: Jan Staschulat <[email protected]>

---------

Signed-off-by: Jan Staschulat <[email protected]>
Co-authored-by: Jan Staschulat <[email protected]>
@gavanderhoorn
Copy link

gavanderhoorn commented Jun 24, 2023

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
Copy link
Contributor Author

@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
Copy link

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
Copy link
Contributor Author

Sure, I meant NON_INITIALIZED_EXECUTOR

@gavanderhoorn
Copy link

gavanderhoorn commented Jun 26, 2023

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
Copy link
Contributor Author

created an issue #378

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants