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

Refactor logging #543

Merged
merged 13 commits into from
Oct 14, 2024
Merged

Refactor logging #543

merged 13 commits into from
Oct 14, 2024

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented Jul 23, 2024

I had issues with writing tests for the demos, and one cause might be an overload of logging in the hardware components.

Since ros-controls/ros2_control#1585 we have logger+clock in the interfaces, so I refactored the hardware components and use throttled logging on top of that.

We encountered an issue with throttled logging from identical plugins ros2/rclcpp#2587, this is why logging in example_6 does not work currently -> I'll open a follow-up PR to remember this.

Note: This is not 1:1 backportable because the API changes were not backported. But this can be achieved by adding local member variables for clock and logger to the hardware components.

Copy link
Contributor

mergify bot commented Aug 5, 2024

This pull request is in conflict. Could you fix it @christophfroehlich?

@christophfroehlich
Copy link
Contributor Author

I reverted the changes to the update rates as suggested here

Copy link
Contributor

mergify bot commented Oct 8, 2024

This pull request is in conflict. Could you fix it @christophfroehlich?

Copy link
Contributor

@fmauch fmauch left a comment

Choose a reason for hiding this comment

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

As written in some examples: There currently seems to be an issue with INT vs DOUBLE datatypes with the test_forward_position_controller.launch.py launches in all examples, as a setpoint of [0, 0] should be [0.0, 0.0]. Should we take this to a new issue instead of fixing it here?

example_1/doc/userdoc.rst Outdated Show resolved Hide resolved
example_14/hardware/rrbot_actuator_without_feedback.cpp Outdated Show resolved Hide resolved
example_14/hardware/rrbot_sensor_for_position_feedback.cpp Outdated Show resolved Hide resolved
example_2/doc/userdoc.rst Outdated Show resolved Hide resolved
example_14/doc/userdoc.rst Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Is example 3 missing a doc update? Also: the example output there is only valid when the user switches to the forward_position_controller either by providing it as a launch argument or by switching the controller during runtime. Maybe, we should migrate the output example to the velocity example?

Additionally: The test_forward_position_controller.launch.py crashed for me:

[publisher_forward_position_controller-1] rclpy.exceptions.InvalidParameterTypeException: Trying to set parameter 'pos2' to '[0, 0]' of type 'INTEGER_ARRAY', expecting type 'DOUBLE_ARRAY': pos2

Updating the rrbot_forward_position_publisher.yaml file to

publisher_forward_position_controller:
  ros__parameters:

    wait_sec_between_publish: 5
    publish_topic: "/forward_position_controller/commands"

    goal_names: ["pos1", "pos2", "pos3", "pos4"]
    pos1: [0.785, 0.785]
    pos2: [0.0, 0.0]
    pos3: [-0.785, -0.785]
    pos4: [0.0, 0.0]

did solve that for me. This could probably be taken to a separate issue, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

example_4/doc/userdoc.rst Outdated Show resolved Hide resolved
example_5/doc/userdoc.rst Outdated Show resolved Hide resolved
example_6/doc/userdoc.rst Outdated Show resolved Hide resolved
christophfroehlich and others added 2 commits October 13, 2024 22:01
Co-authored-by: Felix Exner (fexner) <felix_mauch@web.de>
Co-authored-by: Felix Exner (fexner) <felix_mauch@web.de>
Copy link
Contributor

@fmauch fmauch left a comment

Choose a reason for hiding this comment

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

Without the throttling, output of example 14 looks a lot better!

I have just one minor code suggestion in two places to actually get the output as printed in the docs. Apart from that, this looks fine to me.

example_14/hardware/rrbot_actuator_without_feedback.cpp Outdated Show resolved Hide resolved
example_14/hardware/rrbot_sensor_for_position_feedback.cpp Outdated Show resolved Hide resolved
Co-authored-by: Felix Exner (fexner) <felix_mauch@web.de>
Copy link
Contributor

@fmauch fmauch left a comment

Choose a reason for hiding this comment

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

This is looking good now!

@christophfroehlich christophfroehlich merged commit 1596b79 into master Oct 14, 2024
11 of 15 checks passed
@christophfroehlich christophfroehlich deleted the use_logger_var branch October 14, 2024 05:44
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

LGTM

christophfroehlich added a commit that referenced this pull request Nov 2, 2024
* Refactor logging for example 1 - 6

* Update logger for remaining examples

* Revert change to copyright

* Revert "Reduce update rate to avoid clogging the log"

This reverts commit 10f2e22.

* Don't use throttling for example_6

* Update example_1/doc/userdoc.rst

Co-authored-by: Felix Exner (fexner) <felix_mauch@web.de>

* Update example_2/doc/userdoc.rst

Co-authored-by: Felix Exner (fexner) <felix_mauch@web.de>

* Updating docs

* Merge into one logging statement

* Remove throttled logging from example_14

* Apply suggestions from code review

Co-authored-by: Felix Exner (fexner) <felix_mauch@web.de>

---------

Co-authored-by: Felix Exner (fexner) <felix_mauch@web.de>
christophfroehlich added a commit that referenced this pull request Nov 13, 2024
mergify bot pushed a commit that referenced this pull request Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants