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.

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

New class for return values of interface methods. #3051

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

randaz81
Copy link
Member

This new class can be used in all interface methods to provide extra info about the success/failure of the method.
In this example:
bool getTemperature(int m, double* val) override;
becomes:
yarp::os::yarp_ret_value getTemperature(int m, double* val) override;
So the user can distinguish if a failure is due to missing implementation/internal error/communication error etc.
The class is also automatically converted to bool when tested by conditions.

Extra Notes:

  • No need to change the user application code.
  • Only devices (e.g. xxxmotioncontrol) must be updated.
  • In this example, the error code is not actually propagated through the network (i.e. from the nws to the nwc).

Copy link

update-docs bot commented Nov 21, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update the release notes by adding a file in doc/release/<target_branch>, based on your changes.

@traversaro
Copy link
Member

As we typically use CamelCase names for classes, and given this is return value is device-specific, could we call this type yarp::os::DeviceReturnValue ? Or perhaps we can move it to yarp::dev, unless there is some reason to keep it on yarp::os?

@randaz81
Copy link
Member Author

As we typically use CamelCase names for classes, and given this is return value is device-specific, could we call this type yarp::os::DeviceReturnValue ? Or perhaps we can move it to yarp::dev, unless there is some reason to keep it on yarp::os?

Still not sure about the class name, but I like the idea of moving it to yarp::dev!

Comment on lines +63 to +64
case return_code::return_value_unitialized:
return std::string("return_value_unitialized");
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "uninitialized".

Suggestion: isn't the "return_value_" prefix a bit verbose? I'm also thinking of an overload to LogStream so that this would be possible:

double val;
auto ret = iMotor->getTemperature(0, &val);
yInfo() << ret; // prints "ok", "error_generic" "error_not_implemented" and so on

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a nice idea! Just give me a little more time before proceeding since we need to edit a few things more before.

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