Improve error reporting in SDK/fairmq-dds-command-ui

fairmq/plugins/DDS/DDS.cxx

@@ -344,10 +344,10 @@ auto DDS::HandleCmd(const string& id, sdk::cmd::Cmd& cmd, const string& cond, ui
         case Type::change_state: {
             Transition transition = static_cast<ChangeState&>(cmd).GetTransition();
             if (ChangeDeviceState(transition)) {
-                Cmds outCmds(make<TransitionStatus>(id, fDDSTaskId, Result::Ok, transition));
+                Cmds outCmds(make<TransitionStatus>(id, fDDSTaskId, Result::Ok, transition, GetCurrentDeviceState()));
                 fDDS.Send(outCmds.Serialize(), to_string(senderId));
             } else {
-                Cmds outCmds(make<TransitionStatus>(id, fDDSTaskId, Result::Failure, transition));
+                Cmds outCmds(make<TransitionStatus>(id, fDDSTaskId, Result::Failure, transition, GetCurrentDeviceState()));
                 fDDS.Send(outCmds.Serialize(), to_string(senderId));
             }
             {

fairmq/sdk/Topology.h

@@ -421,13 +421,16 @@ class BasicTopology : public AsioBase<Executor, Allocator>
     auto HandleCmd(cmd::TransitionStatus const& cmd) -> void
     {
         if (cmd.GetResult() != cmd::Result::Ok) {
-            FAIR_LOG(error) << cmd.GetTransition() << " transition failed for " << cmd.GetDeviceId();
             DDSTask::Id taskId(cmd.GetTaskId());
             std::lock_guard<std::mutex> lk(fMtx);
             for (auto& op : fChangeStateOps) {
-                if (!op.second.IsCompleted() && op.second.ContainsTask(taskId) &&
+                if (!op.second.IsCompleted() && op.second.ContainsTask(taskId)) {
-                    fStateData.at(fStateIndex.at(taskId)).state != op.second.GetTargetState()) {
+                    if (fStateData.at(fStateIndex.at(taskId)).state != op.second.GetTargetState()) {
+                        FAIR_LOG(error) << cmd.GetTransition() << " transition failed for " << cmd.GetDeviceId() << ", device is in " << cmd.GetCurrentState() << " state.";
                         op.second.Complete(MakeErrorCode(ErrorCode::DeviceChangeStateFailed));
+                    } else {
+                        FAIR_LOG(debug) << cmd.GetTransition() << " transition failed for " << cmd.GetDeviceId() << ", device is already in " << cmd.GetCurrentState() << " state.";
+                    }
                 }
             }
         }

fairmq/sdk/commands/Commands.cxx

@@ -290,6 +290,7 @@ string Cmds::Serialize(const Format type) const
                 cmdBuilder->add_task_id(_cmd.GetTaskId());
                 cmdBuilder->add_result(GetFBResult(_cmd.GetResult()));
                 cmdBuilder->add_transition(GetFBTransition(_cmd.GetTransition()));
+                cmdBuilder->add_current_state(GetFBState(_cmd.GetCurrentState()));
             }
             break;
             case Type::config: {
@@ -445,7 +446,7 @@ void Cmds::Deserialize(const string& str, const Format type)
                 fCmds.emplace_back(make<CurrentState>(cmdPtr.device_id()->str(), GetMQState(cmdPtr.current_state())));
             break;
             case FBCmd_transition_status:
-                fCmds.emplace_back(make<TransitionStatus>(cmdPtr.device_id()->str(), cmdPtr.task_id(), GetResult(cmdPtr.result()), GetMQTransition(cmdPtr.transition())));
+                fCmds.emplace_back(make<TransitionStatus>(cmdPtr.device_id()->str(), cmdPtr.task_id(), GetResult(cmdPtr.result()), GetMQTransition(cmdPtr.transition()), GetMQState(cmdPtr.current_state())));
             break;
             case FBCmd_config:
                 fCmds.emplace_back(make<Config>(cmdPtr.device_id()->str(), cmdPtr.config_string()->str()));

fairmq/sdk/commands/Commands.h

@@ -50,7 +50,7 @@ enum class Type : int
     subscription_heartbeat,        // args: { interval }
 
     current_state,                 // args: { device_id, current_state }
-    transition_status,             // args: { device_id, task_id, Result, transition }
+    transition_status,             // args: { device_id, task_id, Result, transition, current_state }
     config,                        // args: { device_id, config_string }
     state_change_subscription,     // args: { device_id, task_id, Result }
     state_change_unsubscription,   // args: { device_id, task_id, Result }
@@ -188,12 +188,13 @@ struct CurrentState : Cmd
 
 struct TransitionStatus : Cmd
 {
-    explicit TransitionStatus(const std::string& deviceId, const uint64_t taskId, const Result result, const Transition transition)
+    explicit TransitionStatus(const std::string& deviceId, const uint64_t taskId, const Result result, const Transition transition, State currentState)
         : Cmd(Type::transition_status)
         , fDeviceId(deviceId)
         , fTaskId(taskId)
         , fResult(result)
         , fTransition(transition)
+        , fCurrentState(currentState)
     {}
 
     std::string GetDeviceId() const { return fDeviceId; }
@@ -204,12 +205,15 @@ struct TransitionStatus : Cmd
     void SetResult(const Result result) { fResult = result; }
     Transition GetTransition() const { return fTransition; }
     void SetTransition(const Transition transition) { fTransition = transition; }
+    fair::mq::State GetCurrentState() const { return fCurrentState; }
+    void SetCurrentState(fair::mq::State state) { fCurrentState = state; }
 
   private:
     std::string fDeviceId;
     uint64_t fTaskId;
     Result fResult;
     Transition fTransition;
+    fair::mq::State fCurrentState;
 };
 
 struct Config : Cmd

fairmq/sdk/commands/CommandsFormat.fbs

@@ -56,7 +56,7 @@ enum FBCmd:byte {
     subscription_heartbeat,        // args: { interval }
 
     current_state,                 // args: { device_id, current_state }
-    transition_status,             // args: { device_id, task_id, Result, transition }
+    transition_status,             // args: { device_id, task_id, Result, transition, current_state }
     config,                        // args: { device_id, config_string }
     state_change_subscription,     // args: { device_id, task_id, Result }
     state_change_unsubscription,   // args: { device_id, task_id, Result }

fairmq/sdk/runDDSCommandUI.cxx

@@ -58,12 +58,15 @@ void printControlsHelp()
 
 void handleCommand(const string& command, const string& path, unsigned int timeout, Topology& topo, const string& pKey, const string& pVal)
 {
+    std::pair<std::error_code, fair::mq::sdk::TopologyState> changeStateResult;
+
     if (command == "c") {
         cout << "> checking state of the devices" << endl;
         auto const result = topo.GetCurrentState();
         for (const auto& d : result) {
             cout << d.taskId << " : " << d.state << endl;
         }
+        return;
     } else if (command == "o") {
         cout << "> dumping config of " << (path == "" ? "all" : path) << endl;
         // TODO: extend this regex to return all properties, once command size limitation is removed.
@@ -73,6 +76,7 @@ void handleCommand(const string& command, const string& path, unsigned int timeo
                 cout << d.first << ": " << p.first << " : " << p.second << endl;
             }
         }
+        return;
     } else if (command == "p") {
         if (pKey == "" || pVal == "") {
             cout << "cannot send property with empty key and/or value! given key: '" << pKey << "', value: '" << pVal << "'." << endl;
@@ -83,42 +87,48 @@ void handleCommand(const string& command, const string& path, unsigned int timeo
         topo.SetProperties(props, path);
         // give dds time to complete request
         this_thread::sleep_for(chrono::milliseconds(100));
+        return;
     } else if (command == "i") {
         cout << "> initiating InitDevice transition --> " << (path == "" ? "all" : path) << endl;
-        topo.ChangeState(TopologyTransition::InitDevice, path, std::chrono::milliseconds(timeout));
+        changeStateResult = topo.ChangeState(TopologyTransition::InitDevice, path, std::chrono::milliseconds(timeout));
     } else if (command == "k") {
         cout << "> initiating CompleteInit transition --> " << (path == "" ? "all" : path) << endl;
-        topo.ChangeState(TopologyTransition::CompleteInit, path, std::chrono::milliseconds(timeout));
+        changeStateResult = topo.ChangeState(TopologyTransition::CompleteInit, path, std::chrono::milliseconds(timeout));
     } else if (command == "b") {
         cout << "> initiating Bind transition --> " << (path == "" ? "all" : path) << endl;
-        topo.ChangeState(TopologyTransition::Bind, path, std::chrono::milliseconds(timeout));
+        changeStateResult = topo.ChangeState(TopologyTransition::Bind, path, std::chrono::milliseconds(timeout));
     } else if (command == "x") {
         cout << "> initiating Connect transition --> " << (path == "" ? "all" : path) << endl;
-        topo.ChangeState(TopologyTransition::Connect, path, std::chrono::milliseconds(timeout));
+        changeStateResult = topo.ChangeState(TopologyTransition::Connect, path, std::chrono::milliseconds(timeout));
     } else if (command == "j") {
         cout << "> initiating InitTask transition --> " << (path == "" ? "all" : path) << endl;
-        topo.ChangeState(TopologyTransition::InitTask, path, std::chrono::milliseconds(timeout));
+        changeStateResult = topo.ChangeState(TopologyTransition::InitTask, path, std::chrono::milliseconds(timeout));
     } else if (command == "r") {
         cout << "> initiating Run transition --> " << (path == "" ? "all" : path) << endl;
-        topo.ChangeState(TopologyTransition::Run, path, std::chrono::milliseconds(timeout));
+        changeStateResult = topo.ChangeState(TopologyTransition::Run, path, std::chrono::milliseconds(timeout));
     } else if (command == "s") {
         cout << "> initiating Stop transition --> " << (path == "" ? "all" : path) << endl;
-        topo.ChangeState(TopologyTransition::Stop, path, std::chrono::milliseconds(timeout));
+        changeStateResult = topo.ChangeState(TopologyTransition::Stop, path, std::chrono::milliseconds(timeout));
     } else if (command == "t") {
         cout << "> initiating ResetTask transition --> " << (path == "" ? "all" : path) << endl;
-        topo.ChangeState(TopologyTransition::ResetTask, path, std::chrono::milliseconds(timeout));
+        changeStateResult = topo.ChangeState(TopologyTransition::ResetTask, path, std::chrono::milliseconds(timeout));
     } else if (command == "d") {
         cout << "> initiating ResetDevice transition --> " << (path == "" ? "all" : path) << endl;
-        topo.ChangeState(TopologyTransition::ResetDevice, path, std::chrono::milliseconds(timeout));
+        changeStateResult = topo.ChangeState(TopologyTransition::ResetDevice, path, std::chrono::milliseconds(timeout));
     } else if (command == "q") {
         cout << "> initiating End transition --> " << (path == "" ? "all" : path) << endl;
-        topo.ChangeState(TopologyTransition::End, path, std::chrono::milliseconds(timeout));
+        changeStateResult = topo.ChangeState(TopologyTransition::End, path, std::chrono::milliseconds(timeout));
     } else if (command == "h") {
         cout << "> help" << endl;
         printControlsHelp();
+        return;
     } else {
         cout << "\033[01;32mInvalid input: [" << command << "]\033[0m" << endl;
         printControlsHelp();
+        return;
+    }
+    if (changeStateResult.first != std::error_code()) {
+        cout << "ERROR: ChangeState failed for '" << path << "': " << changeStateResult.first.message() << endl;
     }
 }
 

test/commands/_commands.cxx

@@ -31,7 +31,7 @@ TEST(Format, Construction)
     Cmds setPropertiesCmds(make<SetProperties>(42, props));
     Cmds subscriptionHeartbeatCmds(make<SubscriptionHeartbeat>(60000));
     Cmds currentStateCmds(make<CurrentState>("somedeviceid", State::Running));
-    Cmds transitionStatusCmds(make<TransitionStatus>("somedeviceid", 123456, Result::Ok, Transition::Stop));
+    Cmds transitionStatusCmds(make<TransitionStatus>("somedeviceid", 123456, Result::Ok, Transition::Stop, State::Running));
     Cmds configCmds(make<Config>("somedeviceid", "someconfig"));
     Cmds stateChangeSubscriptionCmds(make<StateChangeSubscription>("somedeviceid", 123456, Result::Ok));
     Cmds stateChangeUnsubscriptionCmds(make<StateChangeUnsubscription>("somedeviceid", 123456, Result::Ok));
@@ -63,6 +63,7 @@ TEST(Format, Construction)
     ASSERT_EQ(static_cast<TransitionStatus&>(transitionStatusCmds.At(0)).GetTaskId(), 123456);
     ASSERT_EQ(static_cast<TransitionStatus&>(transitionStatusCmds.At(0)).GetResult(), Result::Ok);
     ASSERT_EQ(static_cast<TransitionStatus&>(transitionStatusCmds.At(0)).GetTransition(), Transition::Stop);
+    ASSERT_EQ(static_cast<TransitionStatus&>(transitionStatusCmds.At(0)).GetCurrentState(), State::Running);
     ASSERT_EQ(configCmds.At(0).GetType(), Type::config);
     ASSERT_EQ(static_cast<Config&>(configCmds.At(0)).GetDeviceId(), "somedeviceid");
     ASSERT_EQ(static_cast<Config&>(configCmds.At(0)).GetConfig(), "someconfig");
@@ -104,7 +105,7 @@ void fillCommands(Cmds& cmds)
     cmds.Add<SetProperties>(42, props);
     cmds.Add<SubscriptionHeartbeat>(60000);
     cmds.Add<CurrentState>("somedeviceid", State::Running);
-    cmds.Add<TransitionStatus>("somedeviceid", 123456, Result::Ok, Transition::Stop);
+    cmds.Add<TransitionStatus>("somedeviceid", 123456, Result::Ok, Transition::Stop, State::Running);
     cmds.Add<Config>("somedeviceid", "someconfig");
     cmds.Add<StateChangeSubscription>("somedeviceid", 123456, Result::Ok);
     cmds.Add<StateChangeUnsubscription>("somedeviceid", 123456, Result::Ok);
@@ -167,6 +168,7 @@ void checkCommands(Cmds& cmds)
                 ASSERT_EQ(static_cast<TransitionStatus&>(*cmd).GetTaskId(), 123456);
                 ASSERT_EQ(static_cast<TransitionStatus&>(*cmd).GetResult(), Result::Ok);
                 ASSERT_EQ(static_cast<TransitionStatus&>(*cmd).GetTransition(), Transition::Stop);
+                ASSERT_EQ(static_cast<TransitionStatus&>(*cmd).GetCurrentState(), State::Running);
             break;
             case Type::config:
                 ++count;