-
Notifications
You must be signed in to change notification settings - Fork 161
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
user defined allocator should be used for rosout publisher. #1044
user defined allocator should be used for rosout publisher. #1044
Conversation
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
rcl/rcl/src/rcl/logging_rosout.c Line 89 in d15594e
rcl/rcl/src/rcl/logging_rosout.c Line 121 in d15594e
I assume it makes sense to use the allocator from the nodeoptions, since there could be realtime and non-realtime nodes in one process? |
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.
yeah i thought that too, probably __rosout_allocator
is meant to be used to operate related objects but not endpoints. I think allocator from NodeOption
makes sense to manage publisher endpoint. (I think that user would expect.)
we can keep this open to get more feedback!
@clalancette minor fix, what do you think? i would like to hear your comment on this before CI. |
@iuhilnehc-ynos what do you think about this? |
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
This should be backport to humble. |
@Mergifyio backport humble |
✅ Backports have been created
|
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com> (cherry picked from commit 485c89c)
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
address #1040