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

Port hd_monitor to ROS2 #334

Open
wants to merge 4 commits into
base: ros2
Choose a base branch
from
Open

Conversation

limaanto
Copy link

As promised in #319 (though it took quite a while..), here is a ported version of the HD monitor script.

After the discussions of #319, I decided to :

  • drop the temperature check that was not deemed mandatory and would have required significant work
  • use a widely available and windows-compatible tool for the usage check, and shutil.disk_usage which comes with all python installs seemed like a good choice
  • Introduce a new parameter path to configure where the check is ran on. By default this is the home directory but can be changed with a ros param
  • Use only the local hostname as the device_id

I did not take the time to write the test yet, let me know if this is mandatory for merging (or if there is any other point for that matter)

@ct2034
Copy link
Collaborator

ct2034 commented Mar 28, 2024

Thanks for your contribution @limaanto :)
Please add also a test. But you should be able to adopt https://github.com/ros/diagnostics/blob/ros2/diagnostic_common_diagnostics/test/systemtest/test_ntp_monitor_launchtest.py directly.

@ct2034 ct2034 added ros2 PR tackling a ROS2 branch needs more work labels Mar 28, 2024
@limaanto
Copy link
Author

Hi, I rebased the branch on ros2 and implemented a successful launch test (based on the recommended ntp_monitor's)
Tell me if I'm missing something 😃

@ct2034 ct2034 self-requested a review May 13, 2024 13:09
@ct2034 ct2034 self-assigned this May 13, 2024
Comment on lines +51 to +52
FREE_PERCENT_LOW = 0.05
FREE_PERCENT_CRIT = 0.01
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be configurable

Comment on lines +98 to +101
KeyValue(key=f'Name', value=self._path),
KeyValue(key=f'Status', value=DICT_STATUS[diag.level]),
KeyValue(key=f'Total (Go)', value=str(total_Go)),
KeyValue(key=f'Available (%)', value=str(round(percent, 2))),
Copy link
Collaborator

Choose a reason for hiding this comment

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

These don't have to be f-strings

])

diag.message = DICT_USAGE[diag.level]
print(diag)
Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove this

def check_disk_usage(self, diag: DiagnosticStatus) -> DiagnosticStatus:
diag.level = DiagnosticStatus.OK

total, used, free = disk_usage(self._path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
total, used, free = disk_usage(self._path)
total, _, free = disk_usage(self._path)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more work ros2 PR tackling a ROS2 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants