Revert "CANPacker: refactor to avoid undefined signals (commaai#891)" (…

…commaai#899)

This reverts commit 3a0083b.

can/common.h

@@ -94,12 +94,10 @@ class CANPacker {
   const DBC *dbc = NULL;
   std::map<std::pair<uint32_t, std::string>, Signal> signal_lookup;
   std::map<uint32_t, Msg> message_lookup;
-  std::map<std::string, uint32_t> message_name_to_address;
   std::map<uint32_t, uint32_t> counters;
 
 public:
   CANPacker(const std::string& dbc_name);
-  uint32_t address_from_name(const std::string &msg_name);
   std::vector<uint8_t> pack(uint32_t address, const std::vector<SignalPackValue> &values);
   Msg* lookup_message(uint32_t address);
 };

can/common.pxd

@@ -79,5 +79,4 @@ cdef extern from "common.h":
 
   cdef cppclass CANPacker:
    CANPacker(string)
-   uint32_t address_from_name(const string&)
    vector[uint8_t] pack(uint32_t, vector[SignalPackValue]&)

can/packer.cc

@@ -34,39 +34,27 @@ CANPacker::CANPacker(const std::string& dbc_name) {
 
   for (const auto& msg : dbc->msgs) {
     message_lookup[msg.address] = msg;
-    message_name_to_address[msg.name] = msg.address;
     for (const auto& sig : msg.sigs) {
       signal_lookup[std::make_pair(msg.address, std::string(sig.name))] = sig;
     }
   }
   init_crc_lookup_tables();
 }
 
-uint32_t CANPacker::address_from_name(const std::string &msg_name) {
-  auto msg_it = message_name_to_address.find(msg_name);
-  if (msg_it == message_name_to_address.end()) {
-    throw std::runtime_error("CANPacker::address_from_name(): invalid message name " + msg_name);
-  }
-  return msg_it->second;
-}
-
-std::vector<uint8_t> CANPacker::pack(uint32_t address, const std::vector<SignalPackValue> &values) {
-  auto msg_it = message_lookup.find(address);
-  if (msg_it == message_lookup.end()) {
-    throw std::runtime_error("CANPacker::pack(): invalid address " + std::to_string(address));
-  }
-
+std::vector<uint8_t> CANPacker::pack(uint32_t address, const std::vector<SignalPackValue> &signals) {
   std::vector<uint8_t> ret(message_lookup[address].size, 0);
 
   // set all values for all given signal/value pairs
   bool counter_set = false;
-  for (const auto& sigval : values) {
+  for (const auto& sigval : signals) {
     auto sig_it = signal_lookup.find(std::make_pair(address, sigval.name));
     if (sig_it == signal_lookup.end()) {
-      throw std::runtime_error("CANPacker::pack(): undefined signal " + sigval.name + " in " + msg_it->second.name);
+      // TODO: do something more here. invalid flag like CANParser?
+      WARN("undefined signal %s - %d\n", sigval.name.c_str(), address);
+      continue;
     }
-
     const auto &sig = sig_it->second;
+
     int64_t ival = (int64_t)(round((sigval.value - sig.offset) / sig.factor));
     if (ival < 0) {
       ival = (1ULL << sig.size) + ival;

can/packer_pyx.pyx

@@ -1,21 +1,30 @@
 # distutils: language = c++
 # cython: c_string_encoding=ascii, language_level=3
 
-from libc.stdint cimport uint8_t, uint32_t
+from libc.stdint cimport uint8_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
+from .common cimport dbc_lookup, SignalPackValue, DBC
 
 
 cdef class CANPacker:
-  cdef cpp_CANPacker *packer
+  cdef:
+    cpp_CANPacker *packer
+    const DBC *dbc
+    map[string, int] name_to_address
 
   def __init__(self, dbc_name):
-    if not dbc_lookup(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
 
   cdef vector[uint8_t] pack(self, addr, values):
     cdef vector[SignalPackValue] values_thing
@@ -29,12 +38,12 @@ cdef class CANPacker:
 
     return self.packer.pack(addr, values_thing)
 
-  cpdef make_can_msg(self, name_or_addr, bus, values) except +RuntimeError:
-    cdef uint32_t addr
+  cpdef make_can_msg(self, name_or_addr, bus, values):
+    cdef int addr
     if type(name_or_addr) == int:
       addr = name_or_addr
     else:
-      addr = self.packer.address_from_name(name_or_addr.encode("utf8"))
+      addr = self.name_to_address[name_or_addr.encode("utf8")]
 
     cdef vector[uint8_t] val = self.pack(addr, values)
     return [addr, 0, (<char *>&val[0])[:val.size()], bus]

can/tests/test_packer_parser.py

@@ -323,7 +323,6 @@ def test_undefined_signals(self):
       245: ["SIGNED", "64_BIT_LE", "64_BIT_BE", "COUNTER"],
     }
 
-    packer = CANPacker(TEST_DBC)
     for msg, sigs in existing_signals.items():
       for sig in sigs:
         CANParser(TEST_DBC, [(sig, msg)], [(msg, 0)])
@@ -333,11 +332,6 @@ def test_undefined_signals(self):
         self.assertRaises(RuntimeError, CANParser, TEST_DBC, [(sig, msg)], [(new_msg, 0)])
         self.assertRaises(RuntimeError, CANParser, TEST_DBC, [(sig, new_msg)], [(new_msg, 0)])
 
-        packer.make_can_msg(msg, 0, {sig: 0})
-        self.assertRaises(RuntimeError, packer.make_can_msg, msg, 0, {sig + "1": 0})
-        self.assertRaises(RuntimeError, packer.make_can_msg, new_msg, 0, {sig: 0})
-        self.assertRaises(RuntimeError, packer.make_can_msg, new_msg, 0, {sig + "1": 0})
-
 
 if __name__ == "__main__":
   unittest.main()