-
Notifications
You must be signed in to change notification settings - Fork 25
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
changed install directory so other pkgs can ament_target_depend on th… #23
changed install directory so other pkgs can ament_target_depend on th… #23
Conversation
The checks failed because the CI workflow file was pointing to the wrong yaml file. I pushed the necessary changes to the master but now your branch is behind by that commit. Could you please pull master and rebase to it, then push to your PR branch again so I can rerun the tests? Thanks! |
@buckleytoby can you merge master and push to your PR branch again? It should trigger a rebuild so we know the tests pass. Thanks! |
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 but please add documentation to README.md.
Thanks!
@@ -91,6 +91,7 @@ void FFMPEGEncoder::setParameters(rclcpp::Node * node) | |||
profile_ = get_safe_param<std::string>(node, ns + "profile", ""); | |||
preset_ = get_safe_param<std::string>(node, ns + "preset", ""); | |||
tune_ = get_safe_param<std::string>(node, ns + "tune", ""); | |||
delay_ = get_safe_param<std::string>(node, ns + "delay", ""); |
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.
Please update README.md to reflect this new parameter.
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.
Needs Documentation of new parameter "delay" in README.md. Rest looks good.
Since I got no response from @buckleytoby I merged the change without any documentation :( |
hey, sorry, I've been sick all week so haven't had the chance to revisit this. Will return to it on Monday. |
…is library