search

gazebosim / gz-transport

Transport library for component communication based on publication/subscription and service calls.
https://gazebosim.org
Apache License 2.0
29 stars 43 forks source link

Service requests without input parameter #45

Closed osrf-migration closed 8 years ago

osrf-migration commented 8 years ago

Original report (archived issue) by Jose Luis Rivero (Bitbucket: Jose Luis Rivero, GitHub: j-rivero).

Sometimes we want to receive some result but don't have any input parameter to send. We could use the msgs::Empty for not having to modify the wire format when sending a service request. We'd create new Request functions in the Node class without the input parameter (both synchronous and asynchronous versions make still sense).

osrf-migration commented 8 years ago

Original comment by Jose Luis Rivero (Bitbucket: Jose Luis Rivero, GitHub: j-rivero).

osrf-migration commented 8 years ago

Original comment by Jose Luis Rivero (Bitbucket: Jose Luis Rivero, GitHub: j-rivero).

osrf-migration commented 8 years ago

Original comment by nampi (Bitbucket: nampi).

Question 1:

I use discovery_v2 branch.

This is an example of code for a synchronous request. What do you think about this code? 

#!c++
/// \brief Request a new service without input parameter using a blocking call.
      public: template<typename T> bool Request(
        const std::string &_topic,
        const unsigned int &_timeout,
        T &_rep,
        bool &_result)
      {
        msgs::Empty req;
        return Request(_topic, req, _timeout, _rep, _result);
      }

I can copy the content of this method:

#!c++

      /// \brief Request a new service using a blocking call.
      public: template<typename T1, typename T2> bool Request(
        const std::string &_topic,
        const T1 &_req,
        const unsigned int &_timeout,
        T2 &_rep,
        bool &_result);

As I understand I should create only req: 

#!c++

msgs::Empty req;

and then the same code.

osrf-migration commented 8 years ago

Original comment by nampi (Bitbucket: nampi).

Question 2:

I used your recommendations about Ignition-msgs library. I included the line #include <ignition/msgs.hh> in Node.hh file for importing the header with all the message definitions. I used the code from this page. The ign-transport was built successfully.

I have a problem. I want to build examples in example folder. I have an error:

#!c++

In file included from /usr/include/ignition/transport1/ignition/transport.hh:9:0,
                 from /home/user/code/tutorial_45/publisher.cc:24:
/usr/include/ignition/transport1/ignition/transport/Node.hh:37:28: fatal error: ignition/msgs.hh: No such file or directory
 #include <ignition/msgs.hh>
                            ^
compilation terminated.

I tried to use the changes in example folder from pub_cmd pull_request, but there was the same error. I am not familiar with make command. Could you explain how to solve this problem, please?

osrf-migration commented 8 years ago

Original comment by Carlos Agüero (Bitbucket: caguero, GitHub: caguero).

Answer 1: The signature of the function and the body looks good to me.

Answer 2: This is because our library is not exporting the ignition::messages dependency yet. When pull request #130 is merged your code should work. In the meantime, add the following code to your example/CMakeLists.txt:

#!c++

#################################################
# Find ign msgs library
find_package(ignition-msgs0 QUIET)
if (NOT ignition-msgs0_FOUND)
  message(FATAL_ERROR "Looking for ignition-msgs - not found")
else()
  message(STATUS "Looking for ignition-msgs - found")
  include_directories(${IGNITION-MSGS_INCLUDE_DIRS})
  link_directories(${IGNITION-MSGS_LIBRARY_DIRS})
endif()

Also add ${IGNITION-MSGS_LIBRARIES} to the target_link_libraries section of each sample executable that needs to link against Ignition Msgs.

The other part missing is the Advertise() function for advertising a service without the input parameter. The callback signature should be:

void (T2 &_rep, bool &_result)

Also, we should also allow an asynchronous Request().

osrf-migration commented 8 years ago

Original comment by nampi (Bitbucket: nampi).

Question 3:

@caguero I saw that you commented @amtj 's code:

"This lambda function should have the response parameter hardcoded to ignition::msgs::Empty. Then, we can call the "regular" Advertise function with two parameters registering our lambda function as callback. "

I tried it in my code. 

