Address Darwin API review comments on MTRThreadOperationalDataset

src/darwin/Framework/CHIP/MTRThreadOperationalDataset.h

@@ -19,52 +19,65 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
+/**
+ * MTRThreadOperationalDataset allows converting between an "expanded" view of
+ * the dataset (with the separate fields) and a single-blob NSData view.
+ *
+ * The latter can be used to pass Thread network credentials via
+ * MTRCommissioningParameters.
+ */
 @interface MTRThreadOperationalDataset : NSObject
 
 /**
- * The expected lengths of each of the NSData fields in the CHIPThreadOperationalDataset
- *
- * initWithNetworkName must be provided NSData fields with at least these lengths otherwise
- * the object will fail to init.
+ * The expected lengths of each of the NSData fields in the MTRThreadOperationalDataset
  */
 extern size_t const MTRSizeThreadNetworkName;
-extern size_t const MTRSizeThreadExtendedPanId;
+extern size_t const MTRSizeThreadExtendedPANID;
 extern size_t const MTRSizeThreadMasterKey;
 extern size_t const MTRSizeThreadPSKc;
+extern size_t const MTRSizeThreadPANID;
 
 /**
- *  The Thread Network name
+ * The Thread Network name
  */
-@property (nonatomic, nullable, copy, readonly) NSString * networkName;
+@property (nonatomic, copy, readonly) NSString * networkName;
 /**
- *  The Thread Network extendended PAN ID
+ * The Thread Network extendended PAN ID
  */
-@property (nonatomic, nullable, copy, readonly) NSData * extendedPANID;
+@property (nonatomic, copy, readonly) NSData * extendedPANID;
 /**
- *  The 16 byte Master Key
+ * The 16 byte Master Key
  */
-@property (nonatomic, nullable, copy, readonly) NSData * masterKey;
+@property (nonatomic, copy, readonly) NSData * masterKey;
 /**
- *  The Thread PSKc
+ * The Thread PSKc
  */
-@property (nonatomic, nullable, copy, readonly) NSData * PSKc;
+@property (nonatomic, copy, readonly) NSData * PSKc;
 /**
- *  The Thread network channel.  Always an unsigned 16-bit integer.
+ * The Thread network channel.  Always an unsigned 16-bit integer.
  */
 @property (nonatomic, copy, readonly) NSNumber * channel;
 /**
- *  A uint16_t stored as 2-bytes in host order representing the Thread PAN ID
+ * A uint16_t stored as 2-bytes in host order representing the Thread PAN ID
  */
-@property (nonatomic, nullable, copy, readonly) NSData * panID;
+@property (nonatomic, copy, readonly) NSData * panID;
 
 - (instancetype)init NS_UNAVAILABLE;
 + (instancetype)new NS_UNAVAILABLE;
 
 /**
- *  Create a Thread Operational Dataset object with the individual network fields.
- *  This initializer will return nil if any of the NSData fields don't match the expected size.
+ * Create a Thread Operational Dataset object with the individual network
+ * fields.
  *
- *  Note: The panID is expected to be a uint16_t stored as 2-bytes in host order
+ * @param extendedPANID Must be MTRSizeThreadExtendedPANID bytes.  Otherwise nil
+ *                      will be returned.
+ * @param masterKey Must be MTRSizeThreadMasterKey bytes. Otherwise nil will be
+ *                  returned.
+ * @param PSKc Must be MTRSizeThreadPSKc bytes.  Otherwise nil will be returned.
+ * @param channel Must be an unsigned 16-bit value.
+ * @param panID Must be MTRSizeThreadPANID bytes.  Otherwise nil will be
+ *              returned.  In particular, it's expected to be a 16-bit unsigned
+ *              integer stored as 2 bytes in host order.
  */
 - (nullable instancetype)initWithNetworkName:(NSString *)networkName
                                extendedPANID:(NSData *)extendedPANID
@@ -74,13 +87,14 @@ extern size_t const MTRSizeThreadPSKc;
                                        panID:(NSData *)panID;
 
 /**
- *  Create a Thread Operational Dataset object with a RCP formatted active operational dataset.
- *  This initializer will return nil if the input data cannot be parsed correctly
+ * Create a Thread Operational Dataset object with a RCP formatted active operational dataset.
+ * This initializer will return nil if the input data cannot be parsed correctly
  */
 - (nullable instancetype)initWithData:(NSData *)data;
 
 /**
  * Get the underlying data that represents the Thread Active Operational Dataset
+ * This can be used for the threadOperationalDataset of MTRCommissioningParameters.
  */
 - (NSData *)data;
 

src/darwin/Framework/CHIP/MTRThreadOperationalDataset.mm

@@ -22,9 +22,10 @@
 #include <lib/support/ThreadOperationalDataset.h>
 
 size_t const MTRSizeThreadNetworkName = chip::Thread::kSizeNetworkName;
-size_t const MTRSizeThreadExtendedPanId = chip::Thread::kSizeExtendedPanId;
+size_t const MTRSizeThreadExtendedPANID = chip::Thread::kSizeExtendedPanId;
 size_t const MTRSizeThreadMasterKey = chip::Thread::kSizeMasterKey;
 size_t const MTRSizeThreadPSKc = chip::Thread::kSizePSKc;
+size_t const MTRSizeThreadPANID = 2; // Thread's PAN ID is 2 bytes
 
 @interface MTRThreadOperationalDataset ()
 
