Ros2cli diagnostics #301

robobe · 2023-04-26T18:27:57Z

Add ros2cli for diagnostics as alternative for diagnostics_analysis
The project add diagnostics command to ROS2 cli with there verbs.

list
show
csv

check README.md for more

csv --output

ros2diagnostics_cli/package.xml

ct2034

Hi @robobe. Thanks for your PR. This looks amazing. 😁
I hope I will find the time for a proper review soon. Just something I noticed:

All source files are missing copyright headers
Please update the package.xml and do what else is required for the buildfarm to run.

Co-authored-by: Christian Henkel <6976069+ct2034@users.noreply.github.com>

remove math case remove some of pylint warnings

todo: add regex for extract node name from diagnostic name

ct2034

Please move the work on diagnostics_tutorial to a separate PR, such that I can review the ros2diagnostics_cli separately. And if you are still working on this, please use the draft PR feature. Otherwise, this creates a lot of overhead. Some points:

Please add your package under https://github.com/robobe/diagnostics/blob/98b78aaa544b5ad06b4810d61bd5394b425f2013/.github/workflows/lint.yaml#L38-L39 and https://github.com/robobe/diagnostics/blob/98b78aaa544b5ad06b4810d61bd5394b425f2013/.github/workflows/test.yaml#L19-L20 so that it is checked in the github ci.
The package.xml needs some fields filled. The version must be the same as the other packages in this repo. You can add me as a maintainer and yourself as author if you like.
Similar things apply to the setup.py
I am not quite sure what the two verbs show and list do. Both seem to have a functionality that I would call echo at least when compared to how the topic verb does things.
What does the world verb do? It does not seem to be documented.

In general this seems very work in progress. Please make this a draft PR, then.

ct2034 · 2023-05-04T09:42:25Z

I was able to make it into a draft myself, nvm. Just click Ready for review when you are. Or let me know when you need help. I generally suggest to focus on one or two well-implemented verbs and extend on it later.

todo: add regex for extract node name from diagnostic name

…2cli_diagnostics

Signed-off-by: Christian Henkel <christian.henkel2@de.bosch.com>

ct2034

Can you please describe the intended use case for the list and show verb? I feel like there is an overlap. In my opinion, two verbs are required:
list - it lists all available diagnostics sources and the exits
echo - it will run forever and echo all the diagnostics info on a specific source.
Basically similar to how theses two verbs work for the topic command right now

ct2034 · 2023-05-17T12:02:05Z

ros2diagnostics_cli/ros2diagnostics_cli/api/__init__.py

+    base_dir = pathlib.Path(csv_file).parents[0]
+    folder_exists = pathlib.Path(base_dir).is_dir()
+    if not folder_exists:
+        raise Exception(


Please use a more specific exception type

ct2034 · 2023-05-17T12:03:53Z

ros2diagnostics_cli/ros2diagnostics_cli/api/__init__.py

+    b'\x00': ('OK', GREEN + 'OK' + COLOR_DEFAULT),
+    b'\x01': ('WARN', YELLOW + 'WARN' + COLOR_DEFAULT),
+    b'\x02': ('ERROR', RED + 'ERROR' + COLOR_DEFAULT),
+    b'\x03': ('STALE', RED + 'STALE' + COLOR_DEFAULT),


Please use the constants declared in dagnostic_msgs.msg.DiagnosticStatus

And you can also use f-strings here

ct2034 · 2023-05-17T12:14:41Z

ros2diagnostics_cli/package.xml

+  <test_depend>ament_flake8</test_depend>
+  <!-- <test_depend>ament_pep257</test_depend> -->
+  <test_depend>python3-pytest</test_depend>
+  <depend>ros2cli</depend>


All required packages from this repo must be declared here

ct2034 · 2023-05-17T12:20:58Z

ros2diagnostics_cli/ros2diagnostics_cli/verb/csv.py

+            try:
+                self.csv = open_file_for_output(args.output)
+            except Exception as error:
+                print(str(error))


Suggested change

print(str(error))

print(error)

ct2034 · 2023-05-17T12:21:25Z

ros2diagnostics_cli/ros2diagnostics_cli/verb/csv.py

+        ]
+        if verbose:
+            kv: KeyValue
+            for kv in status.values:


Suggested change

for kv in status.values:

line.extend(kv.value for kv in status.values)

ct2034 · 2023-05-17T12:22:19Z

ros2diagnostics_cli/ros2diagnostics_cli/verb/csv.py

+        )
+
+    def render(self, status: DiagnosticStatus, time_sec, verbose=False):
+        level_name, _ = convert_level_to_str(status.level)


I don't see how this is imported

ct2034 · 2023-05-17T12:22:56Z

ros2diagnostics_cli/ros2diagnostics_cli/api/__init__.py

+        if not self.__levels:
+            return False


Suggested change

if not self.__levels:

return False

return False if not self.__levels else level not in self.__levels

ct2034 · 2023-05-17T12:23:28Z

ros2diagnostics_cli/ros2diagnostics_cli/api/__init__.py

+
+def add_common_arguments(parser: ArgumentParser):
+    """Add common arguments for csv and show verbs."""
+    parser.add_argument('-1', '--once', action='store_true', help='run only once')


Suggested change

parser.add_argument('-1', '--once', action='store_true', help='run only once')

parser.add_argument(

'-1',

'--once',

action='store_true',

help='run only once')

ct2034 · 2023-05-17T12:23:53Z

ros2diagnostics_cli/ros2diagnostics_cli/api/__init__.py