#!c++
      /// \brief Advertise a new service without input parameter.
      /// In this version the callback is a free function.
      public: template<typename T> bool Advertise(
        const std::string &_topic,
        void(*_cb)(T &_rep, bool &_result),
        const AdvertiseOptions &_options = AdvertiseOptions())
      {
        std::function<void(const msgs::Empty &, T &, bool &)> f =
          [_cb](T &_internalRep, bool &_internalResult)
        {
          (*_cb)(_internalRep, _internalResult);
        };
        return this->Advertise<msgs::Empty, T>(_topic, f, _options);
      }

It was built successfully, but I have an error compiling the examples.

#!c++

/usr/include/ignition/transport1/ignition/transport/Node.hh:332:9: error: conversion from ‘ignition::transport::Node::Advertise(const string&, void (*)(T&, bool&), const ignition::transport::AdvertiseOptions&) [with T = ignition::msgs::StringMsg; std::string = std::basic_string<char>]::__lambda8’ to non-scalar type ‘std::function<void(const ignition::msgs::Empty&, ignition::msgs::StringMsg&, bool&)>’ requested

I modified responser.cc:

#!c++

void srvEcho(ignition::msgs::StringMsg &_rep, bool &_result)
{
  _rep.set_data("HELLO");
  _result = true;
}

I think I have an error in lambda function, but I haven't found any examples how to define an additional parameter.

Question 2: Should I add the changes in example folder in my pr?

osrf-migration commented 8 years ago

Original comment by Carlos Agüero (Bitbucket: caguero, GitHub: caguero).

osrf-migration commented 8 years ago

Original comment by Carlos Agüero (Bitbucket: caguero, GitHub: caguero).

Question 3: The problem is that you're declaring the lambda type with three arguments, but then, your actual lambda only has two parameters. You're missing the input parameter _internalReq. It should look like this:

#!c++

std::function<void(const msgs::Empty &, T &, bool &)> f =
  [_cb](const msgs::Empty &_internalReq,
        T &_internalRep,
        bool &_internalResult)
{
  (*_cb)(_internalRep, _internalResult);
};

Note how we call the external callback (the user callback) in the body of the lambda function and we ignore the input parameter.

Question 2: It would be nice to have three new files in the example directory. One new responser, one new requester and one new requester_async that exercise this new functionality.

osrf-migration commented 8 years ago

Original comment by nampi (Bitbucket: nampi).

Question 4: I don't understand the logic of public: void WrongEcho() method and public: bool wrongCallbackSrvExecuted variable in src/Node_TEST.cc file in MyTestClass. I think that EXPECT_FALSE(this->wrongCallbackSrvExecuted); is always false in TestServiceCall(). Could you give some explanations, please?

Question 5: I have declined this pr, because it was not ready for review. How can I reopen it?

Question 6: What is the logic of the test test/integration/twoProcessesSrvCallStress.cc? Does it check that a lot of requests (15000) can be processed successfully?

Question 7: What is the req type in ServiceInfo() for service without input? ignition.msgs.Empty?

osrf-migration commented 8 years ago

Original comment by Jose Luis Rivero (Bitbucket: Jose Luis Rivero, GitHub: j-rivero).

4: As far as I can tell WrongEcho() is not being used by the code at all and you are right about the EXPECT_FALSE. I looked where the last commit in the source affecting WrongEcho( ) (blame function in bitbucket) was but no luck (62a46e5ef8b9402565d485773532531e1cdc8784)

5: You can not open two pull request using the same branches at the same time but once declined you can reopen it in the same way, as usual.

6: Reading the code I would say yes to your assumption. Which part is the one you don't understand?

7: is this the ServiceInfo() that you refer to? I can not find any req in the declaration.

osrf-migration commented 8 years ago

Original comment by nampi (Bitbucket: nampi).

5: I didn't understand how to reopen my previous pr. I created a new one.

Thanks! I think that all other questions are clear for me now.

osrf-migration commented 8 years ago

Original comment by Carlos Agüero (Bitbucket: caguero, GitHub: caguero).

See pull request #151

osrf-migration commented 8 years ago

Original comment by Carlos Agüero (Bitbucket: caguero, GitHub: caguero).

Done in pull request #151