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

Move hardware interface README content to sphinx documentation #1342

Merged
merged 3 commits into from
Feb 9, 2024

Conversation

mateusmenezes95
Copy link
Contributor

Overview

Note

During the migration, I have a doubt about the example code for the elapsed time option. Shouldn't the value of period variable in the if statement also be retrieved from the URDF parameters sections?

Copy link

codecov bot commented Jan 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0bdcd41) 47.53% compared to head (6a0995e) 47.53%.
Report is 11 commits behind head on master.

❗ Current head 6a0995e differs from pull request most recent head 00c63b4. Consider uploading reports for the commit 00c63b4 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1342   +/-   ##
=======================================
  Coverage   47.53%   47.53%           
=======================================
  Files          41       41           
  Lines        3547     3547           
  Branches     1930     1930           
=======================================
  Hits         1686     1686           
  Misses        459      459           
  Partials     1402     1402           
Flag Coverage Δ
unittests 47.53% <ø> (ø)

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

@christophfroehlich christophfroehlich linked an issue Jan 29, 2024 that may be closed by this pull request
8 tasks
Copy link
Contributor

@christophfroehlich christophfroehlich 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. I think that these code snippets should give an idea how to solve this. I wouldn't make it more complex with adding parameter handling.

@christophfroehlich christophfroehlich added backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 Iron. labels Jan 31, 2024
bmagyar
bmagyar previously approved these changes Jan 31, 2024
@mateusmenezes95
Copy link
Contributor Author

LGTM, thanks. I think that these code snippets should give an idea how to solve this. I wouldn't make it more complex with adding parameter handling.

But the write and read methods have period as an argument too. Can't this get confusing? Or in fact period in the snippets is the methods argument?

@christophfroehlich
Copy link
Contributor

But the write and read methods have period as an argument too. Can't this get confusing? Or in fact period in the snippets is the methods argument?

ah, you may be right. period is the parameter name of the method, it makes no sense to use this here. Do you want to change it to somewhat similar to 1./desired_hw_update_rate_ with a note that one has to implement parameter handling "like above"?

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.

LGTM

Future note:
After looking at this, I wonder if it's better to have different frequency management of how CM handles controllers, also at the hardware interface level.

destogl
destogl previously approved these changes Feb 1, 2024
hardware_interface/README.md Outdated Show resolved Hide resolved
@destogl destogl dismissed stale reviews from bmagyar, christophfroehlich, and themself via 518856c February 1, 2024 18:45
@bmagyar
Copy link
Member

bmagyar commented Feb 9, 2024

@mateusmenezes95 i wonder if a slightly modified version could work for iron/humble too?

@bmagyar bmagyar merged commit 55eeb09 into ros-controls:master Feb 9, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 Iron.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hardware_interface: Merge good practices documentation
5 participants