Skip to content

Commit 7bcc9c5

Browse files
Fix historical event detection in MTRDevice.
A few fixes here: * Fix MTRDeviceTests to clean up MTRDevice between tests, so each test starts with a known clean slate. * Fix the "are we getting a priming report?" detection in MTRDevice. Relying on reachability state is not correct, because unsolicited reports can mark us reachable even while we are in the middle of a priming report.
1 parent b653e8e commit 7bcc9c5

File tree

2 files changed

+193
-53
lines changed

2 files changed

+193
-53
lines changed

src/darwin/Framework/CHIP/MTRDevice.mm

+37-8
Original file line numberDiff line numberDiff line change
@@ -140,21 +140,34 @@ typedef NS_ENUM(NSUInteger, MTRInternalDeviceState) {
140140
// InitialSubscriptionEstablished means we have at some point finished setting up a
141141
// subscription. That subscription may have dropped since then, but if so it's the ReadClient's
142142
// responsibility to re-establish it.
143-
MTRInternalDeviceStateInitalSubscriptionEstablished = 2,
143+
MTRInternalDeviceStateInitialSubscriptionEstablished = 2,
144+
// Resubscribing means we had established a subscription, but then
145+
// detected a subscription drop due to not receiving a report on time. This
146+
// covers all the actions that happen when re-subscribing (discovery, CASE,
147+
// getting priming reports, etc).
148+
MTRInternalDeviceStateResubscribing = 3,
149+
// LaterSubscriptionEstablished meant that we had a subscription drop and
150+
// then re-created a subscription.
151+
MTRInternalDeviceStateLaterSubscriptionEstablished = 4,
144152
};
145153

146154
// Utility methods for working with MTRInternalDeviceState, located near the
147155
// enum so it's easier to notice that they need to stay in sync.
148156
namespace {
149157
bool HadSubscriptionEstablishedOnce(MTRInternalDeviceState state)
150158
{
151-
return state >= MTRInternalDeviceStateInitalSubscriptionEstablished;
159+
return state >= MTRInternalDeviceStateInitialSubscriptionEstablished;
152160
}
153161

154162
bool NeedToStartSubscriptionSetup(MTRInternalDeviceState state)
155163
{
156164
return state <= MTRInternalDeviceStateUnsubscribed;
157165
}
166+
167+
bool HaveSubscriptionEstablishedRightNow(MTRInternalDeviceState state)
168+
{
169+
return state == MTRInternalDeviceStateInitialSubscriptionEstablished || state == MTRInternalDeviceStateLaterSubscriptionEstablished;
170+
}
158171
} // anonymous namespace
159172

160173
typedef NS_ENUM(NSUInteger, MTRDeviceExpectedValueFieldIndex) {
@@ -878,6 +891,16 @@ - (void)_changeState:(MTRDeviceState)state
878891
}
879892
}
880893

894+
- (void)_changeInternalState:(MTRInternalDeviceState)state
895+
{
896+
os_unfair_lock_assert_owner(&self->_lock);
897+
MTRInternalDeviceState lastState = _internalDeviceState;
898+
_internalDeviceState = state;
899+
if (lastState != state) {
900+
MTR_LOG_DEFAULT("%@ internal state change %lu => %lu", self, static_cast<unsigned long>(lastState), static_cast<unsigned long>(state));
901+
}
902+
}
903+
881904
// First Time Sync happens 2 minutes after reachability (this can be changed in the future)
882905
#define MTR_DEVICE_TIME_UPDATE_INITIAL_WAIT_TIME_SEC (60 * 2)
883906
- (void)_handleSubscriptionEstablished
@@ -886,7 +909,11 @@ - (void)_handleSubscriptionEstablished
886909

887910
// reset subscription attempt wait time when subscription succeeds
888911
_lastSubscriptionAttemptWait = 0;
889-
_internalDeviceState = MTRInternalDeviceStateInitalSubscriptionEstablished;
912+
if (HadSubscriptionEstablishedOnce(_internalDeviceState)) {
913+
[self _changeInternalState:MTRInternalDeviceStateLaterSubscriptionEstablished];
914+
} else {
915+
[self _changeInternalState:MTRInternalDeviceStateInitialSubscriptionEstablished];
916+
}
890917

891918
// As subscription is established, check if the delegate needs to be informed
892919
if (!_delegateDeviceCachePrimedCalled) {
@@ -913,7 +940,7 @@ - (void)_handleSubscriptionError:(NSError *)error
913940
{
914941
std::lock_guard lock(_lock);
915942

916-
_internalDeviceState = MTRInternalDeviceStateUnsubscribed;
943+
[self _changeInternalState:MTRInternalDeviceStateUnsubscribed];
917944
_unreportedEvents = nil;
918945

919946
[self _changeState:MTRDeviceStateUnreachable];
@@ -924,6 +951,7 @@ - (void)_handleResubscriptionNeeded
924951
std::lock_guard lock(_lock);
925952

926953
[self _changeState:MTRDeviceStateUnknown];
954+
[self _changeInternalState:MTRInternalDeviceStateResubscribing];
927955

928956
// If we are here, then the ReadClient either just detected a subscription
929957
// drop or just tried again and failed. Either way, count it as "tried and
@@ -1044,11 +1072,12 @@ - (void)_handleReportBegin
10441072

10451073
_receivingReport = YES;
10461074
if (_state != MTRDeviceStateReachable) {
1047-
_receivingPrimingReport = YES;
10481075
[self _changeState:MTRDeviceStateReachable];
1049-
} else {
1050-
_receivingPrimingReport = NO;
10511076
}
1077+
1078+
// If we currently don't have an established subscription, this must be a
1079+
// priming report.
1080+
_receivingPrimingReport = !HaveSubscriptionEstablishedRightNow(_internalDeviceState);
10521081
}
10531082

10541083
- (NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)_clusterDataToPersistSnapshot
@@ -1461,7 +1490,7 @@ - (void)_setupSubscription
14611490
return;
14621491
}
14631492

1464-
_internalDeviceState = MTRInternalDeviceStateSubscribing;
1493+
[self _changeInternalState:MTRInternalDeviceStateSubscribing];
14651494

14661495
// Set up a timer to mark as not reachable if it takes too long to set up a subscription
14671496
MTRWeakReference<MTRDevice *> * weakSelf = [MTRWeakReference weakReferenceWithObject:self];

0 commit comments

Comments
 (0)