Issue 16691 - Darwin: convenience API Part 2

src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.h

@@ -23,22 +23,37 @@ NS_ASSUME_NONNULL_BEGIN
 
 typedef void (^MTRAsyncCallbackReadyHandler)(id context, NSUInteger retryCount);
 
-// How to queue a new work item:
+// MTRAsyncCallbackQueue high level description
+//   The MTRAsyncCallbackQueue was made to call one readyHandler
+//   block at a time asynchronously, and the readyHandler is
+//   expected to start/schedule a task. When the task finishes
+//   asynchronously in the future (at any time, from any queue
+//   or thread), it is expected to ask the workItem object to
+//   either endWork or retryWork.
+
+// Sequence of steps when queuing a work item:
 //   - Create MTRAsyncCallbackQueueWorkItem object
 //   - Create ready handler block (MTRAsyncCallbackReadyHandler)
-//      - block is called when it's the work item's turn to do work
-//      - its body is to do work with the device
+//      - block is called when it's the WorkItem's turn to do work
+//      - its body is to perform a task that is expected to end asynchronously in the future
 //      - at the end of work, call on the work item object:
 //         - endWork for success or failure
 //         - retryWork for temporary failures
-//   - Set the work handler block to the Item object
-//   - Call enqueueWorkItem on the MTRDevice's work queue property
+//   - Set the readyHandler block on the WorkItem object
+//   - Call enqueueWorkItem on a MTRAsyncCallbackQueue
 
 // A serial one-at-a-time queue for performing work items
 @interface MTRAsyncCallbackWorkQueue : NSObject
 - (instancetype)init NS_UNAVAILABLE;
 + (instancetype)new NS_UNAVAILABLE;
 
+// The context object is only held and passed back as a reference and is opaque to the work queue
+- (instancetype)initWithContext:(id _Nullable)context queue:(dispatch_queue_t)queue;
+
+// Called by the work queue owner to clean up and cancel work items
+- (void)invalidate;
+
+// Work items may be enqueued from any queue or thread
 - (void)enqueueWorkItem:(MTRAsyncCallbackQueueWorkItem *)item;
 
 // TODO: Add a "set concurrency width" method to allow for more than 1 work item at a time
@@ -49,12 +64,17 @@ typedef void (^MTRAsyncCallbackReadyHandler)(id context, NSUInteger retryCount);
 - (instancetype)init NS_UNAVAILABLE;
 + (instancetype)new NS_UNAVAILABLE;
 
+// Both readyHandler and cancelHander will be called on the queue given to initWithQueue
 - (instancetype)initWithQueue:(dispatch_queue_t)queue;
 @property (nonatomic, strong) MTRAsyncCallbackReadyHandler readyHandler;
 @property (nonatomic, strong) dispatch_block_t cancelHandler;
 
-// Called by Cluster object's after async work is done
+// Called by the creater of the work item when async work is done and should
+// be removed from the queue. The work queue will run the next work item.
 - (void)endWork;
+
+// Called by the creater of the work item when async work should be retried.
+// The work queue will call this workItem's readyHandler again.
 - (void)retryWork;
 @end
 

src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm

@@ -18,12 +18,17 @@
 #import <dispatch/dispatch.h>
 #import <os/lock.h>
 
-#import "MTRAsyncCallbackWorkQueue_Internal.h"
+#import "MTRAsyncCallbackWorkQueue.h"
 #import "MTRLogging.h"
 
 #pragma mark - Class extensions
 
 @interface MTRAsyncCallbackWorkQueue ()
+// The lock protects the internal state of the work queue so that these may be called from any queue or thread:
+//   -enqueueWorkItem:
+//   -invalidate
+//   -endWork:
+//   -retryWork:
 @property (nonatomic, readonly) os_unfair_lock lock;
 @property (nonatomic, strong, readonly) id context;
 @property (nonatomic, strong, readonly) dispatch_queue_t queue;
@@ -81,7 +86,8 @@ - (void)invalidate
     [invalidateItems removeAllObjects];
 }
 
-- (void)endWork:(MTRAsyncCallbackQueueWorkItem *)workItem
+// called after executing a work item
+- (void)_postProcessWorkItem:(MTRAsyncCallbackQueueWorkItem *)workItem retry:(BOOL)retry
 {
     os_unfair_lock_lock(&_lock);
     // sanity check if running
@@ -102,41 +108,26 @@ - (void)endWork:(MTRAsyncCallbackQueueWorkItem *)workItem
         return;
     }
 
