search

orocos / soem

ROS package wrapping the Simple Open EtherCAT Master (SOEM) from https://github.com/OpenEtherCATsociety/SOEM
http://wiki.ros.org/soem
77 stars 62 forks source link

Upstream soem release 1.4.0 via subtree #24

Closed mgruhler closed 5 years ago

mgruhler commented 5 years ago

This PR is meant to replace #23, if the decision to use subtree over submodules is made.

It also fixes #16, fixes #17 and fixes #22.

~Also, this is now a proof-of-concept for review and tests by other contributors. The commit history should definitely be cleaned...~ done

~The same issue holds as for in #23: this will be a breaking change for any consumers, since the previous work-around to the issue with header includes does not work anymore!~ found backwards compatible way

again, any feedback about this is highly welcome

mgruhler commented 5 years ago

@wongkaiweng I think I've found a solution to keep the behavior backwards compatible. So you wouldn't have to remove ${soem_INCLUDE_DIRS}/soem from your package (even though you should :-) ). Could you test this again?

@FelixBlix @jonbinney if you'd find the time to verify that this PR is working for your use cases, this would be great!

(~Now I have to figure out why jenkins fails this build due to the added deprecation warning...~ As discussed in this ROS answers, GitHub treats unstable Jenkin builds as failures. The CMake Warning introduced is triggering the job to be unstable. This is far from ideal, but I'd like to have the warning here for the next release. In the one after that, this can be removed again...)

wongkaiweng commented 5 years ago

@mgruhler I test the new commit. It works for me both with ${soem_INCLUDE_DIRS}/soem in my package (backward compatibility) and without ${soem_INCLUDE_DIRS}/soem (the 'fix').

mgruhler commented 5 years ago

@wongkaiweng thanks for the feedback! Very much appreciated!

mgruhler commented 5 years ago

@MatthijsBurgh @k-okada I'm pinging you due to your issues with SOEM 1.3.1 discussed in #9 This PR integrates upstream SOEM 1.4.0. If you still rely on this package, I'd be great if you could provide some feedback if the issue you encountered with 1.3.1. is still present.

jonbinney commented 5 years ago

I also tested this branch by compiling a dependent package in a child workspace, and it worked from both the devel and install space of the parent soem workspace.

The deprecation warning is great to have, but a little different than how i've seen them used in other packages. Usually you only get the deprecation warning if you do something that is deprecated (in this case that would be using the old include directory hack). But here we can't really detect that, so the deprecation warning always shows, which makes the build unstable. @mgruhler what do you think about making that just a normal cmake message() since it happens regardless of whether the user has done something wrong?

mgruhler commented 5 years ago

@jonbinney thanks for the feedback. great to see that this works for you to. Did you only test compilation or also behavior of any ethercat devices?

I agree with your comment about the deprecation warning. I've not found a way to figure out when someone is doing "the wrong thing". But I understand your line of thought of not having this as a deprecation warning that pops up as a warning every time. Thus, I changed it to just print a regular message now. That is still shown always, but does not render the build unstable.

Sidenote: catkin_tools displays this prefixed with Warnings << soem:cmake, catkin_make does not.

ghost commented 5 years ago

Sry for the slow reply. I have now tested both by installing and through devel space, and it works fine. I tested both compilation and behaviour to the extend that all the EtherCAT devices in our robot come online, and I get PDO communication. I did not however test the fault reset, watchdog and so forth, but I don't really see how the specifics would depend on the wrapper package. I had one Issue when using the package through the devel space were catkin_tools would not build the soem package first despite it being stated as a package dependency in both the package.xml and the CMakeLists.txt. This was the case for both our own package rr_ethercat_master, which had the old hack removed, and robotic_ethercat which is an external package that was unchanged. I then build the soem package manually before running catkin build, and it worked a charm. I'm not sure if this is just because I did something dorky in our package, and never noticed because we were using the installed package, or if there's some actual issue.

jonbinney commented 5 years ago

@mgruhler sounds good. I was only testing compilation - sounds like @FelixBlix did some testing with devices.

mgruhler commented 5 years ago

Thanks at @FelixBlix and @jonbinney for your tests and Feedback.

@FelixBlix I have not been able to reproduce your problems with compilation. Can you give more details on what goes wrong and how to reproduce?

Before I merge this, I'd like to give some other people pinged above a little bit more time to test this. Especially since there seem to have problems with release 1.3.1 discussed in #9 All tests that I have made with hardware (which is limited to one manufacturer only...) did not show any problems. I'm not using all functionality of SOEM, though. The more tests with hardware, the better :-)