@@ -61,43 +62,41 @@ - (BOOL)_populateCppOperationalDataset
     _cppThreadOperationalDataset.Clear();
     _cppThreadOperationalDataset.SetNetworkName([self.networkName cStringUsingEncoding:NSUTF8StringEncoding]);
 
-    if (![self _checkDataLength:self.extendedPANID expectedLength:chip::Thread::kSizeExtendedPanId]) {
+    if (![self _checkDataLength:self.extendedPANID expectedLength:MTRSizeThreadExtendedPANID]) {
         MTR_LOG_ERROR("Invalid ExtendedPANID");
         return NO;
     }
-    uint8_t extendedPanId[chip::Thread::kSizeExtendedPanId];
-    [self.extendedPANID getBytes:&extendedPanId length:chip::Thread::kSizeExtendedPanId];
+    uint8_t extendedPanId[MTRSizeThreadExtendedPANID];
+    [self.extendedPANID getBytes:&extendedPanId length:MTRSizeThreadExtendedPANID];
     _cppThreadOperationalDataset.SetExtendedPanId(extendedPanId);
 
-    if (![self _checkDataLength:self.masterKey expectedLength:chip::Thread::kSizeMasterKey]) {
+    if (![self _checkDataLength:self.masterKey expectedLength:MTRSizeThreadMasterKey]) {
         MTR_LOG_ERROR("Invalid MasterKey");
         return NO;
     }
-    uint8_t masterKey[chip::Thread::kSizeMasterKey];
-    [self.masterKey getBytes:&masterKey length:chip::Thread::kSizeMasterKey];
+    uint8_t masterKey[MTRSizeThreadMasterKey];
+    [self.masterKey getBytes:&masterKey length:MTRSizeThreadMasterKey];
     _cppThreadOperationalDataset.SetMasterKey(masterKey);
 
-    if (![self _checkDataLength:self.PSKc expectedLength:chip::Thread::kSizePSKc]) {
+    if (![self _checkDataLength:self.PSKc expectedLength:MTRSizeThreadPSKc]) {
         MTR_LOG_ERROR("Invalid PKSc");
         return NO;
     }
-    uint8_t PSKc[chip::Thread::kSizePSKc];
-    [self.PSKc getBytes:&PSKc length:chip::Thread::kSizePSKc];
+    uint8_t PSKc[MTRSizeThreadPSKc];
+    [self.PSKc getBytes:&PSKc length:MTRSizeThreadPSKc];
     _cppThreadOperationalDataset.SetPSKc(PSKc);
 
     _cppThreadOperationalDataset.SetChannel([self.channel unsignedShortValue]);
 
     // Thread's PAN ID is 2 bytes
-    if (![self _checkDataLength:self.panID expectedLength:2]) {
+    if (![self _checkDataLength:self.panID expectedLength:MTRSizeThreadPANID]) {
         MTR_LOG_ERROR("Invalid PAN ID");
         return NO;
     }
-    uint16_t * valuePtr = (uint16_t *) [self.panID bytes];
-    if (valuePtr == nullptr) {
-        return NO;
-    }
+    uint16_t panID;
+    memcpy(&panID, [self.panID bytes], MTRSizeThreadPANID);
     // The underlying CPP class assumes Big Endianness for the panID
-    _cppThreadOperationalDataset.SetPanId(CFSwapInt16HostToBig(*valuePtr));
+    _cppThreadOperationalDataset.SetPanId(CFSwapInt16HostToBig(panID));
 
     return YES;
 }
@@ -123,7 +122,7 @@ - (nullable instancetype)initWithData:(NSData *)data
     // len+1 for null termination
     char networkName[MTRSizeThreadNetworkName + 1];
     uint8_t pskc[MTRSizeThreadPSKc];
-    uint8_t extendedPANID[MTRSizeThreadExtendedPanId];
+    uint8_t extendedPANID[MTRSizeThreadExtendedPANID];
     uint8_t masterKey[MTRSizeThreadMasterKey];
     uint16_t panID;
     uint16_t channel;
@@ -136,7 +135,7 @@ - (nullable instancetype)initWithData:(NSData *)data
     panID = CFSwapInt16BigToHost(panID);
 
     return [self initWithNetworkName:[NSString stringWithUTF8String:networkName]
-                       extendedPANID:[NSData dataWithBytes:extendedPANID length:MTRSizeThreadExtendedPanId]
+                       extendedPANID:[NSData dataWithBytes:extendedPANID length:MTRSizeThreadExtendedPANID]
                            masterKey:[NSData dataWithBytes:masterKey length:MTRSizeThreadMasterKey]
                                 PSKc:[NSData dataWithBytes:pskc length:MTRSizeThreadPSKc]
                              channel:@(channel)

src/darwin/Framework/CHIPTests/MTRThreadOperationalDatasetTests.mm

@@ -38,7 +38,7 @@ - (void)testThreadOperationalDataset
     const uint16_t panID = 0x28f4;
     MTRThreadOperationalDataset * dataset = [[MTRThreadOperationalDataset alloc]
         initWithNetworkName:@"TestNetwork"
-              extendedPANID:[NSData dataWithBytes:&extendedPANID length:MTRSizeThreadExtendedPanId]
+              extendedPANID:[NSData dataWithBytes:&extendedPANID length:MTRSizeThreadExtendedPANID]
                   masterKey:[NSData dataWithBytes:&masterKey length:MTRSizeThreadMasterKey]
                        PSKc:[NSData dataWithBytes:&PKSc length:MTRSizeThreadPSKc]
                     channel:@(25)