Async Api; static DataParser

[mwopfner]

Hi,

i was in contact with Nicolas Loeffler before the official release of the lib and he has provided me with an preview version of the lib. I integrated the lib in our Software (non ROS) and made some changes to the lib I now propose to add to the mainline driver, because I think they offer some benefits.

This pull request is mainly to start the discussion what and how we could integrate some, or all changes into the mainline driver.

The main things I have clanged are:

• Made all DataParser method to be static methods, because they do not work on class variables. Thus the class must be not instantiated and no memory must be allocated.
• Made the Getters in DataStructure to return a const &. This prevents unnecessary data copy.
• I moved all logging includes into the cpp files. If logging includes are in the header file the c++ using the lib must also define WITHOUT_ROS_LOGGING. Otherwise the ROS headers are included. With all logging headers only in the cpp this is not required. I also added a WITHOUT_LOGGING define that disables all logging output.
• BUGFIX: uint32 was used for start/end angle in CommSettings but the protocol description from SICK uses int32 for this values. Thus it was not possible to use a negative start angle.
• I added a asynchronous interface to SickSafetyscannersBase which allows the lib to be used in synchronous and asynchronous use cases.
• I did some cleanup with the use of the io_service because it was sometimes not used as proposed in the boost docu
• I also added the possibility to inject an external io_service which does not create an additional thread within the lib

I hope to discuss the proposed changes further so that we can improve the lib together.

[lenpuc]

Hello, thank you very much for your contribution. I am happy to discuss this further and integrate suitable changes. I will need some time to look through the changes. In general they sound like improvements. Since the PR is marked as a draft and work in progress, what is still open? Or is this mainly due to be discussed?

[mwopfner]

The reason why it is a draft is to indicate that i want to discuss the changes :-). The proposed features are completely implemented and also tested with a MicroScan3 Laserscanner. Further I would like to document the public methods of SickSafetyscannersBase because right now, this docu is missing. If the async api is to be added I also would like to add an example how to use the synchronous and asynchronous api. If you have any questions regarding the proposed source code fell free to ask.

[lenpuc]

Sorry for the delayed answer. I'll try to adress all your changes and my thoughts, questions on them. Just to clarify the intended use.

Made all DataParser method to be static methods, because they do not work on class variables. Thus the class must be not instantiated and no memory must be allocated.

Sounds like a good improvement.

 Made the Getters in DataStructure to return a const &. This prevents unnecessary data copy.

Same here, this was actually planned to be implemented by me at some stage as well.

 I moved all logging includes into the cpp files. If logging includes are in the header file the c++ using the lib must also define WITHOUT_ROS_LOGGING. Otherwise the ROS headers are included. With all logging headers only in the cpp this is not required. I also added a WITHOUT_LOGGING define that disables all logging output.

So there are three options now? Ros_Logging, Logging and no Logging? This originates since the c++ standalone is based on the ros driver. in the future there probably should just an overlaying ros api. And for the movement of the includes. Make sense, to not have the define in the c++ using the lib again. So thanks for pointing this out.

 BUGFIX: uint32 was used for start/end angle in CommSettings but the protocol description from SICK uses int32 for this values. Thus it was not possible to use a negative start angle.

Thanks, I seem to have missed that.

 I added a asynchronous interface to SickSafetyscannersBase which allows the lib to be used in synchronous and asynchronous use cases.

Could you elaborate a bit more on this? The async interface stores the data and has to be polled then? Or how Is this meant to be?

 I did some cleanup with the use of the io_service because it was sometimes not used as proposed in the boost docu

I'll test this, but sounds like an improvement. Thank you

 I also added the possibility to inject an external io_service which does not create an additional thread within the lib

Could you elaborate a bit more here as well? The external io is created in an overlaying c++? So you basically pass the io service to the lib and therefore create the io thread externally?

All in all I think your contribution make sense and I am happy to include them. I'll still need to test them and will have a look through the code.

[mwopfner]

I also added the possibility to inject an external io_service which does not create an additional thread within the lib

