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

Execute command switch for async hardware components #1951

Closed

Conversation

trampler
Copy link

Call prepare_command_mode_switch and perform_command_mode_switch also on async hardware components. Currently this is not the case as they are stored separately in the resource storage.

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

Hello!

We are planning to move to the new approach : #1567 where this won't be needed.

Maybe this could be needed for Humble. Do you mind targeting Humble?

@trampler trampler changed the base branch from master to humble December 17, 2024 10:21
Copy link
Contributor

mergify bot commented Dec 17, 2024

This pull request is in conflict. Could you fix it @trampler?

Copy link
Contributor

mergify bot commented Dec 17, 2024

@trampler, all pull requests must be targeted towards the master development branch.
Once merged into master, it is possible to backport to humble, but it must be in master
to have these changes reflected into new distributions.

@trampler trampler changed the base branch from humble to master December 17, 2024 10:22
@trampler
Copy link
Author

trampler commented Dec 17, 2024

Sure. No problem. Guess I need to create a new pull request.

@saikishor Hmm, there are currently no async components in humble. So this pull request doesn't really make sense there.

@trampler trampler closed this Dec 17, 2024
@saikishor
Copy link
Member

Sure. No problem. Guess I need to create a new pull request.

@saikishor Hmm, there are currently no async components in humble. So this pull request doesn't really make sense there.

That's true. okay, let me bring this topic tomorrow in the WG meeting

@FranekStark
Copy link

Hello!

We are planning to move to the new approach : #1567 where this won't be needed.

Maybe this could be needed for Humble. Do you mind targeting Humble?

@saikishor Hi, what is the schedule for the refactoring? A the moment the async HW components are not working due to this bug, maybe it would be a good temporary solution to merge in this fix until the big refactoring is done?

@saikishor
Copy link
Member

@FranekStark it is already done. The idea is to merge it soon. Sure, let's open this PR again. I would like to discuss with other maintainers and decide on this. Today we have a WG meeting, I'll discuss it there. If you are interested, feel free to join

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.74%. Comparing base (7374c43) to head (98d50c1).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1951      +/-   ##
==========================================
+ Coverage   87.73%   87.74%   +0.01%     
==========================================
  Files         122      122              
  Lines       13010    13014       +4     
  Branches     1165     1165              
==========================================
+ Hits        11414    11419       +5     
  Misses       1165     1165              
+ Partials      431      430       -1     
Flag Coverage Δ
unittests 87.74% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
hardware_interface/src/resource_manager.cpp 73.42% <100.00%> (+0.12%) ⬆️

... and 2 files with indirect coverage changes

@saikishor
Copy link
Member

Hello!

With @ros-controls/ros2-maintainers we have decided go with the new approach in #1567 instead of going with this fix.

@bmagyar
Copy link
Member

bmagyar commented Dec 19, 2024

I'm closing this PR now but please reopen it if you think it is still relevant.

@bmagyar bmagyar closed this Dec 19, 2024
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.

4 participants