Sometimes cell voltage is wrong on Daly

[SamuelBrucksch]

Follow up to #160

Sometimes wrong values come in. Here is such a sample:

@4000000062e4d4e2223bb9ac INFO:SerialBattery:cell 13 has invalid value: -23.295
@4000000062e4d4e2224cd0ac INFO:SerialBattery:Frame: 0xa5, 0x1, 0x95, 0x8, 0x1, 0xd, 0x29, 0xd, 0x33, 0xd, 0x32, 0x90, 0x89, 0xa5, 0x1, 0x95, 0x8, 0x2, 0xd, 0x2f, 0xd, 0x2e, 0xd, 0x33, 0x90, 0x8c, 0xa5, 0x1, 0x95, 0x8, 0x3, 0xd, 0x33, 0xd, 0x33, 0xd, 0x33, 0x90, 0x96, 0xa5, 0x1, 0x95, 0x8, 0x4, 0xd, 0x33, 0xd, 0x32, 0xd, 0x2f, 0x90, 0x92, 0xa5, 0x1, 0x95, 0x8, 0x5, 0xd, 0x30, 0xd, 0x34, 0xa5, 0x1, 0x95, 0x8, 0x6, 0xd, 0x32, 0xd, 0x34, 0xd, 0x31, 0x90, 0x97, 0xa5, 0x1, 0x90, 0x8, 0x2, 0x1c, 0x0, 0x0, 0x76, 0x56, 0x2, 0x86, 0xb0

Will check this. A first fix is to properly check for the low voltage value here:
#166

Will investigate later if i can improve the byte parsing, maybe there was an error in the order that can be detected earlier.

[SamuelBrucksch]

Data that came in:

0xa5, 0x1, 0x95, 0x8, 0x1, 0xd, 0x29, 0xd, 0x33, 0xd, 0x32, 0x90, 0x89
0xa5, 0x1, 0x95, 0x8, 0x2, 0xd, 0x2f, 0xd, 0x2e, 0xd, 0x33, 0x90, 0x8c
0xa5, 0x1, 0x95, 0x8, 0x3, 0xd, 0x33, 0xd, 0x33, 0xd, 0x33, 0x90, 0x96
0xa5, 0x1, 0x95, 0x8, 0x4, 0xd, 0x33, 0xd, 0x32, 0xd, 0x2f, 0x90, 0x92
0xa5, 0x1, 0x95, 0x8, 0x5, 0xd, 0x30, 0xd, 0x34
0xa5, 0x1, 0x95, 0x8, 0x6, 0xd, 0x32, 0xd, 0x34, 0xd, 0x31, 0x90, 0x97
0xa5, 0x1, 0x90, 0x8, 0x2, 0x1c, 0x0, 0x0, 0x76, 0x56, 0x2, 0x86, 0xb0

@Louisvdw any idea, why the 0x90 command already comes in, although the cell voltage command is not finished yet? Could it be there is some kind of race condition that already fires the next command and we still have to wait for cell voltages to finish before?

[SamuelBrucksch]

Ok did some further investigation and it seems like the daly reports less cells on random frames a few times... Now with the fix in my PR at least the totally wrong values are filtered out, but in this case we should skip the whole frame. Is there any problem with skipping the values (e.g. by returning false)? So that would mean we still wrote them to the cells array, but we should not publish them. Does this have any side effects on min/max cells or whatever if we update the array but do not publish them?

Btw in

dbus-serialbattery/etc/dbus-serialbattery/dbushelper.py

Lines 160 to 176 in 2775113

	error_count = 0
	# Call the battery's refresh_data function
	success = self.battery.refresh_data()
	if success:
	error_count = 0
	self.battery.online = True
	else:
	error_count += 1
	# If the battery is offline for more than 10 polls (polled every second for most batteries)
	if error_count >= 10:
	self.battery.online = False
	# This is to mannage CCL\DCL
	self.battery.manage_charge_current()
	# publish all the data fro the battery object to dbus
	self.publish_dbus()

