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.

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

Conversation

DanielMcInnes
Copy link
Contributor

… empty / full

Fixes #1836

@DanielMcInnes DanielMcInnes linked an issue Jan 16, 2025 that may be closed by this pull request
@@ -61,7 +61,7 @@ Page {
allowed: dataItem.seen && (!standard.dataItem.isValid || standard.currentValue === 2)
dataItem.uid: root.bindPrefix + "/RawValueEmpty"
suffix: rawUnit.value || ""
decimals: 3
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

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.

etc.

Copy link
Contributor Author

@DanielMcInnes DanielMcInnes Jan 17, 2025

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

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"

@DanielMcInnes DanielMcInnes force-pushed the 1836-redundant-decimal-places-shown-in-tank-sensor-settings branch from ea3b094 to 9240c8b Compare January 17, 2025 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redundant decimal places shown in tank sensor settings
4 participants