diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index b0fcb996ff..1e0ae2f0e5 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -181,3 +181,10 @@ jobs: if: always() run: | git grep -n '0x%[0-9-]*" *PRI[^xX]' -- './*' ':(exclude).github/workflows/lint.yml' && exit 1 || exit 0 + + # git grep exits with 0 if it finds a match, but we want + # to fail (exit nonzero) on match. + - name: Check for use of NSLog instead of Matter logging in Matter framework + if: always() + run: | + git grep -n 'NSLog(' -- src/darwin/Framework/CHIP && exit 1 || exit 0 diff --git a/src/darwin/Framework/CHIP/MTRCallbackBridgeBase.h b/src/darwin/Framework/CHIP/MTRCallbackBridgeBase.h index 4304f15cd5..6faa134a1e 100644 --- a/src/darwin/Framework/CHIP/MTRCallbackBridgeBase.h +++ b/src/darwin/Framework/CHIP/MTRCallbackBridgeBase.h @@ -35,6 +35,92 @@ NS_ASSUME_NONNULL_BEGIN * know. */ class MTRCallbackBridgeBase { +public: + /** + * Run the given MTRActionBlock on the Matter thread, after getting a CASE + * session (possibly pre-existing) to the given node ID on the fabric + * represented by the given MTRDeviceController. On success, convert the + * success value to whatever type it needs to be to call the callback type + * we're templated over. Once this function has been called, on a callback + * bridge allocated with `new`, the bridge object must not be accessed by + * the caller. The action block will handle deleting the bridge. + */ + void DispatchAction(chip::NodeId nodeID, MTRDeviceController * controller) && { ActionWithNodeID(nodeID, controller); } + + /** + * Run the given MTRActionBlock on the Matter thread after getting a secure + * session corresponding to the given MTRBaseDevice. On success, convert + * the success value to whatever type it needs to be to call the callback + * type we're templated over. Once this function has been called, on a callback + * bridge allocated with `new`, the bridge object must not be accessed by + * the caller. The action block will handle deleting the bridge. + */ + void DispatchAction(MTRBaseDevice * device) && + { + if (device.isPASEDevice) { + ActionWithPASEDevice(device); + } else { + ActionWithNodeID(device.nodeID, device.deviceController); + } + } + +protected: + MTRCallbackBridgeBase(dispatch_queue_t queue) + : mQueue(queue) + { + } + + virtual ~MTRCallbackBridgeBase() {}; + + virtual void MaybeDoAction(chip::Messaging::ExchangeManager * _Nullable exchangeManager, + const chip::Optional & session, NSError * _Nullable error) + = 0; + virtual void LogRequestStart() = 0; + + void ActionWithPASEDevice(MTRBaseDevice * device) + { + LogRequestStart(); + + [device.deviceController getSessionForCommissioneeDevice:device.nodeID + completion:^(chip::Messaging::ExchangeManager * exchangeManager, + const chip::Optional & session, NSError * error) { + MaybeDoAction(exchangeManager, session, error); + }]; + } + + void ActionWithNodeID(chip::NodeId nodeID, MTRDeviceController * controller) + { + LogRequestStart(); + + [controller getSessionForNode:nodeID + completion:^(chip::Messaging::ExchangeManager * exchangeManager, + const chip::Optional & session, NSError * error) { + MaybeDoAction(exchangeManager, session, error); + }]; + } + + // OnDone and KeepAliveOnCallback really only make sense for subscription + // bridges, but we put them here to avoid many copies of this code in + // generated bits. + void OnDone() + { + if (!mQueue) { + delete this; + return; + } + + // Delete ourselves async, so that any error/data reports we + // queued up before getting OnDone have a chance to run. + auto * self = this; + dispatch_async(mQueue, ^{ + delete self; + }); + } + + void KeepAliveOnCallback() { mKeepAlive = true; } + + dispatch_queue_t mQueue; + bool mKeepAlive = false; }; typedef void (^MTRResponseHandler)(id _Nullable value, NSError * _Nullable error); @@ -72,7 +158,7 @@ template class MTRCallbackBridge : public MTRCallbackBridgeBase { * on it. */ MTRCallbackBridge(dispatch_queue_t queue, MTRResponseHandler handler, T OnSuccessFn) - : mQueue(queue) + : MTRCallbackBridgeBase(queue) , mHandler(handler) , mSuccess(OnSuccessFn) , mFailure(OnFailureFn) @@ -84,7 +170,7 @@ template class MTRCallbackBridge : public MTRCallbackBridgeBase { * on it. */ MTRCallbackBridge(dispatch_queue_t queue, MTRResponseHandler handler, MTRActionBlock _Nonnull action, T OnSuccessFn) - : mQueue(queue) + : MTRCallbackBridgeBase(queue) , mHandler(handler) , mAction(action) , mSuccess(OnSuccessFn) @@ -92,34 +178,6 @@ template class MTRCallbackBridge : public MTRCallbackBridgeBase { { } - /** - * Run the given MTRActionBlock on the Matter thread, after getting a CASE - * session (possibly pre-existing) to the given node ID on the fabric - * represented by the given MTRDeviceController. On success, convert the - * success value to whatever type it needs to be to call the callback type - * we're templated over. Once this function has been called, on a callback - * bridge allocated with `new`, the bridge object must not be accessed by - * the caller. The action block will handle deleting the bridge. - */ - void DispatchAction(chip::NodeId nodeID, MTRDeviceController * controller) && { ActionWithNodeID(nodeID, controller); } - - /** - * Run the given MTRActionBlock on the Matter thread after getting a secure - * session corresponding to the given MTRBaseDevice. On success, convert - * the success value to whatever type it needs to be to call the callback - * type we're templated over. Once this function has been called, on a callback - * bridge allocated with `new`, the bridge object must not be accessed by - * the caller. The action block will handle deleting the bridge. - */ - void DispatchAction(MTRBaseDevice * device) && - { - if (device.isPASEDevice) { - ActionWithPASEDevice(device); - } else { - ActionWithNodeID(device.nodeID, device.deviceController); - } - } - /** * Try to run the given MTRLocalActionBlock on the Matter thread, if we have * a device and it's attached to a running controller, then handle @@ -142,8 +200,8 @@ template class MTRCallbackBridge : public MTRCallbackBridgeBase { asyncDispatchToMatterQueue:^() { CHIP_ERROR err = action(mSuccess, mFailure); if (err != CHIP_NO_ERROR) { - NSLog(@"Failure performing action. C++-mangled success callback type: '%s', error: %s", typeid(T).name(), - chip::ErrorStr(err)); + ChipLogError(Controller, "Failure performing action. C++-mangled success callback type: '%s', error: %s", + typeid(T).name(), chip::ErrorStr(err)); // Take the normal async error-reporting codepath. This will also // handle cleaning us up properly. @@ -155,29 +213,7 @@ template class MTRCallbackBridge : public MTRCallbackBridgeBase { }]; } - void ActionWithPASEDevice(MTRBaseDevice * device) - { - LogRequestStart(); - - [device.deviceController getSessionForCommissioneeDevice:device.nodeID - completion:^(chip::Messaging::ExchangeManager * exchangeManager, - const chip::Optional & session, NSError * error) { - MaybeDoAction(exchangeManager, session, error); - }]; - } - - void ActionWithNodeID(chip::NodeId nodeID, MTRDeviceController * controller) - { - LogRequestStart(); - - [controller getSessionForNode:nodeID - completion:^(chip::Messaging::ExchangeManager * exchangeManager, - const chip::Optional & session, NSError * error) { - MaybeDoAction(exchangeManager, session, error); - }]; - } - - void LogRequestStart() + void LogRequestStart() override { mRequestTime = [NSDate date]; // Generate a unique cookie to track this operation @@ -186,7 +222,7 @@ template class MTRCallbackBridge : public MTRCallbackBridgeBase { } void MaybeDoAction(chip::Messaging::ExchangeManager * _Nullable exchangeManager, - const chip::Optional & session, NSError * _Nullable error) + const chip::Optional & session, NSError * _Nullable error) override { // Make sure we don't hold on to our action longer than we have to. auto action = mAction; @@ -198,8 +234,8 @@ template class MTRCallbackBridge : public MTRCallbackBridgeBase { CHIP_ERROR err = action(*exchangeManager, session.Value(), mSuccess, mFailure, this); if (err != CHIP_NO_ERROR) { - NSLog(@"Failure performing action. C++-mangled success callback type: '%s', error: %s", typeid(T).name(), - chip::ErrorStr(err)); + ChipLogError(Controller, "Failure performing action. C++-mangled success callback type: '%s', error: %s", + typeid(T).name(), chip::ErrorStr(err)); // Take the normal async error-reporting codepath. This will also // handle cleaning us up properly. @@ -209,35 +245,13 @@ template class MTRCallbackBridge : public MTRCallbackBridgeBase { virtual ~MTRCallbackBridge() {}; +protected: static void OnFailureFn(void * context, CHIP_ERROR error) { DispatchFailure(context, [MTRError errorForCHIPErrorCode:error]); } static void DispatchSuccess(void * context, id _Nullable value) { DispatchCallbackResult(context, nil, value); } static void DispatchFailure(void * context, NSError * error) { DispatchCallbackResult(context, error, nil); } -protected: - // OnDone and KeepAliveOnCallback really only make sense for subscription - // bridges, but we put them here to avoid many copies of this code in - // generated bits. - void OnDone() - { - if (!mQueue) { - delete this; - return; - } - - // Delete ourselves async, so that any error/data reports we - // queued up before getting OnDone have a chance to run. - auto * self = this; - dispatch_async(mQueue, ^{ - delete self; - }); - } - - void KeepAliveOnCallback() { mKeepAlive = true; } - - dispatch_queue_t mQueue; - private: static void DispatchCallbackResult(void * context, NSError * _Nullable error, id _Nullable value) { @@ -266,7 +280,9 @@ template class MTRCallbackBridge : public MTRCallbackBridgeBase { MTRResponseHandler mHandler; MTRActionBlock _Nullable mAction; - bool mKeepAlive = false; + // Keep our subclasses from accessing mKeepAlive directly, by putting this + // "using" in our private section. + using MTRCallbackBridgeBase::mKeepAlive; T mSuccess; MTRErrorCallback mFailure;