Add support for DC gensets #1435

DanielMcInnes · 2024-09-03T06:29:05Z

DanielMcInnes · 2024-09-04T01:24:05Z

I haven't been able to test the dc-generator-specific changes with mqtt / wasm, only with dbus via the latest venus-docker simulations 'gdf' and 'gdh'.

chriadam · 2024-09-04T04:31:30Z

components/PageGensetModel.qml

@@ -32,7 +33,9 @@ ObjectModel {
 		}
 	}

-	readonly property var nrOfPhases: VeQuickItem {
+	readonly property bool dcGenset: serviceType === "dcgenset"
+	readonly property int nrOfPhases: phases.valid ? phases.value : dcGenset ? 0 : 3


style nit: would prefer this to be broken over 3 lines, e.g.:

readonly property int nrOfPhases: phases.valid ? phases.value : dcGenset ? 0 : 3

Should be phases.isValid?

yes it should be, thanks Bea.
Style nit fixed also

chriadam · 2024-09-04T04:33:00Z

pages/settings/devicelist/DeviceListPage.qml

+			} else {
+				summary = [ gensetPowerText ]
+			}
+			break;


style nit: for consistency, put an empty whitespace line after the break

chriadam · 2024-09-04T04:33:21Z

pages/settings/devicelist/PageGenset.qml