You check if we return false and add it to the error counter, however even if false is returned, it is still published. Shouldn't we avoid the publishing if false is returned @Louisvdw?

[Louisvdw]

Hi @SamuelBrucksch

The Daly BMS comms protocol is really very bad. The way that they created their protocol and the speed with which the BMS can talk (9600) mean that there is very little time to get all the data from the BMS as there are only a limited amount of bytes that can be transfered in 1 sec. It is for that reason that the Daly driver has the poll step cycles and only read some datasets on every few cycles.

So to answer your question if we can skip cell values if there is a problem I would suggest that we do. Any wrong cell values do have an impact on the min/max cell voltage that we publish. So wrong values are a bit of a issue, but skipped values just give delayed results (and responses). Perhaps keeping the cell values in a temporary variable and only update the battery values when all the checks are good is a good option? Other values still need to be updated (like SOC, current and voltage) each cycle so a complete skip on updates on a false might not be the best option.

We might need to be a bit more smarter with the cycles and split the cell values into two cycle numbers if the amount of cells are too much for the poll time.

[SamuelBrucksch]

How does wrong cells voltage affect the min/max values? those are requested seperately:

dbus-serialbattery/etc/dbus-serialbattery/daly.py

Lines 258 to 272 in 2775113

	def read_cell_voltage_range_data(self, ser):
	minmax_data = self.read_serial_data_daly(ser, self.command_minmax_cell_volts)
	# check if connection success
	if minmax_data is False:
	logger.warning("read_cell_voltage_range_data")
	return False
	cell_max_voltage,self.cell_max_no,cell_min_voltage, self.cell_min_no = unpack_from('>hbhb', minmax_data)
	# Daly cells numbers are 1 based and not 0 based
	self.cell_min_no -= 1
	self.cell_max_no -= 1
	# Voltage is returned in mV
	self.cell_max_voltage = cell_max_voltage / 1000
	self.cell_min_voltage = cell_min_voltage / 1000
	return True

So even if the cells voltages are wrong, if Daly reports the min/max correctly it should be fine, or not?

[Louisvdw]

Originally for the Daly we only read the Min/Max value and not the cell values.
The battery class will check if there are no cells, then it will use the min/max values set, but if there are cells then the min/max is checked from the cell values. Most BMS works with the cell values. What I was thinking is to change the logic there that if we do have the min/max values set to rather use that instead of using the cell values. That would solve any cell issues affecting the min/max, but I will just have to review that the logic will work for the the other BMS types as well.

[SamuelBrucksch]

For now i disabled battery cells voltage again. After a few hours suddenly no cell voltages were read anymore, but all the other values still worked. Very weird. Will try to monitor min/max only now instead of all the single cell voltages, as that are the most important values anyways. Thanks for your feedback. Will disable voltage cell reading as well in my PR, but leave the rest of the logic as it is more correct as before.

[Louisvdw]

I made some Daly bms changes.

• The cell voltages is now enabled again, but the min/max cell voltage read from the BMS will be used and not calculated. Could someone check that this works. So now even if cell voltages might have issues it will still have the min/max as well.
• Then I also played with the update cycle. The min/max voltage are always read and then the alarms and temp data is read on the one cycle and the cell data on the other, so there are only 2 cycles now. It's still 1sec interval so it might get errors - then I need to adjust is more, or make the interval longer.

https://github.com/Louisvdw/dbus-serialbattery/releases/tag/v0.12b2a

[pos-ei-don]

I testet it.
Cell Voltage seems quiet right and stable.
What i see is some drops of the Current to 0, which make it difficult vor me to write some nodered actions.
Even when i pull 50A out of the BMS, Venusos alternates from 0 over 40 to 50 and back to 0.
The Cellphone App does not show this behavior.

[mr-manuel]

Closed in favor of #397.