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 devel #94

Open
wants to merge 118 commits into
base: ros2-devel
from

Conversation

Projects
None yet
6 participants
@vaibhavbhadade
Copy link

vaibhavbhadade commented Dec 7, 2018

Ros 2 porting changes for following sub packages :+1
diagnostic_aggregator
diagnostic_common_diagnostics
diagnostic_updater python

Rohita83 and others added some commits Oct 16, 2018

Revert "Test"
This reverts commit 75a7773.
VaibhavBhadade
Update diagnostic_aggregator/CMakeLists.txt
Co-Authored-By: vaibhavbhadade <43374538+vaibhavbhadade@users.noreply.github.com>
VaibhavBhadade

VaibhavBhadade added some commits Jan 16, 2019

@vaibhavbhadade

This comment has been minimized.

Copy link
Author

vaibhavbhadade commented Jan 16, 2019

@Karsten1987 thanks for valuable feedback , I have fixed this issue. you will get fix in following commit 👍 DiagnosticTask information was not publised on topic
we are actively looking for review and feedback for improvement on this package .

@vaibhavbhadade

This comment has been minimized.

Copy link
Author

vaibhavbhadade commented Jan 16, 2019

@Karsten1987 Can you please share fixes needed for OSX so that we can merge

@Karsten1987

This comment has been minimized.

Copy link

Karsten1987 commented Jan 16, 2019

@vaibhavbhadade the CPU monitor seems to publish some date now.
However, can I ask you how you test these PRs? The hd_monitor still has traceback values in its data messages and I believe the python scripts for the ntp_monitor don't work. How do you execute these?

@vaibhavbhadade

This comment has been minimized.

Copy link
Author

vaibhavbhadade commented Jan 17, 2019

@Karsten1987 only gtest available in ros1 was tested for this package. I have fixed the issues in recent commits . thanks for inputs .
logs for test commands is as follow:

       ros2 run diagnostic_common_diagnostics hd_monitor 

Output:

  	 header:
  stamp:
    sec: 28369
    nanosec: 153787883
  frame_id: ''
status:
- level: "\0"
  name: rootpc-Strix-15-GL503GE HD Temperature
  message: OK
  hardware_id: rootpc-Strix-15-GL503GE
  values:
  - key: Update Status
    value: OK
  - key: Time Since Update
    value: Duration(nanoseconds=7011775267)
  - key: Disk 0 Temp Status
    value: OK
  - key: Disk 0 Mount Pt.
    value: /dev/sda
  - key: Disk 0 Device ID
    value: ST1000LX015-1U7172
  - key: Disk 0 Temp
    value: '41'

 

ros2 run diagnostic_common_diagnostics ntp_monitor ntp-hostname ntp.ubuntu.com

Output :

  header:
  stamp:
    sec: 28603
    nanosec: 611938360
  frame_id: ''
status:
- level: "\x01"
  name: NTP offset from rootpc-Strix-15-GL503GE to ntp.ubuntu.com
  message: NTP Offset Too High
  hardware_id: rootpc-Strix-15-GL503GE
  values:
  - key: Offset (us)
    value: '-3013.0'
  - key: Offset tolerance (us)
    value: '500'
  - key: Offset tolerance (us) for Error
    value: '5000000'
- level: "\x02"
  name: NTP self-offset for rootpc-Strix-15-GL503GE
  message: Error Running ntpdate. Returned 1
  hardware_id: rootpc-Strix-15-GL503GE
  values:
  - key: Offset (us)
    value: N/A
  - key: Offset tolerance (us)
    value: '500'
  - key: Offset tolerance (us) for Error
    value: '5000000'
  - key: Output
    value: o
  - key: Errors
    value: e

Please let me know your comments on this .

@trainman419

This comment has been minimized.

Copy link
Contributor

trainman419 commented Jan 17, 2019

It seems like there has been a lot of activity on this PR, but the previous PR where I had started to do a code review has been closed, and it's not clear if those comments have been resolved here.

I'm happy to do code review for these changes as best I can, but I do them on my spare time. I will do my best to provide prompt reviews as long as you respond in kind.

When this is ready for review and merge, please make sure that tests on Travis CI or some equivalent CI system are enabled. I think OSRF may be hosting a CI service for ROS 2; if you need my help in enabling that, please let me know and provide a link to directions.

@Karsten1987

This comment has been minimized.

Copy link

Karsten1987 commented Jan 17, 2019

@vaibhavbhadade I highly recommend running colcon test on your packages before putting this up for review. AFAIK there are quite some test failures to be fixed before it makes sense to run CI on it.

VaibhavBhadade and others added some commits Jan 18, 2019