+    """Add common arguments for csv and show verbs."""
+    parser.add_argument('-1', '--once', action='store_true', help='run only once')
+    parser.add_argument(
+        '-f', '--filter', type=str, help='filter diagnostic status name'


Suggested change

'-f', '--filter', type=str, help='filter diagnostic status name'

'-f',

'--filter',

type=str,

help='filter diagnostic status name'

russkel · 2023-07-31T01:03:31Z

This looks useful - has there been movement on this?

russkel · 2023-09-19T03:16:23Z

@robobe do you mind if I take over this PR if you're not interested/don't have the time?

ct2034 · 2023-09-26T13:02:42Z

Hi @russkel, thanks for your interest. Since @robobe is fairly unresponsive for quite some time, please feel free to take this over.

russkel · 2023-09-27T08:46:29Z

Hi @ct2034 will do when I get some time.

russkel · 2023-10-07T01:40:04Z

Hi @ct2034

I have begun the work: https://github.com/russkel/diagnostics/tree/ros2cli_diagnostics

I have addressed your suggestions and am going to do the following:

rename to ros2diagnostics to follow convention
as per your suggestion change verbs to list/echo.
convert csv to an output format option on echo: ros2 diagnostics echo -f csv > my-csv.csv (unix philosophy)
refactor DiagnosticsParser into a generator with a WaitSet and remove mode
add self test verb
add tests

Are you able to change the repo of this PR or should I create a new one?

ct2034 · 2023-12-12T16:11:50Z

Thanks for your work @russkel. I can not edit the branch this PR is from. Please create a new one. Than I can close this one

robo added 6 commits April 22, 2023 22:57

ros2diagnostics cli init

0262e0b

rename hello to diagnostics

f177bed

add option with string

c2a4669

csv --output

arg parse examples

6c7269a

add csv verb

871095d

version 0.0.1

e9f6803

ct2034 reviewed Apr 26, 2023

View reviewed changes

ros2diagnostics_cli/package.xml Outdated Show resolved Hide resolved

ct2034 requested changes Apr 26, 2023

View reviewed changes

ct2034 self-assigned this Apr 26, 2023

robobe and others added 10 commits April 26, 2023 22:29

Update ros2diagnostics_cli/package.xml

c4fd102

Co-authored-by: Christian Henkel <6976069+ct2034@users.noreply.github.com>

clean test and scripts file

c4175a7

add copyright

2b13ff7

fix: support python 3.8 remove match

8124d08

format and run linters

064d3f3

fix: suport python 3.8

c4c08f0

remove math case remove some of pylint warnings

fix flake8 errors

d57f8c8

init tutorials

deba6b3

fix: handle name without clone task name separator

2e23d8b

fix: extract node name

98b78aa

todo: add regex for extract node name from diagnostic name

ct2034 requested changes May 4, 2023

View reviewed changes

ct2034 marked this pull request as draft May 4, 2023 09:40

robo and others added 8 commits May 4, 2023 21:01

fix: handle name without clone task name separator

feacb0d

fix: extract node name

c77d4e4

todo: add regex for extract node name from diagnostic name

set version and license

ddfba3b

Merge branch 'ros2cli_diagnostics' of dia:robobe/diagnostics into ros…

e44dbbf

…2cli_diagnostics

remove diagnostic_tutorial from PR

82cc52b

Merge branch 'ros2' into pr/robobe/301

64de761

gh tests for the new package

b8ce45d

Signed-off-by: Christian Henkel <christian.henkel2@de.bosch.com>

fixing flake8 errors

e6a8956

Signed-off-by: Christian Henkel <christian.henkel2@de.bosch.com>

fixing pep257 errors

c5bf964

Signed-off-by: Christian Henkel <christian.henkel2@de.bosch.com>

ct2034 requested changes May 17, 2023

View reviewed changes

ct2034 added enhancement This tackles a new feature of the code (and not a bug) ros2 PR tackling a ROS2 branch needs more work Someone has worked on this but more work is needed labels Sep 26, 2023

russkel mentioned this pull request Jan 21, 2024

Ros2cli diagnostics tool #328

Draft

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ros2cli diagnostics #301

Ros2cli diagnostics #301

robobe commented Apr 26, 2023

ct2034 left a comment •

edited

Loading

ct2034 left a comment

ct2034 commented May 4, 2023

ct2034 left a comment

ct2034 May 17, 2023

ct2034 May 17, 2023

ct2034 May 17, 2023

ct2034 May 17, 2023

ct2034 May 17, 2023

ct2034 May 17, 2023

ct2034 May 17, 2023

ct2034 May 17, 2023

ct2034 May 17, 2023

ct2034 May 17, 2023

russkel commented Jul 31, 2023

russkel commented Sep 19, 2023 •

edited

Loading

ct2034 commented Sep 26, 2023

russkel commented Sep 27, 2023

russkel commented Oct 7, 2023 •

edited

Loading

ct2034 commented Dec 12, 2023

	for kv in status.values:
	line.extend(kv.value for kv in status.values)

	if not self.__levels:
	return False
	return False if not self.__levels else level not in self.__levels

Ros2cli diagnostics #301

Are you sure you want to change the base?

Ros2cli diagnostics #301

Conversation

robobe commented Apr 26, 2023

ct2034 left a comment • edited Loading

Choose a reason for hiding this comment

ct2034 left a comment

Choose a reason for hiding this comment

ct2034 commented May 4, 2023

ct2034 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russkel commented Jul 31, 2023

russkel commented Sep 19, 2023 • edited Loading

ct2034 commented Sep 26, 2023

russkel commented Sep 27, 2023

russkel commented Oct 7, 2023 • edited Loading

ct2034 commented Dec 12, 2023

ct2034 left a comment •

edited

Loading

russkel commented Sep 19, 2023 •

edited

Loading

russkel commented Oct 7, 2023 •

edited

Loading