eclipse-cyclonedds / cyclonedds-python

Other
54 stars 44 forks source link

Feature/initial cyclonedds insight #240

Closed trittsv closed 3 months ago

trittsv commented 3 months ago

Initial commit for the graphical tool CycloneDDS Insight to visualize the current DDS system. Python because in later features like dynamically add and test IDL-Files the app does not have to be recompiled. It can be shipped as one app for everyone. I am willing to provide future bug fixes and support for this tool.

@eboasson It is ready for review, i will fix all of your notes, feel free to comment πŸ˜ƒ If you prefer a different name for this tool then tell me and i will adjust.

Initial features:

... and even more feature are coming this year.

Example image (1/2):

insight-overview

Example image with qos (2/2)

insight-qos
eboasson commented 3 months ago

The beginnings of a GUI tool for Cyclone! πŸ₯³ I only tried it on macOS, but I assume it also works on Linux and Windows. I shouldn't assume such things, but for now that'll do.

There are very few comments I have on the details, there are really just three:

My Qt skills are non-existent and my python ones are insufficient to comment on the implementation details, other than that it looks well-organised and that I do understand it when I read it. So I do feel comfortable that in a pinch I can fix it, or (time permitting) add to it. That's all good.

The bits where I have my doubts are directly related to the GUI, which doesn't really matter that much at this stage. What I would like is to be able to also look at processes and machines β€”Β there are various views for structuring the system, after all. (What I would really like is also to draw a picture that immediately shows bandwidth used, but that data is not easily available today.) When looking at the readers/writers, I think the choice of a tooltip doesn't work so well; a (collapsible) "details" pane would probably work better (and would perhaps allow linking things so you can click through the system). Oh well, there are a thousand-and-one additional things one can wish for in a tool.

For the things mentioned, I can see how you can add that in the structure although I don't have a good sense of how much work it would be to replace the tooltips by a pane, or to have multiple views of the system at hand. I suspect it gets a bit verbose in maintaining the tree view, but that may well be perfectly normal for this type of application. (When did I last write a GUI application? 1998 or so? That was with XForms on X11 ... πŸ§‘β€πŸ¦³)

As for the name, "Insight" sounds vaguely familiar β€” I think my employer once had a tool with that name, but that can't be an issue when this is prefixed with "Cyclone DDS". It may be a bit dull but I can't think of something catchier β€”Β probe, visualise, inspect, discover, analyze, x-ray, monitor, observe ... all equally dull ...

Finally, I am wondering if it makes the most sense to have this inside the python binding's repository. It would probably be nicer to have a separate repository for it. That's easy enough to arrange (but then changing its name becomes harder).

Do let me know what you think of my 3 minor review comments πŸ™‚

trittsv commented 3 months ago

Thanks a lot for the review @eboasson There are so many ideas for this tool, it would probably make sense to document them somewhere.

Question 1:

I'd try using a waitset instead of sleeping in the observer thread reading the built-in topics

i agree, i will fix it. This was laziness from my side by coping blindly from cli-tool.

Question 2:

I'd reconsider using a bool argument to indicate whether something is a reader or a writer

i agree, i will fix it, its better to have a enum.

Question 3:

I'd consistently use "reader" and "writer" instead of "subscriber" and "publisher". There is nothing wrong with the latter except in the universe of DDS, and it matters because this a tool for DDS.

i agree, i will fix it. Its better to be clear throughout the code on the terms.

General 1:

also look at processes and machines

I agree, the tool should support multiple views on how to look on a DDS system. Its relatively easy to add this, but it sounds like it could be a separate Pull-Request. I started with this view because in my daily life using DDS this is most of the time all i need (looking at topics and see its data and don't care from which application on which host it is).

General 2:

I think the choice of a tooltip doesn't work so well

I agree on that, i had the same thoughts. Then immediately thousands of ways to do it came into my mind - so i came to the conclusion, this can be a separate Feature/Pull-Requets and keep it simple for now. But i will have a look into it if i can easily make it collapsible.

General 3:

It would probably be nicer to have a separate repository for it

Yes 100%, a separate repository would be the best. We could make separate releases and don't have to wait for a CycloneDDS release. Could have its own builds jobs, artifacts, issues, feature requests, etc ... And it would increase visibility for the user that there is such a tool - even for users who aren't using python bindings in production.

General 4:

I think my employer once had a tool with that name,

When there already is a DDS related tool with the name Insight we can rename, its your decision. Other suggestions: Explorer, Navigator, Visualizer, Observer ... I tried to avoid tool names from other DDS-Vendors: Monitor, Admin-Console, Inspector

trittsv commented 3 months ago

@eboasson your three questions are implemented πŸ˜ƒ You can have a look again.

In addition to my previous answer: Here are screenshots from ubuntu 22 and windows 10. So it works on all major platforms.

Windows: insight-windows

Ubuntu 22: insight-ubuntu22

eboasson commented 3 months ago

Great!

Now if you could make the PR against https://github.com/eclipse-cyclonedds/cyclonedds-insight instead (with the files moved some levels up in the directory hierarchy). I won't merge it here, but it will merge it there πŸ˜€

trittsv commented 3 months ago

@eboasson great 🀩 , i made the PR against the cyclonedds-insight repo βœ