That being said: if I don't get any negative feedback until Friday next week, I'll merge this PR and trigger a new release.

MatthijsBurgh commented 5 years ago

@MatthijsBurgh I'm pinging you due to your issues with SOEM 1.3.1 discussed in #9 This PR integrates upstream SOEM 1.4.0. If you still rely on this package, I'd be great if you could provide some feedback if the issue you encountered with 1.3.1. is still present.

@mgruhler, I am interested to check if this runs on our robot. I am out of the lab till mid/end August, so I can't test this until then.

mgruhler commented 5 years ago

@MatthijsBurgh thanks for your response!

It'd be great if you could test this. Even though end of august is a long time to wait (enjoy your holidays, though :smile: (I hope)).

I really like to avoid breaking any existing setup. But it is close to impossible to recreate any issues without the different hardware setups where this is used...

All tests on our robots (basically, a simple ethercat system with two ELMO BLDC-drives as slaves, I also tested four) don't show any issues. But there are way more complicated things...

Do you have a suggestion how to proceed?

MatthijsBurgh commented 5 years ago

@mgruhler we run both beckhoff slaves and our own custom slaves. So that is maybe a better test.

I would suggest to set a deadline to merge this update to master. Update the readme of the master and add a compiler message about the upcoming update and the deadline. Also create a tag of the current master version. Include this tag information in the readme and compiler message for easy reverting by users.

At the time of merging the update into master, make sure you have extra time to solve incoming issues.

mgruhler commented 5 years ago

@MatthijsBurgh I agree that your setup is a better test. Thus, any feedback is appreciated.

Even though I've never seen something like this in the ROS world, what about #25? Shows an announcement during compile time (technically, during build system creation) and in the Readme and contains a deadline as well as a the tag anyone can use that requires the old state.

I would really appreciate any feedback from your end, so I set the deadline to end of August. But I don't want to wait any longer with merging. I don't see this package as being used in such a broad audience to justify not upgrading to the new upstream SOEM. Especially since I did not experience any issues (granted, with a limited test setup). As this is not SOEM, but merely a wrapper to "catkinize" this, any issues with SOEM itself would need to be taken up at the upstream repo. This also holds for any "issues" that might arise after the merge.

But I agree, if this is not working as it used to and people haven't read the announcement, I definitely need to be active on GitHub to answer questions. But basically this is a link to the Readme...

ghost commented 5 years ago

I have just tried to make a repo to recreate the build error I was seeing, and cannot, so it must have been a goof in my configuration. This means that I can now confirm that build works with packages using the new (normal) way of including and the old (hack), and both using devel space and install space. The devices I have tested against is a host of different Beckhoff IO's, and a CoE ds 402 motor driver from harmonic drive.

mgruhler commented 5 years ago

Thanks @FelixBlix

So no issues on that end.

Also thanks for the tests. I followed the suggestion by @MatthijsBurgh and just merged an upgrade announcement. I'll merge this PR end of August, if there are no issues.

MRIAC commented 5 years ago

facing issue with interfacing with C++ ros node in ROS..showing fatal error: hpeif2.h: No such file or directory in nicdrv file..please share me the link to resolve this issue.

mgruhler commented 5 years ago

facing issue with interfacing with C++ ros node in ROS..showing fatal error: hpeif2.h: No such file or directory in nicdrv file..please share me the link to resolve this issue.

@MRIAC Please open a new issue for this. Also, please include exactly which version you are using (from source, if yes, which branch/commit-sha)? from debs?) as well as the CMakeLists.txt (shortened to the relevant content OR link to repo) and the exact error message via copy'n'paste.

kimtj5521 commented 5 years ago

Is upgrade done?

mgruhler commented 5 years ago

@kimtj5521 yes, thanks for reminding

ghost commented 5 years ago

Now that master is updated, will this become available through apt as well?

mgruhler commented 5 years ago

@FelixBlix yes, at least for kinetic and melodic. But I'll wait a little bit until I trigger a release to see if there is some additional feedback. I'll try to release within the next two to three weeks, though.

ghost commented 5 years ago

That's perfect. Thank you so much for your work. We appreciate it :)

mgruhler commented 5 years ago

@FelixBlix FYI, just triggered the releases for kinetic ros/rosdistro#22300 and melodic ros/rosdistro#22301