cantools / cantools

CAN bus tools.
MIT License
1.89k stars 565 forks source link

DBC Decode Message In Error #581

Closed MichaelBMiner closed 1 year ago

MichaelBMiner commented 1 year ago

Hello,

I am using DBCPPP to generate CAN data on an embedded linux device. I am sending this CAN data to a Raspberry Pi running cantools.

In my application I can use DBCPPP to encode and decode the message no problem. When cantools receives the message, the decoded data is all wrong. My device is filling and sending PGN

BO_ 2566568016 PGN_64184_EVSE_1_DC_Status_1: 8 EVCC
 SG_ EVSE1_DC_Charging_State : 0|4@1+ (1,0) [0|15] "" EV
 SG_ EVSE1_Isolation_Status : 4|4@1+ (1,0) [0|15] "" EV
 SG_ EVSE1_Present_DC_Charging_Voltage : 8|16@1+ (0.05,0) [0|3212.75] "V" EV
 SG_ EVSE1_Present_DC_Charging_Current : 24|16@1- (0.05,-1600) [-1600|1612.75] "A" EV
 SG_ EVSE1_Voltage_Limit_Achieved : 40|2@1+ (1,0) [0|3] "" EV
 SG_ EVSE1_Current_Limit_Achieved : 42|2@1+ (1,0) [0|3] "" EV
 SG_ EVSE1_Power_Limit_Achieved : 44|2@1+ (1,0) [0|3] "" EV
 SG_ EVSE1_Processing_State : 46|2@1+ (1,0) [0|3] "" EV
 SG_ EVSE1_Status : 48|4@1+ (1,0) [0|15] "" EV
 SG_ EVSE1_Response_Code : 56|8@1+ (1,0) [0|255] "" EV

My CAN frame data field contains the following 0x000000B036D08400, I can confirm the raspberry pi sees the same data. This data is to fill the above PGN with values EVSE1_Present_DC_Charging_Voltage = 700V and EVSE1_Present_DC_Charging_Current 100A.

Instead I get 'EVSE1_Present_DC_Charging_Voltage': 2054.65 'EVSE1_Present_DC_Charging_Current': -1597.3

The two devices share a DBC file so they can properly encode/decode messages. My python code is not complicated. I manipulate the DBC ID and CAN ID internally on both devices, but the tools can always match an ID in the DBC file.

data = self.dbc.decode_message(ext_can_id_with_our_sa, message.data)

andlaus commented 1 year ago

this seems to be a mess-up with the byte/bit order between the devices. since I'm pretty sure that the python part of cantools handles this correctly, how is the data encoded on the embedded device?

MichaelBMiner commented 1 year ago

To encode the data the application does the following

        static const dbcppp_Network* net;
    static uint32_t num_unique_dbc_ids;
    net = dbcppp_NetworkLoadDBCFromFile(DEFAULT_DBC_FILE_LOCATION);
    num_unique_dbc_ids = dbcppp_NetworkMessages_Size(net);
    for (uint32_t i = 0; i < num_unique_dbc_ids; i++) {
        const dbcppp_Message* msg = dbcppp_NetworkMessages_Get(net, i);
        uint64_t msg_id = dbcppp_MessageId(msg);
        if(to_send.can_id == msg_id) {
            for(uint32_t j=0; j<dbcppp_MessageSignals_Size(msg); j++) {
                const dbcppp_Signal* sig = dbcppp_MessageSignals_Get(msg, j);
                printf("Parsing SPN %s\n", dbcppp_SignalName(sig));
                uint64_t temp_raw = 0;
                uint64_t raw = 0; //dbcppp_SignalPhysToRaw(sig, dbc_id_info_array[index].spns[i].phys);
                if(strcmp("EVSE1_Present_DC_Charging_Voltage", dbcppp_SignalName(sig)) == 0) {
                    raw = dbcppp_SignalPhysToRaw(sig, voltage);
                }
                if(strcmp("EVSE1_Present_DC_Charging_Current", dbcppp_SignalName(sig)) == 0) {
                    raw = dbcppp_SignalPhysToRaw(sig, current);
                }
                dbcppp_SignalEncode(sig, raw, &temp_raw);
                uint64_t bitsize = dbcppp_SignalBitSize(sig);
                raw_value |= (temp_raw << bitsize);
            }
        }
    }

    memcpy(to_send.data, &raw_value, sizeof(raw_value));
    printf("temp_raw: %" PRIu64 "\n", raw_value);

