-
Notifications
You must be signed in to change notification settings - Fork 195
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
driver: fix deserialisation of JointFeedbackEx #225
driver: fix deserialisation of JointFeedbackEx #225
Conversation
Always deserialise all submsgs, but keep only those with valid data.
This is a fix for #91. Partially based on the work by @jettan and @alemme (via @andreaskoepf). |
@shaun-edwards: would you have time for a review? If not, I'll ask someone else. |
@alemme, @andreaskoepf: would you perhaps see a chance to test this on your SDA? You would only need to change this file, right? |
Hi everyone. I successfully tested your changes on our sda10d system. |
Thanks @alemme. Appreciated. |
@sinaiaranda-CIDESI: the assertion you're running into is most likely the same as in #91. This PR should fix that one. There are some more issues with your setup and I don't completely understand how, when using #179 things don't work for you though. But please open a separate issue and we can continue there. |
Thanks @shaun-edwards for the review. And thanks @alemme as well. |
@gavanderhoorn The problem has been resolved with this modification of PR. |
I'm slightly confused: #91 is the issue that reported the problem, not a PR. And the commit you link should be the "hacky" way this was fixed, with the PR we're discussing here being a more appropriate way. If I may suggest: use the fix in this PR or just use current Fixes for #226 should fix the remaining issues and make things operational again (note: those haven't been merged yet). |
@gavanderhoorn Excellent, have do not problem. |
See comment on c9c62dd for more info.
Summary: always deserialise every submsg, but only keep those that contain valid information.
I've verified this on our SIA20F, which has two motion groups: robot itself + a linear rail.
Instead of 'upgrading' to
unloadFront(..)
(as suggested by @andreaskoepf), I decided to make this a minimal change and keptunload(..)
.