From 3a0083b41ef950409a0b5e1bf9d7cfd051208ea1 Mon Sep 17 00:00:00 2001 From: Dean Lee Date: Fri, 14 Jul 2023 19:41:27 +0800 Subject: [PATCH] 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 --- can/common.h | 2 ++ can/common.pxd | 1 + can/packer.cc | 24 ++++++++++++++++++------ can/packer_pyx.pyx | 23 +++++++---------------- can/tests/test_packer_parser.py | 6 ++++++ 5 files changed, 34 insertions(+), 22 deletions(-) diff --git a/can/common.h b/can/common.h index 0031d78..3275fbd 100644 --- a/can/common.h +++ b/can/common.h @@ -94,10 +94,12 @@ private: const DBC *dbc = NULL; std::map, Signal> signal_lookup; std::map message_lookup; + std::map message_name_to_address; std::map counters; public: CANPacker(const std::string& dbc_name); + uint32_t address_from_name(const std::string &msg_name); std::vector pack(uint32_t address, const std::vector &values); Msg* lookup_message(uint32_t address); }; diff --git a/can/common.pxd b/can/common.pxd index aeb1f0a..f1027b5 100644 --- a/can/common.pxd +++ b/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]&) diff --git a/can/packer.cc b/can/packer.cc index 6cee2eb..72fbfc5 100644 --- a/can/packer.cc +++ b/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 CANPacker::pack(uint32_t address, const std::vector &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 CANPacker::pack(uint32_t address, const std::vector &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 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; diff --git a/can/packer_pyx.pyx b/can/packer_pyx.pyx index 3304339..4689a0a 100644 --- a/can/packer_pyx.pyx +++ b/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, (&val[0])[:val.size()], bus] diff --git a/can/tests/test_packer_parser.py b/can/tests/test_packer_parser.py index 1b7399c..275ccc7 100755 --- a/can/tests/test_packer_parser.py +++ b/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()