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

Ros2 migrate diagnostic aggregator #118

Merged

Conversation

Karsten1987
Copy link
Contributor

This is currently still work in progress in order to make all tests pass.

This PR requires ros/bond_core#54

@Karsten1987 Karsten1987 self-assigned this Jul 25, 2019
@norro
Copy link
Collaborator

norro commented Aug 7, 2019

@Karsten1987 can you fix the jenkins setup so that this PR builds against the proper bondcpp PR? I don't know how to that tbh.

@Karsten1987
Copy link
Contributor Author

@norro the current PR compiles and passes all linters. However, the test doesn't pass. It's essentially because it can't find the appropriate parameter:

  std::map<std::string, rclcpp::Parameter> parameters;
  if (!n->get_parameters(breadcrumb_, parameters)) {
    RCLCPP_WARN(logger_,
      "Couldn't retrieve parameters for analyzer group '%s', namespace '%s'.",
      breadcrumb_.c_str(), n->get_namespace());
    return false;
  }

Do you know what parameter should be set here?

@norro
Copy link
Collaborator

norro commented Aug 8, 2019

Do you know what parameter should be set here?

This test requires a parameters yaml file, e.g., all_basic_analyzers.yaml. I tried to set this up with launch-testing, but failed.
I added a demo launch setup that should show what I wanted to do for the test as well, see PR in the downstream repo: boschresearch#3

I can't properly test this right now, as somehow the plugin loading seems to be broken right now for me (it was working before an install folder cleanup I just did, probably was working due to some legacy files in my install folder).

@Karsten1987
Copy link
Contributor Author

@norro @ralph-lange This PR compiles but has quite some test failures in flake8 and copyright. Do you plan on addressing them? I think we've talked about not having python support due to the lack of bondpy. I would therefore propose to cleanup the unused python files in order to have a clean state with all passing linters/test before merging this PR.

@norro
Copy link
Collaborator

norro commented Feb 10, 2020

@Karsten1987 Agreed. Should we remove the diagnostic_aggregator python stuff from the branch entirely then? (of course keeping the python scripts for the example and testing)

Regarding the copyright notices: the respective colcon test (I think ament_copyright) is complaining about a lot of hpp files that have a copyright notice in them. It is an old notice (willow garage, 2008), but still. do you know how to address these? they are the same as in the other diagnostic_* packages hpp files.

@Karsten1987
Copy link
Contributor Author

I think it's okay to delete the unused python stuff from this branch. We can open a follow-up github ticket to revive the python aggregator functionality once bond_py is ported. I think it's sufficient to limit the scope of this PR to C++.

As for the copyright linters: You can disable them specifically per project by adding this line to the CMakeLists.txt: set(ament_cmake_copyright_FOUND TRUE).

Please make sure to run colcon build as well as colcon test locally to see that all linters pass before we can trigger CI here and bring this PR forward.

@norro
Copy link
Collaborator

norro commented Feb 11, 2020

@Karsten1987 I removed the python stuff accordingly, all tests/checks running without errors.
I build against boschresearch/bond_core#fix_ros2, which I suggest merging to ros/bondcore as well.

@Karsten1987
Copy link
Contributor Author

Karsten1987 commented Feb 24, 2020

@norro could you please rebase this branch on top of the latest ros2-devel branch?

I took the liberty to rebase your branch as it's already a fair mix of contributions :)

Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
* To be done: logging, assertions, parameter handling

Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
* Migrated from XMLRPC to ROS2 parameters parsing
* Doesn't create working analzers, yet

Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
* Split anaylzers into seperate plugin lib
* Build shared lib to be used by plugin class loader
* Fixed plugin registration of analyzers

Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
* Improved parameter handling of generic_analyzer

Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
* Separate handling of the analyzer's (and analyzer group's):
** "nice" name
** path (path of their results in the robot monitor)
** breadcrumb (prefix of their yaml configuration)
* Logging
* Uncrustify
* Using std::mutex instead of boost::mutex. Using std::lock_guard
instead of boost::scoped_lock since std::scoped_lock was not introduced before C++17
* Using std::regex instead of boost::regex

Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
@norro
Copy link
Collaborator

norro commented Apr 29, 2020

I was able to remove the uuid dependency, this issue doesn't seem to block the builds anymore.

@Karsten1987
Copy link
Contributor Author

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

@Karsten1987
Copy link
Contributor Author

@norro Build looks good so far. I think there's something Linux specific in the CMakeLists.txt:

# see https://github.com/pybind/pybind11/commit/ba33b2fc798418c8c9dfe801c5b9023d3703f417
target_compile_options(${PROJECT_NAME} PRIVATE -Wdeprecated)

-Wdeprecated is not supported on Windows. That needs a special treatment.

Signed-off-by: Karsten Knese <karsten.knese@us.bosch.com>
@Karsten1987
Copy link
Contributor Author

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

Signed-off-by: Karsten Knese <karsten.knese@us.bosch.com>
@Karsten1987
Copy link
Contributor Author

Karsten1987 commented May 1, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status // CI hiccup
  • Windows Build Status

Signed-off-by: Karsten Knese <karsten.knese@us.bosch.com>
@Karsten1987
Copy link
Contributor Author

@norro so your PR builds now on all platforms. There are a non-zero amount of failing tests on Windows. Please have a look at them.

There's a known problem with sending Ctrl-C commands to a test in windows. So if you expect work to be done in a destructor or something, that might be a problem here. If there's no easy workaround, we might have to disable the tests on Windows.

Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
@Karsten1987
Copy link
Contributor Author

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

Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
@norro
Copy link
Collaborator

norro commented May 17, 2020

Sorry, used the wrong path magic. Can you check again, please, @Karsten1987.

@Karsten1987
Copy link
Contributor Author

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

More path fixes inside tests. Tests were expecting to find the node
executable in the CMAKE_BINARY_DIR before, which is true on linux
but not on windows.

Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
@Karsten1987
Copy link
Contributor Author

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

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987
Copy link
Contributor Author

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

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987
Copy link
Contributor Author

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

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987
Copy link
Contributor Author

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

@norro
Copy link
Collaborator

norro commented Jun 3, 2020

Looks like the last Windows build failing was due to a hicc-up in the build setup (pip installation of dependencies).

@Karsten1987
Copy link
Contributor Author

Karsten1987 commented Jun 3, 2020

Windows once more: Build Status // These warnings are unrelated.

@Karsten1987 Karsten1987 merged commit 54f79ff into ros:ros2-devel Jun 3, 2020
@norro
Copy link
Collaborator

norro commented Jun 4, 2020

whoop

@Karsten1987
Copy link
Contributor Author

@norro can that branch be deleted?

@norro
Copy link
Collaborator

norro commented Jul 1, 2020

absolutely.

@norro norro deleted the ros2_migrate_diagnostic_aggregator branch July 1, 2020 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ros2 PR tackling a ROS2 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants