search

robotology-playground / WBI-Toolbox

Simulink Toolbox for rapid prototyping of Whole Body Robot Controllers
2 stars 2 forks source link

[Yarp Read] Add support for optional autoconnect #83

Closed francesco-romano closed 8 years ago

francesco-romano commented 9 years ago

We need to extend the behaviour of yarpRead block. Currently the name of the port specified is the port to which the block will connect. We should link this behaviour (together with the temp port support) only to an autoconnect option (checkbox). In case the user does not set the behaviour to be autoconnect, the block should only create the port. This will ensure the compatibility with the usual yarp modules procedure.

cc @jeljaik

jeljaik commented 9 years ago

I'll fix this. I already had to do some pending improvements.

iron76 commented 9 years ago

:+1: thanks a million @jeljaik

jeljaik commented 9 years ago

I have addressed this issue by adding a checkbox called Autoconnect. When checked, the user can't specify the destination source, but only the source and size of its output. When unchecked, the user can type the name of a port to be created. Upon entering this data, the block will simply create a port and it's up to the user to connect it to any other port via terminal as yarp connect [source] [dest]. In-block documentation has also been updated.

autoconnect_on autoconnect_off

Hope this is what you desired :smile:

Cheers!

iron76 commented 9 years ago

:+1:

jeljaik commented 9 years ago

A couple of issues were identified. We need testers!

Notes

  1. When unchecking Autoconnect make sure that the now-hidden field From is filled with some string, e.g. '...', otherwise you get the error "Cannot retrieve string from parameter 1!!. This behavior will be nevertheless improved.
  2. Matlab will crash if you activate the Autoconnect option while specifying the name of a port that you don't want to connect just yet.
DanielePucci commented 9 years ago

Hi Jorh, well done. I think, however, that it could be useful to add an additional output to this block, which informs the user that the port is actually connected. For instance, if the port is disconnected, we receive zeros, but the user does not know if these zeros are actually sent by a sender. Can we add a binary output that acknowledges the connection?

francesco-romano commented 9 years ago

@DanielePucci in which of the two cases (or both) you want this output?

DanielePucci commented 9 years ago

Both cases? If the autoconnect flag is active, then the binary output can be always true.

iron76 commented 9 years ago

I think the additional output is just a temporary workaround (an I am not really sure necessary right now). The real solution would be to have a visual output (certain Simulink blocks can change their icon) when data are available on the port.

traversaro commented 9 years ago

@iron76 an additional "real" value can be really useful to deal with data still not available on that port (for example: don't start the controller until data is available).

traversaro commented 9 years ago

@DanielePucci it is possible that the port never received anything even if you use the "autoconnect" option.

iron76 commented 9 years ago

@traversaro there is an issue here related to the fact that Simulink always assumes that data are available while YARP does not. I think any solution would be a workaround. In any case I don't really like the additional output solution. I am tempted by using NaN.

DanielePucci commented 9 years ago

NaN would be fine for me. The main point is to distinguish data sent by someone from fake data. I agree with @traversaro, however, that in some cases it would be useful to have an additional output, which may help develop a diagnostics logic in the controller.

jeljaik commented 9 years ago

In "Simulink language", missing data should be represented by NaN. This could definitely be done in conjunction with a change in the color of the block ... although, since the blocks are all colored I'd rather go for a particular icon for this block.

jeljaik commented 9 years ago

In this case, as @traversaro mentioned, a connected port that does not stream data would also be clearly visible in the model. Kind of a time-saver...

francesco-romano commented 9 years ago

to be honest I'm a bit scared about the use of NaNs

iron76 commented 9 years ago

I am more scared about using conventions which might difficult to interpret, such as using an additional binary output. Of course we can document things to allow users retrieving the correct meaning of things.

DanielePucci commented 9 years ago

I agree with the general principle. In this specific case, however, I think that it would be more misleading to have NaNs than outputs like "connection" and "readFailure". In the @iron76 direction, we might stream NaNs if either connection = 0, or readFailure=1. In this case, the conventions should be clear enough.

francesco-romano commented 9 years ago

@jeljaik your issues here: https://github.com/robotology-playground/WBI-Toolbox/issues/83#issuecomment-87736511 should have been fixed now. Can you check it?

nunoguedelha commented 9 years ago

@francesco-romano regarding the issue https://github.com/robotology-playground/WBI-Toolbox/issues/83#issuecomment-87736511 =>(1), and the symmetric issue:

I'm checking if these have been resolved by your fix #90 .

I never reproduced the second issue mentioned by Jorh, even with commit 3123960 from Daniele, before your fix.

nunoguedelha commented 9 years ago

I'm still seeing this issue:

francesco-romano commented 9 years ago

I've just test it and it works.. but I was in the fix/yarpRead branch. Can you test that you get the same behaviour there? Just to understand if it was a merge issue.

nunoguedelha commented 9 years ago

Well, I did a git diff between your branch and the master branch and there are no diffs.

francesco-romano commented 9 years ago

This is good then :)

But how did you test that?

On 09 Apr 2015, at 18:25, Nuno Guedelha notifications@github.com wrote:

Well, I did a git diff between your branch and the master branch and there are no diffs.

— Reply to this email directly or view it on GitHub https://github.com/robotology-playground/WBI-Toolbox/issues/83#issuecomment-91280678.

DanielePucci commented 9 years ago

Can we make a simple model-test?

nunoguedelha commented 9 years ago

I tested with a very basic model that just uses the default yarpRead block. I checked that the "to" field is filled with the default "..." if it is masked (after I check "Autoconnect" and click OK). That doesn't happen with the "from" field (if I uncheck "Autoconnect" instead). Looking at the code WBCLibrary.mdl I actually see the line missing (as far as I can understand)...

nunoguedelha commented 9 years ago

Don't loose your time with this, I'll make a quick check...

nunoguedelha commented 9 years ago

Works fine now. I just pushed the branch "fix/yarpRead_default_from_field" for you to review?

francesco-romano commented 9 years ago

@nunoguedelha the from field is unused if don't set the autoconnect flag. This is way we don't care about it. Further more, leaving the name the user set will allow a faster switch between autoconnect values.

To explain better this is the "terminal" behaviour we want to replicate in the yRead block:

1) $ yarp read ... /source/port
2) $yarp read /destination/port

So if autoconnect is on we want to replicate the first behaviour: we want the user to set an already existent port in the from field. If autoconnect is off, we want the user to specify only the destination port. And we don't really care about the from port.

Lastly, putting ... in the from port is wrong. There will never exist a port called ....

nunoguedelha commented 9 years ago

Yes, you're absolutely right.

So we do agree that the cleanest behaviour of the yard Read bock would be not to check the from field or try to create a source port at all when the autoconnect flag is set. But currently, it is doing this check, right? Do you get the same error as I do?

francesco-romano commented 9 years ago

@nunoguedelha yes ok. I never thought about leaving completely empty the field. I think I will edit the C++ anyway. I have to add another feature to the yRead port, so i put you in stand-by :smile: I will ping you again when I finish adding the feature.

nunoguedelha commented 9 years ago

ok thanks :wink:

francesco-romano commented 9 years ago

@nunoguedelha I completely change the yarp read. I added a new yarp read block. The old is still present but is considered deprecated. I don't know if you want to test it, for us is low priority.

nunoguedelha commented 9 years ago

I'll test it this week end if I have some time.

francesco-romano commented 8 years ago

Given the new yarp read has been quite stable during the last 6 months of work, I think we can close this issue.