robosoft-ai / SMACC2

An Event-Driven, Asynchronous, Behavioral State Machine Library for ROS2 (Robotic Operating System) applications written in C++
https://smacc.dev
Apache License 2.0
246 stars 41 forks source link

Service request failing #563

Open ghost opened 1 month ago

ghost commented 1 month ago

I am using the foxy branch.

I have a pair of states and a pair of transition to move from one to another. I want to send a service request at the exit of each state and entry of each state to another process hosting their service servers. I have kept the service request calls in the onEntry and onExit methods of both the states. Though the onEntry service request is processed successfully and a response is recieved, the onExit service call's are blocked indefinetly at the client side. On the server side for the service inside onExit, the callback exits gracefully, but no response is recived in the statemachine process.

Are we not meant to send service requests from the onExit methods of a state?

Also if I have a updatable state (multiple inheritance from SmaccStateBase and IsmaccUpdateable), even then I can not recive a response for the request sent from within the update function attached to the updatable state. It just blocks indefinitely.

ghost commented 1 month ago

On firther debugging, there were competing mutex lock

  1. https://github.com/robosoft-ai/SMACC2/blob/a64eaa05729b41d514570ef8d67dba9e8d4e5098/smacc2/include/smacc2/smacc_state_base.hpp#L128 2.https://github.com/robosoft-ai/SMACC2/blob/a64eaa05729b41d514570ef8d67dba9e8d4e5098/smacc2/src/smacc2/signal_detector.cpp#L233

I resolved it by scoping the mutex lock in 1 as

 void exit()
  {
    auto * derivedThis = static_cast<MostDerived *>(this);
    this->getStateMachine().notifyOnStateExitting(derivedThis);
    {
      {
        std::lock_guard<std::recursive_mutex> lock(this->getStateMachine().getMutex());
        this->getStateMachine().notifyOnStateExitting(derivedThis);
      }

      try
      {
        {
        std::lock_guard<std::recursive_mutex> lock(this->getStateMachine().getMutex());
        TRACEPOINT(smacc2_state_onExit_start, STATE_NAME);
        }
        // static_cast<MostDerived *>(this)->onExit();
        standardOnExit(*derivedThis);
        {
        std::lock_guard<std::recursive_mutex> lock(this->getStateMachine().getMutex());
        TRACEPOINT(smacc2_state_onExit_end, STATE_NAME);

        }
      }
      catch (...)
      {
      }
      std::lock_guard<std::recursive_mutex> lock(this->getStateMachine().getMutex());
      this->getStateMachine().notifyOnStateExited(derivedThis);
    }
  }

I am afraid it will cause unwanted side effects or this might not be the most elegant way to break the deadlock. Please let me know if this modification is in coherence with the design phiosophy?

yassiezar commented 1 month ago

Hi @emanodus

This is a similar issue to the one reported in #556. The issue was introduced due to a bad merge, so your solution is likely sound. Indeed, its similar to the one that was implemented in Humble (though its not been backported to Foxy yet). We'll look into backporting it soon