build message lookup table in DBC (#942)

* build message lookup table in dbc

* constify it

* no need to make this more dense

* Return empty values and log an error for invalid addresses.

log error for undefined signal

add comment

use LOGE

* save one map lookup

* only try what is to be traught

* rename back

* from merge

* show addr

---------

Co-authored-by: Shane Smiskol <shane@smiskol.com>

can/common.h

@@ -93,11 +93,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] &)
@@ -48,6 +49,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 {
@@ -59,6 +59,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

@@ -200,6 +200,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/packer.cc

@@ -33,7 +33,6 @@ 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;
     }
@@ -42,13 +41,13 @@ CANPacker::CANPacker(const std::string& dbc_name) {
 }
 
 std::vector<uint8_t> CANPacker::pack(uint32_t address, const std::vector<SignalPackValue> &signals) {
-  auto msg_it = message_lookup.find(address);
-  if (msg_it == message_lookup.end()) {
+  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);
+  std::vector<uint8_t> ret(msg_it->second->size, 0);
 
   // set all values for all given signal/value pairs
   bool counter_set = false;
@@ -99,6 +98,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,20 +1,17 @@
 # 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)
@@ -22,9 +19,6 @@ cdef class CANPacker:
       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

@@ -120,18 +120,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

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()