SBP-2: re-login/reconnect after rediscovery, not on a 100 ms timer

ASFWDriver/Protocols/SBP2/Session/LoginSession.cpp

@@ -242,8 +242,8 @@ void LoginSession::HandleBusReset(uint16_t newGeneration) noexcept {
             loginGeneration_ = newGeneration;
             fetchAgent_.Unbind();
             DeallocateResources();
-            SetState(LoginState::Idle);
-            ArmManagementTimer(100, [this]() { (void)Login(); });
+            loginID_ = 0;
+            SetState(LoginState::Suspended);
             break;
 
         case LoginState::LoggedIn:
@@ -260,7 +260,6 @@ void LoginSession::HandleBusReset(uint16_t newGeneration) noexcept {
             DeallocateResources();
             loginGeneration_ = newGeneration;
             SetState(LoginState::Suspended);
-            ArmManagementTimer(100, [this]() { (void)Reconnect(); });
             break;
 
         case LoginState::LoggingOut:

ASFWDriver/Protocols/SBP2/Session/SessionRegistry.cpp

@@ -453,7 +453,8 @@ void SessionRegistry::RefreshTargets(Discovery::Generation gen) {
         session->AbandonSuspended();
     }
     for (auto& session : toReconnect) {
-        (void)session->Reconnect();
+        if (session->LoginID() == 0) (void)session->Login();
+        else (void)session->Reconnect();
     }
 }
 

tests/protocols/LoginSessionTests.cpp

@@ -477,61 +477,40 @@ TEST(LoginSessionTests, BusyTimeoutReplayCancelsInFlightWrite) {
     ExpectBusyTimeoutWrite(rig.bus.WriteAt(reconnectWriteIndex + 1), kTargetNodeID);
 }
 
-TEST(LoginSessionTests, BusResetWhileLoggingInRetriesLoginAfterDelay) {
+TEST(LoginSessionTests, BusResetWhileLoggingInWaitsForDiscoveryBeforeRetry) {
     SessionRig rig;
-
     ASSERT_TRUE(rig.session.Login());
-    ASSERT_EQ(LoginState::LoggingIn, rig.session.State());
-    ASSERT_EQ(1u, rig.bus.WriteCount());
-
     rig.bus.SetGeneration(ASFW::FW::Generation{2});
     rig.session.HandleBusReset(2);
-    EXPECT_EQ(LoginState::Idle, rig.session.State());
-
-    rig.AdvanceMs(99);
+    EXPECT_EQ(LoginState::Suspended, rig.session.State());
+    rig.AdvanceMs(1000);
     EXPECT_EQ(1u, rig.bus.WriteCount());
-
-    rig.AdvanceMs(1);
-    EXPECT_EQ(LoginState::LoggingIn, rig.session.State());
+    auto info = rig.session.TargetInfo();
+    info.targetNodeId = 3;
+    rig.session.Configure(info);
+    ASSERT_TRUE(rig.session.Login());
     EXPECT_EQ(2u, rig.bus.WriteCount());
+    EXPECT_EQ(3u, rig.bus.WriteAt(1).nodeId.value);
     EXPECT_EQ(2u, rig.session.Generation());
 }
 
-TEST(LoginSessionTests, BusResetWhileLoggingInDefersRetryOnScheduler) {
-    SessionRig rig;
-
-    ASSERT_TRUE(rig.session.Login());
-    rig.bus.SetGeneration(ASFW::FW::Generation{2});
-    rig.session.HandleBusReset(2);
-
-    // The retry is deferred on the scheduler, not run inline.
-    EXPECT_GT(rig.scheduler.PendingCount(), 0u);
-    EXPECT_EQ(LoginState::Idle, rig.session.State());
-    EXPECT_EQ(1u, rig.bus.WriteCount());
-}
-
-TEST(LoginSessionTests, BusResetWhileReconnectingRetriesReconnectAfterDelay) {
+TEST(LoginSessionTests, BusResetWhileReconnectingWaitsForFreshNodeBeforeRetry) {
     SessionRig rig;
     rig.LoginSuccessfully();
-
     rig.bus.SetGeneration(ASFW::FW::Generation{2});
     rig.session.HandleBusReset(2);
-    ASSERT_EQ(LoginState::Suspended, rig.session.State());
     ASSERT_TRUE(rig.session.Reconnect());
-    ASSERT_EQ(LoginState::Reconnecting, rig.session.State());
-    ASSERT_EQ(2u, rig.session.Generation());
-    ASSERT_EQ(3u, rig.bus.WriteCount());
-
     rig.bus.SetGeneration(ASFW::FW::Generation{3});
     rig.session.HandleBusReset(3);
     EXPECT_EQ(LoginState::Suspended, rig.session.State());
-
-    rig.AdvanceMs(99);
+    rig.AdvanceMs(1000);
     EXPECT_EQ(3u, rig.bus.WriteCount());
-
-    rig.AdvanceMs(1);
-    EXPECT_EQ(LoginState::Reconnecting, rig.session.State());
+    auto info = rig.session.TargetInfo();
+    info.targetNodeId = 3;
+    rig.session.Configure(info);
+    ASSERT_TRUE(rig.session.Reconnect());
     EXPECT_EQ(4u, rig.bus.WriteCount());
+    EXPECT_EQ(3u, rig.bus.WriteAt(3).nodeId.value);
     EXPECT_EQ(3u, rig.session.Generation());
 }
 

tests/protocols/SessionRegistryTests.cpp

@@ -1285,4 +1285,32 @@ TEST(SessionRegistryTests, LoginStateObserverStaysSilentOnBusResetSuspend) {
     EXPECT_TRUE(events.empty());
 }
 
+// A reset during login invalidates the node ID the login was addressed to.
+// Apple and Linux both re-login after the device is re-enumerated
+// (IOFireWireSBP2Login::resumeNotify, sbp2_update); so does RefreshTargets.
+TEST(SessionRegistryTests, BusResetDuringLoginLogsInAgainAfterRediscovery) {
+    SessionRegistryRig rig;
+    const uint64_t handle = rig.CreateSession();
+    ASSERT_TRUE(rig.registry.StartLogin(SessionRegistryRig::Owner(), handle));
+    ASSERT_EQ(1u, rig.bus.PendingWriteCount());
+
+    rig.registry.OnBusReset(2);
+    rig.deviceManager.SuspendAllForBusReset();
+    rig.AdvanceMs(1000);
+    EXPECT_EQ(1u, rig.bus.WriteCount());
+    auto state = rig.registry.GetSessionState(SessionRegistryRig::Owner(), handle);
+    ASSERT_TRUE(state.has_value());
+    EXPECT_EQ(ASFW::Protocols::SBP2::LoginState::Suspended, state->loginState);
+
+    rig.bus.SetGeneration(ASFW::FW::Generation{2});
+    rig.UpsertDevice(Generation{2}, 0x33);
+    rig.registry.RefreshTargets(Generation{2});
+
+    ASSERT_EQ(2u, rig.bus.WriteCount());
+    EXPECT_EQ(0x33u, rig.bus.WriteAt(1).nodeId.value);
+    state = rig.registry.GetSessionState(SessionRegistryRig::Owner(), handle);
+    ASSERT_TRUE(state.has_value());
+    EXPECT_EQ(ASFW::Protocols::SBP2::LoginState::LoggingIn, state->loginState);
+}
+
 } // namespace