From 75f4877f82a2a73d98d5b45f9a706a2e58cee5cd Mon Sep 17 00:00:00 2001 From: Michael Zanetti Date: Tue, 21 Jul 2020 00:45:46 +0200 Subject: [PATCH] add settingChangedHandler, fix some issues, cleanup some warnings --- libnymea-core/integrations/python/pything.h | 30 ++++++++----- .../integrations/python/pythingdescriptor.h | 4 +- .../python/pythingdiscoveryinfo.h | 4 +- .../integrations/python/pythingpairinginfo.h | 4 +- libnymea-core/integrations/python/pyutils.h | 1 - .../integrations/pythonintegrationplugin.cpp | 42 ++++--------------- .../integrations/pythonintegrationplugin.h | 1 - plugins/pymock/integrationpluginpymock.py | 10 +++++ python-todos.txt | 1 - 9 files changed, 45 insertions(+), 52 deletions(-) diff --git a/libnymea-core/integrations/python/pything.h b/libnymea-core/integrations/python/pything.h index 28e9092b..94325dd6 100644 --- a/libnymea-core/integrations/python/pything.h +++ b/libnymea-core/integrations/python/pything.h @@ -45,6 +45,7 @@ typedef struct _thing { PyObject *pyNameChangedHandler = nullptr; PyObject *pySettingChangedHandler = nullptr; PyObject *pyStates = nullptr; // A copy of the things states + PyThreadState *threadState = nullptr; // The python threadstate this thing belongs to } PyThing; @@ -58,9 +59,10 @@ static PyObject* PyThing_new(PyTypeObject *type, PyObject */*args*/, PyObject */ return (PyObject*)self; } -static void PyThing_setThing(PyThing *self, Thing *thing) +static void PyThing_setThing(PyThing *self, Thing *thing, PyThreadState *threadState) { self->thing = thing; + self->threadState = threadState; // Creating a copy because we cannot access the actual thing from the python thread self->thingClass = new ThingClass(thing->thingClass()); @@ -87,28 +89,36 @@ static void PyThing_setThing(PyThing *self, Thing *thing) // Those lambdas Will be executed in the main thread context. This means we // can access self->thing, but need to hold the GIL for interacting with python QObject::connect(thing, &Thing::nameChanged, [=](){ - PyGILState_STATE s = PyGILState_Ensure(); + PyEval_RestoreThread(self->threadState); Py_XDECREF(self->pyName); self->pyName = PyUnicode_FromString(self->thing->name().toUtf8().data()); if (self->pyNameChangedHandler) { - PyObject_CallFunctionObjArgs(self->pyNameChangedHandler, self, nullptr); + PyObject *ret = PyObject_CallFunctionObjArgs(self->pyNameChangedHandler, self, nullptr); + if (PyErr_Occurred()) { + PyErr_Print(); + } + Py_XDECREF(ret); } - PyGILState_Release(s); + PyEval_ReleaseThread(self->threadState); }); QObject::connect(thing, &Thing::settingChanged, [=](const ParamTypeId ¶mTypeId, const QVariant &value){ - PyGILState_STATE s = PyGILState_Ensure(); + PyEval_RestoreThread(self->threadState); Py_XDECREF(self->pySettings); self->pySettings = PyParams_FromParamList(self->thing->settings()); if (self->pySettingChangedHandler) { - PyObject_CallFunctionObjArgs(self->pySettingChangedHandler, self, PyUnicode_FromString(paramTypeId.toString().toUtf8().data()), QVariantToPyObject(value), nullptr); + PyObject * ret = PyObject_CallFunctionObjArgs(self->pySettingChangedHandler, self, PyUnicode_FromString(paramTypeId.toString().toUtf8().data()), QVariantToPyObject(value), nullptr); + if (PyErr_Occurred()) { + PyErr_Print(); + } + Py_XDECREF(ret); } - PyGILState_Release(s); + PyEval_ReleaseThread(self->threadState); }); QObject::connect(thing, &Thing::stateValueChanged, [=](const StateTypeId &stateTypeId, const QVariant &value){ - PyGILState_STATE s = PyGILState_Ensure(); + PyEval_RestoreThread(self->threadState); for (int i = 0; i < PyList_Size(self->pyStates); i++) { PyObject *pyState = PyList_GetItem(self->pyStates, i); PyObject *pyStateTypeId = PyDict_GetItemString(pyState, "stateTypeId"); @@ -121,7 +131,7 @@ static void PyThing_setThing(PyThing *self, Thing *thing) break; } } - PyGILState_Release(s); + PyEval_ReleaseThread(self->threadState); }); } @@ -342,6 +352,7 @@ static PyMethodDef PyThing_methods[] = { static PyMemberDef PyThing_members[] = { {"params", T_OBJECT_EX, offsetof(PyThing, pyParams), READONLY, "Thing params"}, {"nameChangedHandler", T_OBJECT_EX, offsetof(PyThing, pyNameChangedHandler), 0, "Set a callback for when the thing name changes"}, + {"settingChangedHandler", T_OBJECT_EX, offsetof(PyThing, pySettingChangedHandler), 0, "Set a callback for when a thing setting changes"}, {nullptr, 0, 0, 0, nullptr} /* Sentinel */ }; @@ -409,7 +420,6 @@ static void registerThingType(PyObject *module) for (int i = 0; i < thingErrorEnum.keyCount(); i++) { PyModule_AddObject(module, thingErrorEnum.key(i), PyLong_FromLong(thingErrorEnum.value(i))); } - } diff --git a/libnymea-core/integrations/python/pythingdescriptor.h b/libnymea-core/integrations/python/pythingdescriptor.h index 6ebc0e06..f1d0ba6e 100644 --- a/libnymea-core/integrations/python/pythingdescriptor.h +++ b/libnymea-core/integrations/python/pythingdescriptor.h @@ -36,7 +36,7 @@ static int PyThingDescriptor_init(PyThingDescriptor *self, PyObject *args, PyObj static char *kwlist[] = {"thingClassId", "name", "description", "thingId", "params", nullptr}; PyObject *thingClassId = nullptr, *name = nullptr, *description = nullptr, *thingId = nullptr, *params = nullptr; - qWarning() << "++++ PyThingDescriptor"; + qCDebug(dcPythonIntegrations()) << "+++ PyThingDescriptor"; if (!PyArg_ParseTupleAndKeywords(args, kwds, "|OOOOO", kwlist, &thingClassId, &name, &description, &thingId, ¶ms)) return -1; @@ -65,7 +65,7 @@ static int PyThingDescriptor_init(PyThingDescriptor *self, PyObject *args, PyObj static void PyThingDescriptor_dealloc(PyThingDescriptor * self) { - qWarning() << "---- PyThingDescriptor"; + qCDebug(dcPythonIntegrations()) << "--- PyThingDescriptor"; Py_XDECREF(self->pyThingClassId); Py_XDECREF(self->pyName); Py_XDECREF(self->pyDescription); diff --git a/libnymea-core/integrations/python/pythingdiscoveryinfo.h b/libnymea-core/integrations/python/pythingdiscoveryinfo.h index 9b8e07b7..cfacfe53 100644 --- a/libnymea-core/integrations/python/pythingdiscoveryinfo.h +++ b/libnymea-core/integrations/python/pythingdiscoveryinfo.h @@ -49,7 +49,7 @@ static PyObject* PyThingDiscoveryInfo_new(PyTypeObject *type, PyObject */*args*/ if (self == NULL) { return nullptr; } - qWarning() << "++++ PyThingDiscoveryInfo"; + qCDebug(dcPythonIntegrations()) << "+++ PyThingDiscoveryInfo"; return (PyObject*)self; } @@ -62,7 +62,7 @@ void PyThingDiscoveryInfo_setInfo(PyThingDiscoveryInfo *self, ThingDiscoveryInfo static void PyThingDiscoveryInfo_dealloc(PyThingDiscoveryInfo * self) { - qWarning() << "---- PyThingDiscoveryInfo"; + qCDebug(dcPythonIntegrations()) << "--- PyThingDiscoveryInfo"; Py_DECREF(self->pyThingClassId); Py_DECREF(self->pyParams); Py_TYPE(self)->tp_free(self); diff --git a/libnymea-core/integrations/python/pythingpairinginfo.h b/libnymea-core/integrations/python/pythingpairinginfo.h index 8dce6565..6194a4ea 100644 --- a/libnymea-core/integrations/python/pythingpairinginfo.h +++ b/libnymea-core/integrations/python/pythingpairinginfo.h @@ -60,7 +60,7 @@ static PyMemberDef PyThingPairingInfo_members[] = { static int PyThingPairingInfo_init(PyThingPairingInfo */*self*/, PyObject */*args*/, PyObject */*kwds*/) { - qWarning() << "++++ ThingPairingInfo"; + qCDebug(dcPythonIntegrations()) << "+++ ThingPairingInfo"; return 0; } @@ -77,7 +77,7 @@ void PyThingPairingInfo_setInfo(PyThingPairingInfo *self, ThingPairingInfo *info static void PyThingPairingInfo_dealloc(PyThingPairingInfo * self) { - qWarning() << "---- ThingPairingInfo"; + qCDebug(dcPythonIntegrations()) << "--- ThingPairingInfo"; Py_XDECREF(self->pyTransactionId); Py_XDECREF(self->pyThingClassId); Py_XDECREF(self->pyThingId); diff --git a/libnymea-core/integrations/python/pyutils.h b/libnymea-core/integrations/python/pyutils.h index 2e00da05..65f653ff 100644 --- a/libnymea-core/integrations/python/pyutils.h +++ b/libnymea-core/integrations/python/pyutils.h @@ -69,7 +69,6 @@ QVariant PyObjectToQVariant(PyObject *pyObject) return QVariant(); } - // Write to stdout PyObject* pyLog_write(PyObject* /*self*/, PyObject* args) { diff --git a/libnymea-core/integrations/pythonintegrationplugin.cpp b/libnymea-core/integrations/pythonintegrationplugin.cpp index 8cabf7b3..c588eeb8 100644 --- a/libnymea-core/integrations/pythonintegrationplugin.cpp +++ b/libnymea-core/integrations/pythonintegrationplugin.cpp @@ -108,7 +108,6 @@ PyObject *PythonIntegrationPlugin::pyAutoThingsAppeared(PyObject *self, PyObject PyObject *iter = PyObject_GetIter(pyDescriptors); if (!iter) { - Py_DECREF(pyDescriptors); qCWarning(dcThingManager()) << "Error parsing args. Not a param list"; return nullptr; } @@ -124,6 +123,7 @@ PyObject *PythonIntegrationPlugin::pyAutoThingsAppeared(PyObject *self, PyObject if (next->ob_type != &PyThingDescriptorType) { PyErr_SetString(PyExc_ValueError, "Invalid argument. Not a ThingDescriptor."); + Py_DECREF(next); continue; } PyThingDescriptor *pyDescriptor = (PyThingDescriptor*)next; @@ -155,8 +155,6 @@ PyObject *PythonIntegrationPlugin::pyAutoThingsAppeared(PyObject *self, PyObject QMetaObject::invokeMethod(plugin, "autoThingsAppeared", Qt::QueuedConnection, Q_ARG(ThingDescriptors, descriptors)); Py_DECREF(iter); - Py_DECREF(pyDescriptors); - Py_RETURN_NONE; } @@ -432,13 +430,15 @@ void PythonIntegrationPlugin::setupThing(ThingSetupInfo *info) { PyEval_RestoreThread(m_threadState); + Thing *thing = info->thing(); + PyThing *pyThing = nullptr; - if (m_things.contains(info->thing())) { - pyThing = m_things.value(info->thing()); + if (m_things.contains(thing)) { + pyThing = m_things.value(thing); } else { pyThing = (PyThing*)PyObject_CallObject((PyObject*)&PyThingType, NULL); - PyThing_setThing(pyThing, info->thing()); - m_things.insert(info->thing(), pyThing); + PyThing_setThing(pyThing, thing, m_threadState); + m_things.insert(thing, pyThing); } PyObject *args = PyTuple_New(1); @@ -457,6 +457,7 @@ void PythonIntegrationPlugin::setupThing(ThingSetupInfo *info) connect(info->thing(), &Thing::destroyed, this, [=](){ PyEval_RestoreThread(m_threadState); + m_things.remove(thing); pyThing->thing = nullptr; Py_DECREF(pyThing); m_threadPool->setMaxThreadCount(m_threadPool->maxThreadCount() - 1); @@ -515,31 +516,6 @@ void PythonIntegrationPlugin::thingRemoved(Thing *thing) { PyThing *pyThing = m_things.value(thing); callPluginFunction("thingRemoved", reinterpret_cast(pyThing)); - - m_mutex.lock(); - m_things.remove(thing); - m_mutex.unlock(); -} - -void PythonIntegrationPlugin::dumpError() -{ - if (!PyErr_Occurred()) { - return; - } - - PyObject *ptype, *pvalue, *ptraceback; - PyErr_Fetch(&ptype, &pvalue, &ptraceback); - if (pvalue) { - PyObject *pstr = PyObject_Str(pvalue); - if (pstr) { - const char* err_msg = PyUnicode_AsUTF8(pstr); - if (pstr) { - qCWarning(dcThingManager()) << QString(err_msg); - } - - } - PyErr_Restore(ptype, pvalue, ptraceback); - } } void PythonIntegrationPlugin::exportIds() @@ -658,10 +634,10 @@ bool PythonIntegrationPlugin::callPluginFunction(const QString &function, PyObje PyEval_RestoreThread(threadState); PyObject *pluginFunctionResult = PyObject_CallFunctionObjArgs(pluginFunction, param1, param2, param3, nullptr); - dumpError(); if (PyErr_Occurred()) { qCWarning(dcThingManager()) << "Error calling python method:" << function << "on plugin" << pluginName(); + PyErr_Print(); } Py_DECREF(pluginFunction); diff --git a/libnymea-core/integrations/pythonintegrationplugin.h b/libnymea-core/integrations/pythonintegrationplugin.h index 733777da..1539202f 100644 --- a/libnymea-core/integrations/pythonintegrationplugin.h +++ b/libnymea-core/integrations/pythonintegrationplugin.h @@ -39,7 +39,6 @@ public: void thingRemoved(Thing *thing) override; - static void dumpError(); static PyObject* pyConfiguration(PyObject* self, PyObject* args); static PyObject* pyConfigValue(PyObject* self, PyObject* args); static PyObject* pySetConfigValue(PyObject* self, PyObject* args); diff --git a/plugins/pymock/integrationpluginpymock.py b/plugins/pymock/integrationpluginpymock.py index 42eaa846..31ef8c7c 100644 --- a/plugins/pymock/integrationpluginpymock.py +++ b/plugins/pymock/integrationpluginpymock.py @@ -96,6 +96,8 @@ def confirmPairing(info, username, secret): def setupThing(info): logger.log("setupThing for", info.thing.name) info.finish(nymea.ThingErrorNoError) + info.thing.nameChangedHandler = thingNameChanged + info.thing.settingChangedHandler = thingSettingChanged def postSetupThing(thing): @@ -123,6 +125,14 @@ def autoThings(): return autoThings +def thingNameChanged(thing, name): + logger.log("Thing name changed:", thing.name) + + +def thingSettingChanged(thing, paramTypeId, value): + logger.log("Thing setting changed:", thing.name, paramTypeId, value) + + # Intentionally commented out to also have a test case for unimplmented functions # def thingRemoved(thing): # logger.log("thingRemoved for", thing.name) diff --git a/python-todos.txt b/python-todos.txt index bb95d61c..59fce1f5 100644 --- a/python-todos.txt +++ b/python-todos.txt @@ -1,3 +1,2 @@ -* thing.settingChangedHandler missing * pluginStorage missing