From 47684984e853e45f908a0fb6de21f344e1e639fa Mon Sep 17 00:00:00 2001 From: yunhanw-google Date: Mon, 22 Nov 2021 19:25:28 -0800 Subject: [PATCH] Fix subscription liveness check (#11795) --- src/app/ReadClient.cpp | 16 ++++++++++--- src/app/ReadHandler.cpp | 29 ++++++++++++++++++---- src/app/ReadHandler.h | 1 + src/app/reporting/Engine.cpp | 26 +++++++++++++------- src/messaging/ExchangeContext.cpp | 40 ++++++++++++++++++++++++++----- src/messaging/ExchangeContext.h | 9 +++++++ 6 files changed, 99 insertions(+), 22 deletions(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index fa36ef4ca98e5f..4073e5721e6529 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -559,10 +559,15 @@ CHIP_ERROR ReadClient::ProcessEventReportIBs(TLV::TLVReader & aEventReportIBsRea CHIP_ERROR ReadClient::RefreshLivenessCheckTimer() { + CHIP_ERROR err = CHIP_NO_ERROR; CancelLivenessCheckTimer(); - ChipLogProgress(DataManagement, "Refresh LivenessCheckTime with %d seconds", mMaxIntervalCeilingSeconds); - CHIP_ERROR err = InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->StartTimer( - System::Clock::Seconds16(mMaxIntervalCeilingSeconds), OnLivenessTimeoutCallback, this); + VerifyOrReturnError(mpExchangeCtx != nullptr, err = CHIP_ERROR_INCORRECT_STATE); + + System::Clock::Timeout timeout = System::Clock::Seconds16(mMaxIntervalCeilingSeconds) + mpExchangeCtx->GetAckTimeout(); + // EFR32/MBED/INFINION/K32W's chrono count return long unsinged, but other platform returns unsigned + ChipLogProgress(DataManagement, "Refresh LivenessCheckTime with %lu milliseconds", static_cast(timeout.count())); + err = InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->StartTimer( + timeout, OnLivenessTimeoutCallback, this); if (err != CHIP_NO_ERROR) { @@ -685,6 +690,11 @@ CHIP_ERROR ReadClient::SendSubscribeRequest(ReadPrepareParams & aReadPreparePara mpExchangeCtx = mpExchangeMgr->NewContext(aReadPrepareParams.mSessionHandle, this); VerifyOrExit(mpExchangeCtx != nullptr, err = CHIP_ERROR_NO_MEMORY); mpExchangeCtx->SetResponseTimeout(kImMessageTimeout); + if (mpExchangeCtx->IsBLETransport()) + { + ChipLogError(DataManagement, "IM Subscribe cannot work with BLE"); + SuccessOrExit(err = CHIP_ERROR_INCORRECT_STATE); + } err = mpExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::SubscribeRequest, std::move(msgBuf), Messaging::SendFlags(Messaging::SendMessageFlags::kExpectResponse)); diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index b93000a1cad0f9..5e3ce4ae9f2f78 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -602,21 +602,40 @@ CHIP_ERROR ReadHandler::ProcessSubscribeRequest(System::PacketBufferHandle && aP return CHIP_NO_ERROR; } +void ReadHandler::OnUnblockHoldReportCallback(System::Layer * apSystemLayer, void * apAppState) +{ + VerifyOrReturn(apAppState != nullptr); + ReadHandler * readHandler = static_cast(apAppState); + ChipLogProgress(DataManagement, "Unblock report hold after min %d seconds", readHandler->mMinIntervalFloorSeconds); + readHandler->mHoldReport = false; + if (readHandler->mDirty) + { + InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun(); + } + InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->StartTimer( + System::Clock::Seconds16(readHandler->mMaxIntervalCeilingSeconds - readHandler->mMinIntervalFloorSeconds), + OnRefreshSubscribeTimerSyncCallback, readHandler); +} + void ReadHandler::OnRefreshSubscribeTimerSyncCallback(System::Layer * apSystemLayer, void * apAppState) { - ReadHandler * aReadHandler = static_cast(apAppState); - aReadHandler->mHoldReport = false; + VerifyOrReturn(apAppState != nullptr); InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun(); } CHIP_ERROR ReadHandler::RefreshSubscribeSyncTimer() { - ChipLogProgress(DataManagement, "ReadHandler::Refresh Subscribe Sync Timer with %d seconds", mMinIntervalFloorSeconds); + ChipLogProgress(DataManagement, "ReadHandler::Refresh Subscribe Sync Timer with %d seconds", mMaxIntervalCeilingSeconds); + InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->CancelTimer( + OnUnblockHoldReportCallback, this); InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->CancelTimer( OnRefreshSubscribeTimerSyncCallback, this); mHoldReport = true; - return InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->StartTimer( - System::Clock::Seconds16(mMinIntervalFloorSeconds), OnRefreshSubscribeTimerSyncCallback, this); + ReturnErrorOnFailure( + InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->StartTimer( + System::Clock::Seconds16(mMinIntervalFloorSeconds), OnUnblockHoldReportCallback, this)); + + return CHIP_NO_ERROR; } } // namespace app } // namespace chip diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index 052906382b9e00..5c474ca703e76e 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -157,6 +157,7 @@ class ReadHandler : public Messaging::ExchangeDelegate AwaitingReportResponse, ///< The handler has sent the report to the client and is awaiting a status response. }; + static void OnUnblockHoldReportCallback(System::Layer * apSystemLayer, void * apAppState); static void OnRefreshSubscribeTimerSyncCallback(System::Layer * apSystemLayer, void * apAppState); CHIP_ERROR RefreshSubscribeSyncTimer(); CHIP_ERROR SendSubscribeResponse(); diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index 9ee976d65e42f7..b03544f5c5bad5 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -447,7 +447,15 @@ CHIP_ERROR Engine::SetDirty(ClusterInfo & aClusterInfo) // chunk for read interactions. if (handler.IsGeneratingReports() || handler.IsAwaitingReportResponse()) { - handler.SetDirty(); + for (auto clusterInfo = handler.GetAttributeClusterInfolist(); clusterInfo != nullptr; + clusterInfo = clusterInfo->mpNext) + { + if (aClusterInfo.IsAttributePathSupersetOf(*clusterInfo) || clusterInfo->IsAttributePathSupersetOf(aClusterInfo)) + { + handler.SetDirty(); + break; + } + } } } if (!InteractionModelEngine::GetInstance()->MergeOverlappedAttributePath(mpGlobalDirtySet, aClusterInfo) && @@ -468,20 +476,22 @@ void Engine::UpdateReadHandlerDirty(ReadHandler & aReadHandler) { return; } + + bool intersected = false; for (auto clusterInfo = aReadHandler.GetAttributeClusterInfolist(); clusterInfo != nullptr; clusterInfo = clusterInfo->mpNext) { - bool intersected = false; for (auto path = mpGlobalDirtySet; path != nullptr; path = path->mpNext) { if (path->IsAttributePathSupersetOf(*clusterInfo) || clusterInfo->IsAttributePathSupersetOf(*path)) { intersected = true; + break; } } - if (!intersected) - { - aReadHandler.ClearDirty(); - } + } + if (!intersected) + { + aReadHandler.ClearDirty(); } } @@ -504,8 +514,8 @@ void Engine::OnReportConfirm() } }; // namespace reporting -}; // namespace app -}; // namespace chip +} // namespace app +} // namespace chip void __attribute__((weak)) MatterPreAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath) {} void __attribute__((weak)) MatterPostAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath) {} diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index a3da1cdc2b43e4..b7ca10169f56dc 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -124,12 +124,7 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp // an error arising below. at the end, we have to close it. ExchangeHandle ref(*this); - // If sending via UDP and NoAutoRequestAck send flag is not specificed, - // request reliable transmission. - const Transport::PeerAddress * peerAddress = GetSessionHandle().GetPeerAddress(mExchangeMgr->GetSessionManager()); - // Treat unknown peer address as "not UDP", because we have no idea whether - // it's safe to do MRP there. - bool isUDPTransport = peerAddress && peerAddress->GetTransportType() == Transport::Type::kUdp; + bool isUDPTransport = IsUDPTransport(); // this check is ignored by the ExchangeMsgDispatch if !AutoRequestAck() bool reliableTransmissionRequested = isUDPTransport && !sendFlags.Has(SendMessageFlags::kNoAutoRequestAck); @@ -511,5 +506,38 @@ void ExchangeContext::MessageHandled() Close(); } +bool ExchangeContext::IsUDPTransport() +{ + const Transport::PeerAddress * peerAddress = GetSessionHandle().GetPeerAddress(mExchangeMgr->GetSessionManager()); + return peerAddress && peerAddress->GetTransportType() == Transport::Type::kUdp; +} + +bool ExchangeContext::IsTCPTransport() +{ + const Transport::PeerAddress * peerAddress = GetSessionHandle().GetPeerAddress(mExchangeMgr->GetSessionManager()); + return peerAddress && peerAddress->GetTransportType() == Transport::Type::kTcp; +} + +bool ExchangeContext::IsBLETransport() +{ + const Transport::PeerAddress * peerAddress = GetSessionHandle().GetPeerAddress(mExchangeMgr->GetSessionManager()); + return peerAddress && peerAddress->GetTransportType() == Transport::Type::kBle; +} + +System::Clock::Milliseconds32 ExchangeContext::GetAckTimeout() +{ + System::Clock::Timeout timeout; + if (IsUDPTransport()) + { + timeout = System::Clock::Milliseconds32((CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS + 1) * + (GetIdleRetransmitTimeoutTick() << CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT)); + } + else if (IsTCPTransport()) + { + // TODO: issue 12009, need actual tcp margin value considering restransmission + timeout = System::Clock::Seconds16(30); + } + return timeout; +} } // namespace Messaging } // namespace chip diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h index 449a9ffcb5463d..0b5e18e4f69475 100644 --- a/src/messaging/ExchangeContext.h +++ b/src/messaging/ExchangeContext.h @@ -166,6 +166,15 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen void SetResponseTimeout(Timeout timeout); + /* + * Get the overall acknowledge timeout period for the underneath transport(MRP+UDP/TCP) + */ + System::Clock::Milliseconds32 GetAckTimeout(); + + bool IsUDPTransport(); + bool IsTCPTransport(); + bool IsBLETransport(); + private: Timeout mResponseTimeout{ 0 }; // Maximum time to wait for response (in milliseconds); 0 disables response timeout. ExchangeDelegate * mDelegate = nullptr;