LibreSolar / bms-firmware

Firmware for LibreSolar BMS boards based on bq769x0, bq769x2 or ISL94202
https://libre.solar/bms-firmware/
Apache License 2.0
146 stars 68 forks source link

Cells_V in CAN publication message #21

Closed xsider closed 7 months ago

xsider commented 3 years ago

https://github.com/LibreSolar/bms-firmware/blob/be7f58af9ec26121123eeffc57ef76564c7741cf/src/data_nodes.cpp#L211-L212

What would be the better solution to publish the individual cell voltages over CAN

martinjaeger commented 3 years ago

This is a very good question.

Don't really like to generate a dedicated DataObject for each cell voltage, as the objects are created at compile time and a BMS should be able to detect the actual number of cells at runtime. So the array is much cleaner in my opinion.

However, 8 bytes for classic CAN is definitely too small for up to 15 voltages, no matter how good the compression of the data is...

A previous version of the ThingSet spec had something called TinyTP for splitting longer publication messages. But it was never actually implemented, so I abandoned it to make the protocol not too complex.

At the moment the only good option to retrieve the cell voltages via CAN would be polling. With request/response messaging the maximum message size is 4095 bytes, which should be plenty.

xsider commented 3 years ago

I will consider polling over ISO-TP also. At the moment i am not mastering ISO-TP at all. But that could be a solution. The goal for the next year is, to replace an existing logging facility from volkszaehler-OBIS-messages (over isolated UART) to CAN, because one CAN could serve all chargers/bms at once. To use publication messages would be more straight forward because the host computer needs to listen only, and there could be multiple listeners for redundancy. Maybe thats a subject for [(https://learn.libre.solar/system/)], because it adresses not the component but the whole system.

martinjaeger commented 3 years ago

Yeah, makes sense to use CAN for that. If you're familiar with the ESP32: Quite a bit of that functionality is already implemented in https://github.com/LibreSolar/esp32-edge-firmware, including ISO-TP implementation and a web interface running on the ESP32 itself. It even supports over-the-air firmware upgrades for some STM32 microcontrollers.

I'm currently using it to push data to an MQTT broker, store it in an InfluxDB or VictoriaMetrics database and visualize it with Grafana.

It will need a bit more documentation, though.

xsider commented 3 years ago

This is a proposal for binary data node Cells_V, which does not break current implementation, CBOR-conform and fits in 8 Byte payload of CAN:

martinjaeger commented 3 years ago

Mhh, I think this would kind of break the current implementation, as the data representation would be different depending on whether the ThingSet library is fed with a request in text mode or in binary mode. Or the data would have to be generated outside the ThingSet library and just for this particular CAN message, which is also not ideal.

I think such a problem has to be solved on a lower layer of the protocol stack, i.e. the transport layer. As ISO-TP is not suitable for broadcasting, one solution would be to get back above mentioned TinyTP and create a dedicated CAN ID for multi-frame publications.

Previously the DP and EDP bits (25 and 24) were chosen to avoid ID conflicts with SAE-J1939. However, the ThingSet bitrate is set to 500 kbit/s instead of 250 kbit/s, so both protocols can't run on the same bus anyway. So we might just want to use bit 25 set to 0 for multi-frame publication messages and use bits 8-23 of the ID for packet numbers etc..

What do you think?

xsider commented 3 years ago

So we might just want to use bit 25 set to 0 for multi-frame publication messages and use bits 8-23 of the ID for packet numbers etc..

If we use Bit 8-23 for packet number, we need to set the NodeId inside the payload? So we will need a CBOR-array which consists of: [ NodeID, data_value0, data_value1, data value2, ....]. Number of data_values is (CBOR_array_length -1)?

pasrom commented 10 months ago

Are there any current developments underway?

martinjaeger commented 10 months ago

Yes, there are :)

We have recently added a new packetized report format to the spec and to the implementation. See the pull requests below for further information:

https://github.com/ThingSet/thingset.github.io/pull/24 https://github.com/ThingSet/thingset-zephyr-sdk/pull/4

It's just a matter of updating to a new ThingSet SDK revision in west.yml to pull that into the BMS code.

pasrom commented 7 months ago

@martinjaeger, https://github.com/LibreSolar/bms-firmware/pull/37 pulls your mentioned changes from thingset-zephyr-sdk, so I think this issue is resolved?

martinjaeger commented 7 months ago

Yes, true. We can close this issue.