After this the CAN frame is transmitted.

andlaus commented 1 year ago

does this produce the same data as when encoded with cantools using message.encode()? If it isn't the embedded code is most likely wrong...

juleq commented 1 year ago

How does the embedded device decode? Byte by byte or does it use wider types like u32 or u64? If the latter, endianness could catch you by surprise.

MichaelBMiner commented 1 year ago

To decode I run the code

for (uint32_t i = 0; i < num_unique_dbc_ids; i++) {
        const dbcppp_Message* msg = dbcppp_NetworkMessages_Get(net, i);
        uint64_t msg_id = dbcppp_MessageId(msg);
        if(to_send.can_id == msg_id) {
            for(uint32_t j=0; j<dbcppp_MessageSignals_Size(msg); j++) {
                const dbcppp_Signal* sig = dbcppp_MessageSignals_Get(msg, j);
                printf("Parsing SPN %s\n", dbcppp_SignalName(sig));
                shift_size += dbcppp_SignalBitSize(sig);
                uint64_t mask = (1ULL << dbcppp_SignalBitSize(sig)) - 1;
                uint64_t temp_raw = (raw_value >> shift_size) & mask;
                uint64_t phys = (uint64_t)dbcppp_SignalRawToPhys(sig, temp_raw);
                uint32_t start_bit = dbcppp_SignalStartBit(sig);
                printf("temp_raw: %" PRIu64 ", phys: %" PRIu64 ", start_bit: %u\n", temp_raw, phys, start_bit);
            }
        }
    }
juleq commented 1 year ago

What is the endianness of the machine? Which MCU? You could check if DBCPPP has endianness agnostic code.

If it is not this, not getting the IDs right would also explain.

MichaelBMiner commented 1 year ago

What is the endianness of the machine? Which MCU? You could check if DBCPPP has endianness agnostic code.

If it is not this, not getting the IDs right would also explain.

Still working on an encode example to see if it gives the same. The IDs are always correct.

MichaelBMiner commented 1 year ago

I am using a scheduler to schedule my TX messages. This is how I populate the data for this PGN.

    def send_test(battery, scheduler, time, priority):
    battery.data_dict = {}
    msg = CanHandler.getInstance().find_message_by_name("PGN_64184_EVSE_1_DC_Status_1")
    if msg is not None:
        battery.data_dict["EVSE1_DC_Charging_State"] = 0
        battery.data_dict["EVSE1_Isolation_Status"] = 0
        battery.data_dict["EVSE1_Present_DC_Charging_Voltage"] = 700
        battery.data_dict["EVSE1_Present_DC_Charging_Current"] = 100
        battery.data_dict["EVSE1_Voltage_Limit_Achieved"] = 0
        battery.data_dict["EVSE1_Current_Limit_Achieved"] = 0
        battery.data_dict["EVSE1_Power_Limit_Achieved"] = 0
        battery.data_dict["EVSE1_Processing_State"] = 0
        battery.data_dict["EVSE1_Status"] = 0
        battery.data_dict["EVSE1_Response_Code"] = 0
        CanHandler.getInstance().send_can_frame(battery, msg.frame_id)
    scheduler.enter(time, priority, send_test, (battery, scheduler, time, priority))

