Skip to content

Commit

Permalink
[Python] Keep reference to callback function on timeout (#30877)
Browse files Browse the repository at this point in the history
* [Python] Keep reference to callback function on timeout

When using a timeout when calling GetConnectedDeviceSync() the callback
function DeviceAvailableCallback() gets potentially GC'ed. Make sure
we hold a reference to the instance.

* Use correct namespace for PyObject

* Fix types in pychip_GetConnectedDeviceByNodeId

* Call callback with context (self)

* Correctly pass context

* Use separate closure function
  • Loading branch information
agners authored and pull[bot] committed Dec 15, 2023
1 parent f2eea06 commit 2e7de2b
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 21 deletions.
17 changes: 9 additions & 8 deletions src/controller/python/ChipDeviceController-ScriptBinding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ using namespace chip::DeviceLayer;
extern "C" {
typedef void (*ConstructBytesArrayFunct)(const uint8_t * dataBuf, uint32_t dataLen);
typedef void (*LogMessageFunct)(uint64_t time, uint64_t timeUS, const char * moduleName, uint8_t category, const char * msg);
typedef void (*DeviceAvailableFunc)(DeviceProxy * device, PyChipError err);
typedef void (*DeviceAvailableFunc)(chip::Controller::Python::PyObject * context, DeviceProxy * device, PyChipError err);
typedef void (*ChipThreadTaskRunnerFunct)(intptr_t context);
typedef void (*DeviceUnpairingCompleteFunct)(uint64_t nodeId, PyChipError error);
}
Expand Down Expand Up @@ -207,7 +207,7 @@ const char * pychip_Stack_StatusReportToString(uint32_t profileId, uint16_t stat
void pychip_Stack_SetLogFunct(LogMessageFunct logFunct);

PyChipError pychip_GetConnectedDeviceByNodeId(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeId,
DeviceAvailableFunc callback);
chip::Controller::Python::PyObject * context, DeviceAvailableFunc callback);
PyChipError pychip_FreeOperationalDeviceProxy(chip::OperationalDeviceProxy * deviceProxy);
PyChipError pychip_GetLocalSessionId(chip::OperationalDeviceProxy * deviceProxy, uint16_t * localSessionId);
PyChipError pychip_GetNumSessionsToPeer(chip::OperationalDeviceProxy * deviceProxy, uint32_t * numSessions);
Expand Down Expand Up @@ -737,36 +737,37 @@ namespace {

struct GetDeviceCallbacks
{
GetDeviceCallbacks(DeviceAvailableFunc callback) :
mOnSuccess(OnDeviceConnectedFn, this), mOnFailure(OnConnectionFailureFn, this), mCallback(callback)
GetDeviceCallbacks(chip::Controller::Python::PyObject * context, DeviceAvailableFunc callback) :
mOnSuccess(OnDeviceConnectedFn, this), mOnFailure(OnConnectionFailureFn, this), mContext(context), mCallback(callback)
{}

static void OnDeviceConnectedFn(void * context, Messaging::ExchangeManager & exchangeMgr, const SessionHandle & sessionHandle)
{
auto * self = static_cast<GetDeviceCallbacks *>(context);
auto * operationalDeviceProxy = new OperationalDeviceProxy(&exchangeMgr, sessionHandle);
self->mCallback(operationalDeviceProxy, ToPyChipError(CHIP_NO_ERROR));
self->mCallback(self->mContext, operationalDeviceProxy, ToPyChipError(CHIP_NO_ERROR));
delete self;
}

static void OnConnectionFailureFn(void * context, const ScopedNodeId & peerId, CHIP_ERROR error)
{
auto * self = static_cast<GetDeviceCallbacks *>(context);
self->mCallback(nullptr, ToPyChipError(error));
self->mCallback(self->mContext, nullptr, ToPyChipError(error));
delete self;
}

Callback::Callback<OnDeviceConnected> mOnSuccess;
Callback::Callback<OnDeviceConnectionFailure> mOnFailure;
chip::Controller::Python::PyObject * const mContext;
DeviceAvailableFunc mCallback;
};
} // anonymous namespace

PyChipError pychip_GetConnectedDeviceByNodeId(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeId,
DeviceAvailableFunc callback)
chip::Controller::Python::PyObject * context, DeviceAvailableFunc callback)
{
VerifyOrReturnError(devCtrl != nullptr, ToPyChipError(CHIP_ERROR_INVALID_ARGUMENT));
auto * callbacks = new GetDeviceCallbacks(callback);
auto * callbacks = new GetDeviceCallbacks(context, callback);
return ToPyChipError(devCtrl->GetConnectedDevice(nodeId, &callbacks->mOnSuccess, &callbacks->mOnFailure));
}

