CANPacker: refactor to avoid undefined signals (#891)

* refactor to avoid undefined signals

* add non-zero offset check

* Revert "add non-zero offset check"

This reverts commit bc0bb60f4fdd8ffdca7197ff6dbd3cb50b857c30.

* clean up

* use underscores, we haven't standardized one or the other yet

* use message name

* test it works

---------

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

can/common.h

@@ -94,10 +94,12 @@ 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<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,4 +79,5 @@ 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

@@ -33,6 +33,7 @@ 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;
     }
@@ -40,20 +41,31 @@ CANPacker::CANPacker(const std::string& dbc_name) {
   init_crc_lookup_tables();
 }
 
-std::vector<uint8_t> CANPacker::pack(uint32_t address, const std::vector<SignalPackValue> &signals) {
+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> ret(message_lookup[address].size, 0);
 
   // set all values for all given signal/value pairs
   bool counter_set = false;
-  for (const auto& sigval : signals) {
+  for (const auto& sigval : values) {
     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);
-      continue;
+      throw std::runtime_error("CANPacker::pack(): undefined signal " + sigval.name + " in " + msg_it->second.name);
     }
-    const auto &sig = sig_it->second;
 
+    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,30 +1,21 @@
 # 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
 
 
 cdef class CANPacker:
-  cdef:
-    cpp_CANPacker *packer
-    const DBC *dbc
-    map[string, int] name_to_address
+  cdef cpp_CANPacker *packer
 
   def __init__(self, dbc_name):
-    self.dbc = dbc_lookup(dbc_name)
-    if not self.dbc:
+    if not dbc_lookup(dbc_name):
       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
@@ -38,12 +29,12 @@ cdef class CANPacker:
 
     return self.packer.pack(addr, values_thing)
 
-  cpdef make_can_msg(self, name_or_addr, bus, values):
-    cdef int addr
+  cpdef make_can_msg(self, name_or_addr, bus, values) except +RuntimeError:
+    cdef uint32_t addr
     if type(name_or_addr) == int:
       addr = name_or_addr
     else:
-      addr = self.name_to_address[name_or_addr.encode("utf8")]
+      addr = self.packer.address_from_name(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,6 +323,7 @@ class TestCanParserPacker(unittest.TestCase):
       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)])
@@ -332,6 +333,11 @@ class TestCanParserPacker(unittest.TestCase):
         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()