In my CAN handling code I have a send function

 def send_can_frame(self, battery, frame_id):
        try:
            ext_can_id_without_sa = (frame_id & REMOVE_SOURCE_ADDR_MASK)
            ext_can_id_with_our_sa = (ext_can_id_without_sa | self.sa)
            dbc_can_id = (4 << 29) | frame_id

            data = self.dbc.encode_message(ext_can_id_with_our_sa, battery.data_dict)
            message = can.Message(arbitration_id=ext_can_id_with_our_sa, data=data)
            self.can_tx.send(message)
        except Exception as err:
            print("ERROR {} {}".format(battery.data_dict, err))

This is returning the error

ERROR {'EVSE1_DC_Charging_State': 0, 'EVSE1_Isolation_Status': 0, 'EVSE1_Present_DC_Charging_Voltage': 700, 'EVSE1_Present_DC_Charging_Current': 100, 'EVSE1_Voltage_Limit_Achieved': 0, 'EVSE1_Current_Limit_Achieved': 0, 'EVSE1_Power_Limit_Achieved': 0, 'EVSE1_Processing_State': 0, 'EVSE1_Status': 0, 'EVSE1_Response_Code': 0} Signed integer value 34000 out of range.

34000 is the raw value for a current of 100A. (100 - (-1600))/0.05 = 34000.

andlaus commented 1 year ago

hm, this does not really look like code that uses cantools (at least not a somewhat contemporary version): e.g, there is no .find_message_by_name() function (it is get_message_by_name())

34000 is the raw value for a current of 100A. (100 - (-1600))/0.05 = 34000.

I'm pretty sure that you're not using cantools, since the string "out of range" does not occur in the codebase:

~/src/cantools > find cantools -iname "*.py" | xargs grep -i "out of range"
~/src/cantools >
andlaus commented 1 year ago

ok: after some git archeology, I conclude that your software based on a cantools version which is at least 5 years old: the _find_message_by_name() method was removed by 47714c4867f29f51657c7dab819941c35186d4df...

MichaelBMiner commented 1 year ago

Well this is confusing. I wrote my own function

def find_message_by_name(self, msg_name):
        return_msg = None
        for msg in self.dbc.messages:
            if msg.name == msg_name:
                return_msg = msg
                break
        return return_msg

I also create my CanHandler using data from a config file

 config = configparser.ConfigParser()
    config.read(os.path.abspath(os.path.join(os.path.dirname( __file__ ), '..', 'BMS_Simulator'))+'/BMS_Config.txt')
    print(os.path.abspath(os.path.join(os.path.dirname( __file__ ), '..', 'BMS_Simulator'))+'/BMS_Config.txt')
    can_config = CanConfig(config['CAN']['CANInterface'], config['CAN']['BusType'], config['CAN']['Bitrate'])
    print(config['DBC']['DBCPath'])
    dbc = cantools.database.load_file(os.path.abspath(os.path.join(os.path.dirname( __file__ ), '../', ''))+ config['DBC']['DBCPath'])

This is my CanHandler.py in its entirety.

import can
import threading
from time import time

REMOVE_SOURCE_ADDR_MASK = 0xFFFFFF00
EXT_CAN_ID_PGN_MASK = 0x00FFFF00
EXT_CAN_ID_PGN_BIT_SHIFT = 8

J1939_NULL_ADDRESS = 254

PRIORITY_MASK = 0x1C000000
EDP_MASK = 0x02000000
DP_MASK = 0x01000000
PF_MASK = 0x00FF0000
PS_MASK = 0x0000FF00
SA_MASK = 0x000000FF
PDU1_PGN_MASK = 0x03FF0000
PDU2_PGN_MASK = 0x03FFFF00

