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.

Sign up for GitHub

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

Jump to bottom

More overall improvements to the code #98

Merged
merged 4 commits into from
Jan 25, 2024
Merged

More overall improvements to the code #98

merged 4 commits into from
Jan 25, 2024

Conversation

clalancette
Copy link
Collaborator

This small series does another pass through the subscription, clients, services, and guard condition code, making improvements. In particular:

  1. It fixes a bug where guard conditions would be ready over and over again. This would cause very high CPU load. Now once we get the status of a guard condition, we also reset the trigger.
  2. It encapsulates subscriptions, services, and client code into their own classes. This allows us to make more of the class fields private, and do proper locking around them. It also has a side benefit of moving some code out of rmw_zenoh.cpp, which has grown quite large.

@clalancette
Move more implementation into rmw_subscription_data_t.
d1c9af3 
This moves more of the implementation out of rmw_zenoh.cpp
and into the class handling the subscription data.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Move more of the implementation into rmw_service_data_t.
976cf80 
This just allows us to hide more of the implementation and
remove it from rmw_zenoh.cpp.  It also fixes some of the
locking, which wasn't correct previously.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Move more of the implementation into rmw_client_data_t.
2d1e333 
This allows us to encapsulate more of the client data in
the right place, and also allows us to fix the locking.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Make sure to reset the trigger.
9e1cd9a 
That is, once we have discovered what the guard condition
status is, we should immediately reset the trigger condition
and wait for the next one.  Otherwise we risk triggering
over and over again.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette clalancette requested a review from Yadunund January 23, 2024 14:50
Yadunund
Yadunund approved these changes Jan 25, 2024
View reviewed changes
Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for cleaning up the implementation.

@Yadunund Yadunund merged commit 76fec8b into rolling Jan 25, 2024
5 checks passed
@delete-merged-branch delete-merged-branch bot deleted the clalancette/encapsulate branch January 25, 2024 07:06
@Yadunund Yadunund mentioned this pull request Feb 8, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yadunund Yadunund Yadunund approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@clalancette @Yadunund