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

[CM] Add more logging for easier debugging #1645

Merged

Conversation

saikishor
Copy link
Member

Hello!

In our tests, we found it difficult with some missing important logs in some places. It would be really interesting to have them upstream.

  • When the hardware returns ERROR in the read and write cycle, it is deactivated and the controllers that use the interfaces are also deactivated but no log is printed, so checking always with the CLI is a bit cumbersome.
  • It is also interesting to print the activated and deactivated controllers in the switch cycles in non-RT thread
  • Printing what controller and type is being loaded in the controller manager helps.

This is needed because when we communicate from some applications without using the spawner it would be interesting to know from the logs.

The following are the prints reported from the tests:

[ERROR] [1722329073.729615011] [test_controller_manager]: Deactivating following hardware components as their read cycle resulted in an error : [ TestActuatorHardware TestSystemHardware ]
[ERROR] [1722329073.729679693] [test_controller_manager]: Deactivating following controllers as their hardware components read cycle resulted in an error : [ test_broadcaster_all test_controller_actuator test_broadcaster_all test_controller_system ]
[INFO] [1722329183.338697199] [foo_namespace.test_controller_manager]: Activating controllers: [ ctrl_2 ctrl_3 ]
[INFO] [1722329184.450285285] [foo_namespace.test_controller_manager]: Deactivating controllers: [ ctrl_1 ctrl_2 ctrl_3 ]
[INFO] [1722329149.544712217] [test_controller_manager]: Loading controller : 'ctrl_1' of type 'controller_manager/test_controller'
[INFO] [1722329149.966687923] [test_controller_manager]: Configuring controller : 'ctrl_1'
[INFO] [1722329183.333766888] [test_controller_manager]: Unloading controller : 'ctrl_1'

Copy link

codecov bot commented Jul 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.07%. Comparing base (a1ad523) to head (858780a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1645   +/-   ##
=======================================
  Coverage   88.07%   88.07%           
=======================================
  Files         108      108           
  Lines       10023    10049   +26     
  Branches      892      894    +2     
=======================================
+ Hits         8828     8851   +23     
- Misses        876      878    +2     
- Partials      319      320    +1     
Flag Coverage Δ
unittests 88.07% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
controller_manager/src/controller_manager.cpp 77.80% <100.00%> (+0.25%) ⬆️

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

minor comments. LGTM, thanks

controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
bmagyar
bmagyar previously approved these changes Aug 9, 2024
Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

generally looks ok, just one question about the string concatenation

Copy link
Member Author

@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.

@bmagyar How about something like this? Reserving some memory ahead it is better right?. Any other suggestions are welcome

@christophfroehlich
Copy link
Contributor

@bmagyar How about something like this? Reserving some memory ahead it is better right?. Any other suggestions are welcome

don't you also need .append() instead of the plus operator to avoid temporary copies?

controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

Why not applying the same logic to the activate/deactivate list?

controller_manager/src/controller_manager.cpp Show resolved Hide resolved
controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
@saikishor
Copy link
Member Author

Well as they are in the non-RT thread, I thought it might not be needed 😅😅

Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
@christophfroehlich
Copy link
Contributor

Well as they are in the non-RT thread, I thought it might not be needed 😅😅

OK this is an argument, haven't checked this but only the diff in the UI 😇

@saikishor
Copy link
Member Author

OK this is an argument, haven't checked this but only the diff in the UI 😇

Applied your suggestions :)

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Thank you!

@bmagyar bmagyar merged commit 4919aba into ros-controls:master Aug 14, 2024
19 checks passed
@saikishor saikishor deleted the add/more/introspection/logging branch August 17, 2024 08:19
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