Data length mismatch jkbms_ble #593

iLeeeZi · 2023-04-28T17:34:01Z

Describe the bug
There seems to be issue with jkbms_ble branch after fix for #397

Reverted changes for utils.py and everything is working again

Battery/BMS:

BMS/Battery type: JBD
Cells: 4
Interface: USB-RS232

Logs

2023-04-28 11:00:29.136398500 INFO:SerialBattery:dbus-serialbattery v1.0.0-jkbms_ble (20230427)
2023-04-28 11:00:29.136853500 INFO:SerialBattery:Testing LltJbd
2023-04-28 11:00:45.109948500 ERROR:SerialBattery:>>> ERROR: No reply - returning [len:32/31]
2023-04-28 11:00:45.611988500 INFO:SerialBattery:Testing LltJbd
2023-04-28 11:01:01.585117500 ERROR:SerialBattery:>>> ERROR: No reply - returning [len:32/31]
2023-04-28 11:01:02.087294500 INFO:SerialBattery:Testing LltJbd
2023-04-28 11:01:18.049461500 ERROR:SerialBattery:>>> ERROR: No reply - returning [len:32/31]
2023-04-28 11:01:18.553355500 ERROR:SerialBattery:ERROR >>> No battery connection at /dev/ttyUSB3

The text was updated successfully, but these errors were encountered:

mr-manuel · 2023-04-28T17:52:07Z

Could you try to install the latest version?

If you have the same problem, please try to swap these lines and check, if it works again:

Before
https://github.com/Louisvdw/dbus-serialbattery/blob/jkbms_ble/etc/dbus-serialbattery/utils.py#L443-L451

After

        data = bytearray(res)
        while len(data) <= length + length_check:
            res = ser.read(length + length_check)
            data.extend(res)

mr-manuel · 2023-04-28T17:57:31Z

@transistorgit FYI. Could you troubleshoot this?

With my JKBMS it's working over Bluetooth and Serial connection.

iLeeeZi · 2023-04-28T18:07:12Z

Latest version has the same issue but changing those lines fixed the problem

Edit: The title might be confusing, I have a JBD bms but the branch used is jkbms_ble

iLeeeZi · 2023-04-29T14:29:05Z

I uncommented the debug log line
https://github.com/Louisvdw/dbus-serialbattery/blob/jkbms_ble/etc/dbus-serialbattery/utils.py#L453

Results with old working code

2023-04-29 14:07:40.748629500 INFO:SerialBattery:serial data length 36
2023-04-29 14:07:41.739302500 INFO:SerialBattery:serial data length 36
2023-04-29 14:07:41.868828500 INFO:SerialBattery:serial data length 15
2023-04-29 14:07:42.745934500 INFO:SerialBattery:serial data length 36
2023-04-29 14:07:43.756215500 INFO:SerialBattery:serial data length 36
2023-04-29 14:07:43.927321500 INFO:SerialBattery:serial data length 15
2023-04-29 14:07:44.742979500 INFO:SerialBattery:serial data length 36
2023-04-29 14:07:44.870181500 INFO:SerialBattery:serial data length 15
2023-04-29 14:07:45.749538500 INFO:SerialBattery:serial data length 36
2023-04-29 14:07:45.926565500 INFO:SerialBattery:serial data length 15
2023-04-29 14:07:46.754996500 INFO:SerialBattery:serial data length 36

And with current code which does not work

2023-04-29 14:18:47.827407500 INFO:SerialBattery:serial data length 32
2023-04-29 14:18:47.933849500 INFO:SerialBattery:serial data length 32
2023-04-29 14:18:48.041761500 INFO:SerialBattery:serial data length 32
2023-04-29 14:18:48.147568500 INFO:SerialBattery:serial data length 32
2023-04-29 14:18:48.254427500 INFO:SerialBattery:serial data length 32
2023-04-29 14:18:48.360248500 INFO:SerialBattery:serial data length 32

transistorgit · 2023-04-29T18:12:07Z

Hi,
in my commit, I fixed the serial receive loop for really using the given length values. It was only tested with Daly.

I checked the JBD file and compared it with a protocol description I found here.
In line 60 of the driver, you can see that
LENGTH_CHECK = 6
but the protocol has only 2 bytes checksum. Therefore it now waits for 36 bytes but gets only 32, resulting in timeouts.

@mr-manuel how to proceed? I think the bug is in the JBD code. But it may be that also some other types are affected. (also the values in daly.py were wrong).

@iLeeeZi can you make a test with the new code and change line 60 in the driver to
LENGTH_CHECK = 2
?

I would help, but problem is, I just can test with daly...

brgds
Bernd

mr-manuel · 2023-04-29T19:00:32Z

Please insert debuggig info, if it fails because of this problem, so that if the driver fails on another BMS the user has only to post the logs and we know how to fix it.

Please open a PR in the jkbms_ble branch with the fix.

mr-manuel · 2023-04-29T21:34:34Z

Now I also have problems with my JKBMS.

2023-04-29 21:33:14.240937500 INFO:SerialBattery:Testing Jkbms
2023-04-29 21:33:30.235914500 ERROR:SerialBattery:>>> ERROR: No reply - returning [len:297/296]
2023-04-29 21:33:30.739856500 INFO:SerialBattery:Testing Jkbms
2023-04-29 21:33:46.737382500 ERROR:SerialBattery:>>> ERROR: No reply - returning [len:297/296]
2023-04-29 21:33:47.241881500 INFO:SerialBattery:Testing Jkbms
2023-04-29 21:34:03.235518500 ERROR:SerialBattery:>>> ERROR: No reply - returning [len:297/296]

Since before all BMS were working. Is it not better to fix it in a way, that only the Daly BMS is affected?

transistorgit · 2023-04-29T23:57:17Z

yes, that would be easier. The design of a generic serial read function in utils.py is complicated. The protocols are quite different. I would suggest that each driver has its own local serial read function. I can do this for daly, and other bms‘ can stay with the generic function as long as it works.

While checking the JBD protocol I saw that it has a stop byte to make parsing safer, but it can‘t be handled by the generic function.

Also, looking into the jkbms protocol spec, it says it has 4 checksum bytes. But In the driver is CHECK_LENGTH = 1. Also Jkbms counts all bytes of the frame except the 2 start bytes. Daly counts just the data frame bytes. So the read function in utils.py can‘t handle them all correctly. It is more coincidence that it works.

Take the sample telegram in jkbms.py:18 - it is 21 bytes long, length code is 0x13 = 19d. Lets say the receive function has already received the full 21 bytes. In the while statement in utils.py:373 it waits for 19+1 bytes. As it has already received 21 bytes, it suceeds by coincidence.
Therefore the extra 1 byte in your log that you send in the last message.
(line numbers refering to files from branch Louisvdw master)

If you agree, I will move the serial read function into the daly.py and revert utils.py to the original state.

mr-manuel · 2023-04-30T08:02:12Z

yes, that would be easier. The design of a generic serial read function in utils.py is complicated. The protocols are quite different. I would suggest that each driver has its own local serial read function. I can do this for daly, and other bms‘ can stay with the generic function as long as it works.

Thats a good idea. When not defined in a specific battery driver, then the serial read function of the battery.py is used. So every battery class can overwrite it, if needed.

If you agree, I will move the serial read function into the daly.py and revert utils.py to the original state.

Yes please. Update your PR (#484) and open another PR for the jkbms_ble branch.

transistorgit · 2023-05-01T07:48:56Z

yes, integrating the serial reader into battery.py would also be nice, because that would be a cleaner architecture. right now it would be quite a big change. I just did it for daly now. If the serial stuff would go into the battery class, we also need to inject the serial port from outside. otherwise the class would depend on hardware what it make it impossible to unit test in future.

transistorgit · 2023-05-01T08:11:18Z

I corrected the serial stuff in #484 but added some additional stuff in jkbms_ble. hope that is Ok, as I don't now your process how you handle changes.

mr-manuel · 2023-05-01T09:00:52Z

That's ok, thanks!

mr-manuel · 2023-05-01T14:56:15Z

@iLeeeZi could you try with the the latest nightly?

iLeeeZi · 2023-05-01T16:17:19Z

No errors with the latest nightly, seems to be working now!

mr-manuel closed this as completed May 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data length mismatch jkbms_ble #593

Data length mismatch jkbms_ble #593

iLeeeZi commented Apr 28, 2023

mr-manuel commented Apr 28, 2023 •

edited

Loading

mr-manuel commented Apr 28, 2023 •

edited

Loading

iLeeeZi commented Apr 28, 2023 •

edited

Loading

iLeeeZi commented Apr 29, 2023

transistorgit commented Apr 29, 2023

mr-manuel commented Apr 29, 2023

mr-manuel commented Apr 29, 2023 •

edited

Loading

transistorgit commented Apr 29, 2023

mr-manuel commented Apr 30, 2023

transistorgit commented May 1, 2023

transistorgit commented May 1, 2023

mr-manuel commented May 1, 2023

mr-manuel commented May 1, 2023

iLeeeZi commented May 1, 2023

Data length mismatch jkbms_ble #593

Data length mismatch jkbms_ble #593

Comments

iLeeeZi commented Apr 28, 2023

mr-manuel commented Apr 28, 2023 • edited Loading

mr-manuel commented Apr 28, 2023 • edited Loading

iLeeeZi commented Apr 28, 2023 • edited Loading

iLeeeZi commented Apr 29, 2023

transistorgit commented Apr 29, 2023

mr-manuel commented Apr 29, 2023

mr-manuel commented Apr 29, 2023 • edited Loading

transistorgit commented Apr 29, 2023

mr-manuel commented Apr 30, 2023

transistorgit commented May 1, 2023

transistorgit commented May 1, 2023

mr-manuel commented May 1, 2023

mr-manuel commented May 1, 2023

iLeeeZi commented May 1, 2023

mr-manuel commented Apr 28, 2023 •

edited

Loading

mr-manuel commented Apr 28, 2023 •

edited

Loading

iLeeeZi commented Apr 28, 2023 •

edited

Loading

mr-manuel commented Apr 29, 2023 •

edited

Loading