Skip to content

Commit

Permalink
Move shared callback bridge functionality to MTRCallbackBridgeBase. (…
Browse files Browse the repository at this point in the history 
…#24738)

Also eliminates the last direct (and hence not interceptible by API consumers)
NSLog calls in the Darwin framework and adds a lint for those.

Fixes project-chip/connectedhomeip#23597
  • Loading branch information
@bzbarsky-apple @kkasperczyk-no
bzbarsky-apple authored and kkasperczyk-no committed Mar 15, 2023
1 parent de6ff56 commit 7eb62b2
Show file tree
Hide file tree
Showing 2 changed files with 105 additions and 82 deletions.
7 changes: 7 additions & 0 deletions .github/workflows/lint.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
180 changes: 98 additions & 82 deletions src/darwin/Framework/CHIP/MTRCallbackBridgeBase.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<chip::SessionHandle> & 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<chip::SessionHandle> & 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<chip::SessionHandle> & 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);
Expand Down Expand Up @@ -72,7 +158,7 @@ template <class T> 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)
Expand All @@ -84,42 +170,14 @@ template <class T> 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)
, mFailure(OnFailureFn)
{
}

/**
* 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
Expand All @@ -142,8 +200,8 @@ template <class T> 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.
Expand All @@ -155,29 +213,7 @@ template <class T> class MTRCallbackBridge : public MTRCallbackBridgeBase {
}];
}

void ActionWithPASEDevice(MTRBaseDevice * device)
{
LogRequestStart();

[device.deviceController getSessionForCommissioneeDevice:device.nodeID
completion:^(chip::Messaging::ExchangeManager * exchangeManager,
const chip::Optional<chip::SessionHandle> & 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<chip::SessionHandle> & session, NSError * error) {
MaybeDoAction(exchangeManager, session, error);
}];
}

void LogRequestStart()
void LogRequestStart() override
{
mRequestTime = [NSDate date];
// Generate a unique cookie to track this operation
Expand All @@ -186,7 +222,7 @@ template <class T> class MTRCallbackBridge : public MTRCallbackBridgeBase {
}

void MaybeDoAction(chip::Messaging::ExchangeManager * _Nullable exchangeManager,
const chip::Optional<chip::SessionHandle> & session, NSError * _Nullable error)
const chip::Optional<chip::SessionHandle> & session, NSError * _Nullable error) override
{
// Make sure we don't hold on to our action longer than we have to.
auto action = mAction;
Expand All @@ -198,8 +234,8 @@ template <class T> 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.
Expand All @@ -209,35 +245,13 @@ template <class T> 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)
{
Expand Down Expand Up @@ -266,7 +280,9 @@ template <class T> 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;
Expand Down

0 comments on commit 7eb62b2

Please sign in to comment.