Merge pull request #1 from Karsten1987/ros2_devel
fix warnings for OSX build
@vaibhavbhadade

This comment has been minimized.

Copy link
Author

vaibhavbhadade commented Jan 21, 2019

It seems like there has been a lot of activity on this PR, but the previous PR where I had started to do a code review has been closed, and it's not clear if those comments have been resolved here.
> yes , we had incorporate your comments in this PR.
I'm happy to do code review for these changes as best I can, but I do them on my spare time. I will do my best to provide prompt reviews as long as you respond in kind.
> Yes :)
When this is ready for review and merge, please make sure that tests on Travis CI or some equivalent CI system are enabled. I think OSRF may be hosting a CI service for ROS 2; if you need my help in enabling that, please let me know and provide a link to directions.
> We had tried to test on Travis CI but it was not supported for boinic (18.04) so we have removed .travis.yml file as per recommendation of dirk thomos in one of his reply.

@norro

This comment has been minimized.

Copy link

norro commented Feb 1, 2019

The cpu_monitor works for me on (l)ubuntu 18.04 and ROS 2 bionic. The hd_monitor shows zero-values only, though, and ntp_monitor shows errors calling ntp_date.

ros2 run diagnostic_common_diagnostics cpu_monitor

header:
  stamp:
    sec: 72403
    nanosec: 324504098
  frame_id: ''
status:
- level: "\0"
  name: 'cpu_monitor_lubuntu: CPU Information'
  message: CPU Average 53.0 percent
  hardware_id: lubuntu
  values:
  - key: CPU 0 Load
    value: '54.5'
  - key: CPU 1 Load
    value: '51.5'

Two additional technical hints:

  • IIRC, I had to install uuid_dev as a dependency, which could be indicated as dependency
  • I had to install gtest and gmock, which I didn't have to do for other ROS 2 packages before. So it seems that it is not using the ROS 2 infrastructure in that regard
@vaibhavbhadade

This comment has been minimized.

Copy link
Author

vaibhavbhadade commented Feb 4, 2019

The cpu_monitor works for me on (l)ubuntu 18.04 and ROS 2 bionic. The hd_monitor shows zero-values only, though, and ntp_monitor shows errors calling ntp_date.

ros2 run diagnostic_common_diagnostics cpu_monitor

header:
  stamp:
    sec: 72403
    nanosec: 324504098
  frame_id: ''
status:
- level: "\0"
  name: 'cpu_monitor_lubuntu: CPU Information'
  message: CPU Average 53.0 percent
  hardware_id: lubuntu
  values:
  - key: CPU 0 Load
    value: '54.5'
  - key: CPU 1 Load
    value: '51.5'

Two additional technical hints:

* IIRC, I had to install `uuid_dev` as a dependency, which could be indicated as dependency

* I had to install `gtest` and `gmock`, which I didn't have to do for other ROS 2 packages before. So it seems that it is not using the ROS 2 infrastructure in that regard

Can you please share logs for ntp monitor

@Karsten1987

This comment has been minimized.

Copy link

Karsten1987 commented Feb 5, 2019

@vaibhavbhadade can you share a bit about your current effort of repacking this PR? We would like to aim for diagnostics to be ready for the next ROS2 release and are willing to help to get this PR in. But obviously we would like to avoid duplication of work.

@vaibhavbhadade

This comment has been minimized.

Copy link
Author

vaibhavbhadade commented Feb 6, 2019

@Karsten1987 at present this code is working with all functionality provided in ROS 1, Its a matter of giving this code in small commits/ pull request for review. I am ready to give pull request at regular basis .
But Migration from ROS1 to ROS2 is not an only few code changes , its equal to writing new package. so we cant insure code compilation at initial changes commits. we can start from aggregator package with some initial commits PR which will have functionality . then we can move for other packages .
Also please let me know if you have any proposal to refactor this PR1 for review .

@Karsten1987

This comment has been minimized.

Copy link

Karsten1987 commented Feb 7, 2019

I think @trainman419 's proposal on discourse makes a lot of sense:

Renames and code changes in separate pull requests.
Separate pull requests for each package within the diagnostics repository.
If you have functional or structural changes that you’re making, let’s discuss those before we discuss the details of the code changes.

https://discourse.ros.org/t/diagnostic-aggregator-and-diagnostic-updater-porting-to-ros2/6382/15

I don't think it's necessary to guarantee full code functionality at each stage or with each individual PR, but as long as a chain of connected PRs is visible, a review should be straightforward. That being said, I would start with an initial PR which migrates the core of the diagnostic packages and put AMENT_IGNORE files in every dependent package. This allows that even though it's just the bare minimum, the migrated code compiles and CI can be triggered for each PR. Starting from there, each package individually can be added in a new PR.

