[Darwin] Surface granular pairing-failure context in MTRError + DNS-SD / BLE error preservation#72211
Open
woody-apple wants to merge 1 commit into
Conversation
pullapprove
Bot
added
review - pending
platform
darwin
lib
controller
core
protocols
and removed
platform
darwin
lib
controller
core
protocols
labels
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request enhances error reporting and diagnostic fidelity across the commissioning, attestation, DNS-SD, and BLE layers. It introduces several specific CHIP_ERROR codes (e.g., for CASE failures, DNS-SD NXDOMAIN, and ICD temporary unavailability) and surfaces detailed debug text and error values in CompletionStatus. Additionally, it exposes structured attestation verification results and device identifiers in Darwin's NSError userInfo. The review feedback highlights two important issues: a potential runtime crash in MTRDeviceAttestationDelegateBridge.mm when inserting nullable vendor/product IDs into an Objective-C dictionary literal, and a potential build-host path leak on Windows environments in MTRError.mm due to hardcoded forward-slash path separator checks.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR preserves more granular commissioning and pairing failure context across controller, secure-channel, DNS-SD, and Darwin-facing error surfaces.
Changes:
- Adds new CASE, DNS-SD, and ICD-specific
CHIP_ERROR_*values and error-string coverage. - Extends commissioning status propagation with debug text, operational certificate status, network error values, Wi-Fi band capability data, and ICD LIT state.
- Enriches Darwin
NSErroruserInfo and BLE metrics with structured underlying failure details.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/protocols/secure_channel/PASESession.cpp |
Maps additional secure-channel status reports to specific CHIP errors. |
src/protocols/secure_channel/CASESession.cpp |
Refines CASE Sigma failure mapping and logs session-resumption fallback. |
src/platform/Darwin/dnssd/DnssdError.cpp |
Adds Darwin DNS-SD mappings for NXDOMAIN, service-not-running, and timeout. |
src/platform/Darwin/BleConnectionDelegateImpl.mm |
Preserves CoreBluetooth error codes in BLE metrics. |
src/lib/core/tests/TestCHIPErrorStr.cpp |
Adds string-test coverage for new CHIP error constants. |
src/lib/core/CHIPError.h |
Defines new public CHIP error constants. |
src/lib/core/CHIPError.cpp |
Adds human-readable descriptions for new CHIP errors. |
src/darwin/Framework/CHIP/MTRError.mm |
Adds richer CHIP error metadata to bridged NSError objects. |
src/darwin/Framework/CHIP/MTRError.h |
Declares new public NSError userInfo keys. |
src/darwin/Framework/CHIP/MTRError_Internal.h |
Adds an internal bridge overload for extra userInfo. |
src/darwin/Framework/CHIP/MTRDeviceAttestationDelegateBridge.mm |
Adds attestation result and VID/PID context to attestation failure errors. |
src/controller/DevicePairingDelegate.h |
Adds structured CompletionStatus commissioning-failure callback overload. |
src/controller/CommissioningDelegate.h |
Extends commissioning report/status structs with additional failure context. |
src/controller/CHIPDeviceController.cpp |
Preserves new commissioning context and reads supported Wi-Fi bands. |
src/controller/AutoCommissioner.cpp |
Propagates new report fields into final commissioning completion status. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
woody-apple
added a commit
to woody-apple/connectedhomeip
that referenced
this pull request
May 27, 2026
…p#72211 Two fixes per project-chip#72211 review: * MTRDeviceAttestationDelegateBridge.mm: switch the attestationUserInfo construction from an NSDictionary literal to a mutable dictionary with nil checks. NSDictionary literal syntax @{key:value} raises NSInvalidArgumentException on a nil value; the @(uint16_t) boxing for the VID/PID NSNumbers always succeeds today, but defensive construction guards against future DeviceInfoForAttestation struct changes that could make those fields nullable. * MTRError.mm: handle Windows-style backslash path separators in errorCode.GetFile() basename stripping. CHIP builds run on Windows for some embedded controller targets; without the fallback to strrchr(file, '\\'), full Windows build-host paths would land in MTRCHIPErrorSourceFileKey, defeating the privacy intent. Both flagged by gemini-code-assist's automated review.
|
PR #72211: Size comparison from b3a691c to bae572a Full report (5 builds for cc32xx, realtek, stm32)
|
andy31415
reviewed
May 28, 2026
| AES_CCM_decrypt(parsedSigma2.msgR2EncryptedPayload.data(), parsedSigma2.msgR2EncryptedPayload.size(), nullptr, 0, | ||
| parsedSigma2.msgR2MIC.data(), parsedSigma2.msgR2MIC.size(), sr2k.KeyHandle(), kTBEData2_Nonce, | ||
| kTBEDataNonceLength, parsedSigma2.msgR2EncryptedPayload.data()); | ||
| VerifyOrReturnError(aesErr == CHIP_NO_ERROR, CHIP_ERROR_CASE_SIGMA_DECRYPT_FAILURE); |
Contributor
There was a problem hiding this comment.
I think this is approach is generally frowned upon: dropping the original error (which contains also an original source) and translating it to a different one.
What we would really want is the equivalent of exceptions and some "origin" information, however that is probably too expensive for embedded.
How about having some form of ChipError::MapErrorTo() that would map any chip-error to the given subsitution error, but on large devices (linux/mac/android) we can choose to also log the original error so we do not lose it?
Then I can read ReturnErrorOnFailure(.... .MapErrorTo(CHIP_ERROR_SIGMA_DECRYPT_FAILURE)) and on large devices I would have a propper log. Maybe we can incur the cost of a call on all platforms and log these transforms.
Comment applies for all other places too.
Contributor
Author
There was a problem hiding this comment.
(FYI: This is Claude, on behalf of @woody-apple) Good call — agreed that translating without preserving the original drops useful diagnostic context. Implemented ChipError::MapErrorTo(target) in 1c0a11c:
- Logs the original error (and source location when
CHIP_CONFIG_ERROR_SOURCEis enabled) before returning the target code, so the underlying cause survives in diagnostics. - Compiles to nothing when
CHIP_DETAIL_LOGGING=0, so it stays cheap on resource-constrained builds.
Applied to all six CASE translation sites (Sigma2/Sigma3 AES decrypt, NOC chain, ECDSA signature). Pattern is now:
ReturnErrorOnFailure(aesErr.MapErrorTo(CHIP_ERROR_CASE_SIGMA_DECRYPT_FAILURE));Let me know if you'd prefer a different naming or want logging via a separate log id.
Contributor
Author
There was a problem hiding this comment.
(FYI: This is Claude — follow-up) Quick update: had to drop the in-method logging from MapErrorTo in 632d730. Adding the support:text_only_logging dep to lib/core:error created a GN cycle:
core:error -> support:text_only_logging -> support:memory -> core:error
The method now just performs the substitution. Callers that want diagnostic preservation can log the original error themselves at the call site before invoking MapErrorTo.
If you'd prefer the original-error-preserving behavior to live in core, we could add a registered logging hook (function pointer the caller installs from a layer with logging access) — happy to do that if it's worth the API surface. Otherwise the current shape is just a named, explicit substitution helper.
Contributor
There was a problem hiding this comment.
I think in practice nobody will actually do the logging then, so essentially this pattern will become a "we drop the original error to the floor, including any logging.
Could there be alternatives?
- figure out if we can fix the loop (may be hard indeed, since CHIP_ERROR is very low level)
- a macro, like we have for the Verify and Return macros, so I have
ReturnErrorOnFailure(RemapErrorsTo(..., CHIP_ERROR...)))and this can do logging - Have large platforms support a "original error chain" somehow (however then maybe logging is more complex and I am unsure how long the chain could be)
I think macro is the most short-term viable (but keep the MapErrorTo function as well, it is readable)
Contributor
There was a problem hiding this comment.
Another idea would be on big platforms to have something like std::unique_ptr<CHIP_ERROR> mReason and then we can build chains given that large platforms have heap.
I am also questioning these changes somewhat: the description of the PR complains that several errors are being collapsed into a common error. Yet this is exactly what this code does more: takes an error code that could be diverse and collapses it. So I wonder if this change should be split out with a different description instead.
Contributor
Author
There was a problem hiding this comment.
(FYI: This is Claude) Split done — the CASE Sigma2/Sigma3 error-code additions and the VerifyCredentials remap have moved to #72227, where they can be reviewed and merged on their own merits. The remap-vs-preserve-original tradeoff is exactly what that PR's description addresses. This PR (#72211) is now scoped to the structured-context plumbing and no longer touches CASESession.cpp or adds CASE error codes.
woody-apple
added a commit
to woody-apple/connectedhomeip
that referenced
this pull request
May 28, 2026
…URCE Per Copilot review on PR project-chip#72211: - ChipError::GetFile() / GetLine() are only defined when CHIP is built with CHIP_CONFIG_ERROR_SOURCE; builds with that flag disabled fail to compile. Wrap the source-info userInfo block under the same #if so the keys are simply absent on those builds. - Initialize legacyAdditionalUserInfo to nil at declaration. ARC zero-inits __strong locals already, but the explicit nil makes the contract clear.
|
PR #72211: Size comparison from 8a8893e to e6ab8f0 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72211: Size comparison from ff181cb to 640d368 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72211: Size comparison from 4894db9 to 421c2bb Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72211: Size comparison from 4894db9 to e31a329 Full report (21 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32)
|
|
PR #72211: Size comparison from 8a162c6 to 74553e8 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72211: Size comparison from 8a162c6 to ccafbbd Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72211: Size comparison from 8a162c6 to 1bbc964 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72211: Size comparison from 8a162c6 to f989eb7 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72211: Size comparison from 8a162c6 to f0dc9dd Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72211: Size comparison from 8a162c6 to 6962ec9 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72211: Size comparison from 8a162c6 to b92d51f Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72211: Size comparison from 8a162c6 to ab53774 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72211: Size comparison from 8a162c6 to e4fdf39 Full report (5 builds for cc32xx, realtek, stm32)
|
|
PR #72211: Size comparison from cb6d219 to 4fe8baa Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72211: Size comparison from cb6d219 to f34abd5 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72211: Size comparison from ca3e67c to 727d7e2 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72211: Size comparison from 9cf1485 to 3bfe220 Full report (22 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, esp32, nrfconnect, psoc6, qpg, realtek, stm32)
|
|
PR #72211: Size comparison from 9cf1485 to 38cf510 Full report (25 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32)
|
|
PR #72211: Size comparison from 3ef818f to 95a9253 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
…D / BLE error preservation Consumers of MTRError currently see most commissioning failures surface as a single "Error Domain=com.apple.MatterSupport Code=1 (null)". This change plumbs structured detail into the bridged NSError so callers can disambiguate failure modes, and tightens Darwin's CoreBluetooth / DNS-SD error mapping at the same time. Scope: this commit is intentionally limited to the Darwin framework (MTRError), two new core DNS-SD CHIP_ERROR codes + their descriptions, and the Darwin platform glue (DnssdError mapping, BLE error preservation). It is additive only - it touches 13 files and reverts nothing on master. The earlier core-controller plumbing (CompletionStatus widening, DevicePairingDelegate overload, PASE/CASE status-code mapping) is already merged on master via project-chip#72227/project-chip#72228 and is NOT part of this commit. Darwin framework (MTRError): - Add NSError userInfo keys with unprefixed string values (MTR_PROVISIONALLY_AVAILABLE): MTRAttestationVerificationResultKey (= @"attestationVerificationResult") MTRDeviceBasicInformationVendorIDKey (= @"deviceBasicInformationVendorID") MTRDeviceBasicInformationProductIDKey (= @"deviceBasicInformationProductID") MTRUnderlyingErrorCodeKey (= @"errorCode" - same string as the key already documented for MTRErrorCodeGeneralError). The Device*BasicInformation* names reflect what the keys actually store: the VID/PID the device asserts in its BasicInformation cluster, which is NOT guaranteed to match the device's certification declaration (mismatch is itself one of the attestation-failure modes). - Surface AttestationVerificationResult enum + BasicInformation VID/PID in attestation-failure NSErrors. - New errorForCHIPErrorCode:logContext:additionalUserInfo: overload lets bridge call sites attach attestation metadata without losing the existing MTRErrorHolder association, and merges additionalUserInfo on both the IM-status and core-error bridge paths (framework keys win). - NSError (Matter) category: mtr_underlyingMatterErrorSourceFile and mtr_underlyingMatterErrorSourceLine read source location on demand from the existing MTRErrorHolder associated object via a single helper. DNS-SD codes (src/lib/core): - New CHIP_ERROR_DNSSD_NXDOMAIN (0xBE) - DNS-SD operational instance does not exist. - New CHIP_ERROR_DNSSD_SERVICE_NOT_RUNNING (0xC7) - DNS-SD platform service is not running. - Description arms in CHIPError.cpp; test entries in TestCHIPErrorStr.cpp; ERROR_CODES.md regenerated. Platform/Darwin: - DnssdError.cpp: map kDNSServiceErr_NoSuchName / NoSuchRecord to CHIP_ERROR_DNSSD_NXDOMAIN, kDNSServiceErr_ServiceNotRunning to CHIP_ERROR_DNSSD_SERVICE_NOT_RUNNING, kDNSServiceErr_Timeout to CHIP_ERROR_TIMEOUT (instead of collapsing to CHIP_ERROR_INTERNAL). - BleConnectionDelegateImpl.mm: WrapCBErrorCodeAsKOS preserves the CoreBluetooth NSError.code in a kOS-range CHIP_ERROR for triage, with a 24-bit overflow guard that falls back to the cross-platform BLE_ERROR_GATT_* sentinel; the existing HandleConnectionError contract is unchanged for non-Darwin consumers. Tests: - MTRErrorMappingTests.m (new): MTRUnderlyingErrorCodeKey population + round-trip via errorToCHIPIntegerCode:, pins all 4 userInfo key string values (rename guard), proves the new key resolves to the @"errorCode" string already documented for MTRErrorCodeGeneralError, and guards additionalUserInfo merge on both the IM-status and core-error paths. - MTRErrorTests.m: basename-only source-path assertions + MTRErrorBasenameForPath mixed-separator coverage. - TestCHIPErrorStr.cpp: the 2 new DNS-SD codes (harness asserts each listed code yields a non-default description). Known test-coverage limitations: - DnssdError.cpp's DNSServiceErrorType -> CHIP_ERROR mapping has no direct unit test: src/platform/Darwin has no unit-test target, and standing one up for a 3-case switch is disproportionate. The underlying error codes are unit-tested in TestCHIPErrorStr.cpp. - WrapCBErrorCodeAsKOS is a file-local static inline; testing it directly would require exposing a triage helper in the platform API surface, which is not worth the API cost for a defensive overflow guard (CoreBluetooth codes are documented small non-negative integers). - Attestation-key population in MTRDeviceAttestationDelegateBridge.mm requires an end-to-end attestation failure to exercise and is not unit tested. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
PR #72211: Size comparison from b468d0f to ac3b723 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
bzbarsky-apple
requested changes
Jun 9, 2026
Comment on lines
+230
to
+231
| * For MTRInteractionErrorDomain errors the source location reflects the StatusIB | ||
| * encoding site inside the Matter SDK, not the device-reported origin of the status. |
Contributor
There was a problem hiding this comment.
I'm not sure this is right. A StatusIB does not carry a source location with it... So presumably this would reflect the place where the StatusIB was converted to CHIP_ERROR or something? Might need some re-wording here.
|
|
||
| /** | ||
| * The source-line where the underlying Matter error was originally created. Returns 0 | ||
| * when not available. |
Contributor
There was a problem hiding this comment.
Suggested change
| * when not available. | |
| * when not available. Intended for triage; not stable across releases. |
| return [MTRError errorForIMStatus:status]; | ||
| NSError * imError = [MTRError errorForIMStatus:status]; | ||
| if (imError != nil) { | ||
| // Merge additionalUserInfo into the IM-domain NSError if any was supplied so callers |
Contributor
There was a problem hiding this comment.
Suggested change
| // Merge additionalUserInfo into the IM-domain NSError if any was supplied so callers | |
| // Merge additionalUserInfo, if any was supplied, into the IM-domain NSError, so that callers |
| NSMutableDictionary<NSErrorUserInfoKey, id> * mergedUserInfo = | ||
| [NSMutableDictionary dictionaryWithDictionary:additionalUserInfo]; | ||
| [mergedUserInfo addEntriesFromDictionary:(imError.userInfo ?: @{})]; | ||
| mergedUserInfo[MTRUnderlyingErrorCodeKey] = @(errorCode.AsInteger()); |
Contributor
There was a problem hiding this comment.
This looks wrong. If we have no additionalUserInfo this will not add the MTRUnderlyingErrorCodeKey entry to the dictionary, which seems wrong.
Note that errorForIMStatus does NOT add MTRUnderlyingErrorCodeKey.
The right logic here should probably just create a new dictionary unconditionally, copy additionalUserInfo into it if present, copy the error's current userInfo if present, set MTRUnderlyingErrorCodeKey (with a comment right before that line that we want it to reflect our errorCode, not whatever happens to be in additionalUserInfo or the userInfo created by errorForIMStatus), then create a clone of the error, etc.
Comment on lines
+126
to
+127
| // Concretely: bridge a CHIP error that has no specific MTRErrorCode mapping (so we land in | ||
| // the GeneralError fallback) and confirm both spellings return the same NSNumber. |
Contributor
There was a problem hiding this comment.
Why is this test useful given the XCTAssertEqualObjects above?
Comment on lines
+219
to
+221
| id underlying = bridged.userInfo[MTRUnderlyingErrorCodeKey]; | ||
| XCTAssertTrue([underlying isKindOfClass:[NSNumber class]]); | ||
| XCTAssertEqual([(NSNumber *) underlying unsignedIntValue], kIntegrityCheckFailed); |
Contributor
There was a problem hiding this comment.
This seems like a complicated way to write:
Suggested change
| id underlying = bridged.userInfo[MTRUnderlyingErrorCodeKey]; | |
| XCTAssertTrue([underlying isKindOfClass:[NSNumber class]]); | |
| XCTAssertEqual([(NSNumber *) underlying unsignedIntValue], kIntegrityCheckFailed); | |
| XCTAssertEqualObjects(bridged.userInfo[MTRUnderlyingErrorCodeKey], @(kIntegrityCheckFailed)); |
Comment on lines
+241
to
+243
| id underlying = bridged.userInfo[MTRUnderlyingErrorCodeKey]; | ||
| XCTAssertTrue([underlying isKindOfClass:[NSNumber class]]); | ||
| XCTAssertEqual([(NSNumber *) underlying unsignedIntValue], kCBOperationCancelled); |
Contributor
There was a problem hiding this comment.
Again, why not XCTAssertEqualObjects here?
Comment on lines
+258
to
+259
| uint32_t code1 = [(NSNumber *) err1.userInfo[MTRUnderlyingErrorCodeKey] unsignedIntValue]; | ||
| uint32_t code2 = [(NSNumber *) err2.userInfo[MTRUnderlyingErrorCodeKey] unsignedIntValue]; |
Contributor
There was a problem hiding this comment.
Could avoid the casting and unsignedIntValue if the tests below used XCTAssertNotEqualObjects/XCTAssertEqualObjects.
Comment on lines
+293
to
+297
| id underlying = bridged.userInfo[MTRUnderlyingErrorCodeKey]; | ||
| XCTAssertTrue([underlying isKindOfClass:[NSNumber class]], | ||
| @"MTRUnderlyingErrorCodeKey missing for %@ (chip 0x%08X)", label, chipCode); | ||
| NSNumber * key = (NSNumber *) underlying; | ||
| XCTAssertEqual([key unsignedIntValue], chipCode, @"%@ key value mismatch", label); |
Contributor
There was a problem hiding this comment.
Suggested change
| id underlying = bridged.userInfo[MTRUnderlyingErrorCodeKey]; | |
| XCTAssertTrue([underlying isKindOfClass:[NSNumber class]], | |
| @"MTRUnderlyingErrorCodeKey missing for %@ (chip 0x%08X)", label, chipCode); | |
| NSNumber * key = (NSNumber *) underlying; | |
| XCTAssertEqual([key unsignedIntValue], chipCode, @"%@ key value mismatch", label); | |
| XCTAssertEqualObjects(bridged.userInfo[MTRUnderlyingErrorCodeKey], samples[label], label); |
Comment on lines
+513
to
+515
| uint32_t a = [(NSNumber *) nxdomain.userInfo[MTRUnderlyingErrorCodeKey] unsignedIntValue]; | ||
| uint32_t b = [(NSNumber *) serviceNotRunning.userInfo[MTRUnderlyingErrorCodeKey] unsignedIntValue]; | ||
| uint32_t c = [(NSNumber *) unauthorized.userInfo[MTRUnderlyingErrorCodeKey] unsignedIntValue]; |
Contributor
There was a problem hiding this comment.
Could avoid casts and unsignedIntValue if you just use XCTAssertNotEqualObjects.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[Darwin] Surface granular pairing-failure context in MTRError + DNS-SD / BLE error preservation
Consumers of MTRError currently see most commissioning failures surface as a
single "Error Domain=com.apple.MatterSupport Code=1 (null)". This change plumbs
structured detail into the bridged NSError so callers can disambiguate failure
modes, and tightens Darwin's CoreBluetooth / DNS-SD error mapping at the same
time.
Scope: this commit is intentionally limited to the Darwin framework
(MTRError), two new core DNS-SD CHIP_ERROR codes + their descriptions, and the
Darwin platform glue (DnssdError mapping, BLE error preservation). It is
additive only - it touches 13 files and reverts nothing on master. The earlier
core-controller plumbing (CompletionStatus widening, DevicePairingDelegate
overload, PASE/CASE status-code mapping) is already merged on master via
#72227/#72228 and is NOT part of this commit.
Darwin framework (MTRError):
(MTR_PROVISIONALLY_AVAILABLE):
MTRAttestationVerificationResultKey (= @"attestationVerificationResult")
MTRAttestationDeviceVendorIDKey (= @"deviceVendorID")
MTRAttestationDeviceProductIDKey (= @"deviceProductID")
MTRUnderlyingErrorCodeKey (= @"errorCode" - same string as the key
already documented for MTRErrorCodeGeneralError, so existing readers of
userInfo[@"errorCode"] keep working).
attestation-failure NSErrors (VID/PID are what the device asserts; they may
not match the device's certification).
call sites attach attestation metadata without losing the existing
MTRErrorHolder association, and merges additionalUserInfo on both the
IM-status and core-error bridge paths (framework-reserved keys win).
mtr_underlyingMatterErrorSourceLine read source location on demand from the
existing MTRErrorHolder associated object via a single helper, rather than
baking source file/line into userInfo on every mapped error.
DNS-SD codes (src/lib/core):
exist.
not running.
ERROR_CODES.md regenerated.
Platform/Darwin:
CHIP_ERROR_DNSSD_NXDOMAIN, kDNSServiceErr_ServiceNotRunning to
CHIP_ERROR_DNSSD_SERVICE_NOT_RUNNING, kDNSServiceErr_Timeout to
CHIP_ERROR_TIMEOUT (instead of collapsing to CHIP_ERROR_INTERNAL).
CoreBluetooth NSError.code in a kOS-range CHIP_ERROR for triage, with a
24-bit overflow guard that falls back to the cross-platform BLE_ERROR_GATT_*
sentinel; the existing HandleConnectionError contract is unchanged for
non-Darwin consumers.
Testing
Unit tests added/extended (run by existing CI targets):
Core error-string coverage in
src/lib/core/tests/TestCHIPErrorStr.cpp:the two new
CHIP_ERROR_DNSSD_NXDOMAIN/CHIP_ERROR_DNSSD_SERVICE_NOT_RUNNINGcodes are added to the formattable-error list so
chip::ErrorStris assertedto produce a description for each, pinning the additive error-code allocation.
Darwin framework coverage in
src/darwin/Framework/CHIPTests/MTRErrorMappingTests.m:(
MTRAttestationVerificationResultKey == "attestationVerificationResult",VID/PID keys), and
MTRUnderlyingErrorCodeKeymatches the existingdocumented key for general errors.
MTRUnderlyingErrorCodeKeyis populated on bridged errors and on non-generalerror codes, round-trips through
MTRErrorHolder, and is not corruptible by acaller-supplied
additionalUserInfovalue.additionalUserInfois merged on both the IM-status and core-error bridgepaths, with framework-reserved keys winning on both paths; attestation keys
(verification result + VID/PID) are populated on the integrity-check bridge and
survive on non-integrity-check / general error codes.
boundary values; the two new DNS-SD codes bridge and round-trip through a
general error; category accessors return empty for arbitrary non-bridged
NSErrors.
src/darwin/Framework/CHIPTests/MTRErrorTests.m: underlying-error source-filebasename stripping handles mixed path separators.
Coverage limitations:
DnssdError.cpp'sDNSServiceErrorType->CHIP_ERRORswitch and the file-local
WrapCBErrorCodeAsKOSoverflow guard have no directunit tests (no Darwin platform unit-test target; the underlying error codes are
covered in
TestCHIPErrorStr.cpp, and CoreBluetooth codes are documented smallnon-negative integers). Attestation-key population at the
MTRDeviceAttestationDelegateBridge.mmcall site requires an end-to-endattestation failure to exercise; the bridge merge logic it relies on is covered
by the mapping tests above.
(Edited by Claude)