search

nimbuscontrols / EIPScanner

Free implementation of EtherNet/IP in C++
https://eipscanner.readthedocs.io/en/latest/
MIT License
233 stars 93 forks source link

Resolve Windows (MinGW) issues and minor improvements #47

Closed Broekman closed 3 years ago

Broekman commented 3 years ago

Refer to https://github.com/nimbuscontrols/EIPScanner/issues/46 for more information.

jadamroth commented 3 years ago

Thank you. Please give me a few days to review

jadamroth commented 3 years ago

Sorry for the delay. I just found time to look at this.

For Linux (arm), I'm receiving compile errors (see below) relating to your EIPSCANNER_SOCKET_ERROR in sockets/Platform.h

/home/adam/Downloads/EIPScanner/src/sockets/Platform.h:10:38: error: ‘WSAEINPROGRESS’ was not declared in this scope
 #define EIPSCANNER_SOCKET_ERROR(err) WSA##err
                                      ^~~
/home/adam/Downloads/EIPScanner/src/sockets/TCPSocket.cpp:58:39: note: in expansion of macro ‘EIPSCANNER_SOCKET_ERROR’
     if (BaseSocket::getLastError() == EIPSCANNER_SOCKET_ERROR(EINPROGRESS)) {
                                       ^~~~~~~~~~~~~~~~~~~~~~~
/home/adam/Downloads/EIPScanner/src/sockets/Platform.h:10:38: note: suggested alternative: ‘EINPROGRESS’
 #define EIPSCANNER_SOCKET_ERROR(err) WSA##err
                                      ^~~
/home/adam/Downloads/EIPScanner/src/sockets/TCPSocket.cpp:58:39: note: in expansion of macro ‘EIPSCANNER_SOCKET_ERROR’
     if (BaseSocket::getLastError() == EIPSCANNER_SOCKET_ERROR(EINPROGRESS)) {
                                       ^~~~~~~~~~~~~~~~~~~~~~~
/home/adam/Downloads/EIPScanner/src/sockets/Platform.h:10:38: error: ‘WSAEINTR’ was not declared in this scope
 #define EIPSCANNER_SOCKET_ERROR(err) WSA##err
                                      ^~~
/home/adam/Downloads/EIPScanner/src/sockets/TCPSocket.cpp:67:52: note: in expansion of macro ‘EIPSCANNER_SOCKET_ERROR’
       if (res < 0 && BaseSocket::getLastError() != EIPSCANNER_SOCKET_ERROR(EINTR)) {
                                                    ^~~~~~~~~~~~~~~~~~~~~~~
/home/adam/Downloads/EIPScanner/src/sockets/Platform.h:10:38: note: suggested alternative: ‘EINTR’
 #define EIPSCANNER_SOCKET_ERROR(err) WSA##err
                                      ^~~
/home/adam/Downloads/EIPScanner/src/sockets/TCPSocket.cpp:67:52: note: in expansion of macro ‘EIPSCANNER_SOCKET_ERROR’
       if (res < 0 && BaseSocket::getLastError() != EIPSCANNER_SOCKET_ERROR(EINTR)) {
                                                    ^~~~~~~~~~~~~~~~~~~~~~~
/home/adam/Downloads/EIPScanner/src/sockets/Platform.h:10:38: error: ‘WSAETIMEDOUT’ was not declared in this scope
 #define EIPSCANNER_SOCKET_ERROR(err) WSA##err
                                      ^~~
/home/adam/Downloads/EIPScanner/src/sockets/TCPSocket.cpp:82:32: note: in expansion of macro ‘EIPSCANNER_SOCKET_ERROR’
        throw std::system_error(EIPSCANNER_SOCKET_ERROR(ETIMEDOUT), BaseSocket::getErrorCategory());
                                ^~~~~~~~~~~~~~~~~~~~~~~
/home/adam/Downloads/EIPScanner/src/sockets/Platform.h:10:38: note: suggested alternative: ‘ETIMEDOUT’
 #define EIPSCANNER_SOCKET_ERROR(err) WSA##err
                                      ^~~
/home/adam/Downloads/EIPScanner/src/sockets/TCPSocket.cpp:82:32: note: in expansion of macro ‘EIPSCANNER_SOCKET_ERROR’
        throw std::system_error(EIPSCANNER_SOCKET_ERROR(ETIMEDOUT), BaseSocket::getErrorCategory());
                                ^~~~~~~~~~~~~~~~~~~~~~~
src/CMakeFiles/EIPScannerS.dir/build.make:348: recipe for target 'src/CMakeFiles/EIPScannerS.dir/sockets/TCPSocket.cpp.o' failed
make[2]: *** [src/CMakeFiles/EIPScannerS.dir/sockets/TCPSocket.cpp.o] Error 1
CMakeFiles/Makefile2:94: recipe for target 'src/CMakeFiles/EIPScannerS.dir/all' failed
make[1]: *** [src/CMakeFiles/EIPScannerS.dir/all] Error 2
Makefile:129: recipe for target 'all' failed
make: *** [all] Error 2

I'm also receiving compile errors on MacOS relating to endpoint.h

  1. Will you please add the preprocessor || defined(__APPLE__) in Endpoint.h line 8 as is in the current repo. This will fix compile errors, I'm receiving
  2. Please add the same preprocessor at the top of BaseSocket.cpp line 5
  3. Same thing in Endpoint.cpp lines 8 and 35
  4. Same thing in TCPSocket.cpp line 5
  5. Once the processors are added, I get the same compile errors as in Linux with the Platform.h errors

Once these errors are resolved and the app properly compiles, I'll work more towards testing the new changes on both platforms. Thanks again for the pull request

Broekman commented 3 years ago

No problem, I'll have a look later today. I also compiled (and tested) on Arch Linux (x64) and had no issues for now, strange that MAC doesn't respond to the unix def.

Broekman commented 3 years ago

I stand corrected, the updates/cleaning I did later broke the build on Linux. Moved the #if defined too far down in Platform.h. I also thought that unix also captured both linux and apple but seems not to be the case. Added it again and updated the platform file. It now compiles again on my Linux machine, will test a bit more (and see why Travis build fails).

jadamroth commented 3 years ago

Thanks for the update.

Once your changes are merged, I'll need to get a windows travis server up and running. I'd imagine there will be enough future usage for windows applications to justify. I've never tested multiple OS's in one CI server for a single repo, but I'd assume this is a relatively common task.

I don't know if there's enough use on macos to justify it's own build server, I just use it for miscellaneous testing, and most things that work for linux work similarly as mac.

Broekman commented 3 years ago

Ah, "static constexpr" has become explicitly inline >=C++17, not yet in 14 (causes a linker error). I wasn't getting these on my end as I'm compiling with C++20. Will fix in a bit after which you can re-check. Thanks!

Broekman commented 3 years ago

Ok all updated, just briefly tested discovery, implicit and explicit messaging on my Linux (and Windows) machines, looks good.

jadamroth commented 3 years ago

Thank you. I will test today on linux and mac and follow up with you

jadamroth commented 3 years ago

For mac, please add preprocessor || defined(__APPLE__) on

  1. DiscoveryManager.cpp line 15
  2. BaseSocket.cpp line 5

With those changes, mac works. Testing linux

Broekman commented 3 years ago

Oops, my "find and replace all" skills are lacking, sorry, done.

jadamroth commented 3 years ago

I tested with Linux (arm), and it works. I don't have the ability to test linux x64, but I'd assume this library works the same as arm.

I'm going to go ahead and merge to master and create a new release 1.2.0. If there's any bugs, we'll build a new release from there (1.2.1 and so on).

Thanks for your pull request and for using the library. If you plan on making a lot of changes in the future, let me know, and I can give you write access

Broekman commented 3 years ago

Thanks, I will be using the library more heavily this year, probably also in a larger industrial setting for support tooling on our embedded controllers or for simulation software. So access for the future might come in handy.