eclipse-iceoryx / iceoryx

Eclipse iceoryx™ - true zero-copy inter-process-communication
https://iceoryx.io
Apache License 2.0
1.62k stars 383 forks source link

Help Wanted: Compiling issue when using listener with untypedSubscriber. #707

Closed ZhenshengLee closed 3 years ago

ZhenshengLee commented 3 years ago

Required information

Operating system: E.g. Ubuntu 20.04 LTS

Compiler version: E.g. GCC 9.3.0

Observed result or behaviour: according to example in iceoryx_examples/callbacks/ice_callbacks_subscriber.cpp I wrote the code using listener with untyppedSubscriber

listener_.attachEvent(port_receiver_, iox::popo::SubscriberEvent::DATA_RECEIVED, [&](iox::popo::UntypedSubscriber*){})
        .or_else([](auto) {
          RMW_SET_ERROR_MSG("unable to attach port_receiver_");
          std::cerr << "" << std::endl;
          std::terminate();
        });

but the gcc never compiles successfully

In file included from /home/zs/Programs/rmw_iceoryx/rmw_iceoryx_cpp/src/rmw_node.cpp:21:
/home/zs/Programs/rmw_iceoryx/rmw_iceoryx_cpp/src/./types/iceoryx_node.hpp: In constructor ‘IceoryxGraphChangeNotifier::IceoryxGraphChangeNotifier(rmw_guard_condition_t*)’:
/home/zs/Programs/rmw_iceoryx/rmw_iceoryx_cpp/src/./types/iceoryx_node.hpp:59:122: error: no matching function for call to ‘iox::popo::Listener::attachEvent(IceoryxGraphChangeNotifier::port_receiver_t&, iox::popo::SubscriberEvent, IceoryxGraphChangeNotifier::IceoryxGraphChangeNotifier(rmw_guard_condition_t*)::<lambda(iox::popo::UntypedSubscriber*)>)’
   59 |     listener_.attachEvent(port_receiver_, iox::popo::SubscriberEvent::DATA_RECEIVED, [&](iox::popo::UntypedSubscriber*){})
      |                                                                                                                          ^
In file included from /home/zs/Programs/rmw_iceoryx/rmw_iceoryx_cpp/src/./types/iceoryx_node.hpp:23,
                 from /home/zs/Programs/rmw_iceoryx/rmw_iceoryx_cpp/src/rmw_node.cpp:21:
/usr/local/include/iceoryx_posh/popo/listener.hpp:92:34: note: candidate: ‘template<class T> iox::cxx::expected<iox::popo::ListenerError> iox::popo::Listener::attachEvent(T&, iox::popo::Listener::CallbackRef_t<T>)’
   92 |     cxx::expected<ListenerError> attachEvent(T& eventOrigin, CallbackRef_t<T> eventCallback) noexcept;
      |                                  ^~~~~~~~~~~
/usr/local/include/iceoryx_posh/popo/listener.hpp:92:34: note:   template argument deduction/substitution failed:
In file included from /home/zs/Programs/rmw_iceoryx/rmw_iceoryx_cpp/src/rmw_node.cpp:21:
/home/zs/Programs/rmw_iceoryx/rmw_iceoryx_cpp/src/./types/iceoryx_node.hpp:59:122: note:   mismatched types ‘void(T*)’ and ‘iox::popo::SubscriberEvent’
   59 |     listener_.attachEvent(port_receiver_, iox::popo::SubscriberEvent::DATA_RECEIVED, [&](iox::popo::UntypedSubscriber*){})
      |                                                                                                                          ^
In file included from /home/zs/Programs/rmw_iceoryx/rmw_iceoryx_cpp/src/./types/iceoryx_node.hpp:23,
                 from /home/zs/Programs/rmw_iceoryx/rmw_iceoryx_cpp/src/rmw_node.cpp:21:
/usr/local/include/iceoryx_posh/popo/listener.hpp:105:5: note: candidate: ‘template<class T, class EventType, class> iox::cxx::expected<iox::popo::ListenerError> iox::popo::Listener::attachEvent(T&, EventType, iox::popo::Listener::CallbackRef_t<T>)’
  105 |     attachEvent(T& eventOrigin, const EventType eventType, CallbackRef_t<T> eventCallback) noexcept;
      |     ^~~~~~~~~~~
/usr/local/include/iceoryx_posh/popo/listener.hpp:105:5: note:   template argument deduction/substitution failed:
In file included from /home/zs/Programs/rmw_iceoryx/rmw_iceoryx_cpp/src/rmw_node.cpp:21:
/home/zs/Programs/rmw_iceoryx/rmw_iceoryx_cpp/src/./types/iceoryx_node.hpp:59:122: note:   mismatched types ‘void(T*)’ and ‘IceoryxGraphChangeNotifier::IceoryxGraphChangeNotifier(rmw_guard_condition_t*)::<lambda(iox::popo::UntypedSubscriber*)>’
   59 |     listener_.attachEvent(port_receiver_, iox::popo::SubscriberEvent::DATA_RECEIVED, [&](iox::popo::UntypedSubscriber*){})
      |                                                                                                                          ^
