-
Notifications
You must be signed in to change notification settings - Fork 142
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 methods related to node options to NodeBuilder #186
Conversation
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.
Thanks! I'll add more comments later.
Co-authored-by: Nikolai Morin <nnmmgit@gmail.com>
Co-authored-by: Nikolai Morin <nnmmgit@gmail.com>
@Soya-Onishi Great work! Could you also add |
Co-authored-by: Nikolai Morin <nnmmgit@gmail.com>
Co-authored-by: Nikolai Morin <nnmmgit@gmail.com>
I tested it out and got an error about Apart from that, I only have one other comment (about enable_rosout), then it's ready to merge! |
Co-authored-by: Nikolai Morin <nnmmgit@gmail.com>
Co-authored-by: Nikolai Morin <nnmmgit@gmail.com>
@Soya-Onishi Could you adjust the CString operations to match #190, which was merged in the meantime? |
@Soya-Onishi I have another PR that conflicts with this one, #193. If you plan to make the couple of remaining fixes this week, I can wait with my PR until we merge this one, so you don't have extra work. |
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.
@Soya-Onishi thanks for all the hard work and for the patience iterating with us! 🙂
Wonderful! Thanks @Soya-Onishi! |
Feel free to join the Matrix chat by the way. |
Closes #142
This PR add node options related to
rcl_node_options_t
.However, methods related to allocator options (e.g. NodeBuilder::allocators.allocate) are not implemented because there are no way to return allocated heap pointer as far as I know.