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

Fixup timer #693

Closed
wants to merge 9 commits into from
Closed

Fixup timer #693

wants to merge 9 commits into from

Conversation

dorezyuk
Copy link
Contributor

Clean up minor issues in the timer.hpp/cpp

Most notably:

  • possible exception in the d'tor of the GenericTimer
  • missing virtual keywork in the d'tor of the BaseTimer
  • move from custom exception raising to the generic interface of 'throw_from_rcl_error'

[!] Someone with the right permissions must trigger the ci

@tfoote tfoote added the in review Waiting for review (Kanban column) label Apr 14, 2019
@sloretz
Copy link
Contributor

sloretz commented Apr 15, 2019

CI (building above rclcpp, testing rclcpp and test_rclcpp) (edit: oops, run 3 with fixed build args)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@sloretz
Copy link
Contributor

sloretz commented Apr 15, 2019

@dorezyuk Looks like CI is yellow because of complaints from uncrustify and cpplint. Mind fixing them?

https://ci.ros2.org/job/ci_linux/6691/testReport/

@dorezyuk
Copy link
Contributor Author

I have here also the same question about the linter for dependencies as #692 (comment)

@sloretz
Copy link
Contributor

sloretz commented Apr 18, 2019

Hi @dorezyuk , it looks like there's a merge conflict. Would you mind rebasing to resolve it?

@sloretz sloretz added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Apr 22, 2019
Signed-off-by: Dmitrij Dorezyuk <dmitrij.dorezyuk@hotmail.de>
Signed-off-by: Dmitrij Dorezyuk <dmitrij.dorezyuk@hotmail.de>
Signed-off-by: Dmitrij Dorezyuk <dmitrij.dorezyuk@hotmail.de>
Signed-off-by: Dmitrij Dorezyuk <dmitrij.dorezyuk@hotmail.de>
Signed-off-by: Dmitrij Dorezyuk <dmitrij.dorezyuk@hotmail.de>
Signed-off-by: Dmitrij Dorezyuk <dmitrij.dorezyuk@hotmail.de>
Signed-off-by: Dmitrij Dorezyuk <dmitrij.dorezyuk@hotmail.de>
Signed-off-by: Dmitrij Dorezyuk <dmitrij.dorezyuk@hotmail.de>
Copy link
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, with green CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

rclcpp/include/rclcpp/timer.hpp Show resolved Hide resolved
Signed-off-by: Dmitrij Dorezyuk <dmitrij.dorezyuk@hotmail.de>
@Karsten1987 Karsten1987 added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 30, 2019
@Karsten1987
Copy link
Contributor

Karsten1987 commented Apr 30, 2019

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

it looks like this PR is not up to date with the latest master.

@Karsten1987 Karsten1987 self-assigned this Apr 30, 2019
@nuclearsandwich
Copy link
Member

@dorezyuk are you able to update this pull request to target the latest master branch so review can continue?

@nuclearsandwich nuclearsandwich removed the in review Waiting for review (Kanban column) label Aug 15, 2019
@clalancette
Copy link
Contributor

There's been no response here for quite a while, so I'm going to close this out. Feel free to reopen and respond to the latest requests to continue moving this forward.

DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
…ion (ros2#693)

* Add `rosbag2_py::Player::play` pybind wrapper to replace `rosbag2_transport_python` version

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants