Skip to content

Conversation

@barbalberto
Copy link
Collaborator

This PR remove the backward compatibility of the remote control board with wrapper that has ONLY the state:o port.

This device is therefore expecting the wrapper to have the stateExt:o port, which was introduced in 30/07/2014.

return true;

// protocol did not match
yError("expecting protocol %d %d %d, but remotecontrolboard returned protocol version %d %d %d\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: connectin --> connecting

@traversaro
Copy link
Member

I can't withdraw my review, but there is an important point that I think we already discussed some time ago but I forgot about the outcome of that discussion.
This PR (by changing the protocol number) will break the YARP-level compatibility between master and devel (even if, bypassing the protocol check) everything will be working fine.
As the protocol on the stateExt:o did not change, perhaps we can think about avoiding this unnecessary incompatibility?

@barbalberto
Copy link
Collaborator Author

I changed the protocol number to have a clear error message.
Without it, if a user tries to connect a remotecontrolboard after this change to a controlboardwrapper that only has state:o port, he/she will get a connection error whitout knowing why or what to do about it.
I think this PR does change the protocol, in the sense that some data was going through a port, and now goes though a different port (if you see from the perspective of a CBW with only state:o to a CBW with the stateExt:o)
It works if both client/server are updated enough, it will not work anymore if thay are old (pre compatibility hack was introduced). Since this hack it is what I'm removing, for them it is a change of protocol.

Anyway the next change I'm working on will change the protocol for sure since I'm gonna change the data structure of stateExt :-P

For sure it is annoying to break master / devel compatibility.
I don't see any other way, but I'm open to suggestions

@traversaro
Copy link
Member

I see, then I agree that a protocol version change is inevitable, especially if you plan to change it again in a short time.. Can we wait at least tomorrow for merging this? : (

@barbalberto
Copy link
Collaborator Author

Yes, no rush in merging it

@drdanz
Copy link
Member

drdanz commented Dec 13, 2016

Please add a line or two (for this PR, but in general for any PR on yarp) in the relative release notes file (in this case doc/release/v2_3_70.md)

@traversaro
Copy link
Member

I documented the necessary steps to use devel with the superbuild in https://github.com/robotology/codyco-superbuild#handling-the-devel-branch , for what it concerns that problem the PR can be merged.

Alberto Cardellino added 2 commits December 13, 2016 15:14
@barbalberto
Copy link
Collaborator Author

Updated doc and fixed typo in error message.
Can we merge?

* YARP_math can no longer be built using GSL. The `CREATE_LIB_MATH_USING_GSL`
option was removed. Only Eigen is supported. `FindGSL.cmake` is no longer
installed.
* 31/12/2016: `RemoteControlBoard` device is no longer compatible with `ControlBoardWrapper2`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

31/12/2016 30/07/2014? Just forget about the dates next time, nobody cares... 😆

@drdanz drdanz merged commit 01f205a into robotology:devel Dec 13, 2016
@drdanz
Copy link
Member

drdanz commented Dec 13, 2016

Merged, thanks!

@barbalberto barbalberto deleted the CBW2_remove_singlePort_compatibility branch December 13, 2016 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants