Merge branch 'master' of https://github.com/commaai/opendbc into alfa…

…-romeo

.github/workflows/tests.yml

@@ -22,7 +22,7 @@ jobs:
     - name: Build opendbc
       run: ${{ env.RUN }} "cd ../ && scons -j$(nproc) --minimal"
     - name: Unit tests
-      run: ${{ env.RUN }} "python -m unittest discover ."
+      run: ${{ env.RUN }} "pytest"
 
   static-analysis:
     name: static analysis

.pre-commit-config.yaml

@@ -48,6 +48,7 @@ repos:
         args:
         - --error-exitcode=1
         - --language=c++
+        - --inline-suppr
         - --force
         - --quiet
         - -j4

can/common.cc

@@ -103,12 +103,16 @@ void gen_crc_lookup_table_16(uint16_t poly, uint16_t crc_lut[]) {
   }
 }
 
-void init_crc_lookup_tables() {
-  // At init time, set up static lookup tables for fast CRC computation.
-  gen_crc_lookup_table_8(0x2F, crc8_lut_8h2f);    // CRC-8 8H2F/AUTOSAR for Volkswagen
-  gen_crc_lookup_table_8(0x1D, crc8_lut_j1850);    // CRC-8 SAE-J1850
-  gen_crc_lookup_table_16(0x1021, crc16_lut_xmodem);    // CRC-16 XMODEM for HKG CAN FD
-}
+// Initializes CRC lookup tables at module initialization
+struct CrcInitializer {
+  CrcInitializer() {
+    gen_crc_lookup_table_8(0x2F, crc8_lut_8h2f);  // CRC-8 8H2F/AUTOSAR for Volkswagen
+    gen_crc_lookup_table_8(0x1D, crc8_lut_j1850);  // CRC-8 SAE-J1850
+    gen_crc_lookup_table_16(0x1021, crc16_lut_xmodem);  // CRC-16 XMODEM for HKG CAN FD
+  }
+};
+
+static CrcInitializer crcInitializer;
 
 unsigned int volkswagen_mqb_checksum(uint32_t address, const Signal &sig, const std::vector<uint8_t> &d) {
   // Volkswagen uses standard CRC8 8H2F/AUTOSAR, but they compute it with

can/common.h

@@ -13,6 +13,7 @@
 #include "cereal/gen/cpp/log.capnp.h"
 #endif
 
+#include "opendbc/can/logger.h"
 #include "opendbc/can/common_dbc.h"
 
 #define INFO printf
@@ -23,8 +24,6 @@
 #define MAX_BAD_COUNTER 5
 #define CAN_INVALID_CNT 5
 
-void init_crc_lookup_tables();
-
 // Car specific functions
 unsigned int honda_checksum(uint32_t address, const Signal &sig, const std::vector<uint8_t> &d);
 unsigned int toyota_checksum(uint32_t address, const Signal &sig, const std::vector<uint8_t> &d);
@@ -93,11 +92,10 @@ class CANPacker {
 private:
   const DBC *dbc = NULL;
   std::map<std::pair<uint32_t, std::string>, Signal> signal_lookup;
-  std::map<uint32_t, Msg> message_lookup;
   std::map<uint32_t, uint32_t> counters;
 
 public:
   CANPacker(const std::string& dbc_name);
   std::vector<uint8_t> pack(uint32_t address, const std::vector<SignalPackValue> &values);
-  Msg* lookup_message(uint32_t address);
+  const Msg* lookup_message(uint32_t address);
 };

can/common.pxd

@@ -6,6 +6,7 @@ from libcpp cimport bool
 from libcpp.pair cimport pair
 from libcpp.string cimport string
 from libcpp.vector cimport vector
+from libcpp.unordered_map cimport unordered_map
 
 
 ctypedef unsigned int (*calc_checksum_type)(uint32_t, const Signal&, const vector[uint8_t] &)
@@ -49,6 +50,8 @@ cdef extern from "common_dbc.h":
     string name
     vector[Msg] msgs
     vector[Val] vals
+    unordered_map[uint32_t, const Msg*] addr_to_msg
+    unordered_map[string, const Msg*] name_to_msg
 
   cdef struct SignalValue:
     uint32_t address

can/common_dbc.h

@@ -1,8 +1,8 @@
 #pragma once
 
-#include <cstddef>
 #include <cstdint>
 #include <string>
+#include <unordered_map>
 #include <vector>
 
 struct SignalPackValue {
@@ -60,6 +60,8 @@ struct DBC {
   std::string name;
   std::vector<Msg> msgs;
   std::vector<Val> vals;
+  std::unordered_map<uint32_t, const Msg*> addr_to_msg;
+  std::unordered_map<std::string, const Msg*> name_to_msg;
 };
 
 typedef struct ChecksumState {

can/dbc.cc

@@ -202,6 +202,8 @@ DBC* dbc_parse_from_stream(const std::string &dbc_name, std::istream &stream, Ch
 
   for (auto& m : dbc->msgs) {
     m.sigs = signals[m.address];
+    dbc->addr_to_msg[m.address] = &m;
+    dbc->name_to_msg[m.name] = &m;
   }
   for (auto& v : dbc->vals) {
     v.sigs = signals[v.address];

can/logger.h

@@ -0,0 +1,27 @@
+#pragma once
+
+#ifdef SWAGLOG
+// cppcheck-suppress preprocessorErrorDirective
+#include SWAGLOG
+#else
+
+#define CLOUDLOG_DEBUG 10
+#define CLOUDLOG_INFO 20
+#define CLOUDLOG_WARNING 30
+#define CLOUDLOG_ERROR 40
+#define CLOUDLOG_CRITICAL 50
+
+#define cloudlog(lvl, fmt, ...) printf(fmt "\n", ## __VA_ARGS__)
+#define cloudlog_rl(burst, millis, lvl, fmt, ...) printf(fmt "\n", ##__VA_ARGS__)
+
+#define LOGD(fmt, ...) cloudlog(CLOUDLOG_DEBUG, fmt, ## __VA_ARGS__)
+#define LOG(fmt, ...) cloudlog(CLOUDLOG_INFO, fmt, ## __VA_ARGS__)
+#define LOGW(fmt, ...) cloudlog(CLOUDLOG_WARNING, fmt, ## __VA_ARGS__)
+#define LOGE(fmt, ...) cloudlog(CLOUDLOG_ERROR, fmt, ## __VA_ARGS__)
+
+#define LOGD_100(fmt, ...) cloudlog_rl(2, 100, CLOUDLOG_DEBUG, fmt, ## __VA_ARGS__)
+#define LOG_100(fmt, ...) cloudlog_rl(2, 100, CLOUDLOG_INFO, fmt, ## __VA_ARGS__)
+#define LOGW_100(fmt, ...) cloudlog_rl(2, 100, CLOUDLOG_WARNING, fmt, ## __VA_ARGS__)
+#define LOGE_100(fmt, ...) cloudlog_rl(2, 100, CLOUDLOG_ERROR, fmt, ## __VA_ARGS__)
+
+#endif

can/packer.cc

@@ -33,24 +33,28 @@ CANPacker::CANPacker(const std::string& dbc_name) {
   assert(dbc);
 
   for (const auto& msg : dbc->msgs) {
-    message_lookup[msg.address] = msg;
     for (const auto& sig : msg.sigs) {
       signal_lookup[std::make_pair(msg.address, sig.name)] = sig;
     }
   }
-  init_crc_lookup_tables();
 }
 
 std::vector<uint8_t> CANPacker::pack(uint32_t address, const std::vector<SignalPackValue> &signals) {
-  std::vector<uint8_t> ret(message_lookup[address].size, 0);
+  auto msg_it = dbc->addr_to_msg.find(address);
+  if (msg_it == dbc->addr_to_msg.end()) {
+    LOGE("undefined address %d", address);
+    return {};
+  }
+
+  std::vector<uint8_t> ret(msg_it->second->size, 0);
 
   // set all values for all given signal/value pairs
   bool counter_set = false;
   for (const auto& sigval : signals) {
     auto sig_it = signal_lookup.find(std::make_pair(address, sigval.name));
     if (sig_it == signal_lookup.end()) {
       // TODO: do something more here. invalid flag like CANParser?
-      WARN("undefined signal %s - %d\n", sigval.name.c_str(), address);
+      LOGE("undefined signal %s - %d\n", sigval.name.c_str(), address);
       continue;
     }
     const auto &sig = sig_it->second;
@@ -61,9 +65,9 @@ std::vector<uint8_t> CANPacker::pack(uint32_t address, const std::vector<SignalP
     }
     set_value(ret, sig, ival);
 
-    counter_set = counter_set || (sigval.name == "COUNTER");
-    if (counter_set) {
+    if (sigval.name == "COUNTER") {
       counters[address] = sigval.value;
+      counter_set = true;
     }
   }
 
@@ -93,6 +97,6 @@ std::vector<uint8_t> CANPacker::pack(uint32_t address, const std::vector<SignalP
 }
 
 // This function has a definition in common.h and is used in PlotJuggler
-Msg* CANPacker::lookup_message(uint32_t address) {
-  return &message_lookup[address];
+const Msg* CANPacker::lookup_message(uint32_t address) {
+  return dbc->addr_to_msg.at(address);
 }

can/packer_pyx.pyx

@@ -1,30 +1,24 @@
 # distutils: language = c++
 # cython: c_string_encoding=ascii, language_level=3
 
-from libc.stdint cimport uint8_t
+from libc.stdint cimport uint8_t, uint32_t
 from libcpp.vector cimport vector
-from libcpp.map cimport map
-from libcpp.string cimport string
 
 from .common cimport CANPacker as cpp_CANPacker
-from .common cimport dbc_lookup, SignalPackValue, DBC
+from .common cimport dbc_lookup, SignalPackValue, DBC, Msg
 
 
 cdef class CANPacker:
   cdef:
     cpp_CANPacker *packer
     const DBC *dbc
-    map[string, int] name_to_address
 
   def __init__(self, dbc_name):
     self.dbc = dbc_lookup(dbc_name)
     if not self.dbc:
       raise RuntimeError(f"Can't lookup {dbc_name}")
 
     self.packer = new cpp_CANPacker(dbc_name)
-    for i in range(self.dbc[0].msgs.size()):
-      msg = self.dbc[0].msgs[i]
-      self.name_to_address[string(msg.name)] = msg.address
 
   def __dealloc__(self):
     if self.packer:
@@ -43,11 +37,17 @@ cdef class CANPacker:
     return self.packer.pack(addr, values_thing)
 
   cpdef make_can_msg(self, name_or_addr, bus, values):
-    cdef int addr
+    cdef uint32_t addr = 0
+    cdef const Msg* m
     if isinstance(name_or_addr, int):
       addr = name_or_addr
     else:
-      addr = self.name_to_address[name_or_addr.encode("utf8")]
+      try:
+        m = self.dbc.name_to_msg.at(name_or_addr.encode("utf8"))
+        addr = m.address
+      except IndexError:
+        # The C++ pack function will log an error message for invalid addresses
+        pass
 
     cdef vector[uint8_t] val = self.pack(addr, values)
     return [addr, 0, (<char *>&val[0])[:val.size()], bus]

can/parser.cc

@@ -10,7 +10,6 @@
 #include <sys/stat.h>
 #include <sys/mman.h>
 
-#include "cereal/logger/logger.h"
 #include "opendbc/can/common.h"
 
 int64_t get_raw_value(const std::vector<uint8_t> &msg, const Signal &sig) {
@@ -65,7 +64,7 @@ bool MessageState::parse(uint64_t nanos, const std::vector<uint8_t> &dat) {
 
   // only update values if both checksum and counter are valid
   if (checksum_failed || counter_failed) {
-    LOGE("0x%X message checks failed, checksum failed %d, counter failed %d", address, checksum_failed, counter_failed);
+    LOGE_100("0x%X message checks failed, checksum failed %d, counter failed %d", address, checksum_failed, counter_failed);
     return false;
   }
 
@@ -97,7 +96,6 @@ CANParser::CANParser(int abus, const std::string& dbc_name, const std::vector<st
   : bus(abus), aligned_buf(kj::heapArray<capnp::word>(1024)) {
   dbc = dbc_lookup(dbc_name);
   assert(dbc);
-  init_crc_lookup_tables();
 
   bus_timeout_threshold = std::numeric_limits<uint64_t>::max();
 
@@ -121,18 +119,7 @@ CANParser::CANParser(int abus, const std::string& dbc_name, const std::vector<st
       bus_timeout_threshold = std::min(bus_timeout_threshold, state.check_threshold);
     }
 
-    const Msg* msg = NULL;
-    for (const auto& m : dbc->msgs) {
-      if (m.address == address) {
-        msg = &m;
-        break;
-      }
-    }
-    if (!msg) {
-      fprintf(stderr, "CANParser: could not find message 0x%X in DBC %s\n", address, dbc_name.c_str());
-      assert(false);
-    }
-
+    const Msg *msg = dbc->addr_to_msg.at(address);
     state.name = msg->name;
     state.size = msg->size;
     assert(state.size <= 64);  // max signal size is 64 bytes
@@ -150,7 +137,6 @@ CANParser::CANParser(int abus, const std::string& dbc_name, bool ignore_checksum
 
   dbc = dbc_lookup(dbc_name);
   assert(dbc);
-  init_crc_lookup_tables();
 
   for (const auto& msg : dbc->msgs) {
     MessageState state = {
@@ -238,7 +224,10 @@ void CANParser::UpdateCans(uint64_t nanos, const capnp::List<cereal::CanData>::R
     //  continue;
     //}
 
-    std::vector<uint8_t> data(dat.size(), 0);
+    // TODO: can remove when we ignore unexpected can msg lengths
+    // make sure the data_size is not less than state_it->second.size
+    size_t data_size = std::max<size_t>(dat.size(), state_it->second.size);
+    std::vector<uint8_t> data(data_size, 0);
     memcpy(data.data(), dat.begin(), dat.size());
     state_it->second.parse(nanos, data);
   }
@@ -290,9 +279,9 @@ void CANParser::UpdateValid(uint64_t nanos) {
     if (state.check_threshold > 0 && (missing || timed_out)) {
       if (show_missing && !bus_timeout) {
         if (missing) {
-          LOGE("0x%X '%s' NOT SEEN", state.address, state.name.c_str());
+          LOGE_100("0x%X '%s' NOT SEEN", state.address, state.name.c_str());
         } else if (timed_out) {
-          LOGE("0x%X '%s' TIMED OUT", state.address, state.name.c_str());
+          LOGE_100("0x%X '%s' TIMED OUT", state.address, state.name.c_str());
         }
       }
       _valid = false;

can/parser_pyx.pyx

@@ -36,27 +36,21 @@ cdef class CANParser:
     self.vl = {}
     self.vl_all = {}
     self.ts_nanos = {}
-    msg_name_to_address = {}
-    address_to_msg_name = {}
-
-    for i in range(self.dbc[0].msgs.size()):
-      msg = self.dbc[0].msgs[i]
-      name = msg.name.decode("utf8")
-
-      msg_name_to_address[name] = msg.address
-      address_to_msg_name[msg.address] = name
 
     # Convert message names into addresses and check existence in DBC
     cdef vector[pair[uint32_t, int]] message_v
     for i in range(len(messages)):
       c = messages[i]
-      address = c[0] if isinstance(c[0], numbers.Number) else msg_name_to_address.get(c[0])
-      if address not in address_to_msg_name:
+      try:
+        m = self.dbc.addr_to_msg.at(c[0]) if isinstance(c[0], numbers.Number) else self.dbc.name_to_msg.at(c[0])
+      except IndexError:
         raise RuntimeError(f"could not find message {repr(c[0])} in DBC {self.dbc_name}")
+
+      address = m.address
       message_v.push_back((address, c[1]))
       self.addresses.push_back(address)
 
-      name = address_to_msg_name[address]
+      name = m.name.decode("utf8")
       self.vl[address] = {}
       self.vl[name] = self.vl[address]
       self.vl_all[address] = defaultdict(list)
@@ -128,14 +122,6 @@ cdef class CANDefine():
     if not self.dbc:
       raise RuntimeError(f"Can't find DBC: '{dbc_name}'")
 
-    address_to_msg_name = {}
-
-    for i in range(self.dbc[0].msgs.size()):
-      msg = self.dbc[0].msgs[i]
-      name = msg.name.decode("utf8")
-      address = msg.address
-      address_to_msg_name[address] = name
-
     dv = defaultdict(dict)
 
     for i in range(self.dbc[0].vals.size()):
@@ -144,7 +130,11 @@ cdef class CANDefine():
       sgname = val.name.decode("utf8")
       def_val = val.def_val.decode("utf8")
       address = val.address
-      msgname = address_to_msg_name[address]
+      try:
+        m = self.dbc.addr_to_msg.at(address)
+      except IndexError:
+        raise KeyError(address)
+      msgname = m.name.decode("utf-8")
 
       # separate definition/value pairs
       def_val = def_val.split()