CANParser: parse all signals for given messages (commaai#828)

* CANParser: parse all signals for a message

* update tests

* just use a pair

* rm enforce checks

* rm that

* spacing

* fix nonexistent message test

* message addr check should not have been deleted

* can be cleaned up more

* remove that too

* add comment back

* revert default bus behavior

revert default bus behavior

* can combine this loop

* unused map

* add all

* ensure we track all signals

* remove sanity check

* this wasn't tested before

* Revert "this wasn't tested before"

This reverts commit eb5e920.

---------

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

can/common.h

@@ -76,8 +76,7 @@ class CANParser {
   uint64_t can_invalid_cnt = CAN_INVALID_CNT;
 
   CANParser(int abus, const std::string& dbc_name,
-            const std::vector<MessageParseOptions> &options,
-            const std::vector<SignalParseOptions> &sigoptions);
+            const std::vector<std::pair<uint32_t, int>> &messages);
   CANParser(int abus, const std::string& dbc_name, bool ignore_checksum, bool ignore_counter);
   #ifndef DYNAMIC_CAPNP
   void update_string(const std::string &data, bool sendcan);

can/common.pxd

@@ -3,6 +3,7 @@
 
 from libc.stdint cimport uint8_t, uint16_t, uint32_t, uint64_t
 from libcpp cimport bool
+from libcpp.pair cimport pair
 from libcpp.string cimport string
 from libcpp.vector cimport vector
 
@@ -48,14 +49,6 @@ cdef extern from "common_dbc.h":
     vector[Msg] msgs
     vector[Val] vals
 
-  cdef struct SignalParseOptions:
-    uint32_t address
-    string name
-
-  cdef struct MessageParseOptions:
-    uint32_t address
-    int check_frequency
-
   cdef struct SignalValue:
     uint32_t address
     uint64_t ts_nanos
@@ -74,7 +67,7 @@ cdef extern from "common.h":
   cdef cppclass CANParser:
     bool can_valid
     bool bus_timeout
-    CANParser(int, string, vector[MessageParseOptions], vector[SignalParseOptions])
+    CANParser(int, string, vector[pair[uint32_t, int]])
     void update_strings(vector[string]&, vector[SignalValue]&, bool) except +
 
   cdef cppclass CANPacker:

can/common_dbc.h

@@ -12,16 +12,6 @@ struct SignalPackValue {
   double value;
 };
 
-struct SignalParseOptions {
-  uint32_t address;
-  std::string name;
-};
-
-struct MessageParseOptions {
-  uint32_t address;
-  int check_frequency;
-};
-
 struct SignalValue {
   uint32_t address;
   uint64_t ts_nanos;

can/parser.cc

@@ -89,67 +89,47 @@ bool MessageState::update_counter_generic(int64_t v, int cnt_size) {
 }
 
 
-CANParser::CANParser(int abus, const std::string& dbc_name,
-          const std::vector<MessageParseOptions> &options,
-          const std::vector<SignalParseOptions> &sigoptions)
+CANParser::CANParser(int abus, const std::string& dbc_name, const std::vector<std::pair<uint32_t, int>> &messages)
   : 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();
 
-  for (const auto& op : options) {
-    MessageState &state = message_states[op.address];
-    state.address = op.address;
+  for (const auto& [address, frequency] : messages) {
+    MessageState &state = message_states[address];
+    state.address = address;
     // state.check_frequency = op.check_frequency,
 
     // msg is not valid if a message isn't received for 10 consecutive steps
-    if (op.check_frequency > 0) {
-      state.check_threshold = (1000000000ULL / op.check_frequency) * 10;
+    if (frequency > 0) {
+      state.check_threshold = (1000000000ULL / frequency) * 10;
 
       // bus timeout threshold should be 10x the fastest msg
       bus_timeout_threshold = std::min(bus_timeout_threshold, state.check_threshold);
     }
 
     const Msg* msg = NULL;
     for (const auto& m : dbc->msgs) {
-      if (m.address == op.address) {
+      if (m.address == address) {
         msg = &m;
         break;
       }
     }
     if (!msg) {
-      fprintf(stderr, "CANParser: could not find message 0x%X in DBC %s\n", op.address, dbc_name.c_str());
+      fprintf(stderr, "CANParser: could not find message 0x%X in DBC %s\n", address, dbc_name.c_str());
       assert(false);
     }
 
     state.name = msg->name;
     state.size = msg->size;
     assert(state.size <= 64);  // max signal size is 64 bytes
 
-    // track checksums and counters for this message
-    for (const auto& sig : msg->sigs) {
-      if (sig.type != SignalType::DEFAULT) {
-        state.parse_sigs.push_back(sig);
-        state.vals.push_back(0);
-        state.all_vals.push_back({});
-      }
-    }
-
-    // track requested signals for this message
-    for (const auto& sigop : sigoptions) {
-      if (sigop.address != op.address) continue;
-
-      for (const auto& sig : msg->sigs) {
-        if (sig.name == sigop.name && sig.type == SignalType::DEFAULT) {
-          state.parse_sigs.push_back(sig);
-          state.vals.push_back(0);
-          state.all_vals.push_back({});
-          break;
-        }
-      }
-    }
+    // track all signals for this message
+    state.parse_sigs = msg->sigs;
+    state.vals.resize(msg->sigs.size());
+    state.all_vals.resize(msg->sigs.size());
   }
 }
 

can/parser_pyx.pyx

@@ -2,14 +2,14 @@
 # cython: c_string_encoding=ascii, language_level=3
 
 from cython.operator cimport dereference as deref, preincrement as preinc
+from libcpp.pair cimport pair
 from libcpp.string cimport string
 from libcpp.vector cimport vector
 from libcpp.unordered_set cimport unordered_set
 from libc.stdint cimport uint32_t
-from libcpp.map cimport map
 
 from .common cimport CANParser as cpp_CANParser
-from .common cimport SignalParseOptions, MessageParseOptions, dbc_lookup, SignalValue, DBC
+from .common cimport dbc_lookup, SignalValue, DBC
 
 import numbers
 from collections import defaultdict
@@ -19,7 +19,6 @@ cdef class CANParser:
   cdef:
     cpp_CANParser *can
     const DBC *dbc
-    map[uint32_t, string] address_to_msg_name
     vector[SignalValue] can_values
 
   cdef readonly:
@@ -28,10 +27,7 @@ cdef class CANParser:
     dict ts_nanos
     string dbc_name
 
-  def __init__(self, dbc_name, signals, checks=None, bus=0, enforce_checks=True):
-    if checks is None:
-      checks = []
-
+  def __init__(self, dbc_name, messages, bus=0):
     self.dbc_name = dbc_name
     self.dbc = dbc_lookup(dbc_name)
     if not self.dbc:
@@ -41,71 +37,32 @@ cdef class CANParser:
     self.vl_all = {}
     self.ts_nanos = {}
     msg_name_to_address = {}
-    msg_address_to_signals = {}
+    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
-      msg_address_to_signals[msg.address] = set()
-      for sig in msg.sigs:
-        msg_address_to_signals[msg.address].add(sig.name.decode("utf8"))
+      address_to_msg_name[msg.address] = name
 
-      self.address_to_msg_name[msg.address] = name
       self.vl[msg.address] = {}
       self.vl[name] = self.vl[msg.address]
       self.vl_all[msg.address] = {}
       self.vl_all[name] = self.vl_all[msg.address]
       self.ts_nanos[msg.address] = {}
       self.ts_nanos[name] = self.ts_nanos[msg.address]
 
-    # Convert message names into addresses
-    for i in range(len(signals)):
-      s = signals[i]
-      address = s[1] if isinstance(s[1], numbers.Number) else msg_name_to_address.get(s[1])
-      if address not in msg_address_to_signals:
-        raise RuntimeError(f"could not find message {repr(s[1])} in DBC {self.dbc_name}")
-      if s[0] not in msg_address_to_signals[address]:
-        raise RuntimeError(f"could not find signal {repr(s[0])} in {repr(s[1])}, DBC {self.dbc_name}")
-
-      signals[i] = (s[0], address)
-
-    for i in range(len(checks)):
-      c = checks[i]
-      if not isinstance(c[0], numbers.Number):
-        if c[0] not in msg_name_to_address:
-          print(msg_name_to_address)
-          raise RuntimeError(f"could not find message {repr(c[0])} in DBC {self.dbc_name}")
-        c = (msg_name_to_address[c[0]], c[1])
-        checks[i] = c
-
-    if enforce_checks:
-      checked_addrs = {c[0] for c in checks}
-      signal_addrs = {s[1] for s in signals}
-      unchecked = signal_addrs - checked_addrs
-      if len(unchecked):
-        err_msg = ", ".join(f"{self.address_to_msg_name[addr].decode()} ({hex(addr)})" for addr in unchecked)
-        raise RuntimeError(f"Unchecked addrs: {err_msg}")
-
-    cdef vector[SignalParseOptions] signal_options_v
-    cdef SignalParseOptions spo
-    for sig_name, sig_address in signals:
-      spo.address = sig_address
-      spo.name = sig_name
-      signal_options_v.push_back(spo)
-
-    message_options = dict((address, 0) for _, address in signals)
-    message_options.update(dict(checks))
-
-    cdef vector[MessageParseOptions] message_options_v
-    cdef MessageParseOptions mpo
-    for msg_address, freq in message_options.items():
-      mpo.address = msg_address
-      mpo.check_frequency = freq
-      message_options_v.push_back(mpo)
-
-    self.can = new cpp_CANParser(bus, dbc_name, message_options_v, signal_options_v)
+    # 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:
+        raise RuntimeError(f"could not find message {repr(c[0])} in DBC {self.dbc_name}")
+      message_v.push_back((address, c[1]))
+
+    self.can = new cpp_CANParser(bus, dbc_name, message_v)
     self.update_strings([])
 
   def update_strings(self, strings, sendcan=False):

can/tests/test_checksums.py

@@ -11,14 +11,8 @@ class TestCanChecksums(unittest.TestCase):
   def test_honda_checksum(self):
     """Test checksums for Honda standard and extended CAN ids"""
     dbc_file = "honda_accord_2018_can_generated"
-
-    signals = [
-      ("CHECKSUM", "LKAS_HUD"),
-      ("CHECKSUM", "LKAS_HUD_A"),
-    ]
-    checks = [("LKAS_HUD", 0), ("LKAS_HUD_A", 0)]
-
-    parser = CANParser(dbc_file, signals, checks, 0)
+    msgs = [("LKAS_HUD", 0), ("LKAS_HUD_A", 0)]
+    parser = CANParser(dbc_file, msgs, 0)
     packer = CANPacker(dbc_file)
 
     values = {

can/tests/test_dbc_exceptions.py

@@ -11,29 +11,23 @@ class TestCanParserPackerExceptions(unittest.TestCase):
   def test_civic_exceptions(self):
     dbc_file = "honda_civic_touring_2016_can_generated"
     dbc_invalid = dbc_file + "abcdef"
-    signals = [
-      ("STEER_TORQUE", "STEERING_CONTROL"),
-      ("STEER_TORQUE_REQUEST", "STEERING_CONTROL"),
-    ]
-    checks = [("STEERING_CONTROL", 50)]
+    msgs = [("STEERING_CONTROL", 50)]
     with self.assertRaises(RuntimeError):
-      CANParser(dbc_invalid, signals, checks, 0)
+      CANParser(dbc_invalid, msgs, 0)
     with self.assertRaises(RuntimeError):
       CANPacker(dbc_invalid)
     with self.assertRaises(RuntimeError):
       CANDefine(dbc_invalid)
     with self.assertRaises(KeyError):
       CANDefine(TEST_DBC)
 
-    with self.assertRaises(RuntimeError):
-      CANParser(dbc_file, signals, [], 0)
-
-    parser = CANParser(dbc_file, signals, checks, 0)
+    parser = CANParser(dbc_file, msgs, 0)
     with self.assertRaises(RuntimeError):
       parser.update_strings([b''])
 
     # Everything is supposed to work below
-    CANParser(dbc_file, signals, checks, 0)
+    CANParser(dbc_file, msgs, 0)
+    CANParser(dbc_file, [], 0)
     CANPacker(dbc_file)
     CANDefine(dbc_file)
 

can/tests/test_dbc_parser.py

@@ -21,7 +21,7 @@ def test_parse_all_dbcs(self):
 
     for dbc in ALL_DBCS:
       with self.subTest(dbc=dbc):
-        CANParser(dbc, [], [], 0)
+        CANParser(dbc, [], 0)
 
 
 if __name__ == "__main__":