class CanHandler:
    __instance = None

    @staticmethod
    def getInstance():
        if CanHandler.__instance is None:
            raise Exception("Initialize with a can config and dbc file")
        return CanHandler.__instance

    def clearData(self):
        self.rx_dict.clear

    def __init__(self, can_config, dbc, sa=J1939_NULL_ADDRESS):
        if CanHandler.__instance is not None:
            raise Exception("Call get instance!")
        else:
            CanHandler.__instance = self
            print("Starting CAN Interface", can_config.interface, can_config.bustype, can_config.bitrate)
            self.can_tx = can.interface.Bus(can_config.interface, bustype=can_config.bustype, bitrate=can_config.bitrate)
            self.can_rx = can.interface.Bus(can_config.interface, bustype=can_config.bustype, bitrate=can_config.bitrate)
            self.dbc = dbc
            self.sa = sa
            self.rx_dict = {}
            threading.Thread(target=self.can_rx_thread).start()

    def send_can_frame(self, battery, frame_id):
        try:
            ext_can_id_without_sa = (frame_id & REMOVE_SOURCE_ADDR_MASK)
            ext_can_id_with_our_sa = (ext_can_id_without_sa | self.sa)
            dbc_can_id = (4 << 29) | frame_id

            data = {
                'EVSE1_DC_Charging_State': 3,
                'EVSE1_Isolation_Status': 2,
                'EVSE1_Present_DC_Charging_Voltage': 700,
                'EVSE1_Present_DC_Charging_Current': 82,
                'EVSE1_Voltage_Limit_Achieved': 1,
                'EVSE1_Current_Limit_Achieved': 0,
                'EVSE1_Power_Limit_Achieved': 2,
                'EVSE1_Processing_State': 1,
                'EVSE1_Status': 7,
                'EVSE1_Response_Code': 127
            }

            data = self.dbc.encode_message(ext_can_id_with_our_sa, data)
            message = can.Message(arbitration_id=ext_can_id_with_our_sa, data=data)
            self.can_tx.send(message)
        except Exception as err:
            print("ERROR {} {}\n".format(data, err))

    def find_message_by_name(self, msg_name):
        return_msg = None
        for msg in self.dbc.messages:
            if msg.name == msg_name:
                return_msg = msg
                break
        return return_msg

    def can_rx_thread(self):
        while True:
            message = self.can_rx.recv()
            if message is not None:
                self.decode_rx_msg(message)

    def decode_rx_msg(self, message):
        try:
            ext_can_id = message.arbitration_id
            dbc_can_id = (4 << 29) | message.arbitration_id

            priority = (PRIORITY_MASK & ext_can_id) >> 26
            edp = (EDP_MASK & ext_can_id) >> 25
            dp = (DP_MASK & ext_can_id) >> 24
            PF = (ext_can_id & PF_MASK) >> 16
            PS = (ext_can_id & PS_MASK) >> 8
            SA = (ext_can_id & SA_MASK)
            if PF >= 0xF0: #240
                DA = 255
                PGN = (ext_can_id & PDU2_PGN_MASK) >> 8
            else:
                DA = PS
                PGN = (ext_can_id & PDU1_PGN_MASK) >> 8

            if SA != self.sa:
                data = self.dbc.decode_message(ext_can_id, message.data)
            else:
                ext_can_id_without_sa = (ext_can_id & REMOVE_SOURCE_ADDR_MASK)
                ext_can_id_with_our_sa = (ext_can_id_without_sa | self.sa)
                data = self.dbc.decode_message(ext_can_id_with_our_sa, message.data)
            if(PGN == 64184):
                hex_data = ' '.join([hex(byte) for byte in message.data])
                print(hex_data)
            self.rx_dict[PGN] = data
        except Exception as e:
            # print(message.data)
            print("dbc_can_id {9} ext_can_id {0} priority {1}, edp {2} dp {3} PF {4} PS {5} SA {6} DA {7} PGN {8} e {10}".format(ext_can_id, priority, edp, dp, PF, PS, SA, DA, PGN, dbc_can_id, e))
            pass
MichaelBMiner commented 1 year ago

To clarify further in main I create an object called dbc

dbc = cantools.database.load_file(os.path.abspath(os.path.join(os.path.dirname( __file__ ), '../', ''))+ config['DBC']['DBCPath'])

This is passed into another object along with a Battery simulator, and source address.

Bus(dict(config.items('Battery')), can_config, dbc, sa)