@trainman419 would this make sense to you? If so, could you advise which package within this repo is the root and should be converted first? By just looking at the package.xml I feel that the diagnostic_updater could be a starting point as it's only depending on diagnostics_msgs.

@ralph-lange

This comment has been minimized.

Copy link

ralph-lange commented Feb 8, 2019

I’m a colleague of @Karsten1987 and @norro. I collected feedback from a business unit project that made intensive use of ROS 1 diagnostics. The project’s developers cope very well the current architecture, API and package organization and thus would appreciate if it is ported rather directly.

One exception is the diagnostics_agg message format: In their application, the diagnostics_agg messages are sent to a Cloud service over a low bandwitdh channel. Therefore, they propose to use a delta coding, i.e. to omit entries which did not change since the last diagnostics_agg message. In normal operation, the diagnostics_agg message would be almost empty and just serve as a heartbeat to indicate that diagnostics is working at all. This would be a larger change, of course. Yet, IMHO, the current status of the PR is a good point in time to discuss this idea. What do you think about it?

Also, the mentioned developers use the rqt_robot_monitor very much. Therefore, I propose to consider at least a basic ROS 2 command line tool for visualizing diagnostics_agg messages in the present undertaking.

A proposal to consider in the long-term are debouncing and healing mechanisms, i.e. the ability to define thresholds or temporal predicates (in the analyzers) when to consider a component/node as failed or successfully recovered.

Regarding making the PR manageable: I agree with @trainman419 and @Karsten1987. When looking at the current PR, I think reformatting is also an issue: @vaibhavbhadade, do you plan to stick with the current code style or switch to the ROS 2 style? In the latter case, I propose to first run ament_uncrustify on the ROS 1 code base and then do the PR against the reformatted code.

@vaibhavbhadade

This comment has been minimized.

Copy link
Author

vaibhavbhadade commented Feb 11, 2019

@ralph-lange We had already run ament_uncrustify on current PR and code is already formated in ROS2 style so I am planning to raised PR with reformatted code .

@ralph-lange

This comment has been minimized.

Copy link

ralph-lange commented Feb 11, 2019

@vaibhavbhadade That's good to hear. My idea was to first make a PR where the ROS 1 code is reformatted with ament_uncrustify without any other change. Then, I expect that the subsequent code changes for the port to ROS 2 are easier to review.

@trainman419

This comment has been minimized.

Copy link
Contributor

trainman419 commented Feb 11, 2019

rosdistro PR to enable CI for the ros2_devel branch: ros/rosdistro#20277

@trainman419

This comment has been minimized.

Copy link
Contributor

trainman419 commented Feb 11, 2019

@ralph-lange I'm happy to consider doing a delta coding of the /diagnotics_agg topic. I think that would be better done with a new topic, and I'm curious to know how such a delta coding would ensure that sync is maintained even if some messages are dropped.

I also like the idea of starting with a PR to reformat the existing code, and then submitting future PRs to upgrade individual packages or make specific large feature changes.

@vaibhavbhadade

This comment has been minimized.

Copy link
Author

vaibhavbhadade commented Feb 15, 2019

@ralph-lange I have tried to run ament_uncrustify on ROS1 code with modification in cmake file but i am not able to run test . I have no idea weather how to run this test without building code in ROS2 . Can you please guide us on this .

@ralph-lange

This comment has been minimized.

Copy link

ralph-lange commented Feb 15, 2019

@vaibhavbhadade I ran ament_uncrustify on the last commit before you started the port to ROS 2 (i.e. 1532c70 ) and I could reproduce the error. In detail, uncrustify failed at the gtest sources in the folders ./diagnostic_aggregator/gtest-1.7.0/ and ./self_test/gtest-1.7.0/.
When excluding those two folders, ament_uncrustify ran successfully. There was no need to touch the CMakeLists.txt files. See boschresearch@8e0c728

@Karsten1987

This comment has been minimized.

Copy link

Karsten1987 commented Mar 6, 2019

after talking offline with @vaibhavbhadade Bosch was willing to help preparing the migration of this package for easier review. We hereby stick to the following meta ticket, tracking the process: boschresearch#1

Apart from this PR being pretty big and currently having conflicting files, I believe the current state is not mergeable due to a mismatch of license agreements. The licenses were changed from a BSD clause, mentioning Willow Garage as an author to Apache2, OSRF which I believe is not easily doable without having permission from all authors of this package and the OSRF, respectively. I therefore believe it's best to close this PR - and for completeness also close #71 as it has been open for over one year now without activities.

thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.