@@ -0,0 +1,59 @@
+/*
+** Copyright (C) 2023 Victron Energy B.V.


chriadam · 2024-09-04T04:39:33Z

components/PageGensetModel.qml

+							text: qsTrId("genset_bms_control")
+							secondaryText: CommonWords.press_to_reset
+							visible: defaultAllowed && bmsControlled.dataItem.value === 1
+							// TODO: gui-v1 has "cornerMark: true" for this button. What does this mean? Does gui-v2 support this?


Seems like it's just an icon in the bottom-right hand corner of the button, which will change colour if the list item to which it belongs is the currently selected list item. It looks like it's mostly set to true for items which are editable, or activatable (e.g. slider). I suspect it's just to visualise the fact that the item is interactable in some way.

I don't think we need it, as in gui-v2 we have blue borders for interactable things, generally, and we (hopefully) visualize disabled items with grey instead.

chriadam · 2024-09-04T04:41:58Z

components/PageGensetModel.qml

+						}
+
+						ListSpinBox {
+							text: CommonWords.charge_current


Hmm, I would have assumed the string to be "Charge current limit" rather than "Charge current" (i.e. the maximum current which should be generated by the genset when charging the battery) but perhaps I am misunderstanding?

Updated as suggested. It is "Charge current" in gui-v1, but I think you are right

blammit

Have made some initial comments; I'll try to test against the gui-v1 version to check that as well.

blammit · 2024-09-05T04:07:31Z

components/PageGensetModel.qml

@@ -32,7 +33,9 @@ ObjectModel {
 		}
 	}

-	readonly property var nrOfPhases: VeQuickItem {
+	readonly property bool dcGenset: serviceType === "dcgenset"
+	readonly property int nrOfPhases: phases.valid ? phases.value : dcGenset ? 0 : 3


Should be phases.isValid?

blammit · 2024-09-05T04:08:42Z

components/PageGensetModel.qml

-	readonly property var nrOfPhases: VeQuickItem {
+	readonly property bool dcGenset: serviceType === "dcgenset"
+	readonly property int nrOfPhases: phases.valid ? phases.value : dcGenset ? 0 : 3
+	readonly property var phases: VeQuickItem {


Does this need to be a property or can it just be a child item?

If it needs to be a property then it can be strongly typed as a VeQuickItem type.

This needs to be a property, changed type to VeQuickItem for strong typing

blammit · 2024-09-05T04:14:31Z

components/PageGensetModel.qml

+							//% "BMS Controlled"
+							text: qsTrId("genset_bms_controlled")
+							dataItem.uid: root.bindPrefix + "/Settings/BmsPresent"
+							enabled: false


Can just make this a ListTextItem if it's not actually clickable?

blammit · 2024-09-05T04:14:52Z

components/PageGensetModel.qml

+							allowed: defaultAllowed && dataItem.isValid
+						}
+
+						ListTextItem {


I think this could be a ListLabel instead, and placed as a caption within the bmsControlled component (using bottomContent) since it's only visible when that is enabled.

done. looks like this:

blammit · 2024-09-05T04:17:04Z

components/PageGensetModel.qml

+							dataItem.uid: root.bindPrefix + "/Settings/ChargeVoltage"
+							decimals: 1
+							stepSize: 0.1
+							suffix: "V"


Can fetch from units.cpp instead of hardcoding, e.g. Units.defaultUnitString(VenusOS.Units_Volt)

Similarly for other suffix values elsewhere.

blammit · 2024-09-05T04:32:11Z

components/PageGensetModel.qml

@@ -91,7 +94,7 @@ ObjectModel {
 		text: qsTrId("ac-in-genset_error")
 		dataItem.uid: root.bindPrefix + "/ErrorCode"
 		allowed: defaultAllowed && dataItem.isValid
-		nrOfPhases: root.nrOfPhases.value || 3
+		nrOfPhases: root.nrOfPhases


I noticed this part of #1377 hasn't been done yet: port "Refactored MbItemFpGensetError to MbItemGensetError" to gui-v2

The main thing there is to rewrite ListFpGensetErrorItem so that it fetches the error description from GensetError instead of having a massive switch statement for the secondaryText. Note that GensetError will need to be registered as a QML type, similar to what's done for ChargerError and WakespeedError.

ListFpGensetErrorItem has been deleted in main, so I guess this goes away until we need to translate a generator error code

blammit · 2024-09-09T05:35:49Z

Have made some initial comments; I'll try to test against the gui-v1 version to check that as well.

Did some more checking via venus-docker instead. The pages all seem fine, I noticed various warnings like these:

UID "AutoStartEnabled" contains invalid part: "AutoStartEnabled"
UID "NextTestRun" contains invalid part: "NextTestRun"
UID "TestRunIntervalRuntime" contains invalid part: "TestRunIntervalRuntime"

Which I think is just due to a lack of checks on whether root.startStopBindPrefix is empty, when assigning dataItem.uid values in PageGenerator.qml.

Fixes #1377

DanielMcInnes · 2024-09-13T05:35:10Z

Have made some initial comments; I'll try to test against the gui-v1 version to check that as well.

Did some more checking via venus-docker instead. The pages all seem fine, I noticed various warnings like these:
UID "AutoStartEnabled" contains invalid part: "AutoStartEnabled"
UID "NextTestRun" contains invalid part: "NextTestRun"
UID "TestRunIntervalRuntime" contains invalid part: "TestRunIntervalRuntime"
Which I think is just due to a lack of checks on whether root.startStopBindPrefix is empty, when assigning dataItem.uid values in PageGenerator.qml.

I can't reproduce this, I think it may have been fixed. Would you mind checking again?

chriadam

LGTM, modulo those invalid uid part warnings which Bea mentioned - @blammit could you double check whether those are fixed?

blammit · 2024-09-16T03:19:18Z

components/PageGensetModel.qml

+	readonly property bool dcGenset: serviceType === "dcgenset"
+	readonly property int nrOfPhases: phases.valid ? phases.value
+												   : dcGenset ? 0
+															  : 3


valid -> isValid

Also could we just have readonly property int nrOfPhases: phases.isValid ? phases.value : (dcGenset ? 0 : 3) to avoid the odd indentation?

[Edit: I see that @chriadam suggested this change to go over multiple lines ... hm. I wish this could be done without such massive indentation]

After changing this to isValid, I get this error:

<Unknown File>: Can't create role for unsupported data type

Any ideas?

Is there a ListModel involved somewhere? I think it can serve ListElement entries or JS objects which have only QML basic type properties, and not more complex JavaScript types (including arrays).

Long shot, but does explicitly qualifying to root.phases make any difference? It shouldn't, of course.

It's something to do with GensetErrorModel, I'll figure it out

Fixed, ListModel was complaining about setting an element's value to undefined

blammit · 2024-09-16T03:38:53Z

I can't reproduce this, I think it may have been fixed. Would you mind checking again?

LGTM, modulo those invalid uid part warnings which Bea mentioned - @blammit could you double check whether those are fixed?

@DanielMcInnes I can't reproduce those anymore, but when running with the 'gdh' simulation, if I click "Hatz Genset" in the Device List and then "Settings", there is a warning:

UID "Capabilities" contains invalid part: "Capabilities"

... hang on, this is only when running via MQTT. Let me see, I think this is related to the GensetService check in PageGensetModel.

[Edit: I will add a comment to PageGensetModel.qml regarding a fix for setting root.generatorWithGensetService to resolve this.]

blammit · 2024-09-16T03:51:33Z

components/PageGensetModel.qml

@@ -12,6 +12,7 @@ ObjectModel {
 	property string bindPrefix
 	property string settingsBindPrefix: Global.systemSettings.serviceUid + "/Settings/Generator1"
 	property string startStopBindPrefix: startStop1Uid
+	readonly property string serviceType: BackendConnection.serviceTypeFromUid(bindPrefix)

 	// On D-Bus, the startstop1 generator is at com.victronenergy.generator.startstop1.
 	// On MQTT, the startstop1 generator is the one with GensetService=com.victronenergy.genset.*


This part needs to be updated to check for the "dcgenset" service as well, for MQTT backends. Basically, on MQTT we need to find a generator with a matching GensetService, because we don't have a straightforward way to find the startstop1 service there, unlike on a D-Bus backend. Here is a patch that seems to fix this, from testing via MQTT with the gdh simulation:

diff --git a/components/PageGensetModel.qml b/components/PageGensetModel.qml index b386ae1e..66aeb72e 100644 --- a/components/PageGensetModel.qml +++ b/components/PageGensetModel.qml @@ -16,6 +16,7 @@ ObjectModel { // On D-Bus, the startstop1 generator is at com.victronenergy.generator.startstop1. // On MQTT, the startstop1 generator is the one with GensetService=com.victronenergy.genset.* + // (or GensetService=com.victronenergy.dcgenset.* if this is a dcgenset) readonly property string startStop1Uid: BackendConnection.type === BackendConnection.MqttSource ? generatorWithGensetService : BackendConnection.uidPrefix() + "/com.victronenergy.generator.startstop1" @@ -26,7 +27,8 @@ ObjectModel { delegate: VeQuickItem { uid: model.device.serviceUid + "/GensetService" onValueChanged: { - if (value !== undefined && value.startsWith("com.victronenergy.genset.")) { + if ( (isValid && root.dcGenset && value.startsWith("com.victronenergy.dcgenset.")) + || (isValid && !root.dcGenset && value.startsWith("com.victronenergy.genset.")) ) { root.generatorWithGensetService = model.device.serviceUid } }

thanks Bea, updated as suggested.

As per gui-v1 commit b20574be6c3be329e5452928ee03e521c1c0e94d

blammit

Great, lgtm :)

DanielMcInnes linked an issue Sep 3, 2024 that may be closed by this pull request

Add support for DC gensets #1377

Closed

DanielMcInnes marked this pull request as draft September 3, 2024 06:40

DanielMcInnes force-pushed the 1377-add-support-for-dc-gensets branch 2 times, most recently from ae56c39 to b5ecad9 Compare September 4, 2024 01:13

DanielMcInnes marked this pull request as ready for review September 4, 2024 01:14

DanielMcInnes force-pushed the 1377-add-support-for-dc-gensets branch from b5ecad9 to a81fdcd Compare September 4, 2024 01:17

chriadam reviewed Sep 4, 2024

View reviewed changes

blammit reviewed Sep 5, 2024

View reviewed changes

DanielMcInnes force-pushed the 1377-add-support-for-dc-gensets branch from a81fdcd to 815f04a Compare September 10, 2024 04:36

DanielMcInnes mentioned this pull request Sep 10, 2024

Add Qt6 support for GensetError victronenergy/veutil#4

Merged

DanielMcInnes force-pushed the 1377-add-support-for-dc-gensets branch from 815f04a to fef8e19 Compare September 11, 2024 03:24

DanielMcInnes marked this pull request as draft September 11, 2024 03:30

Add support for DC gensets

f54505e

Fixes #1377

DanielMcInnes force-pushed the 1377-add-support-for-dc-gensets branch 3 times, most recently from 1ead03b to f4eb315 Compare September 13, 2024 05:30

DanielMcInnes marked this pull request as ready for review September 13, 2024 05:35

chriadam approved these changes Sep 16, 2024

View reviewed changes

blammit reviewed Sep 16, 2024

View reviewed changes

DanielMcInnes force-pushed the 1377-add-support-for-dc-gensets branch 2 times, most recently from 53576e7 to ba4194f Compare September 16, 2024 05:17

Add dedicated page for gensets

18e31de

As per gui-v1 commit b20574be6c3be329e5452928ee03e521c1c0e94d

DanielMcInnes force-pushed the 1377-add-support-for-dc-gensets branch from ba4194f to 18e31de Compare September 16, 2024 06:10

blammit approved these changes Sep 16, 2024

View reviewed changes

DanielMcInnes merged commit 2c69461 into main Sep 16, 2024
2 checks passed

DanielMcInnes deleted the 1377-add-support-for-dc-gensets branch September 16, 2024 06:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for DC gensets #1435

Add support for DC gensets #1435

Add support for DC gensets #1435

Add support for DC gensets #1435

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment