Add iCubCluster functionality to yarpmanager #1306

Nicogene · 2017-07-31T12:29:48Z

This PR import iCubCluster inside yarpmanager that looks like:

It has been successfully tested on iCubGenova01 setup and it has (for now) the same functionalities of icub-cluster.py.

on WIN32 the iCubCluster panel is for now disabled (sorry @randaz81 , @pattacini and @lornat75 😅 ).

@traversaro you owe me a beer 😛.

We should discuss about the cross dependency with icub-main( it needs only the cluster-config.xml) and about the deployer for windows.
We could keep it as it is for now and try to figure out later how to launch commands from and to windows machines.

@claudiofantacci @barbalberto @vtikha

Fixes #730.

TODO before merging:

Implement the More... button, maybe in a smarter way...
Make ui nicer
Fix the cross dipendence with iCub..
Fix codacy

Nicogene · 2017-08-01T15:12:18Z

Sorry guys for spam, I tried to make codacy happy but every push it finds something new

diegoferigo · 2017-08-01T15:30:22Z

I've been busier that usual recently and the work slowed down a bit, but most of codacy warnings can be addressed automatically by setting up any clang-format.

barbalberto · 2017-08-01T15:38:57Z

It is fine if there are a lot of inaccuracy in the code. The tool is there to catch them all for you.
My point was simply: can we have a single mail with all the messages?
Or even better: a single mail with "codacy found some issue, go on github and check them out"
Too many mail are just spam.

claudiofantacci · 2017-08-01T15:47:28Z

@barbalberto that is a valid point! There should be somewhere in settings a way to stop emails. Let me check!

claudiofantacci · 2017-08-01T15:56:05Z

Ok, according to Codacy doc, this kind of option is on GitHub side, under Webhook and Services. I can't do anything more as I'm not enabled to change settings on robotology.
@drdanz should look into this.

Anyway, now let's go back on track with the PR :-)

drdanz · 2017-08-02T07:10:38Z

I wonder why it says "drdanz deleted a comment from codacy-bot", I didn't delete anything 😮

drdanz

LGTM, great addition, but please change all the methods passing a std::string by value to passing by const reference

drdanz

Now for the nitpicks...

drdanz · 2017-08-02T07:14:20Z

src/libYARP_manager/include/yarp/manager/xmlclusterloader.h

+};
+
+} // namespace yarp
+} // namespace manager


This is actually the opposite, i.e.

} // namespace manager } // namespace yarp

drdanz · 2017-08-02T07:14:51Z

src/libYARP_manager/include/yarp/manager/xmlclusterloader.h

+} // namespace manager
+
+
+#endif // __YARP_MANAGER_XMLCLUSTERLOADER_


The comment here is wrong

drdanz · 2017-08-02T07:15:41Z

src/libYARP_manager/src/xmlclusterloader.cpp

+#include <yarp/os/Value.h>
+#ifdef WITH_GEOMETRY
+#include <yarp/os/Property.h> // for parsing geometry information
+#endif


what is this WITH_GEOMETRY used for? I cannot find it anywhere else.

drdanz · 2017-08-02T07:16:47Z

src/libYARP_manager/include/yarp/manager/xmlclusterloader.h

+ */
+
+
+#ifndef YARP_MANAGER_XMLCLUSTERLOADER


According to our policies this should be YARP_MANAGER_XMLCLUSTERLOADER_H

drdanz · 2017-08-02T07:21:12Z

src/libYARP_manager/include/yarp/manager/xmlclusterloader.h

