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

ROS1 hd_monitor.py issue on noetic #185

Closed
chrisflesher opened this issue Feb 12, 2021 · 11 comments
Closed

ROS1 hd_monitor.py issue on noetic #185

chrisflesher opened this issue Feb 12, 2021 · 11 comments

Comments

@chrisflesher
Copy link

chrisflesher commented Feb 12, 2021

Am getting the following error after running hd_monitor.py in noetic on Ubuntu 20.04 (pulling from latest apt):

Traceback (most recent call last):
  File "/opt/ros/noetic/lib/diagnostic_common_diagnostics/hd_monitor.py", line 361, in <module>
    hostname_clean = string.translate(hostname, string.maketrans('-','_'))
AttributeError: module 'string' has no attribute 'translate'
@chrisflesher
Copy link
Author

chrisflesher commented Feb 12, 2021

Also am getting the following error after running sensors_monitor.py:

Traceback (most recent call last):
  File "/opt/ros/noetic/lib/diagnostic_common_diagnostics/sensors_monitor.py", line 46, in <module>
    from StringIO import StringIO
ModuleNotFoundError: No module named 'StringIO'

@chrisflesher chrisflesher changed the title ROS1 hd_monitor.py fails on noetic ROS1 hd_monitor.py issue on noetic Feb 12, 2021
@g-gemignani
Copy link
Collaborator

Hi @chrisflesher and thank you for your message.
It looks like those nodes were never completely ported to python 3. I quickly did it. Can you please double-check that they are now working? You can find the fixes in https://github.com/ros/diagnostics/tree/noetic/fix-diagnostic-common-diagnostics
If it works for you I'll take care of creating a new release :)

@chrisflesher
Copy link
Author

chrisflesher commented Feb 13, 2021

Thanks! I was able to get hd_monitor.py working after making the following fix:

diff --git a/diagnostic_common_diagnostics/src/diagnostic_common_diagnostics/sensors_monitor.py b/diagnostic_common_diagnostics/src/diagnostic_common_diagnostics/sensors_monitor.py
index e26fc8d..ffebb65 100755
--- a/diagnostic_common_diagnostics/src/diagnostic_common_diagnostics/sensors_monitor.py
+++ b/diagnostic_common_diagnostics/src/diagnostic_common_diagnostics/sensors_monitor.py
@@ -150,7 +150,7 @@ def _rpm_to_rads(rpm):
     return rpm * (2 * math.pi) / 60
 
 def parse_sensors_output(output):
-    out = StringIO(output)
+    out = StringIO(output.decode('utf-8'))
 
     sensorList = []
     for line in out.readlines():

I tried fixing sensors_monitor.py but found that code may need a significant rewrite for the parse_sensor_line function to be robust. Its crashing for multiple reasons with the following sensors output:

BAT0-acpi-0
Adapter: ACPI interface
in0:          10.00 V  

@chrisflesher
Copy link
Author

chrisflesher commented Feb 13, 2021

I think I'd be pretty happy with the fix for hd_monitor.py now.

Maybe we could try to fix sensors_monitor.py as a separate issue? Would setting up some unit tests be helpful? I wouldn't mind taking a crack at that if I get some time to work on it.

@chrisflesher
Copy link
Author

The sensors issue seems like a duplicate of #69

@g-gemignani
Copy link
Collaborator

Hi @chrisflesher, you are right, there was still an issue with sensors_monitor.py. Can you try now?

@chrisflesher
Copy link
Author

chrisflesher commented Feb 18, 2021

Yes sensors_monitor.py seems to run now. Thanks!

@chrisflesher
Copy link
Author

A new release of this would be great. I have only tested it on a VM, could test on actual hardware next week and report results if that would be helpful.

@g-gemignani
Copy link
Collaborator

We are in no rush. Let's wait for your additional test and then I take care of merging and releasing. Thank you!

@g-gemignani
Copy link
Collaborator

@chrisflesher any update on this?

@g-gemignani
Copy link
Collaborator

Merged to noetic-devel. Closing the issue

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

No branches or pull requests

2 participants