Expand Down
35 changes: 22 additions & 13 deletions src/controller/python/chip/ChipDeviceCtrl.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
#
# CHIP_ERROR is actually signed, so using c_uint32 is weird, but everything
# else seems to do it.
_DeviceAvailableFunct = CFUNCTYPE(None, c_void_p, PyChipError)
_DeviceAvailableCallbackFunct = CFUNCTYPE(None, py_object, c_void_p, PyChipError)

_IssueNOCChainCallbackPythonCallbackFunct = CFUNCTYPE(
None, py_object, PyChipError, c_void_p, c_size_t, c_void_p, c_size_t, c_void_p, c_size_t, c_void_p, c_size_t, c_uint64)
Expand All @@ -100,6 +100,11 @@ class NOCChain:
adminSubject: int


@_DeviceAvailableCallbackFunct
def _DeviceAvailableCallback(closure, device, err):
closure.deviceAvailable(device, err)


@_IssueNOCChainCallbackPythonCallbackFunct
def _IssueNOCChainCallbackPythonCallback(devCtrl, status: PyChipError, noc: c_void_p, nocLen: int, icac: c_void_p,
icacLen: int, rcac: c_void_p, rcacLen: int, ipk: c_void_p, ipkLen: int, adminSubject: int):
Expand Down Expand Up @@ -759,25 +764,29 @@ def GetConnectedDeviceSync(self, nodeid, allowPASE=True, timeoutMs: int = None):
returnErr = None
deviceAvailableCV = threading.Condition()

@_DeviceAvailableFunct
def DeviceAvailableCallback(device, err):
nonlocal returnDevice
nonlocal returnErr
nonlocal deviceAvailableCV
with deviceAvailableCV:
returnDevice = c_void_p(device)
returnErr = err
deviceAvailableCV.notify_all()

if allowPASE:
res = self._ChipStack.Call(lambda: self._dmLib.pychip_GetDeviceBeingCommissioned(
self.devCtrl, nodeid, byref(returnDevice)), timeoutMs)
if res.is_success:
print('Using PASE connection')
return DeviceProxyWrapper(returnDevice)

class DeviceAvailableClosure():
def deviceAvailable(self, device, err):
nonlocal returnDevice
nonlocal returnErr
nonlocal deviceAvailableCV
with deviceAvailableCV:
returnDevice = c_void_p(device)
returnErr = err
deviceAvailableCV.notify_all()
ctypes.pythonapi.Py_DecRef(ctypes.py_object(self))

closure = DeviceAvailableClosure()
ctypes.pythonapi.Py_IncRef(ctypes.py_object(closure))
self._ChipStack.Call(lambda: self._dmLib.pychip_GetConnectedDeviceByNodeId(
self.devCtrl, nodeid, DeviceAvailableCallback), timeoutMs).raise_on_error()
self.devCtrl, nodeid, ctypes.py_object(closure), _DeviceAvailableCallback),
timeoutMs).raise_on_error()

# The callback might have been received synchronously (during self._ChipStack.Call()).
# Check if the device is already set before waiting for the callback.
Expand Down Expand Up @@ -1568,7 +1577,7 @@ def _InitLib(self):
self._dmLib.pychip_ScriptDevicePairingDelegate_SetFabricCheckCallback.restype = PyChipError

self._dmLib.pychip_GetConnectedDeviceByNodeId.argtypes = [
c_void_p, c_uint64, _DeviceAvailableFunct]
c_void_p, c_uint64, py_object, _DeviceAvailableCallbackFunct]
self._dmLib.pychip_GetConnectedDeviceByNodeId.restype = PyChipError

self._dmLib.pychip_FreeOperationalDeviceProxy.argtypes = [
Expand Down

0 comments on commit 2e7de2b

Please sign in to comment.