ifl-camp / simple Goto Github PK

I'm using simple-static in my IDP project (also within CAMP). It seems that ZeroMQ on Windows 10 also requires linking to ws2_32 and Iphlpapi. In my project's CMakeLists.txt, I added ws2_32 and Iphlpapi to target_link_libraries like this:

target_link_libraries(${PROJECT_NAME}
      ${OpenCV_LIBS}
      ${EIGEN3_LIBS}
      ws2_32 #ZeroMQ dependency
      Iphlpapi #ZeroMQ dependency
      simple::simple-static
   )

I followed your install guide (https://github.com/IFL-CAMP/simple/wiki/install:-windows) for ZeroMQ and set ENABLE_CURVE off.

Consider updating the usage page (https://github.com/IFL-CAMP/simple/wiki#cmake) with ws2_32 and Iphlpapi.

My system information:
Windows 10 Home, Version 1803
Visual Studio Community 2017, Version 15.6.0
CMake 3.11.2, using generator for Visual Studio 15 2017 Win64

The Radio/Dish sockets from ZMQ are planned to replace the pub/sub ones, we could already make them available using the same interface and remove the pub/sub ones when deprecated.

Currently, each server (or subscriber) in simple is limited to one callback. This means that if I have more than one service (on the same node), I need to create an additional server object, which uses an additional port. For servers with many services, this could result in a complex configuration--especially if there is other server software running on that node and competing for the ports.

It would be nice to have a single server object and then add multiple services to it like this:

// assume service callbacks are defined here

int main(int argc, char * argv[]) try
{
   simple::Service<simple_msgs::Point> service1{ service1_callback };
   simple::Service<simple_msgs::Image> service2{ service2_callback };
   simple::Server server{ "tcp://*:5000", {service1, service2} };
   std::cout << "Press a key to exit.\n";
   std::cin.get();
   return 0;
}

We should provide Doxygen documentation that is automatically created during the CMake generation step. This should be an optional step for users.
Have a look how it is done in SUPRA.

This would make it very easy to redistribute the library

Rather use forward declaration and move the includes to the cpp files.
This and the static linkage would allow to distribute SIMPLE binaries without the dependencies.

It looks like request-reply is currently symmetric. In other words, the entire object is sent to the server, which processes the object, and then the entire object is sent back to the client. I have a scenario where the request object is large, and the reply is only a boolean. My application is performance-critical, so I want to minimize the data which is returned to the client. It would be helpful if simple could support an asymmetric request-reply like this:

bool server_callback(simple_msgs::HugeObject & ho)
{
   bool status = true;
   //process the ho, and set status accordingly
   return status;
}

int main(int argc, char * argv[]) try
{
   simple::Server<simple_msgs::HugeObject, bool> server{ "tcp://*:5000", server_callback };
   std::cout << "Press a key to exit.\n";
   std::cin.get();
   return 0;
}

I have several issues with the image data field:

To completely specify positioning of an image in 3D space, it should have an additional vector describing the pixel scaling of each axis - that way, together with the rest of the pose fields, it is possible to fully map from a pixel to 3D space even if there is non-uniform scaling, which is common for some modalities. This might already be stored with the image resolution field, but I cannot really find out what the supposed semantics are due to missing docs. If so, maybe this could be renamed to pixel scaling etc. for clarity, image resolution usually refers to the number of pixels in each dimension...

Furthermore, there are some fields which are unnecessarily signed:
width/height/depth/channels/image_size have no (obvious) need for a sign bit, so I would suggest making them unsigned unless there is some specific meaning related to negative numbers.

Moving a subscriber directly after initialization crashes when the publisher is already alive.

In the example image subscriber, if you change the lines to:

  simple::Subscriber<simple_msgs::Image<uint8_t>> sub{"tcp://localhost:6021", example_callback};
  simple::Subscriber<simple_msgs::Image<uint8_t>> sub2{std::move(sub)};

The output is

Creating a subscriber for Image messages.
Assertion failed: !_more (D:\lib\vcpkg\buildtrees\zeromq\src\cfd4629ebf-3440bac6e9\src\fq.cpp:113)
Assertion failed: !_more (D:\lib\vcpkg\buildtrees\zeromq\src\cfd4629ebf-3440bac6e9\src\fq.cpp:113)

Platform: Windows 10

Code is still not entirely documented, we should add Doxygen style documentation when possible and inline documentation when code is unclear.

Not a real issue, I just put it here for documentation.

The new thread-safe singleton ContextManager class (see #22), triggers an error in libzmq when that is build with Tweetnacl.

Tweetnacl is the a library for cryptography used by libzmq as default option in their CMake, but it is not recommended for production code.
A better alternative is to build libzmq with libsodium and use that.

Anyhow, since we don't use any of those, we can just build libzmq without any support for cryptography.
That means turning off the CMake option ENABLE_CURVE (i.e. -DENABLE_CURVE=OFF).

More on this at zeromq/libzmq#2991

Wiki pages about installation will change according to this and refer to this issue.

This is required for windows when using simple from a dll (not using simple as a dll).

In this case, the zmq context has to be shut down BEFORE the dll is unloaded, because otherwise the winsock dll might be unloaded by the time the conctext is destroyed. This will cause a "Assertion failed: Successful WSASTARTUP not yet performed" exception on application shutdown.

Thus, in this scenario we need a way to explicitly shut down the context from DllMain before the dll is completely unloaded.

see also this issue:
zeromq/libzmq#1708

Currently whenever the option to build tests in SIMPLE is set to true, everything is built with code coverage options. Those also include flags to prevent optimization.
These options should be only be active when one wants to generate infos on code coverage, e.g. do that only when the build type is COVERAGE

Most methods of the message classes do not have the parameter names in the declarations in the header files.
Missing more elaborate doxygen strings on the parameters as well, in some places it is hard to figure out the semantics of the constructor or method parameters.

Example:

/**
 * @brief Construct a Header message using the given parameters.
 */
Header(int, const std::string&, long long);

Without looking at the source file, you can't really figure out what is what. Was the timestamp the first or the last parameter? Could the last param be the sequence number?

Suggested fix: Add all parameter names in the header files. I can do it but won't do the busy work if there was a conscious decision not to include the parameter names in the header files for some reason?