Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remote control board: remove single port compatibility #1010

Merged
merged 5 commits into from
Dec 13, 2016
Merged

Remote control board: remove single port compatibility #1010

merged 5 commits into from
Dec 13, 2016

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.

traversaro
traversaro reviewed Dec 12, 2016
View reviewed changes
src/libYARP_dev/src/devices/RemoteControlBoard/RemoteControlBoard.cpp
@@ -3983,11 +3393,10 @@ class yarp::dev::RemoteControlBoard :
return true;

// protocol did not match
yError("expecting protocol %d %d %d, but remotecontrolboard returned protocol version %d %d %d\n",
yError("expecting protocol %d %d %d, but the device we are connectin to has 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.

@barbalberto
Copy link
Collaborator Author

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

drdanz
drdanz approved these changes Dec 13, 2016
View reviewed changes
doc/release/v2_3_70.md
@@ -11,6 +11,10 @@ Important Changes
* 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
Reviewers

@drdanz drdanz drdanz approved these changes

@traversaro traversaro traversaro approved these changes

Assignees
No one assigned
Labels
Component: Deprecated Component: Devices Resolution: Merged Target: YARP v2.3.70 Type: Cleanup Involves cleaning up some part of YARP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@barbalberto @traversaro @drdanz