-    // since work is done, remove from queue and call ready on the next item
-    [self.items removeObjectAtIndex:0];
+    // if work item is done (no need to retry), remove from queue and call ready on the next item
+    if (!retry) {
+        [self.items removeObjectAtIndex:0];
+    }
 
     // when "concurrency width" is implemented this will be decremented instead
     self.runningWorkItemCount = 0;
     [self _callNextReadyWorkItem];
     os_unfair_lock_unlock(&_lock);
 }
 
-- (void)retryWork:(MTRAsyncCallbackQueueWorkItem *)workItem
-{
-    // reset BOOL and call again
-    os_unfair_lock_lock(&_lock);
-    // sanity check if running
-    if (!self.runningWorkItemCount) {
-        // something is wrong with state - nothing is currently running
-        os_unfair_lock_unlock(&_lock);
-        MTR_LOG_ERROR("retryWork: no work is running on work queue");
-        return;
-    }
 
-    // sanity check the same work item is running
-    // when "concurrency width" is implemented need to check first N items
-    MTRAsyncCallbackQueueWorkItem * firstWorkItem = self.items.firstObject;
-    if (firstWorkItem != workItem) {
-        // something is wrong with this work item - should not be currently running
-        os_unfair_lock_unlock(&_lock);
-        MTR_LOG_ERROR("retryWork: work item is not first on work queue");
-        return;
-    }
+- (void)endWork:(MTRAsyncCallbackQueueWorkItem *)workItem
+{
+    [self _postProcessWorkItem:workItem retry:NO];
+}
 
-    // when "concurrency width" is implemented this will be decremented instead
-    self.runningWorkItemCount = 0;
-    [self _callNextReadyWorkItem];
-    os_unfair_lock_unlock(&_lock);
+- (void)retryWork:(MTRAsyncCallbackQueueWorkItem *)workItem
+{
+    [self _postProcessWorkItem:workItem retry:YES];
 }
 
 // assume lock is held while calling this
@@ -166,13 +157,11 @@ - (instancetype)initWithQueue:(dispatch_queue_t)queue
     return self;
 }
 
-// Called by Cluster object's after async work is done
 - (void)endWork
 {
     [self.workQueue endWork:self];
 }
 