+namespace yarp {
+namespace manager {
+
+struct ClusNode


I think that saving the ter from cluster here is not really worth, I'd rename it ClusterNode

drdanz · 2017-08-02T07:31:57Z

I changed the settings, codacy should now just send one message with a summary.
I'll try it for a while, but to be honest I don't see a good reason for not having the comments on the pull request, so if I don't like the new setting I'll switch back! 💪

randaz81 · 2017-08-02T08:24:10Z

"Type the command you wish to execute:" ...
super! what is the syntax? can I execute the command on a generic machine (one of the machines listed above?) It could be useful to start roscore, if needed.

About the --ros option: Can you add an additional green tick to show if the nameserver has been started with --ros option? (just checking if a /ros port is active, I think..)

Nicogene · 2017-08-02T08:39:16Z

"Type the command you wish to execute:" was already implemented also on the "old" cluster, in this one you can execute a command via ssh -f on the nodes you selected in the list.

The idea was to additionally check that the /ros port is up if --ros is checked when you check if the nameserver is up, but it is more difficult than expected. NetworkBase::exists() sends to the port an admin rpc cmd ver, the /ros port that it is not a yarp port throws an exception: Response: dict (faultCode 1) (faultString "<type 'exceptions.Exception'>:method \"ver\" is not supported").
For now I think we will merge without the ros check and will add it later as improvement

drdanz · 2017-08-02T09:41:58Z

Merged, thanks!

Nicogene requested review from drdanz, traversaro and apaikan July 31, 2017 12:29

robotology deleted a comment Jul 31, 2017

Nicogene force-pushed the ImportICubCluster branch from 70d9572 to cf7e1f2 Compare August 1, 2017 15:09

robotology deleted a comment Aug 1, 2017

drdanz changed the title ~~Import iCubCluster~~ Add iCubCluster functionality to yarpmanager Aug 2, 2017

drdanz removed the Issue Type: Feat/Enh Req This issue requests some new feature or enhancement label Aug 2, 2017

drdanz suggested changes Aug 2, 2017

View reviewed changes

Nicogene added 2 commits August 2, 2017 10:08

clusterWidget: add button disable if nothing is selected

295d437

Update v2_3_72.md

1c41b23

Nicogene force-pushed the ImportICubCluster branch from cf7e1f2 to 1c41b23 Compare August 2, 2017 08:09

Nicogene changed the title ~~Add iCubCluster functionality to yarpmanager~~ [WIP] Add iCubCluster functionality to yarpmanager Aug 2, 2017

Nicogene changed the title ~~[WIP] Add iCubCluster functionality to yarpmanager~~ Add iCubCluster functionality to yarpmanager Aug 2, 2017

drdanz merged commit 208cc3c into robotology:devel Aug 2, 2017

drdanz added the Resolution: Merged label Aug 2, 2017

Nicogene deleted the ImportICubCluster branch August 2, 2017 09:44

Nicogene mentioned this pull request Aug 2, 2017

icub-cluster in yarpmanager #730

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add iCubCluster functionality to yarpmanager #1306

Add iCubCluster functionality to yarpmanager #1306

Nicogene commented Jul 31, 2017 •

edited

Loading

Nicogene commented Aug 1, 2017

diegoferigo commented Aug 1, 2017

barbalberto commented Aug 1, 2017 •

edited

Loading

claudiofantacci commented Aug 1, 2017

claudiofantacci commented Aug 1, 2017

drdanz commented Aug 2, 2017

drdanz left a comment

drdanz left a comment

drdanz Aug 2, 2017

drdanz Aug 2, 2017

drdanz Aug 2, 2017

drdanz Aug 2, 2017

drdanz Aug 2, 2017

drdanz commented Aug 2, 2017

randaz81 commented Aug 2, 2017

Nicogene commented Aug 2, 2017 •

edited

Loading

drdanz commented Aug 2, 2017

		} // namespace manager


		#endif // __YARP_MANAGER_XMLCLUSTERLOADER_

		*/


		#ifndef YARP_MANAGER_XMLCLUSTERLOADER

Add iCubCluster functionality to yarpmanager #1306

Add iCubCluster functionality to yarpmanager #1306

Conversation

Nicogene commented Jul 31, 2017 • edited Loading

Nicogene commented Aug 1, 2017

diegoferigo commented Aug 1, 2017

barbalberto commented Aug 1, 2017 • edited Loading

claudiofantacci commented Aug 1, 2017

claudiofantacci commented Aug 1, 2017

drdanz commented Aug 2, 2017

drdanz left a comment

Choose a reason for hiding this comment

drdanz left a comment

Choose a reason for hiding this comment

drdanz Aug 2, 2017

Choose a reason for hiding this comment

drdanz Aug 2, 2017

Choose a reason for hiding this comment

drdanz Aug 2, 2017

Choose a reason for hiding this comment

drdanz Aug 2, 2017

Choose a reason for hiding this comment

drdanz Aug 2, 2017

Choose a reason for hiding this comment

drdanz commented Aug 2, 2017

randaz81 commented Aug 2, 2017

Nicogene commented Aug 2, 2017 • edited Loading

drdanz commented Aug 2, 2017

Nicogene commented Jul 31, 2017 •

edited

Loading

barbalberto commented Aug 1, 2017 •

edited

Loading

Nicogene commented Aug 2, 2017 •

edited

Loading