Added #define-based namespaces to prevent linking errors.

[a-price]

In the current setup, it is impossible to link against the ur5 or ur10 versions of the kinematics library, as all expose the same ur_kinematics::inverse() signature.
Since all libraries are exported in the catkin_package() macro, this results in undefined behavior when linking.
GCC just picks the first suitable signature it encounters, in this case the ur3 version, and continues without an error.
Adding a namespace allows the linker to find the correct library call.

[gavanderhoorn]

Related: #220.

[mxgrey]

Hi, Andrew!

I've opened a PR to your fork with some tweaks that I think will make this fix more digestible for end users. The motivations are explained in my comments in this review. If you're okay with merging those changes, then I'll be happy to approve this PR.

However, there may be one last concern, which is that this PR breaks ABI compatibility. I'm not sure what the ABI policy is for the universal_robot package. There's an REP about ABI compatibility which suggests that ABI should be maintained for even versions, and the current version is 1.2.1. If universal_robot follows that policy, then we'd either have to

1. increment the version to 1.3.0 or
2. create a copy of each of these functions (forward, forward_all, and inverse) outside of the namespace that simply call the namespaced version.

I would probably recommend option (2), because it's the least intrusive. If any universal_robot maintainers have a preference, input would be appreciated.

[mxgrey]

This approach will require users to always define one of these options before including the header. We could approximate the original behavior by doing this instead:

#ifdef UR10_PARAMS
#define UR_NAMESPACE ur10
#elif UR5_PARAMS
#define UR_NAMESPACE ur5
#else
#define UR_NAMESPACE ur3
#endif

Using the approach in this PR, users will get a crytpic linking error if they neglect to specify one of these options.

[a-price]

Agreed. It does slightly privilege the UR3, but that's probably fine.

[mxgrey]

If we make this an inline namespace, then we can keep API compatibility the same, so people don't need to add this extra namespace to their function calls.

[a-price]

Interesting idea. I wasn't familiar with inline namespace.

[mxgrey]

If @a-price is willing to merge the PR a-price#1, then I'll be happy to approve.

[a-price]

Grey! It's good to hear from you!

Thanks for the suggestions. I wasn't aware of the ABI recommendations; that's good to know about. Here's a couple of thoughts:

1. Inlining the namespace: I think this would preserve the API for the package, but it's not clear to me what exactly ur_kinematics::inverse() should compute? In the context it's used, it works because the CMakeLists.txt defines which robot we're referring to via set_target_properties(ur3_moveit_plugin PROPERTIES COMPILE_DEFINITIONS "UR3_PARAMS") etc. In a sense, each of the files is used as a template/macro to share the basic math with changing numerical parameters.

2. Preserving the ABI: Since the binary interface to the library from outside the package is, by my understanding, currently using undefined behavior, I don't have a huge problem with breaking it. So, I'd advocate for advancing the version to 1.3.0.

Given the current state of the interfaces, I'd be a little surprised if anyone else is trying to link directly to the library rather than accessing the kinematics through the plugins/ROS interface. I'd be happy to be corrected, though.

[a-price]

The way I'm using the library currently is by calling:

#define UR5_PARAMS
#include <ur_kinematics/ur_kin.h>

which is admittedly a little awkward and non-C++-like. It does have the advantage that the user has to explicitly call out which UR arm they wish to use, since it's not clear from the filename. I could see a couple of ways around this. One would be to use CMake to generate different headers, e.g. #include <ur_kinematics/ur_5_kin.h>. Another might be to add an error message to the namespace definition block:

#if defined UR10_PARAMS
#define UR_NAMESPACE ur10
#elif defined UR5_PARAMS
#define UR_NAMESPACE ur5
#elif defined UR3_PARAMS
#define UR_NAMESPACE ur3
#else
#error "You must #define which UR model you wish to use. Options are { UR10_PARAMS, UR5_PARAMS, UR3_PARAMS }."
#endif

which does emit a comprehensible error message when I try to compile without the #define.

[mxgrey]

Preserving the ABI: Since the binary interface to the library from outside the package is, by my understanding, currently using undefined behavior, I don't have a huge problem with breaking it.

It's undefined by the C++ standard, but I believe on ROS's supported platform (Ubuntu) it's well-defined: Whichever version of the symbol the linker finds first is the one it will use, and the order in which the linker finds the symbol is based on the build system configuration. Users will get consistent behavior as long as they don't change their build system configuration, and some users might be depending on that behavior.

The changes recommended here will maintain API+ABI compatibility and keep the behavior consistent for any users who don't change their code. That would allow us to increment the patch version instead of the minor release version.

[a-price]

Well, I definitely agree that the better way would be to have a distinct header that is properly namespaced and hides all the macro business from the end user. The library in its current state, to my understanding, cannot be used directly (i.e. without using the plugin code) with 2/3 of the robots which it services, at least if you want to use ${catkin_LIBRARIES} in your target_link_libraries. (This was my experience anyway, when trying to link directly to the UR5's kinematics.) So, while it's well-principled to try to minimize breaking changes to the end-user, I don't see that there's much for any end users to use at the moment.

If I seem hesitant to incorporate maintaining the current interface, that's probably two-fold. First, I spent several hours troubleshooting why calling the kinematics gave bizarre results, and I'd like to prevent future users from encountering the same pitfall. Second, it's still not clear to me what that function ought to do. Maybe link it to the UR3 kinematics, since that's what it happened to link against up to this point, and mark it as deprecated?

Moving forward, would there be any objection to adding a new header file containing

namespace ur_kinematics {
namespace ur3 {
void forward( ...
...
}
namespace ur5 {
void forward( ...
...
}
...

That would at least get rid of the necessity to use a #define before the #include, though it would still break the interface, which I think likely ought to be broken.

[gavanderhoorn]

re: breaking existing setups: personally I would be ok with that in this instance. The current situation with ur_kinematics is that it's broken (at least when directly linking), and people that have persevered to get it to work probably have an understanding of the issue that would make it not too hard for them to incorporate any fixes we merge.

I would normally be in favour of maintaining ABI as @mxgrey suggests, but in this case I believe I'm ok with ignoring that.

@a-price: could I also ask you to revert the version number increment? That cannot be done for individual packages -- we'll need to do that to the entire repository in order to be able to properly tag and release it.

[Levi-Armstrong]

Is there an advantage to using compiler definitions versus a data structure which is provided the inverse function?

Parameter data structure:

/** @brief The Universal Robot kinematic parameters */
struct URParameters
{
  URParameters() = default;
  URParameters(double d1, double a2, double a3, double d4, double d5, double d6)
    : d1(d1), a2(a2), a3(a3), d4(d4), d5(d5), d6(d6)
  {
  }

  double d1{ 0 };
  double a2{ 0 };
  double a3{ 0 };
  double d4{ 0 };
  double d5{ 0 };
  double d6{ 0 };
};

/** @brief The UR10 kinematic parameters */
const static URParameters UR10Parameters(0.1273, -0.612, -0.5723, 0.163941, 0.1157, 0.0922);

/** @brief The UR5 kinematic parameters */
const static URParameters UR5Parameters(0.089159, -0.42500, -0.39225, 0.10915, 0.09465, 0.0823);

/** @brief The UR3 kinematic parameters */
const static URParameters UR3Parameters(0.1519, -0.24365, -0.21325, 0.11235, 0.08535, 0.0819);

Change Inverse function:

int inverse(const Eigen::Isometry3d& T, const URParameters& params, double* q_sols, double q6_des)