Skip to content

Commit a568793

Browse files
committed
This should fix a dangeerous situation when paths
/Debug/BatteryOperationalLimits/SolarVoltageOffset /Debug/BatteryOperationalLimits/VebusVoltageOffset /Debug/BatteryOperationalLimits/CurrentOffset were not handled correctly and as a writable paths allowed to enter any value. These values are used in _adjust_battery_operational_limits and _update_solarchargers_and_vecan Setting a large positive voltage offset could cause overcharging of batteries, potentially leading to thermal runaway, fire, or explosion in lithium battery systems. Setting a large negative current offset could prevent charging entirely. These debug paths override BMS safety limits which exist specifically to prevent dangerous conditions. Implemented sort of a limit, applied to all three values and based on tests I found. Unfortunately there's no deployment code to exclude these paths from production releases.
1 parent a250f7a commit a568793

File tree

2 files changed

+72
-5
lines changed

2 files changed

+72
-5
lines changed

delegates/dvcc.py

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1043,9 +1043,28 @@ def _property(path, self):
10431043
except ValueError:
10441044
return None
10451045

1046-
solarvoltageoffset = property(partial(_property, '/Debug/BatteryOperationalLimits/SolarVoltageOffset'))
1047-
invertervoltageoffset = property(partial(_property, '/Debug/BatteryOperationalLimits/VebusVoltageOffset'))
1048-
currentoffset = property(partial(_property, '/Debug/BatteryOperationalLimits/CurrentOffset'))
1046+
# these +- limitations should work with intention bounds set in tests/hub_test.py:876-915
1047+
MAX_VOLTAGE_OFFSET = 0.5 # Volts
1048+
MAX_CURRENT_OFFSET = 5.0 # Amps
1049+
1050+
# just a helper
1051+
@staticmethod
1052+
def _clamp(value, limit):
1053+
if value is None:
1054+
return None
1055+
return min(max(value, -limit), limit)
1056+
1057+
@property
1058+
def solarvoltageoffset(self):
1059+
return self._clamp(Dvcc._property('/Debug/BatteryOperationalLimits/SolarVoltageOffset', self), self.MAX_VOLTAGE_OFFSET)
1060+
1061+
@property
1062+
def invertervoltageoffset(self):
1063+
return self._clamp(Dvcc._property('/Debug/BatteryOperationalLimits/VebusVoltageOffset', self), self.MAX_VOLTAGE_OFFSET)
1064+
1065+
@property
1066+
def currentoffset(self):
1067+
return self._clamp(Dvcc._property('/Debug/BatteryOperationalLimits/CurrentOffset', self), self.MAX_CURRENT_OFFSET)
10491068

10501069
@property
10511070
def internal_maxchargepower(self):

tests/hub_test.py

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -924,6 +924,55 @@ def test_debug_chargeoffsets(self):
924924
'/Dc/0/MaxChargeCurrent': 999
925925
}})
926926

927+
def test_debug_chargeoffsets_clamped(self):
928+
self._update_values()
929+
self._monitor.add_value('com.victronenergy.vebus.ttyO1', '/Hub/ChargeVoltage', 12.6)
930+
self._monitor.set_value('com.victronenergy.vebus.ttyO1', '/State', 2)
931+
self._add_device('com.victronenergy.solarcharger.ttyO1', {
932+
'/State': 0,
933+
'/Link/NetworkMode': 0,
934+
'/Link/ChargeVoltage': None,
935+
'/Link/VoltageSense': None,
936+
'/Dc/0/Voltage': 12.4,
937+
'/Dc/0/Current': 9.7,
938+
'/FirmwareVersion': 0x129},
939+
connection='VE.Direct')
940+
self._add_device('com.victronenergy.battery.ttyO2', product_name='battery',
941+
values={
942+
'/Dc/0/Voltage': 12.5,
943+
'/Dc/0/Current': 5.3,
944+
'/Dc/0/Power': 65,
945+
'/Soc': 15.3,
946+
'/DeviceInstance': 2,
947+
'/Info/BatteryLowVoltage': 10,
948+
'/Info/MaxChargeCurrent': 25,
949+
'/Info/MaxChargeVoltage': 12.6,
950+
'/Info/MaxDischargeCurrent': 50})
951+
952+
# Set extreme offsets that exceed safe bounds
953+
self._service.set_value('/Debug/BatteryOperationalLimits/SolarVoltageOffset', 10.0)
954+
self._service.set_value('/Debug/BatteryOperationalLimits/VebusVoltageOffset', 5.0)
955+
self._service.set_value('/Debug/BatteryOperationalLimits/CurrentOffset', 100)
956+
self._update_values(3000)
957+
958+
# Voltage offset should be clamped to 0.5V, not 10V
959+
# BMS MaxChargeVoltage=12.6, solar offset clamped to 0.5 => 13.1
960+
self._check_external_values({
961+
'com.victronenergy.solarcharger.ttyO1': {
962+
'/Link/ChargeVoltage': 13.1
963+
}})
964+
965+
# Vebus offset clamped to 0.5V => 12.6 + 0.5 = 13.1
966+
self._check_external_values({
967+
'com.victronenergy.vebus.ttyO1': {
968+
'/BatteryOperationalLimits/MaxChargeVoltage': 13.1
969+
}})
970+
# Current offset clamped to 5A => 15 + 5 = 20 (not 115)
971+
self._check_external_values({
972+
'com.victronenergy.vebus.ttyO1': {
973+
'/BatteryOperationalLimits/MaxChargeCurrent': 20
974+
}})
975+
927976
def test_hub1_legacy_voltage_control(self):
928977
# BOL support is off initialy
929978
self._set_setting('/Settings/Services/Bol', 0)
@@ -1258,7 +1307,6 @@ def test_bms_selection(self):
12581307
# Check that this is also used as the Bms Service
12591308
self._check_values({'/ActiveBmsService': 'com.victronenergy.battery.ttyO2'})
12601309

1261-
12621310
def test_bms_selection_lowest_deviceinstance(self):
12631311
""" Test that if there is more than one BMS in the system,
12641312
the lowest device instance """
@@ -1307,7 +1355,7 @@ def test_bms_selection_no_bms(self):
13071355
'/DeviceInstance': 0})
13081356
self.assertEqual(Dvcc.instance.bms, None)
13091357
self._check_values({'/ActiveBmsService': None})
1310-
1358+
13111359
def test_bms_explicitly_selected(self):
13121360
# Default battery selection will pick the lowest DeviceInstance
13131361
self._set_setting('/Settings/SystemSetup/BatteryService', 'default')

0 commit comments

Comments
 (0)