Skip to content

Conversation

@skyegalaxy
Copy link
Member

No description provided.

@skyegalaxy skyegalaxy self-assigned this Apr 3, 2025
Mauro Passerino and others added 10 commits May 14, 2025 16:00
* Refs #18846: PoC ignore local endpoints: extend RCLCPP API

Signed-off-by: JLBuenoLopez-eProsima <[email protected]>

* Refs #18846: PoC ignore local endpoints: modify RCLCPP publish logic

Signed-off-by: JLBuenoLopez-eProsima <[email protected]>

---------

Signed-off-by: JLBuenoLopez-eProsima <[email protected]>
Co-authored-by: JLBuenoLopez-eProsima <[email protected]>
(cherry picked from commit 106c03a)

style
Signed-off-by: Mauro Passerino <[email protected]>
(cherry picked from commit 58d2a04)

Remove redundant `add_subscription` cherry pick artifact

use const ref instead of ptr, add missing capacity fn

more style
* allow for deferred responses with ipc

* fix send response

* fix use member service ipc process

* add map to store CallbackInfoVariant

* add send_response to base class, set function ptr to get handle

* move service intra process outside base

* copy response into shared pointer for ipc

* add typename for service template

* remove ref signature

* try without std::ref

* try without ref wrapper

* try emplace

* make pair with variant

* some cleanup

* erase callback info if client invalid

* add post_init_setup for services

* fix extra comma

* add post init setup after lifecycle node services

* add documentation for ServiceIntraProcess template change

* use weak ptr to service in ServiceIntraProcess

* move check for valid service handle to beginning of function

* add comment for callback info map

(cherry picked from commit aa95a48)
* check request header for intraprocess

* set request header intraprocess to false in execute_service

(cherry picked from commit a617f93)
@skyegalaxy skyegalaxy force-pushed the smedeiros/jazzy-cherry-picks-from-iron branch from 007c4ca to b7e5649 Compare May 14, 2025 23:01
@skyegalaxy skyegalaxy marked this pull request as ready for review May 16, 2025 17:51
@apojomovsky
Copy link

Hey @skyegalaxy , great work with the cherry-picks, they all LGTM!

Just a small suggestion: for future migrations, it might be helpful to keep any additional changes you make on top of the cherry-picks in a separate PR. That way, we’d have a clear reference point explaining the motivation behind those changes, which could be useful in upcoming migrations.

Totally up to you whether you want to apply this approach in this PR, I’m just echoing what I remember being the team’s general recommendation based on past experiences with similar work.

@skyegalaxy
Copy link
Member Author

Hey @skyegalaxy , great work with the cherry-picks, they all LGTM!

Just a small suggestion: for future migrations, it might be helpful to keep any additional changes you make on top of the cherry-picks in a separate PR. That way, we’d have a clear reference point explaining the motivation behind those changes, which could be useful in upcoming migrations.

Totally up to you whether you want to apply this approach in this PR, I’m just echoing what I remember being the team’s general recommendation based on past experiences with similar work.

That's totally fair. Most of what's on top of the cherry picks involved whitespace / linting changes that were needed to make ament_cpplint/ament_uncrustify happy, so I think before I merge, I'll go back and rebase some of the commits such that anything that was just linting or whitespace gets rolled into the last relevant commit.

The only other changes on top were minor and can be isolated to their own commit so we can refer to them in a confluence doc as was done in previous migrations. This branch will eventually also require a future PR to fix the failing cases in rclcpp_action/test_client, which currently are the only tests that fail.

mauropasse and others added 11 commits May 19, 2025 14:55
* Include node namespace in IPC Action service name
* Include node namespace in IPC Client/Service service name
---------
Co-authored-by: Mauro Passerino <[email protected]>

(cherry picked from commit f5b2001)

remove whitespace in service.hpp
* store map of unique request id to client id and callback info pair

* fix map end check

* fix undefined reference

* remove unnecessary request id erase, remove/fix unique id comment

* improve unique id comment

(cherry picked from commit b865383)
action client / server ipc decrustification

whitespace / line length / uncrustify - service_intra_process

const correctness
* add logs and minor fixes

Signed-off-by: Alberto Soragna <[email protected]>

* use >0 rather than ==1 in comparison

Signed-off-by: Alberto Soragna <[email protected]>

---------

Signed-off-by: Alberto Soragna <[email protected]>
Signed-off-by: Alberto Soragna <[email protected]>
(cherry picked from commit d4dd4e4)
…2#2564)

* avoid adding notify waitable twice to events-executor entities collection

Signed-off-by: Alberto Soragna <[email protected]>

* remove redundant mutex lock

Signed-off-by: Alberto Soragna <[email protected]>

---------

Signed-off-by: Alberto Soragna <[email protected]>
(cherry picked from commit f27bdbf)
* Remove expired timers before updating the collection

