Skip to content
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.

Sign up for GitHub

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

Change the number of decimals used to display tank sensor values when… #1851

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+25 −4
Open

Change the number of decimals used to display tank sensor values when… #1851

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions pages/settings/devicelist/tank/PageTankSetup.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ Page {
allowed: dataItem.seen && (!standard.dataItem.isValid || standard.currentValue === 2)
dataItem.uid: root.bindPrefix + "/RawValueEmpty"
suffix: rawUnit.value || ""
decimals: 3
stepSize: 0.005
decimals: rawUnit.decimals
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems odd to have decimals=1 but stepSize=0.005 in the cm case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes step size for the cm case to 0.1

stepSize: rawUnit.stepSize
}

ListSpinBox {
Expand All @@ -71,8 +71,8 @@ Page {
allowed: dataItem.seen && (!standard.dataItem.isValid || standard.currentValue === 2)
dataItem.uid: root.bindPrefix + "/RawValueFull"
suffix: rawUnit.value || ""
decimals: 3
stepSize: 0.005
decimals: rawUnit.decimals
stepSize: rawUnit.stepSize
}

ListRadioButtonGroup {
Expand Down Expand Up @@ -160,6 +160,27 @@ Page {

VeQuickItem {
id: rawUnit

// The possible values here are not well defined. The doco says: "can be V, and probably also mA and R or O."
// At least one installation uses "cm".
property int decimals: {
switch (value) {
case "cm":
return 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to suggest adding cm as a unit to units.h and providing some default precision for it, but I suppose the 3 decimals for the volts here is already non-standard, so ... maybe it's okay like this even if it's not ideal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So long as this is an isolated case this might be ok.
However a more consistent approach would be nice if it were possible to define the types of units available for display - and define a) a default decimals for that unit and b) override the chosen unit's decimal on a per-instance basis. I had to deal with this exact problem in a former life so I know how hard b) in particular is to solve.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm misunderstanding something, but we do support that already, don't we? e.g.

gui-v2/pages/settings/devicelist/PageMeteo.qml

Line 50 in 6040259

precision: 1
etc.

Copy link
Contributor Author

@DanielMcInnes DanielMcInnes Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked about this on slack, we'll wait and see what comes back.
https://victrondevelopment.slack.com/archives/C020QG3JVEW/p1737080996093759

Copy link
Contributor

@blammit blammit Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MikeTrahearn-Qinetic we do have a list of units available for display, and they have default precisions, as per units.h/cpp, but this case it's more difficult to determine as this is a "raw unit string" rather than a fixed unit type, so in theory the unit could be anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MV says 1 decimal is fine for cm, and then leave the rest at 3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This page doesn't use default precision, because it's an odd case where we want higher precision for tuning tank sensors. MV says: "You can change it to be 1 decimal for centimeters. And then we leave this. Its not important: sensor value is a debug value. The value that matters is the tank level in liters or m3"

default:
return 3
}
}

property real stepSize: {
switch (value) {
case "cm":
return 0.1
default:
return 0.005
}
}

uid: root.bindPrefix + "/RawUnit"
}
}
Loading