The Bus object contains a CanHandler object which accepts the dbc object

self.can_handler = CanHandler(can_config, dbc, sa)

The CanHandler code shared above is using the dbc object created from cantools.

andlaus commented 1 year ago

I suppose I cannot really help you with this. To get the binary data which cantools would send, you can use

import cantools

db = cantools.database.load_file("my_dbc_file.dbc")
signal_dict ={ 
    "EVSE1_DC_Charging_State": 0,
    "EVSE1_Isolation_Status": 0,
    "EVSE1_Present_DC_Charging_Voltage": 700,
    "EVSE1_Present_DC_Charging_Current": 100,
    "EVSE1_Voltage_Limit_Achieved": 0,
    "EVSE1_Current_Limit_Achieved": 0,
    "EVSE1_Power_Limit_Achieved": 0,
    "EVSE1_Processing_State": 0,
    "EVSE1_Status": 0,
    "EVSE1_Response_Code": 0,
}

msg = db.get_message_by_name("PGN_64184_EVSE_1_DC_Status_1")

print(msg.encode(signal_dict).hex())

(modulo minor mistakes, I haven't run the code above)

MichaelBMiner commented 1 year ago

I have taken your code directly.


import cantools

try:
    dbc = cantools.database.load_file("BMS_Simulator/DefaultDBC.dbc")

    print(dbc.messages)

    signal_dict ={ 
        "EVSE1_DC_Charging_State": 0,
        "EVSE1_Isolation_Status": 0,
        "EVSE1_Present_DC_Charging_Voltage": 700,
        "EVSE1_Present_DC_Charging_Current": 100,
        "EVSE1_Voltage_Limit_Achieved": 0,
        "EVSE1_Current_Limit_Achieved": 0,
        "EVSE1_Power_Limit_Achieved": 0,
        "EVSE1_Processing_State": 0,
        "EVSE1_Status": 0,
        "EVSE1_Response_Code": 0,
    }

    msg = dbc.get_message_by_name("PGN_64184_EVSE_1_DC_Status_1")
    data = msg.encode(signal_dict).hex()

    print(data)
except Exception as err:
    print("ERROR {} {}\n".format(signal_dict, err))

When running msg.encode I get the error

ERROR {'EVSE1_DC_Charging_State': 0, 'EVSE1_Isolation_Status': 0, 'EVSE1_Present_DC_Charging_Voltage': 700, 'EVSE1_Present_DC_Charging_Current': 100, 'EVSE1_Voltage_Limit_Achieved': 0, 'EVSE1_Current_Limit_Achieved': 0, 'EVSE1_Power_Limit_Achieved': 0, 'EVSE1_Processing_State': 0, 'EVSE1_Status': 0, 'EVSE1_Response_Code': 0} Signed integer value 34000 out of range.

MichaelBMiner commented 1 year ago

Did some more digging in dbcppp for the data

signal_dict ={ "EVSE1_DC_Charging_State": 0, "EVSE1_Isolation_Status": 0, "EVSE1_Present_DC_Charging_Voltage": 700, "EVSE1_Present_DC_Charging_Current": 30, "EVSE1_Voltage_Limit_Achieved": 0, "EVSE1_Current_Limit_Achieved": 0, "EVSE1_Power_Limit_Achieved": 0, "EVSE1_Processing_State": 0, "EVSE1_Status": 0, "EVSE1_Response_Code": 0, }

I get a hex value of 0x7f5836b0000000 The same data in cantools gives 0xb036587f000000

MichaelBMiner commented 1 year ago

If I change my PGN to remove the signed data

BO_ 2566568000 PGN_64184_EVSE_1_DC_Status_1: 8 EV
 SG_ EVSE1_DC_Charging_State : 0|4@1+ (1,0) [0|15] "" EVCC
 SG_ EVSE1_Isolation_Status : 4|4@1+ (1,0) [0|15] "" EVCC
 SG_ EVSE1_Present_DC_Charging_Voltage : 8|16@1+ (0.05,0) [0|3212.75] "V" EVCC
 SG_ EVSE1_Present_DC_Charging_Current : 24|16@1+ (0.05,0) [0|1612.75] "A" EVCC
 SG_ EVSE1_Voltage_Limit_Achieved : 40|2@1+ (1,0) [0|3] "" EVCC
 SG_ EVSE1_Current_Limit_Achieved : 42|2@1+ (1,0) [0|3] "" EVCC
 SG_ EVSE1_Power_Limit_Achieved : 44|2@1+ (1,0) [0|3] "" EVCC
 SG_ EVSE1_Processing_State : 46|2@1+ (1,0) [0|3] "" EVCC
 SG_ EVSE1_Status : 48|4@1+ (1,0) [0|15] "" EVCC
 SG_ EVSE1_Response_Code : 56|8@1+ (1,0) [0|255] "" EVCC

Now I can set my current to any valid value and my hex output data is what I expect, but in a different order.

andlaus commented 1 year ago

hm, maybe this is an exception raised by the bitstruct module. this indicates that the EVSE1_Present_DC_Charging_Current has to few bits to represent a signed integer value of 34000 (it must be at least 17 bits long).

MichaelBMiner commented 1 year ago

My question then is, how do I proceed? Why is the voltage able to be properly converted from 700 to 14000? Why when I remove the negative offset and minimum value does it start to work?

MichaelBMiner commented 1 year ago

Here is my original PGN entry in the DBC file

BO_ 2566568016 PGN_64184_EVSE_1_DC_Status1: 8 EVCC SG EVSE1_DC_ChargingState : 0|4@1+ (1,0) [0|15] "" EV SG EVSE1_IsolationStatus : 4|4@1+ (1,0) [0|15] "" EV SG EVSE1_Present_DC_ChargingVoltage : 8|16@1+ (0.05,0) [0|3212.75] "V" EV SG EVSE1_Present_DC_ChargingCurrent : 24|16@1- (0.05,-1600) [-1600|1612.75] "A" EV SG EVSE1_Voltage_LimitAchieved : 40|2@1+ (1,0) [0|3] "" EV SG EVSE1_Current_LimitAchieved : 42|2@1+ (1,0) [0|3] "" EV SG EVSE1_Power_LimitAchieved : 44|2@1+ (1,0) [0|3] "" EV SG EVSE1_ProcessingState : 46|2@1+ (1,0) [0|3] "" EV SG EVSE1Status : 48|4@1+ (1,0) [0|15] "" EV SG EVSE1_Response_Code : 56|8@1+ (1,0) [0|255] "" EV

I have changed it to

BO_ 2566568016 PGN_64184_EVSE_1_DC_Status1: 8 EVCC SG EVSE1_DC_ChargingState : 0|4@1+ (1,0) [0|15] "" EV SG EVSE1_IsolationStatus : 4|4@1+ (1,0) [0|15] "" EV SG EVSE1_Present_DC_ChargingVoltage : 8|16@1+ (0.05,0) [0|3212.75] "V" EV SG EVSE1_Present_DC_ChargingCurrent : 24|16@1+ (0.05,-1600) [-1600|1612.75] "A" EV SG EVSE1_Voltage_LimitAchieved : 40|2@1+ (1,0) [0|3] "" EV SG EVSE1_Current_LimitAchieved : 42|2@1+ (1,0) [0|3] "" EV SG EVSE1_Power_LimitAchieved : 44|2@1+ (1,0) [0|3] "" EV SG EVSE1_ProcessingState : 46|2@1+ (1,0) [0|3] "" EV SG EVSE1Status : 48|4@1+ (1,0) [0|15] "" EV SG EVSE1_Response_Code : 56|8@1+ (1,0) [0|255] "" EV

The endianness of the EVSE1_Present_DC_Charging_Current signal was the issue.

juleq commented 1 year ago

Thanks for letting us know. I do not know what the Motorola people were smoking, but there is only one valid endianness. Someone should have told them :).