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

Deprecate IControlMode::setXXXMode(int) methods #1223

Merged
merged 3 commits into from
Jun 6, 2017
Merged

Deprecate IControlMode::setXXXMode(int) methods #1223

merged 3 commits into from
Jun 6, 2017

Conversation

PeterBowman
Copy link
Member

Unused, doesn't belong to any controlboard interface, leads to confusion due to the recent deletion of IPositionDirect::setPositionDirectMode.

@randaz81 randaz81 self-assigned this May 22, 2017
@randaz81
Copy link
Member

Thank you for reporting this. Actually, all similar methods setXXXMode() from IControlMode.h should be deprecated in favor of setControlMode() from IControlMode2.h
To be merged after #1220

@PeterBowman
Copy link
Member Author

Actually, all similar methods setXXXMode() from IControlMode.h should be deprecated in favor of setControlMode() from IControlMode2.h

I've made some progress at PeterBowman/yarp@e5b2734. Is it OK if I add this to the previous commit and edit the PR title?

@drdanz
Copy link
Member

drdanz commented May 25, 2017

@PeterBowman Sure, go on! Thanks

PeterBowman
PeterBowman commented May 25, 2017
View reviewed changes
src/libYARP_dev/src/devices/ControlBoardWrapper/ControlBoardWrapper.cpp Outdated
legacySetControlMode(j, mode);
YARP_WARNING_PUSH
YARP_DISABLE_DEPRECATED_WARNING
ret = legacySetControlMode(j, mode);
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed return value.

PeterBowman
PeterBowman commented May 25, 2017
View reviewed changes
src/libYARP_dev/src/devices/RemoteControlBoard/RemoteControlBoard.cpp
YARP_DEPRECATED bool setImpedancePositionMode(int j) { return setControlMode(j,VOCAB_CM_IMPEDANCE_POS); }
YARP_DEPRECATED bool setImpedanceVelocityMode(int j) { return setControlMode(j,VOCAB_CM_IMPEDANCE_VEL); }
#if !defined(_MSC_VER)
YARP_WARNING_POP
Copy link
Member Author

Choose a reason for hiding this comment

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

Notice how this is different from current master. I guess it was a typo (c5b8f28).

@PeterBowman PeterBowman changed the title Remove yarp::dev::RemoteControlBoard::setPositionDirectMode(int) Deprecate IControlMode::setXXXMode(int) methods May 25, 2017
@PeterBowman PeterBowman mentioned this pull request May 25, 2017
Closed
4 tasks
@PeterBowman
Copy link
Member Author

I see two additional issues here:

  • No bindings for the IControlMode2 interface, which effectively prevents from setting/changing modes. In fact, all IXXXControl2-like interfaces are still missing in yarp.i (e.g. IPositionControl2) - is this deliberate? Are there any plans with this regard?
  • Similarly, ImplementControlMode.h will be rendered (almost?) useless. Shouldn't this be deprecated in favour of ImplementControlMode2.h?

@drdanz drdanz added Component: Library - YARP_dev PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Target: YARP v2.3.70 Type: Cleanup Involves cleaning up some part of YARP labels Jun 5, 2017
@PeterBowman @drdanz
Remove yarp::dev::RemoteControlBoard::setPositionDirectMode(int)
12f80c8 
Unused, doesn't belong to any controlboard interface, leads to confusion
due to the recent deletion of IPositionDirect::setPositionDirectMode.
@PeterBowman @drdanz
Deprecate IControlMode::setXXXMode(int) methods
997e906
@PeterBowman @drdanz
Remove redundant YARP_DEPRECATED symbols
7275637
@drdanz drdanz force-pushed the unused-method-remotecb branch from 48ea5b0 to 7275637 Compare June 6, 2017 09:40
@drdanz drdanz self-assigned this Jun 6, 2017
@drdanz
Copy link
Member

drdanz commented Jun 6, 2017

Rebased and fixed conflict with the latest commits.

drdanz
drdanz approved these changes Jun 6, 2017
View reviewed changes
Copy link
Member

@drdanz drdanz left a comment

Choose a reason for hiding this comment

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

LGTM

randaz81
randaz81 approved these changes Jun 6, 2017
View reviewed changes
Copy link
Member

@randaz81 randaz81 left a comment

Choose a reason for hiding this comment

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

Good work

@drdanz
Copy link
Member

drdanz commented Jun 6, 2017

I will merge it as soon as the tests are complete. Thanks

@drdanz drdanz merged commit c92a2ac into robotology:devel Jun 6, 2017
@drdanz
Copy link
Member

drdanz commented Jun 6, 2017

Merged. Thanks!

@PeterBowman PeterBowman deleted the unused-method-remotecb branch June 6, 2017 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@randaz81 randaz81 randaz81 approved these changes

@drdanz drdanz drdanz approved these changes

Assignees

@drdanz drdanz

@randaz81 randaz81

Labels
Component: Library - YARP_dev PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP 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
@PeterBowman @randaz81 @drdanz