-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
395 keep settings up to date with gui v1 #414
395 keep settings up to date with gui v1 #414
Conversation
04e3bbd
to
d81715c
Compare
pages/settings/PageGenerator.qml
Outdated
@@ -20,7 +20,17 @@ Page { | |||
|
|||
function getState() | |||
{ | |||
if (generatorState.value === 10) { | |||
switch(generatorState.value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need: case 1: return "Running";
and also: case 10: // fall-through; default: return CommonWords.error;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather keep the logic the same as gui-v1. Maybe generatorState.value can have valid values for 5,6,7,8,9, in which case they want to fall through to the second switch statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair point, ok let's keep it in sync with gui-v1.
nitpick: Should we add a whitespace between switch(
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely, updated as suggested
@@ -119,7 +129,7 @@ Page { | |||
text: qsTrId("settings_page_relay_generator_run_time") | |||
secondaryText: dataValid ? Utils.secondsToString(dataValue, false) : "0" | |||
dataSource: root.startStopBindPrefix + "/Runtime" | |||
visible: generatorState.value === 1 | |||
visible: generatorState.value in [1, 2, 3] // Running, Warm-up, Cool-down |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
, 4] // ... stopping ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? I'd rather keep it the same as gui-v1. They might have a good reason for not including stopping. I could ask victron every time I see something that looks suspicious, but then porting settings would take weeks, not days
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, keeping it in sync with gui-v1 is probably best.
@@ -18,8 +18,7 @@ Page { | |||
AccessLevelRadioButtonGroup {} | |||
|
|||
ListTextField { | |||
//% "Set root password" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I agree with this change at all.
It's a sentence and not a single word. Setting a password is a pretty important action, and must be able to be described in various different languages. The translators can leave root
untranslated if they want, but the rest of the sentence should be translated, surely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, couldn't agree more. Deleted this change.
@@ -20,8 +20,9 @@ LanguageModel::LanguageModel(QObject *parent) | |||
m_languages.append({ "Español", "es", QLocale::Spanish }); | |||
m_languages.append({ "Français", "fr", QLocale::French }); | |||
m_languages.append({ "Italiano", "it", QLocale::Italian }); | |||
m_languages.append({ "Nederlands", "nl", QLocale::Dutch }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space/tab whitespace difference I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, fixed
@@ -24,8 +24,9 @@ LanguageModel::LanguageModel(QObject *parent) | |||
m_languages.append({ "Polski", "pl", QLocale::Polish }); | |||
m_languages.append({ "Русский", "ru", QLocale::Russian }); | |||
m_languages.append({ "Română", "ro", QLocale::Romanian }); | |||
m_languages.append({ "Svenska", "se", QLocale::NorthernSami }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly, some whitespace problems in this commit too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -16,8 +16,9 @@ LanguageModel::LanguageModel(QObject *parent) | |||
{ | |||
m_languages.append({ "English", "en", QLocale::English }); | |||
m_languages.append({ "Čeština", "cs", QLocale::Czech }); | |||
m_languages.append({ "Deutsch", "de", QLocale::German }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
white space problems again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
ae41796
to
a76942a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but would be good if Bea can review also
a76942a
to
40fb9eb
Compare
pages/settings/PageDeviceInfo.qml
Outdated
@@ -17,6 +17,7 @@ Page { | |||
//% "Connection" | |||
text: qsTrId("settings_deviceinfo_connection") | |||
dataSource: root.bindPrefix + "/Mgmt/Connection" | |||
invalidate: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The invalidate property in DataPoint just shows the current value of VeQuickItem::invalidate, so changing it won't actually change the value of VeQuickItem::invalidate. This is similar to how DataPoint has value but to change the actual value you need to call setValue.
We could add setInvalidate to DataPoint to change the value explicitly and call that here like Component.onCompleted: setInvalidate(false) which is a bit ugly, but does the job for now. Alternatively we could have onInvalidateChanged in DataPoint that updates the backend value of invalidate, but in that case maybe we should remove setData as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done this using 'Qt.binding', what do you think? I've tested it with dbus and mqtt and it seems to do what I want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused... Jeroen provided this usage example from gui-v1: https://github.com/victronenergy/gui/commit/9c6ab64139a2dc2a71e752f76509b2ea81acaf5a
where he does just set invalidate to false in cases where he wants the value() to return the cached/stale value even after the item is invalidated.
So I thought what you had originally was correct. Am I wrong / misunderstanding something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chriadam that works because it's setting the value of VeQuickItem::invalidate. But in Daniel's original patch it was setting DataPoint::invalidate, which did not affect the value of VeQuickItem::invalidate.
@DanielMcInnes that works well for passing the value change onto dbus/mqtt, but then DataPoint::invalidate won't reflect the value of VeQuickItem::invalidate (e.g. if it changes in the backend, then checking DataPoint::invalidate won't reflect the updated value). Ideally it would work as a two-way binding, like an alias
property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, silly me, I see. Yes, it's the VeQuickItem::invalidate which needs to be set, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, using Bea's suggestion of "sourceObject.onInvalidateChanged.connect(function() { root.invalidate = sourceObject.invalidate })"
//% "Stop generator when AC-input is available" | ||
text: qsTrId("page_generator_conditions_stop_generator_when_ac_input_available") | ||
currentIndex: stopOnAc1Item.value === 1 | ||
? 1 : stopOnAc2Item.value === 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs updateOnClick: false
, otherwise this currentIndex
binding will stop working after an option is clicked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks Bea, done
text: qsTrId("page_settings_generator_warm_up_time") | ||
visible: capabilities.value & warmupCapability | ||
dataSource: settingsBindPrefix + "/WarmUpTime" | ||
to: 60 * 60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If to
is unspecified, ListSpinBox will automatically use the max
value for the DataPoint.
Seeing as gui-v1 doesn't hardcode a to
value for this, we can leave it out here and just use the max from the DataPoint.
Same for "Cool-down time" below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Bea, done
ListRadioButtonGroup { | ||
//% "Peak shaving" | ||
text: qsTrId("settings_ess_peak_shaving") | ||
currentIndex: batteryLifeState.dataValue === VenusOS.Ess_BatteryLifeState_KeepCharged ? 1 : peakshaveItem.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also needs updateOnClick: false
to avoid overwriting the currentIndex
binding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks Bea, done
components/datapoints/DataPoint.qml
Outdated
@@ -27,6 +27,7 @@ QtObject { | |||
property bool hasMax | |||
property var min: hasMin && sourceObject ? sourceObject.min : undefined | |||
property var max: hasMax && sourceObject ? sourceObject.max : undefined | |||
property var invalidate: sourceObject !== undefined ? sourceObject.invalidate : undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This DataPoint API needs a bit of a clean-up: the sourceType
, value
, min
, max
and invalidate
properties should all be read-only, to make it clear that changing them has no effect in the backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated as suggested, except for 'invalidate', which is now changeable from qml. When I make the changes to 'value' / 'setValue', I will remove the 'readonly' modifier for 'value'.
539e441
to
56e70a4
Compare
- Add Warm-up and Cool-down states - RunTime is shown while warming-up and cooling down too - Add warm-up and Cool-down times, but only if generator is on AC-1. - Add feature to stop Generator when AC2 becomes available - This replaces the old AC1 switch, with a dropdown selector that ensures it can only be active on one of the inputs
As reported here, victronenergy/venus#1002, there are users with really long passwords. This changes the gui to accept such a long password.
This avoids having to use an additional setting to store the InverterIds.
c947a33
to
abb83cf
Compare
sourceObject = comp.createObject(root, { "source": root.source }) | ||
sourceObject = comp.createObject(root, | ||
{ | ||
"source": root.source, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"source":
but invalidate:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we use both formats throughout the code, would be nice to run some lint tool over the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
if (!sourceObject) { | ||
console.warn("Failed to create object from DataPointDBusImpl.qml", _dbusImpl.errorString()) | ||
return | ||
} | ||
sourceObject.onInvalidateChanged.connect(function() { root.invalidate = sourceObject.invalidate }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this cause an immediate complaint about a binding loop if something external ever sets root.invalidate to a different value than it is initialized with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it will loop once, not sure if it's enough to cause the warning. I think DataPoint needs a bit of refactoring as the external source URL loading is no longer necessary -- previously we thought that VeQuickItem couldn't be used directly, but that was when it was part of velib
, not veutil
. I'll look at cleaning this up and maybe in the process these loading functions will go away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem to. I made a debug build, where if you call VeQuickItem::setInvalidate(false), it starts a 1s single shot timer to restore invalidate to true. On the qml side, I have a 3s timer setting dataInvalidate to false. It behaves sensibly, and does what I want. No complaints about binding loops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Ok, LGTM.
2635915
to
958685a
Compare
When a product disappears all its values used to be invalidated, including the product information (so it is also impossible to see which product got disconnected). Since it is now possible to show the last valid values, switch to that for the product information.
Once the battery is discharged to the active/minimum SOC limit, this option allows the user to choose whether discharge should be completely disabled, or if it should remain enabled only to observe the AC input current limit of the Multi/Quattro. This option is meant for systems that should always do peak shaving. Since peak-shaving is always done when the ESS mode is KeepBatteriesCharged, we also show a read-only "Always" when in this mode.
Add a new language: Polish.
Improve the ESS Scheduled menu to more clearly indicate when a scheduled operation is active, what SOC is being targeted, and to provide an option to self-consume battery energy above the limit. The use case for this is when a schedule with a lower SOC limit follows one with a higher limit. The user may want to self-consume down to the lower level. victronenergy/venus#970
The sourceType, value, min, and max properties should all be read-only, to make it clear that changing them has no effect in the backend.
958685a
to
b6b28e1
Compare
No description provided.