make[2]: *** [CMakeFiles/rmw_iceoryx_cpp.dir/build.make:244: CMakeFiles/rmw_iceoryx_cpp.dir/src/rmw_node.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:282: CMakeFiles/rmw_iceoryx_cpp.dir/all] Error 2
make: *** [Makefile:146: all] Error 2
---
Failed   <<< rmw_iceoryx_cpp [0.72s, exited with code 2]

Expected result or behaviour: the code can be compiled

Conditions where it occurred / Performed steps: compile the code

elfenpiff commented 3 years ago

@ZhenshengLee You have to be the owner of the callback since the callback is called sometime later in the background thread of the listener and the listener does not take ownership! If you construct a closure (lambda) in the call you implicitly assume that the listener takes ownership of the lambda since it goes out of scope right after the call.

With our restrictions (no heap usage), it is currently not possible to take ownership of callables since we know their size at runtime which either requires heap or a callable container that has preallocated memory. @MatthiasKillat is currently implementing this (https://github.com/eclipse-iceoryx/iceoryx/pull/473).

Until then you have to implement your functionality without lambdas like:

void myCallback(iox::popo::UntypedSubscriber*) {}

listener_.attachEvent(port_receiver_, iox::popo::SubscriberEvent::DATA_RECEIVED, myCallback)
        .or_else([](auto) {
          RMW_SET_ERROR_MSG("unable to attach port_receiver_");
          std::cerr << "" << std::endl;
          std::terminate();
        });

If you really want to use lambdas you can also write:

auto myCallback = [](iox::popo::UntypedSubscriber*) {};
void (decltype(myCallback)::*myCallbackPtr)(iox::popo::UntypedSubscriber*) const = &decltype(myCallback)::operator();

listener_.attachEvent(port_receiver_, iox::popo::SubscriberEvent::DATA_RECEIVED, *myCallbackPtr)
        .or_else([](auto) {
          RMW_SET_ERROR_MSG("unable to attach port_receiver_");
          std::cerr << "" << std::endl;
          std::terminate();
        });

But you are not allowed to use lambdas with capturing since they are sadly not convertable to c function pointers.

ZhenshengLee commented 3 years ago

@elfenpiff thank you for your quick reply! Actually I am trying to get rmw_iceoryx worked with iceoryx 1.0.0 I am not familiar with fuction programming even with cpp, and I found your callback solution and lambda solution both do not work in the situation when the callback is a class member function. Could you please write a little more? Much appreciation.

the compiler output shows below errors: when it is with common callback function which is a bit easier to understand I tried with std::bind and did make it better.

/home/zs/Programs/rmw_iceoryx/rmw_iceoryx_cpp/src/./types/iceoryx_node.hpp:68:114: error: invalid use of non-static member function ‘void IceoryxGraphChangeNotifier::callback(iox::popo::UntypedSubscriber*)’
   68 |     listener_.attachEvent(port_receiver_, iox::popo::SubscriberEvent::DATA_RECEIVED, IceoryxGraphChangeNotifier::callback)

when it comes with your lambda solution(sorry I need to read a book to understand the labmda solution)

/home/zs/Programs/rmw_iceoryx/rmw_iceoryx_cpp/src/./types/iceoryx_node.hpp:62:87: error: invalid use of unary ‘*’ on pointer to member
   62 |     listener_.attachEvent(port_receiver_, iox::popo::SubscriberEvent::DATA_RECEIVED, *myCallbackPtr)

here is the code(maybe from your team)

// We currently use the iceoryx port introspection
// which is updated whenever a sender port comes or goes or
// does OFFER / STOP_OFFER or a receiver port comes or goes or does SUB / UNSUB
// TODO(mphnl) check with the list of ros2 graph events

class IceoryxGraphChangeNotifier
{
public:
  explicit IceoryxGraphChangeNotifier(rmw_guard_condition_t * guard_condition)
  {
    if (!guard_condition || !guard_condition->data) {
      RMW_SET_ERROR_MSG("invalid input for GraphChangeNotifier");
      iceoryx_guard_condition_ = nullptr;
    } else {
      iceoryx_guard_condition_ = static_cast<IceoryxGuardCondition *>(guard_condition->data);
      RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
        IceoryxGraphChangeNotifier
        : guard_condition,
        guard_condition->implementation_identifier,
        rmw_get_implementation_identifier(),
        iceoryx_guard_condition_ = nullptr);
    }

    // subscribe with a callback for changes in the iceoryx graph
    // todo: change to the dds_common graph
    // todo: how to use listener with UntypedSubscriber
    // https://github.com/eclipse-iceoryx/iceoryx/issues/707
    // auto myCallback = [](iox::popo::UntypedSubscriber*) {};
    // void (decltype(myCallback)::*myCallbackPtr)(iox::popo::UntypedSubscriber*) const = &decltype(myCallback)::operator();
    // listener_.attachEvent(port_receiver_, iox::popo::SubscriberEvent::DATA_RECEIVED, *myCallbackPtr)
    //     .or_else([](auto) {
    //       RMW_SET_ERROR_MSG("unable to attach port_receiver_");
    //       std::cerr << "" << std::endl;
    //       std::terminate();
    //     });
    listener_.attachEvent(port_receiver_, iox::popo::SubscriberEvent::DATA_RECEIVED, IceoryxGraphChangeNotifier::callback)
        .or_else([](auto) {
          RMW_SET_ERROR_MSG("unable to attach port_receiver_");
          std::cerr << "" << std::endl;
          std::terminate();
        });
    port_receiver_.subscribe();
  }

  ~IceoryxGraphChangeNotifier()
  {
    listener_.detachEvent(port_receiver_, iox::popo::SubscriberEvent::DATA_RECEIVED);
    port_receiver_.unsubscribe();
  }

private:
  void callback(iox::popo::UntypedSubscriber*)
  {
      if (nullptr != iceoryx_guard_condition_) {
        iceoryx_guard_condition_->trigger();
      }
  }
  IceoryxGuardCondition * iceoryx_guard_condition_{nullptr};
  using port_receiver_t = iox::popo::UntypedSubscriber;
  port_receiver_t port_receiver_{iox::roudi::IntrospectionPortService};
  using listener_t = iox::popo::Listener;
  listener_t listener_;
};
elfenpiff commented 3 years ago

@ZhenshengLee you discovered a real bad issue on our side. At the moment this is not realizable with our current Listener implementation (except when you use really dirty hacks) but since 1.0 is not released yet I'll try to fix this until Wednesday.

Please be patient until then and hopefully, I can present you with a solution in two days.

The problem is when you would like to change a nonglobal variable or object inside of the callback you have to either capture it inside a lambda, which is forbidden, or have to allow callable objects, which is also not allowed.

What I will implement is that you can add an additional pointer that is provided to you inside the callback so that you have access to it.

Your code would then look like this:

//....
    listener_.attachEvent(port_receiver_, iox::popo::SubscriberEvent::DATA_RECEIVED, IceoryxGraphChangeNotifier::callback, this)
        .or_else([](auto) {
          RMW_SET_ERROR_MSG("unable to attach port_receiver_");
          std::cerr << "" << std::endl;
          std::terminate();
        });
//...
  static void callback(iox::popo::UntypedSubscriber*, IceoryxGraphChangeNotifier * self)
  {
      if (nullptr != iceoryx_guard_condition_) {
        self->iceoryx_guard_condition_->trigger();
      }
  }
ZhenshengLee commented 3 years ago

Thanks a lot! I will test it later when all commits are merge into master or release1.0.0 branch.

elfenpiff commented 3 years ago

@ZhenshengLee the feature is implemented and you can find an example here in master and release_1.0.

Here is how you would have to modify your example from this issue:

class IceoryxGraphChangeNotifier
{
public:
    //...

    // use iox::popo::createNotificationCallback(IceoryxGraphChangeNotifier::callback, *this) 
    // to attach the callback and provide the this pointer to gain access to IceoryxGraphChangeNotifier
    listener_.attachEvent(port_receiver_, iox::popo::SubscriberEvent::DATA_RECEIVED, 
iox::popo::createNotificationCallback(IceoryxGraphChangeNotifier::callback, *this))
        .or_else([](auto) {
          RMW_SET_ERROR_MSG("unable to attach port_receiver_");
          std::cerr << "" << std::endl;
          std::terminate();
        });
    port_receiver_.subscribe();
  }

//...

private:
  // must be a static method to be convertable to c function pointer
  // second argument (self) is the this pointer of the current object
  static void callback(iox::popo::UntypedSubscriber*, IceoryxGraphChangeNotifier * self)
  {
      if (self->nullptr != iceoryx_guard_condition_) {
        self->iceoryx_guard_condition_->trigger();
      }
  }

Could you please try it out - if this is working for you I would close the issue.

ZhenshengLee commented 3 years ago

Could you please try it out - if this is working for you I would close the issue.

It can be compiled. I will give it more tests.

Thank you very much!

mossmaurice commented 3 years ago

Closing this issue. @ZhenshengLee Feel free to re-open in case of other questions around that topic.