Performance: refactor unit of measurement storage and lookup #2753

ybott-qinetic · 2026-01-07T08:37:56Z

blammit · 2026-01-07T10:38:35Z

src/units.cpp

+
+static UnitMetadata UnitTable[] {
+	// unit                                                                                      label    defaultPrec scalable  veUnit
+	{ Victron::VenusOS::Enums::Units_None,                                                       "",      1,          false,    Unit::Default },


I'd use 0 as the default precision for Units_None to avoid confusion. Even if it's a behavioural change it is never used with a precision anyway.

Also, this table could be sorted in alphabetical order (aside from putting Units_None first), to make it easier to read and find values. And for consistency, could do the same for the Units_Type enum in enums.h - this should be fine since there are no dependencies on particular numerical values; the user-preferred units are saved as strings or veutil equivalent values.

I agree, but whatever the order is in this data structure, the order in the enums.h should be the same. If the table is sorted differently to the enum values, then we can't use the enum value as the index into the array. We could change the datastructure to be a map instead of an array, but honestly for perf reasons I think an array with index lookup is preferable. (We can discuss using std::vector instead of a plain array, shrug.)

src/units.cpp

blammit · 2026-01-07T11:12:44Z

src/units.cpp

+	{ Victron::VenusOS::Enums::Units_Speed_MetresPerSecond,                                   "m/s",      0,          false,    Unit::MetresPerSecond },
+	{ Victron::VenusOS::Enums::Units_Speed_KilometresPerHour,                                 "k/h",      0,          false,    Unit::KilometresPerHour },
+	{ Victron::VenusOS::Enums::Units_Speed_MilesPerHour,                                      "m/h",      0,          false,    Unit::MilesPerHour },
+	{ Victron::VenusOS::Enums::Units_Speed_Knots,                                               "k",      0,          false,    Unit::Knots },


"k" -> "kn"

blammit · 2026-01-07T11:20:15Z

src/units.cpp

+	{ Victron::VenusOS::Enums::Units_Hertz,                                                    "Hz",      1,          true,     Unit::Default },
+	{ Victron::VenusOS::Enums::Units_Energy_KiloWattHour,                                     "kWh",      3,          true,     Unit::Default },
+	{ Victron::VenusOS::Enums::Units_AmpHour,                                                    "",      1,          true,     Unit::Default },
+	{ Victron::VenusOS::Enums::Units_WattsPerSquareMetre,                                    "W/m2",      0,          false,    Unit::Default },


Can you check the scalable values? In main, isScalingSupported() returns true for these:

Units_WattsPerSquareMetre

Units_RevolutionsPerMinute

Units_Speed_MetresPerSecond

Units_Speed_KilometresPerHour

Units_Speed_MilesPerHour

Units_Speed_Knots

Units_Volume_Litre

Units_Volume_GallonUS

Units_Volume_GallonImperial

But in this table, they are false.

I see that you added scaling support for Units_Altitude_Metre, which makes sense, but maybe do that in an extra commit on top to be clear.

I make a logic judgement on if all the above actually make sense being able to scale in the way the software provides. I have changed it so it replicates behavior.

I see, I didn't realise those changes were all deliberate. In that case, can you mention the changes in the commit so that it's clear that the commit doesn't only make refactoring changes, but also makes changes in the scaling behaviour?

Or alternatively, combine those changes with the Units_Altitude_Metre commit.

@ybott-qinetic ^ I don't see those changes in the latest commit - are you still planning to add them?

src/units.cpp

blammit · 2026-01-07T11:35:09Z

src/units.cpp

-		return QStringLiteral("W/m2");
-	case VenusOS::Enums::Units_Percentage:
-		return QStringLiteral("%");
-	case VenusOS::Enums::Units_Temperature_Celsius:


You still need to check the formatHint for these temperature cases. Otherwise with this PR the "Environment" tab shows the C/F unnecessarily, below the gauges:

I'm seeing some compass directions here now - do you see that in your build?

Also the Boat page has some extra precision now:

@blammit Is that a build artifact issue? Should I look at fixing how gui_v2 builds?

I did a clean build and I'm still seeing these issues. Hmm - wonder what is going on.

I modified the code manually and reverted it, and now everything is fine, so seems like there was some caching issue there.

chriadam · 2026-01-08T05:04:22Z

src/units.cpp

 namespace {

+// \u33A5 is not supported by the font, so use two characters \u006D\u00B3 instead.
+static const QString CubicMetre = QStringLiteral("m³");


I would prefer the unicode points to be spelled out, e.g. "\u0060\u00B3" in code, rather than the comment. That way the comment can be trimmed to just // \u33A5 is not supported by the font and the code itself becomes self-documenting.

chriadam · 2026-01-08T05:05:49Z

src/units.cpp

+	std::string label;
+	int precision;
+	bool scalable;
+	Unit::Type veUnit;


super nitpicky, and maybe a bad suggestion if it degrades readability, but it might be worth considering changing the order pf the struct a bit differently, with std::string first, then the two enums, then the int, then the bool, just for data packing reasons.

chriadam · 2026-01-08T05:10:37Z

src/units.cpp

+
+struct UnitMetadata {
+	Victron::VenusOS::Enums::Units_Type unit;
+	std::string label;


why not store as a QString OOI? When we return it, I think we always convert to QString, and it's better to pay the cost once (when constructing the table) rather than on every read, IMO.

chriadam · 2026-01-13T23:47:21Z

src/units.cpp

+	bool scalable;
+};
+
+static UnitMetadata UnitTable[] {


static const, maybe?
also, could be a static const std::vector { ... };

chriadam

One nitpick, aside from that, LGTM.
But I don't think this should be merged yet, as it's not for v1.2.x series.

Contributes to #2708

ybott-qinetic requested review from DanielMcInnes, blammit and chriadam January 7, 2026 08:37

blammit reviewed Jan 7, 2026

View reviewed changes

blammit mentioned this pull request Jan 7, 2026

Add Electric Vehicle device support to device list #2754

Merged

chriadam reviewed Jan 8, 2026

View reviewed changes

ybott-qinetic force-pushed the ttomkins/unit-refactor branch 5 times, most recently from 79ef992 to 61c336e Compare January 12, 2026 07:34

ybott-qinetic requested review from blammit and chriadam January 12, 2026 07:51

ybott-qinetic force-pushed the ttomkins/unit-refactor branch from 61c336e to 7f96c87 Compare January 13, 2026 21:28

chriadam reviewed Jan 13, 2026

View reviewed changes

chriadam approved these changes Jan 13, 2026

View reviewed changes

ybott-qinetic force-pushed the ttomkins/unit-refactor branch from 7f96c87 to 68e8b8b Compare January 14, 2026 04:53

blammit approved these changes Jan 19, 2026

View reviewed changes

ybott-qinetic modified the milestone: gui-v2 v1.3.0 / Venus OS v3.80 Jan 22, 2026

ybott-qinetic added 3 commits January 22, 2026 17:00

API Cleanup: reorder units enums

6f038d7

Contributes to #2708

Performance: refactor unit of measurement storage and lookup

077c81f

Contributes to #2708

API Cleanup: change scaling on some units

c7f8db0

ybott-qinetic force-pushed the ttomkins/unit-refactor branch from 68e8b8b to c7f8db0 Compare January 22, 2026 07:06

Performance: refactor unit of measurement storage and lookup #2753

Are you sure you want to change the base?

Performance: refactor unit of measurement storage and lookup #2753

Conversation

ybott-qinetic commented Jan 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ybott-qinetic Jan 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blammit Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriadam left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ybott-qinetic Jan 11, 2026 •

edited

Loading

blammit Jan 12, 2026 •

edited

Loading