Skip to content

Commit

Permalink
Fix historical event detection in MTRDevice. (project-chip#33462)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bzbarsky-apple authored May 16, 2024
1 parent ef68373 commit bb12311
Show file tree
Hide file tree
Showing 2 changed files with 193 additions and 53 deletions.
45 changes: 37 additions & 8 deletions src/darwin/Framework/CHIP/MTRDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -140,21 +140,34 @@ typedef NS_ENUM(NSUInteger, MTRInternalDeviceState) {
// InitialSubscriptionEstablished means we have at some point finished setting up a
// subscription. That subscription may have dropped since then, but if so it's the ReadClient's
// responsibility to re-establish it.
MTRInternalDeviceStateInitalSubscriptionEstablished = 2,
MTRInternalDeviceStateInitialSubscriptionEstablished = 2,
// Resubscribing means we had established a subscription, but then
// detected a subscription drop due to not receiving a report on time. This
// covers all the actions that happen when re-subscribing (discovery, CASE,
// getting priming reports, etc).
MTRInternalDeviceStateResubscribing = 3,
// LaterSubscriptionEstablished meant that we had a subscription drop and
// then re-created a subscription.
MTRInternalDeviceStateLaterSubscriptionEstablished = 4,
};

// Utility methods for working with MTRInternalDeviceState, located near the
// enum so it's easier to notice that they need to stay in sync.
namespace {
bool HadSubscriptionEstablishedOnce(MTRInternalDeviceState state)
{
return state >= MTRInternalDeviceStateInitalSubscriptionEstablished;
return state >= MTRInternalDeviceStateInitialSubscriptionEstablished;
}

bool NeedToStartSubscriptionSetup(MTRInternalDeviceState state)
{
return state <= MTRInternalDeviceStateUnsubscribed;
}

bool HaveSubscriptionEstablishedRightNow(MTRInternalDeviceState state)
{
return state == MTRInternalDeviceStateInitialSubscriptionEstablished || state == MTRInternalDeviceStateLaterSubscriptionEstablished;
}
} // anonymous namespace

typedef NS_ENUM(NSUInteger, MTRDeviceExpectedValueFieldIndex) {
Expand Down Expand Up @@ -878,6 +891,16 @@ - (void)_changeState:(MTRDeviceState)state
}
}

- (void)_changeInternalState:(MTRInternalDeviceState)state
{
os_unfair_lock_assert_owner(&self->_lock);
MTRInternalDeviceState lastState = _internalDeviceState;
_internalDeviceState = state;
if (lastState != state) {
MTR_LOG_DEFAULT("%@ internal state change %lu => %lu", self, static_cast<unsigned long>(lastState), static_cast<unsigned long>(state));
}
}

// First Time Sync happens 2 minutes after reachability (this can be changed in the future)
#define MTR_DEVICE_TIME_UPDATE_INITIAL_WAIT_TIME_SEC (60 * 2)
- (void)_handleSubscriptionEstablished
Expand All @@ -886,7 +909,11 @@ - (void)_handleSubscriptionEstablished

// reset subscription attempt wait time when subscription succeeds
_lastSubscriptionAttemptWait = 0;
_internalDeviceState = MTRInternalDeviceStateInitalSubscriptionEstablished;
if (HadSubscriptionEstablishedOnce(_internalDeviceState)) {
[self _changeInternalState:MTRInternalDeviceStateLaterSubscriptionEstablished];
} else {
[self _changeInternalState:MTRInternalDeviceStateInitialSubscriptionEstablished];
}

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

_internalDeviceState = MTRInternalDeviceStateUnsubscribed;
[self _changeInternalState:MTRInternalDeviceStateUnsubscribed];
_unreportedEvents = nil;

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

[self _changeState:MTRDeviceStateUnknown];
[self _changeInternalState:MTRInternalDeviceStateResubscribing];

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

_receivingReport = YES;
if (_state != MTRDeviceStateReachable) {
_receivingPrimingReport = YES;
[self _changeState:MTRDeviceStateReachable];
} else {
_receivingPrimingReport = NO;
}

// If we currently don't have an established subscription, this must be a
// priming report.
_receivingPrimingReport = !HaveSubscriptionEstablishedRightNow(_internalDeviceState);
}

- (NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)_clusterDataToPersistSnapshot
Expand Down Expand Up @@ -1461,7 +1490,7 @@ - (void)_setupSubscription
return;
}

_internalDeviceState = MTRInternalDeviceStateSubscribing;
[self _changeInternalState:MTRInternalDeviceStateSubscribing];

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

0 comments on commit bb12311

Please sign in to comment.