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

Remove boost::math dependency #103

Closed

Conversation

traversaro
Copy link
Contributor

As discussed in #70 (comment) :

Remove the boost::math dependency by implementing the quaternion conjugate operation, which should be straight-forward.

It turns out that it was necessary also to implement a couple more operators, so the PR is ready but it is mostly open just to receive feedback, in particular:

  • I added a test to ensure that the estimateVelocity continued to work exactly as it did with boost::math . For the time being I did not use any test library, if you have any preference I can migrate the test to your favored framework.
  • To ensure that the code remained real-time safe with no allocation as it was before, I could not define functions that operated on the abb::egm::wrapper::Quaternion structure, but I had to introduce a small private class to contain internal quaternion operations. I intentionally hided this object from the public headers to avoid to maintain them in the future, let me know if this is ok.

The ground truth data is obtained from the existing boost::math
implementation.
@gavanderhoorn
Copy link
Member

@jontje: would this be something you could check and review?

Would be great to get this merged.

Sorry @traversaro :S

@traversaro
Copy link
Contributor Author

traversaro commented Nov 20, 2020

Sorry @traversaro :S

No problem, I know how it works with open source software maintenance. : )

The fact that ABB's even accepts downstream contributions for their EGM reference implementation is still mind-blowing w.r.t. to the kind of support/openness provided by some other industrial robotics vendors as of 2020 (we can always hope this will change in the future : ) ).

@gavanderhoorn
Copy link
Member

The fact that ABB's even accepts downstream contributions for their EGM reference implementation is still mind-blowing

It's just a trick: whenever something doesn't work, we check git blame and then forward complaints to whoever contributed that change ;D

(just kidding of course, I don't work for ABB)

@jontje
Copy link
Contributor

jontje commented Nov 20, 2020

@jontje: would this be something you could check and review?

Would be great to get this merged.

I agree it would be great, and I have been meaning to get around to this. I will try to do it as soon as possible.

Sorry @traversaro :S

@traversaro, I am also sorry for the slow response rate 😔

@traversaro
Copy link
Contributor Author

I saw that the PR has no some conflicts, I could fix them know but perhaps it could make sense to wait for a first review, to avoid solving other different conflicts in the future? In any case, the conflicts are minor and are not part of the core of the PR.

@traversaro
Copy link
Contributor Author

I saw that the PR has no some conflicts, I could fix them know but perhaps it could make sense to wait for a first review, to avoid solving other different conflicts in the future? In any case, the conflicts are minor and are not part of the core of the PR.

I am trying to cleanup the PRs that I have in https://github.com/pulls, so given that this seems stale, I am going to close this. If anyone is interested in this in the future, feel free to ping me and I would be happy to rebase it on the top of the latest master, thanks!

@traversaro traversaro closed this Apr 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants