-
Notifications
You must be signed in to change notification settings - Fork 442
Add state_interfaces_broadcaster #2006
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
Add state_interfaces_broadcaster #2006
Conversation
|
This pull request is in conflict. Could you fix it @saikishor? |
0c75da3 to
a0b0b1c
Compare
...aces_state_broadcaster/include/interfaces_state_broadcaster/interfaces_state_broadcaster.hpp
Outdated
Show resolved
Hide resolved
Juliaj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saikishor, I needed to debug my example code that depends on this controller. I've added the changes made on my machine to make it compile with the updated control_msgs.
...aces_state_broadcaster/include/interfaces_state_broadcaster/interfaces_state_broadcaster.hpp
Outdated
Show resolved
Hide resolved
...aces_state_broadcaster/include/interfaces_state_broadcaster/interfaces_state_broadcaster.hpp
Outdated
Show resolved
Hide resolved
...aces_state_broadcaster/include/interfaces_state_broadcaster/interfaces_state_broadcaster.hpp
Outdated
Show resolved
Hide resolved
...aces_state_broadcaster/include/interfaces_state_broadcaster/interfaces_state_broadcaster.hpp
Outdated
Show resolved
Hide resolved
interfaces_state_broadcaster/test/test_interfaces_state_broadcaster.cpp
Outdated
Show resolved
Hide resolved
interfaces_state_broadcaster/test/test_interfaces_state_broadcaster.cpp
Outdated
Show resolved
Hide resolved
interfaces_state_broadcaster/test/test_interfaces_state_broadcaster.cpp
Outdated
Show resolved
Hide resolved
interfaces_state_broadcaster/test/test_interfaces_state_broadcaster.cpp
Outdated
Show resolved
Hide resolved
interfaces_state_broadcaster/test/test_interfaces_state_broadcaster.hpp
Outdated
Show resolved
Hide resolved
|
Thanks a lot for taking care @Juliaj . I was about to fix it |
Co-authored-by: Julia Jia <[email protected]>
Co-authored-by: Julia Jia <[email protected]>
Co-authored-by: Julia Jia <[email protected]> Co-authored-by: Surya! <[email protected]>
Co-authored-by: Julia Jia <[email protected]>
Juliaj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
christophfroehlich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this LGTM. But some parts of docs/meta package updates are missing:
Thank you @Juliaj for taking time and testing it |
christophfroehlich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw: Why did you call it interfaces_state_broadcaster instead of state_interfaces_broadcaster?
|
|
||
| Interfaces State Broadcaster | ||
| -------------------------------- | ||
| The Interfaces State Broadcaster publishes the state of specified hardware interfaces that support double data type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not 100% true. "is_castable_to_double" is true for all datatypes we currently support, but the get_optional only works for double and bool with additional warning.
We should either limit it to double-only, or support all castable datatypes for real ;)
This is what I started here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change the logic internally to the code to reflect that
Good question, first I named the project |
Co-authored-by: Christoph Fröhlich <[email protected]>
christophfroehlich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some minor cleanups, rest LGTM
...interfaces_broadcaster/include/state_interfaces_broadcaster/state_interfaces_broadcaster.hpp
Outdated
Show resolved
Hide resolved
state_interfaces_broadcaster/src/state_interfaces_broadcaster.cpp
Outdated
Show resolved
Hide resolved
christophfroehlich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ef3167d
into
ros-controls:master
(cherry picked from commit ef3167d) # Conflicts: # doc/release_notes.rst
(cherry picked from commit ef3167d)
This PR adds a new broadcaster that publishes the values of the interfaces in the same order defined in the broadcaster configuration. This is very useful, especially for the physical AI application, where the observation vector can be properly created beforehand
needs: ros-controls/control_msgs#273