-// Called by Cluster object's after async work is done
 - (void)retryWork
 {
     [self.workQueue retryWork:self];

src/darwin/Framework/CHIP/MTRBaseDevice.h

@@ -30,6 +30,7 @@ NS_ASSUME_NONNULL_BEGIN
  *
  *                MTRAttributePathKey : MTRAttributePath object. Included for attribute value.
  *                MTRCommandPathKey : MTRCommandPath object. Included for command response.
+ *                MTREventPathKey : MTREventPath object. Included for event value.
  *                MTRErrorKey : NSError object. Included to indicate an error.
  *                MTRDataKey: Data-value NSDictionary object.
  *                              Included when there is data and when there is no error.
@@ -67,11 +68,18 @@ NS_ASSUME_NONNULL_BEGIN
  *                MTRDataKey : Data-value NSDictionary object.
  */
 typedef void (^MTRDeviceResponseHandler)(NSArray<NSDictionary<NSString *, id> *> * _Nullable values, NSError * _Nullable error);
+
+/**
+ * Handler for -subscribeWithQueue: attribute and event reports
+ *
+ * @param values This array contains MTRAttributeReport objects for attribute reports, and MTREventReport objects for event reports
+ */
 typedef void (^MTRDeviceReportHandler)(NSArray * values);
 typedef void (^MTRDeviceErrorHandler)(NSError * error);
 
 extern NSString * const MTRAttributePathKey;
 extern NSString * const MTRCommandPathKey;
+extern NSString * const MTREventPathKey;
 extern NSString * const MTRDataKey;
 extern NSString * const MTRErrorKey;
 extern NSString * const MTRTypeKey;

src/darwin/Framework/CHIP/MTRBaseDevice.mm

@@ -50,6 +50,7 @@
 
 NSString * const MTRAttributePathKey = @"attributePath";
 NSString * const MTRCommandPathKey = @"commandPath";
+NSString * const MTREventPathKey = @"eventPath";
 NSString * const MTRDataKey = @"data";
 NSString * const MTRErrorKey = @"error";
 NSString * const MTRTypeKey = @"type";

src/darwin/Framework/CHIP/MTRBaseDevice_Internal.h

@@ -68,6 +68,10 @@ NS_ASSUME_NONNULL_BEGIN
 - (instancetype)initWithPath:(const chip::app::ConcreteDataAttributePath &)path;
 @end
 
+@interface MTREventPath ()
+- (instancetype)initWithPath:(const chip::app::ConcreteEventPath &)path;
+@end
+
 @interface MTRCommandPath ()
 - (instancetype)initWithPath:(const chip::app::ConcreteCommandPath &)path;
 @end

src/darwin/Framework/CHIP/MTRDevice.h

@@ -23,41 +23,61 @@ NS_ASSUME_NONNULL_BEGIN
 @class MTRDeviceController;
 @class MTRAsyncCallbackWorkQueue;
 
-@protocol MTRDeviceSubscriptionDelegate;
+typedef NS_ENUM(NSUInteger, MTRDeviceState) {
+    MTRDeviceStateUnknown = 0,
+    MTRDeviceStateReachable = 1,
+    MTRDeviceStateUnreachable = 2
+};
+
+@protocol MTRDeviceDelegate;
 
 @interface MTRDevice : NSObject
 - (instancetype)init NS_UNAVAILABLE;
 + (instancetype)new NS_UNAVAILABLE;
 
 /**
+ * TODO: Document usage better
+ *
  * Directly instantiate a MTRDevice with a MTRDeviceController as a shim.
  *
  * All device-specific information would be stored on the device controller, and
  * retrieved when performing actions using a combination of MTRBaseDevice
  * and MTRAsyncCallbackQueue.
  */
-+ (instancetype)deviceWithDeviceID:(uint64_t)deviceID deviceController:(MTRDeviceController *)deviceController;
++ (instancetype)deviceWithNodeID:(uint64_t)nodeID deviceController:(MTRDeviceController *)deviceController;
+
+/**
+ * The current state of the device.
+ *
+ * The three states:
+ *   MTRDeviceStateUnknkown
+ *      Unable to determine the state of the device at the moment.
+ *
+ *   MTRDeviceStateReachable
+ *      Communication with the device is expected to succeed.
+ *
+ *   MTRDeviceStateUnreachable
+ *      The device is currently unreachable.
+ */
+@property (nonatomic, readonly) MTRDeviceState state;
 
 /**
- * Subscribe to receive attribute reports for everything (all endpoints, all
- * clusters, all attributes, all events) on the device.
+ * Set the delegate to receive asynchronous callbacks about the device.
  *
- * The subscriber provides a delegate object conforming to MTRDeviceSubscriptionDelegate
+ * The delegate will be called on the provided queue, for attribute reports, event reports, and device state changes.
  */
-- (void)subscribeWithDelegate:(id<MTRDeviceSubscriptionDelegate>)delegate
-                        queue:(dispatch_queue_t)queue
-                  minInterval:(uint16_t)minInterval
-                  maxInterval:(uint16_t)maxInterval
-                       params:(MTRSubscribeParams * _Nullable)params;
+- (void)setDelegate:(id<MTRDeviceDelegate>)delegate queue:(dispatch_queue_t)queue;
 
 /**
  * Read attribute in a designated attribute path
  *
+ * TODO: Need to document that this returns "the system's best guess" of attribute values.
+ *
  * @return a data-value dictionary of the attribute as described in MTRDeviceResponseHandler
  */
-- (NSDictionary<NSString *, id> *)readAttributeWithEndpointId:(NSNumber * _Nullable)endpointId
-                                                    clusterId:(NSNumber * _Nullable)clusterId
-                                                  attributeId:(NSNumber * _Nullable)attributeId
+- (NSDictionary<NSString *, id> *)readAttributeWithEndpointID:(NSNumber *)endpointID
+                                                    clusterID:(NSNumber *)clusterID
+                                                  attributeID:(NSNumber *)attributeID
                                                        params:(MTRReadParams * _Nullable)params;
 
 /**
@@ -66,20 +86,25 @@ NS_ASSUME_NONNULL_BEGIN
  * @param value       A data-value NSDictionary object as described in
  *                    MTRDeviceResponseHandler.
  *
- * @param expectedValueIntervalMs  interval that the write value is assumed to hold true before actual interaction happens. This
- * value will be clamped to timeoutMs.
+ * @param expectedValueInterval  maximum interval in milliseconds during which reads of the attribute will return the value being
+ * written. This value will be clamped to timeoutMs
  *
- * @param timeoutMs   timeout in milliseconds for timed write, or nil.
+ * TODO: document that -readAttribute... will return the expected value for the [endpoint,cluster,attribute] until one of the
+ * following:
+ *  1. Another write for the same attribute happens.
+ *  2. expectedValueIntervalMs (clamped) expires. Need to figure out phrasing here.
+ *  3. We succeed at writing the attribute.
+ *  4. We fail at writing the attribute and give up on the write
  *
- *                    Received values are an NSArray object with response-value element as described in
- *                    readAttributeWithEndpointId:clusterId:attributeId:clientQueue:completion:.
+ * @param timeout   timeout in milliseconds for timed write, or nil.
+ * TODO: make timeout arguments uniform
  */
-- (void)writeAttributeWithEndpointId:(NSNumber *)endpointId
-                           clusterId:(NSNumber *)clusterId
-                         attributeId:(NSNumber *)attributeId
+- (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
+                           clusterID:(NSNumber *)clusterID
+                         attributeID:(NSNumber *)attributeID
                                value:(id)value
-               expectedValueInterval:(NSNumber *)expectedValueIntervalMs
-                   timedWriteTimeout:(NSNumber * _Nullable)timeoutMs;
+               expectedValueInterval:(NSNumber *)expectedValueInterval
+                   timedWriteTimeout:(NSNumber * _Nullable)timeout;
 
 /**
  * Invoke a command with a designated command path
@@ -92,61 +117,55 @@ NS_ASSUME_NONNULL_BEGIN
  * @param expectedValues  array of dictionaries containing the expected values in the same format as
  *                       attribute read completion handler. Requires MTRAttributePathKey values.
  *                       See MTRDeviceResponseHandler definition for dictionary details.
+ * TODO: document better the expectedValues is how this command is expected to change attributes when read, and that the next
+ * readAttribute will get these values
  *
- * @param expectedValueIntervalMs  interval that the write value is assumed to hold true before actual interaction happens. This
- * value will be clamped to timeoutMs.
+ * @param expectedValueInterval  maximum interval in milliseconds during which reads of the attribute will return the value being
+ * written. This value will be clamped to timeout
  *
- * @param timeoutMs   timeout in milliseconds for timed invoke, or nil.
+ * @param timeout   timeout in milliseconds for timed invoke, or nil.
  *
  * @param completion  response handler will receive either values or error.
  */
-- (void)invokeCommandWithEndpointId:(NSNumber *)endpointId
-                          clusterId:(NSNumber *)clusterId
-                          commandId:(NSNumber *)commandId
+- (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
+                          clusterID:(NSNumber *)clusterID
+                          commandID:(NSNumber *)commandID
                       commandFields:(id)commandFields
-                     expectedValues:(NSArray<NSDictionary<NSString *, id> *> *)expectedValues
-              expectedValueInterval:(NSNumber *)expectedValueIntervalMs
-                 timedInvokeTimeout:(NSNumber * _Nullable)timeoutMs
+                     expectedValues:(NSArray<NSDictionary<NSString *, id> *> * _Nullable)expectedValues
+              expectedValueInterval:(NSNumber * _Nullable)expectedValueInterval
+                 timedInvokeTimeout:(NSNumber * _Nullable)timeout
                         clientQueue:(dispatch_queue_t)clientQueue
                          completion:(MTRDeviceResponseHandler)completion;
 
 @end
 
-@protocol MTRDeviceSubscriptionDelegate <NSObject>
+@protocol MTRDeviceDelegate <NSObject>
 @required
 /**
- * subscriptionEstablished
+ * device:stateChanged:
  *
- * Called once the subscription is established.  This will
- * be _after_ the first (priming) call to both report callbacks.
+ * @param state The current state of the device
  */
-- (void)subscriptionEstablished;
+- (void)device:(MTRDevice *)device stateChanged:(MTRDeviceState)state;
 
 /**
- * subscriptionReceivedAttributeReport:
+ * device:receivedAttributeReport:
  *
  * Notifies delegate of attribute reports from the MTRDevice
  *
  * @param attributeReport  An array of response-value objects as described in MTRDeviceResponseHandler
  */
-- (void)subscriptionReceivedAttributeReport:(NSArray *)attributeReport;
+- (void)device:(MTRDevice *)device receivedAttributeReport:(NSArray<NSDictionary<NSString *, id> *> *)attributeReport;
 
 /**
  * subscriptionReceivedEventReport:
  *
  * Notifies delegate of event reports from the MTRDevice
  *
- * @param eventReport  An array of MTREventReport objects
+ * @param eventReport  An array of response-value objects as described in MTRDeviceResponseHandler
  */
-- (void)subscriptionReceivedEventReport:(NSArray *)eventReport;
+- (void)device:(MTRDevice *)device receivedEventReport:(NSArray<NSDictionary<NSString *, id> *> *)eventReport;
 
-/**
- * subscriptionEndedWithError:
- *
- * Called any time there is an error for the
- * entire subscription (with a non-nil "error"), and terminate the subscription
- */
-- (void)subscriptionEndedWithError:(NSError *)error;
 @end
 
 NS_ASSUME_NONNULL_END