diff --git a/ASFWDriver/Protocols/SBP2/Session/LoginSession.cpp b/ASFWDriver/Protocols/SBP2/Session/LoginSession.cpp index dff85a8a..a0a963f3 100644 --- a/ASFWDriver/Protocols/SBP2/Session/LoginSession.cpp +++ b/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: diff --git a/ASFWDriver/Protocols/SBP2/Session/SessionRegistry.cpp b/ASFWDriver/Protocols/SBP2/Session/SessionRegistry.cpp index e26cc24d..2c3d9930 100644 --- a/ASFWDriver/Protocols/SBP2/Session/SessionRegistry.cpp +++ b/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(); } } diff --git a/tests/protocols/LoginSessionTests.cpp b/tests/protocols/LoginSessionTests.cpp index 97fb7cfd..50b3c63e 100644 --- a/tests/protocols/LoginSessionTests.cpp +++ b/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()); } diff --git a/tests/protocols/SessionRegistryTests.cpp b/tests/protocols/SessionRegistryTests.cpp index 0444668a..45df9527 100644 --- a/tests/protocols/SessionRegistryTests.cpp +++ b/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