Signed-off-by: Alexis Pojomovsky <[email protected]>

* Add regression test for reinitialized timers bug

Signed-off-by: Alexis Pojomovsky <[email protected]>

* Add missing includes

Signed-off-by: Alexis Pojomovsky <[email protected]>

* Relocate test under the executors directory

Signed-off-by: Alexis Pojomovsky <[email protected]>

* Extend test to run with all supported executors

Signed-off-by: Alexis Pojomovsky <[email protected]>

* Adjust comment in fix to make it more generic

Signed-off-by: Alexis Pojomovsky <[email protected]>

* Apply ament clang format to test

Signed-off-by: Alexis Pojomovsky <[email protected]>

* Fix uncrustify findings

Signed-off-by: Alexis Pojomovsky <[email protected]>

---------

Signed-off-by: Alexis Pojomovsky <[email protected]>
Co-authored-by: Alexis Pojomovsky <[email protected]>
(cherry picked from commit 9ef9646)
…149)

Co-authored-by: Alexis Pojomovsky <[email protected]>
(cherry picked from commit 30050c1)

exclude lock free queue from linting

actually lint lock_free_events_queue

lock_free_events_queue copyright

whitespace / line length / uncrustify - lock_free_events_queue
Co-authored-by: Alexis Pojomovsky <[email protected]>
(cherry picked from commit aef928d)
whitespace / line length / uncrustify - rclcpp_action client

whitespace / line length / uncrustify - rclcpp_action server

fix duplicate member definitions in rclcpp_action server.hpp

fixes for IPC action server

delete log

whitespace / line length / uncrustify - rclcpp action server again

minor action client fixes

whitespace / line length / uncrustify - rclcpp action client again
skyegalaxy and others added 3 commits May 19, 2025 15:24
whitespace / line length / uncrustify - publisher

proper inter_process_publish_needed

whitespace / line length / uncrustify - rclcpp publisher
Co-authored-by: Mauro Passerino <[email protected]>
(cherry picked from commit cf182e0)

whitespace / line length / uncrustify - test_actions and test_transient_local

whitespace / line length / uncrustify - test actions and transient_local again

remove redundant test_actions
@skyegalaxy skyegalaxy force-pushed the smedeiros/jazzy-cherry-picks-from-iron branch from b7e5649 to d637f90 Compare May 19, 2025 22:24
@skyegalaxy
Copy link
Member Author

Hey @skyegalaxy , great work with the cherry-picks, they all LGTM!
Just a small suggestion: for future migrations, it might be helpful to keep any additional changes you make on top of the cherry-picks in a separate PR. That way, we’d have a clear reference point explaining the motivation behind those changes, which could be useful in upcoming migrations.
Totally up to you whether you want to apply this approach in this PR, I’m just echoing what I remember being the team’s general recommendation based on past experiences with similar work.

That's totally fair. Most of what's on top of the cherry picks involved whitespace / linting changes that were needed to make ament_cpplint/ament_uncrustify happy, so I think before I merge, I'll go back and rebase some of the commits such that anything that was just linting or whitespace gets rolled into the last relevant commit.

The only other changes on top were minor and can be isolated to their own commit so we can refer to them in a confluence doc as was done in previous migrations. This branch will eventually also require a future PR to fix the failing cases in rclcpp_action/test_client, which currently are the only tests that fail.

I've gone ahead and folded those whitespace changes into the rest of the branch. Currently, the only additional semantic change I needed to make is this commit: d637f90, which is the last one in the list of cherry-picks and can be linked to independently when documenting these changes.

@skyegalaxy skyegalaxy merged commit c6d04f7 into irobot/jazzy May 20, 2025
@apojomovsky
Copy link

Hey @skyegalaxy , great work with the cherry-picks, they all LGTM!
Just a small suggestion: for future migrations, it might be helpful to keep any additional changes you make on top of the cherry-picks in a separate PR. That way, we’d have a clear reference point explaining the motivation behind those changes, which could be useful in upcoming migrations.
Totally up to you whether you want to apply this approach in this PR, I’m just echoing what I remember being the team’s general recommendation based on past experiences with similar work.

That's totally fair. Most of what's on top of the cherry picks involved whitespace / linting changes that were needed to make ament_cpplint/ament_uncrustify happy, so I think before I merge, I'll go back and rebase some of the commits such that anything that was just linting or whitespace gets rolled into the last relevant commit.
The only other changes on top were minor and can be isolated to their own commit so we can refer to them in a confluence doc as was done in previous migrations. This branch will eventually also require a future PR to fix the failing cases in rclcpp_action/test_client, which currently are the only tests that fail.

I've gone ahead and folded those whitespace changes into the rest of the branch. Currently, the only additional semantic change I needed to make is this commit: d637f90, which is the last one in the list of cherry-picks and can be linked to independently when documenting these changes.

Sounds great, thank you for addressing this!

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.

8 participants