Could you elaborate a bit more here as well? The external io is created in an overlaying c++? So you basically pass the io service to the lib and therefore create the io thread externally?

Yes that is correct. In our use case for example we have a software component that has already an io_service on which one or more threads work (call the io_service::run() method). Thus it is desirable to just use the existing io_service and not create a new one in the lib.

Thus the SickSafetyscannersBase has now two constructors: SickSafetyscannersBase(const packetReceivedCallbackFunction& newPacketReceivedCallbackFunction );

SickSafetyscannersBase(boost::asio::io_service& io_service, const packetReceivedCallbackFunction& newPacketReceivedCallbackFunction);

The first constructor creates an internal io_service and thread. The second constructor uses an external io_service and no internal thread is created.

I also replaced the bool run(); with a boost::system::error_code start(const sick::datastructure::CommSettings& settings); method and added a new void stop(); method.

The start() and stop() method allow to start/stop the driver without destructing the complete SickSafetyscannersBase instance and is in my opinion a clearer interface. Further the start method returns a boost::system:error_code which contains information about the reason in case of a start failure.

I think the replacement of the run() method is the only braking change I made to the lib interface, because I could not find an other good solution for this. The rest of the interface should be compatible to the exiting one,

[mwopfner]

I moved all logging includes into the cpp files. If logging includes are in the header file the c++ using the lib must also define WITHOUT_ROS_LOGGING. Otherwise the ROS headers are included. With all logging headers only in the cpp this is not required. I also added a WITHOUT_LOGGING define that disables all logging output.

So there are three options now? Ros_Logging, Logging and no Logging? This originates since the c++ standalone is based on the ros driver. in the future there probably should just an overlaying ros api. And for the movement of the includes. Make sense, to not have the define in the c++ using the lib again. So thanks for pointing this out.

Yes, correct, there are now three options. In my case I completely disable the logging (WITHOUT_LOGGING), because the asynchronous interface uses the boost::system::error_code functionality like it is used in asio. This provides additional information in case of an error an allows the caller to react on error conditions. The synchronous API just print log messages but does not return any error information. To have this functionality for synchronous methods the easiest way would be to change the return type from void to boost::system::error_code.

[mwopfner]

I added a asynchronous interface toSickSafetyscannersBasewhich allows the lib to be used in synchronous and asynchronous use cases.

Could you elaborate a bit more on this? The async interface stores the data and has to be polled then? Or how Is this meant to be?

The async interface is analog to the async interface boost::asio uses. You provide the data as a parameter (also return values are passed as a parameter) and a handler that should be called if the async operation is completed or an error occurred (thus the handler function has a boost::system::error_code as parameter). So the process is as follow:

1. start async request by calling the async method
2. wait until the handler is called
3. make call to the next async method (maybe also parallel calls are supported by the sensor but I have to check this and will document it in the SickSafetyscannersBase doc).

So no polling is required for the async api.

I also changed the internal implementation of the sync methods to use the async api to prevent code duplication or different functionality if one interface is forgotten. This is done by using the promise/future functionality from C++11.

For example the code for the sync requestTypeCode() method is:

{
  CompletePromisePtr promise = std::make_shared<CompletePromise>();

  asyncRequestTypeCode(settings, type_code, [promise](const boost::system::error_code& ec) {
    promise->set_value(ec);
  });

  promise->get_future().wait();
}

1. Create a promise that is resolved in the lambda handler of the async method.
2. Block the calling thread by calling the wait method of the future object.

[mwopfner]

Just wanted to ask if you had time to look into the changes or if I should change something before it could be merged.

[lenpuc]

Hi, we are currently working on a refactoring of the driver. This refactoring also includes a synchronous interface. So most likely there will be cherry picks of your pull request in an upcoming release here.

[mwopfner]

Thanks for your reply. I think you wanted to write "includes an asynchronous interface" :wink:. I will wait until this new release or a beta and test it when it is available.

[lenpuc]

Hi, the update is finally released, now the base class provides an synchronous and an asynchronous interface. I hope this will be to your needs.

[mwopfner]

I will check it and give you my feedback.