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

Change type of received_message #168

Closed
CursedRock17 opened this issue Sep 1, 2023 · 4 comments · Fixed by #170
Closed

Change type of received_message #168

CursedRock17 opened this issue Sep 1, 2023 · 4 comments · Fixed by #170
Labels
enhancement New feature or request

Comments

@CursedRock17
Copy link
Contributor

Description

The point of this issue is to highlight discussion to change from a type T to message_info in the OnMessageReceived Function. I would be able to make a pull request for these changes, if the libstatistics_collector library wanted to go along with this

Related Issues

This was discussed in pull request 2247 over in the rclcpp library. If you want to follow discussion up to this point, it has all happened there.

Completion Criteria

  • Must change type T in const T & received_message within all OnMessageReceived functions to either rmw_message_info_t or if we wanted to import rclcpp (which we probably don't) clcpp::MessageInfo
  • Then deprecate all OnMessageReceived functions that are templated with type T

Testing Notes / Suggestions

Not much other than changing the message types passed to the test functions.

@CursedRock17 CursedRock17 added the enhancement New feature or request label Sep 1, 2023
@clalancette
Copy link
Contributor

* Must change type `T` in `const T & received_message` within all `OnMessageReceived` functions to either `rmw_message_info_t` or if we wanted to import rclcpp (which we probably don't) `clcpp::MessageInfo`

Agreed that we can't take a new dependency on rclcpp here. That would lead to a circular dependency that would be hard to undo. This package already depends on rcl, which in turn depends on rmw, so it is OK to take an explicit dependency on it for this feature.

@MichaelOrlov
Copy link
Member

@CursedRock17 Sounds good 👍

@CursedRock17
Copy link
Contributor Author

Sounds good, it's been checked off as ready to go and all the uncrustify errors are fixed so once it's reviewed and the CI is ran on it, I'll change up rclcpp message handling.

@fujitatomoya
Copy link

@CursedRock17 i totally missed this one, i will take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants