search

mr-manuel / venus-os_dbus-serialbattery

Battery Monitor driver for serial battery in VenusOS GX systems
https://mr-manuel.github.io/venus-os_dbus-serialbattery_docs/
MIT License
48 stars 18 forks source link

JK Inverter BMS aka PB model CAN support #108

Open Hooorny opened 3 days ago

Hooorny commented 3 days ago
mr-manuel commented 3 days ago

Nice. I was currently checking the Jkbms_can driver and saw a few question marks there.

  1. There is no real reading of all cell voltages. The cell voltages are build from the min/max cell voltages and their index. This is no good idea. I have to check, if we need the single cell data, or if it‘s enough, if we populate only two cells.
  2. Only a fraction of the send frames are read. This means that some important events can get lost. Do you think you can add a function to utils.py that monitors all CAN messages for 1 second (to check if that suits) and then elaborate only the last message for every single type from this second? That way nothing should get lost.
Hooorny commented 3 days ago

Thanks for checking.

  1. Did you check jkbms_pb_can.py? The min/max population is from the "old" implementation for the JK BMS. The new one reads all cell data by consuming CAN frames CELL_VOLT_EXT1-5
  2. I will have a look how to monitor the CAN bus
mr-manuel commented 2 days ago
  1. I was only wondering why you are using then the JKBMS_CAN_CELL_COUNT constant. If you get the cells. then please do not use this constant. I'm reworking the other driver to remove it.

For the new JKBMS_PB_CAN_DEVICE_ADDRESSES = please remove it and use the MODBUS_ADDRESSES constant. I will rename it to DEVICE_ADDRESSES later, so it can be used for both.

Do you have a protocol documentation which you can post here?

What for a protocol have you selected in the JKBMS app for the CAN port?

Hooorny commented 2 days ago
  1. I don't get the cell count directly, only by receiving CAN frames with at most 4 cell voltages in it. According to the spec up to 25 cells are supported ... after some seconds of receiving the cell count could be calculated ... so i just used the constant instead
mr-manuel commented 2 days ago
  1. This was answered by the protocol you provided :-)
  2. Why not? It only tries do find data, and if there is none it will do nothing. If there are multiple BMS on the same CAN line, then it should be more efficient, if only one subtask is listening and merging the data together for all instances.
  3. Thanks. There are a bunch of JKBMS websites out there. None of them which I know of, had that much information about the CAN protocol. It seems that also my normal JKBMS JK_B1A8S10PHC has the identical CAN frames. Also for the cell voltages.
Hooorny commented 2 days ago

I committed a thread safe implementation of a CanReceiverThread handling CAN-BUS communication ... seems working for me. Unfortunately i have a ERROR:SerialBattery:Non blocking exception occurred: KeyError('/CurrentAvg') of type <class 'KeyError'> in /opt/victronenergy/dbus-serialbattery/dbushelper.py line 989 in the logs, but cant find a dependency to my commit

hmmm, the error came from the changes in jkbms_pb_can.py ... i need further investigation whats going on here.

mr-manuel commented 2 days ago

It also works for my JKBMS, which is no PB model. Very nice!

In my opinion the CanReceiverThread should be started as soon as possible and directly in dbus-serialbattery.py after expected_bms_types. Each battery instance (which are added directly after) then should only get data from the CAN cache and search for the correct data.

In the test_connection() function we need to check as fast as possible, if the BMS driver matches the BMS and return as fast as possible false, if they are not matching. --> Done

If they match, we have more time. We need to get all the needed data to correctly start the driver. In the protocol documentation the slowest data cycles are the cell voltages. Therefore I would wait a bit more than 1 second before continuing with the start of the driver, so that we received all data at least once. This permit us to remove the JKBMS_CAN_CELL_COUNT from the config file. --> Done

I fixed the KeyError('/CurrentAvg') bug, which raised in your case, because not all values were loaded on driver startup.

I pushed the changes to your branch.

mr-manuel commented 2 days ago

This is not working as expected. Maybe you have a better idea https://github.com/mr-manuel/venus-os_dbus-serialbattery/pull/108/commits/7c21c4fa4dfb573b92b8107a544e6a0935e442ae

Hooorny commented 1 day ago

Very cool changes!

I extracted the CanReceiver class to a new file, which is only imported by CAN BMS, I think this is the cleanest solution.

In my opinion the CanReceiverThread should be started as soon as possible and directly in dbus-serialbattery.py after expected_bms_types. Each battery instance (which are added directly after) then should only get data from the CAN cache and search for the correct data.

Thats the way it is working. The first battery makes an instance which is a singleton for the "channel, bustype, bitrate" tuple, all following batteries with the same tuple will get the same instance, so they all share the same bus listener. I dont know if the port (can1 at my CerboGX) is the same for the second BMS-CAN jack

Runs very smooth so far :)

Hooorny commented 1 day ago

I just figured out, that the device address is configured by the dip switches and not in the JK BMS App. Seems to be the same as for RS485

mr-manuel commented 1 day ago

Indeed it's cleaner now. Thanks.

I still would prefer to have self.can_thread = CanReceiverThread.get_instance(bustype=self.CAN_BUS_TYPE, channel=self.port, bitrate=self.baud_rate) in the dbus-serialbattery.py before testing the different batteries. Why? Because else every battery driver test in the future will maybe wait some seconds that all frames are send, which is not needed on a battery driver base, but on a common base. Also on the same CAN bus port all devices have the same "channel, bustype, bitrate".

mr-manuel commented 1 day ago

I dont know if the port (can1 at my CerboGX) is the same for the second BMS-CAN jack

I tried to connect it to the BMS-CAN and it also works, but the JK BMS CAN Protocol (500K) V2.0 has to be selected. There would be a workaround to set it also to 250k, but it's not worth the effort.

I just figured out, that the device address is configured by the dip switches and not in the JK BMS App. Seems to be the same as for RS485

Really strange, as always from JKBMS. On my BMS I have no dip switches, so it seems to be only possible with the JKBMS PB Model. Maybe it works at some point with newer BMSes in the future, if anyone reaches JKBMS to tell them that.

Hooorny commented 1 day ago

I currently used one of the 2 BMS-CAN Ports at the CerboGX with the 500k JK Protocol, both ports are listed under "can1" using canshow. The 2 VE-CAN Ports show up as "can0" ... so I may connect 2 BMS to the same CAN-Port "can0" or "can1". Perhaps its also possible to daisy chain further BMS via RS485 and only use the master for CAN connection ... ?! Would be interesting if the other BMS will be on CAN with different frame ids