Improve UID mapper #237

chriadam · 2022-12-16T05:07:35Z

Instead of listening to all active topics, only listen to specific service name to path mapping changes.

src/uidhelper.h

DanielMcInnes · 2022-12-16T06:32:49Z

components/datapoints/DataPoint.qml

@@ -84,7 +84,7 @@ QtObject {
 			sourceObject.destroy()
 			sourceObject = null
 		}
-		sourceObject = _mqttImpl.createObject(root, { uid: _mqttUidHelper.mqttUid })
+		sourceObject = _mqttImpl.createObject(root, { uid: Qt.binding(function() { return _mqttUidHelper.mqttUid }) })


what would have to happen for _mqttUidHelper.mqttUid to change?

e.g. pathForServiceNameChanged() if the provider gets new data for a service name we've already connected to (in the case where, at the time we first connected, the provider hadn't enumerated all of the values from the MQTT broker yet).

blammit

I had some trouble building this -- it seems this references a version of velib that has already moved the VeQItem etc. to veutil, so then files like ve_qitem_dbus_publisher.hpp are no longer available, even though they are still referenced in gui-v2/CMakeLists.txt.

blammit · 2022-12-19T02:52:18Z

src/uidhelper.cpp

+	if (path.endsWith(QStringLiteral("/ServiceName"))) {
+		const QString serviceName = message.toString();
+		m_pathToServiceName.insert(path, serviceName);
+		const QString devicePath = path.mid(0, path.length() - 11);


Could put "/ServiceName" in a static const string and reference the string length here to make this more explicit?

Instead of listening to all active topics, only listen to specific service name to path mapping changes.

chriadam · 2023-01-05T03:46:26Z

I've now ported on top of veutil etc.
BUT: I can't push to veutil repo directly, so I had to create a fork and PR from there: https://github.com/victronenergy/veutil/pull/1/files

So, until that PR is merged, the veutil submodule in this repo will point to the wrong sha1...
To test locally, just do: git format-patch -1 in my branch of the veutil repo to get my patch, then apply it in the submodule repo here with git am *.patch.

chriadam · 2023-01-05T04:59:29Z

src/main.cpp

+#include "veutil/qt/ve_qitems_mqtt.hpp"
+#include "veutil/qt/ve_qitem_table_model.hpp"
+#include "veutil/qt/ve_qitem_sort_table_model.hpp"
+#include "veutil/qt/ve_qitem_child_model.hpp"


We currently register types into Victron.Velib, should we register them into Victron.Veutil instead?

Yes, that sounds best

blammit

I'm seeing this when running for MQTT:

expression for onCompleted@qrc:/data/mqtt/EssImpl.qml:90
Too many arguments, ignoring 2
expression for onCompleted@qrc:/data/mqtt/EssImpl.qml:95
Too many arguments, ignoring 2
expression for onCompleted@qrc:/data/mqtt/EssImpl.qml:101
Too many arguments, ignoring 2
expression for onCompleted@qrc:/data/mqtt/EssImpl.qml:107
Too many arguments, ignoring 2

In EssImpl, looks like the first two Component.onCompleted calls for veState and veHub4Mode do nothing, and can be removed. The second two for veMinimumSocLimit and veSocLimit could just have a Component.onCompleted that duplicates the functionality of their onValueChanged handlers.

There are a lot of warnings like this when running on device with dbus:

qt.core.qobject.connect: QObject::connect: No such signal VeQItemDbus::stateChanged(VeQItem*,State)
qt.core.qobject.connect: QObject::connect:  (sender name:   'CustomName')
qt.core.qobject.connect: QObject::connect: No such signal VeQItemDbus::stateChanged(VeQItem*,State)
qt.core.qobject.connect: QObject::connect:  (sender name:   'ProductName')
qt.core.qobject.connect: QObject::connect: Incompatible sender/receiver arguments
        VeQItemDbus::valueChanged(QVariant) --> DBusService::updateDescription(VeQItem*,QVariant)
qt.core.qobject.connect: QObject::connect: Incompatible sender/receiver arguments
        VeQItemDbus::initValue(QVariant) --> DBusService::updateDescription(VeQItem*,QVariant)
qt.core.qobject.connect: QObject::connect: Incompatible sender/receiver arguments
        VeQItemDbus::valueChanged(QVariant) --> DBusService::updateDescription(VeQItem*,QVariant)
qt.core.qobject.connect: QObject::connect: Incompatible sender/receiver arguments
        VeQItemDbus::initValue(QVariant) --> DBusService::updateDescription(VeQItem*,QVariant)
qt.core.qobject.connect: QObject::connect: No such signal VeQItemDbus::stateChanged(VeQItem*,State)
qt.core.qobject.connect: QObject::connect:  (sender name:   'com.victronenergy.battery.ttyUSB0')

Looking at https://github.com/victronenergy/gui/blob/master/src/dbus_service.cpp#L16, src/gui-v1/dbus_service.cpp needs to be updated to connect to e.g. SIGNAL(stateChanged(VeQItem::State)) instead of SIGNAL(stateChanged(VeQItem*,State)) ?

chriadam · 2023-01-06T04:38:02Z

Thanks, fixed all the warnings now as far as I know.

DanielMcInnes · 2023-01-12T01:34:41Z

src/gui-v1/alarmmonitor.h

@@ -2,7 +2,7 @@
 #define ALARMMONITOR_H

 #include <QObject>
-#include <velib/qt/ve_qitem.hpp>
+#include <veutil/qt/ve_qitem.hpp>


this #include could be replaced by a forward declaration

I haven't changed these files, aside from search/replacing velib to veutil in the paths.

DanielMcInnes · 2023-01-12T01:38:38Z

src/gui-v1/dbus_services.h

@@ -3,8 +3,8 @@

 #include <QObject>

-#include <velib/qt/ve_qitem.hpp>
-#include <velib/qt/ve_qitem_table_model.hpp>
+#include <veutil/qt/ve_qitem.hpp>


this #include could be replaced with a forward declaration

ditto, I don't want to change these files if possible, aside from updating paths from velib to veutil

DanielMcInnes

lgtm, 2 nits

chriadam · 2023-02-01T03:43:30Z

I've now updated the veutil submodule sha1 to point to victronenergy/veutil#2 which is based in a "proper" branch there. Once that PR is merged, we'll have to update it again, but victronenergy/veutil#1 languished for over a month without any movement, so let's not wait longer.

chriadam requested review from blammit and DanielMcInnes December 16, 2022 05:07

chriadam mentioned this pull request Dec 16, 2022

Handle D-Bus to MQTT uid conversions where there are multiple device ids #235

Closed

chriadam commented Dec 16, 2022

View reviewed changes

src/uidhelper.h Outdated Show resolved Hide resolved

chriadam commented Dec 16, 2022

View reviewed changes

src/uidhelper.h Outdated Show resolved Hide resolved

DanielMcInnes reviewed Dec 16, 2022

View reviewed changes

chriadam force-pushed the chriadam-uid-mapper-fix branch from 923332d to 21ded2c Compare December 16, 2022 08:09

blammit reviewed Dec 19, 2022

View reviewed changes

chriadam mentioned this pull request Dec 20, 2022

'Services' settings page #236

Merged

Improve UID mapper

7897fba

Instead of listening to all active topics, only listen to specific service name to path mapping changes.

chriadam force-pushed the chriadam-uid-mapper-fix branch from 21ded2c to a892b42 Compare January 5, 2023 03:44

chriadam mentioned this pull request Jan 5, 2023

Settings firmware #249

Merged

chriadam commented Jan 5, 2023

View reviewed changes

blammit reviewed Jan 5, 2023

View reviewed changes

chriadam force-pushed the chriadam-uid-mapper-fix branch 2 times, most recently from a48dd73 to 3eacd3a Compare January 6, 2023 04:37

blammit approved these changes Jan 8, 2023

View reviewed changes

DanielMcInnes reviewed Jan 12, 2023

View reviewed changes

DanielMcInnes approved these changes Jan 12, 2023

View reviewed changes

blammit mentioned this pull request Jan 29, 2023

Remove duplicate data implementations #258

Merged

Port to veutil base

b693fc8

chriadam force-pushed the chriadam-uid-mapper-fix branch from 3eacd3a to b693fc8 Compare February 1, 2023 03:27

chriadam merged commit a213673 into main Feb 1, 2023

chriadam deleted the chriadam-uid-mapper-fix branch February 27, 2023 04:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve UID mapper #237

Improve UID mapper #237

chriadam commented Dec 16, 2022

DanielMcInnes Dec 16, 2022

chriadam Dec 16, 2022

blammit left a comment •

edited

Loading

blammit Dec 19, 2022

chriadam commented Jan 5, 2023

chriadam Jan 5, 2023

blammit Jan 5, 2023

chriadam Jan 6, 2023

blammit left a comment

chriadam commented Jan 6, 2023

DanielMcInnes Jan 12, 2023

chriadam Feb 1, 2023

DanielMcInnes Jan 12, 2023 •

edited

Loading

chriadam Feb 1, 2023

DanielMcInnes left a comment

chriadam commented Feb 1, 2023

Improve UID mapper #237

Improve UID mapper #237

Conversation

chriadam commented Dec 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blammit left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriadam commented Jan 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blammit left a comment

Choose a reason for hiding this comment

chriadam commented Jan 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielMcInnes Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielMcInnes left a comment

Choose a reason for hiding this comment

chriadam commented Feb 1, 2023

blammit left a comment •

edited

Loading

DanielMcInnes Jan 12, 2023 •

edited

Loading