Improve maintainability and robustness

Added typing information to dbcfeeder to make code easier to read
and to allow more meaningful linting.

Also refactored some very large functions into multiple smaller
functions to improve readability and separation of concern.

dbc2val/dbcfeederlib/clientwrapper.py

@@ -38,6 +38,15 @@ def __init__(self, ip: str, port: int, token_path: str, tls: bool = True):
         self._registered = False
         self._root_ca_path: Optional[str] = None
         self._tls_server_name: Optional[str] = None
+        self._do_init()
+
+    def _do_init(self):
+        """
+        Perform any implementation specific additional initialization.
+
+        Called at the end of __init__.
+        This default implementation does nothing.
+        """
 
     def set_ip(self, ip: str):
         """ Set IP address to use """
@@ -63,19 +72,18 @@ def get_tls(self) -> bool:
     def set_root_ca_path(self, path: str):
         """ Set Path for Root CA (CA.pem) """
         self._root_ca_path = path
+        log.info("Using root CA path: %s", self._root_ca_path)
 
     def set_tls_server_name(self, name: str):
         """ Set Path for Root CA (CA.pem) """
         self._tls_server_name = name
+        log.info("Using TLS server name: %s", self._tls_server_name)
 
     def set_token_path(self, token_path: str):
         self._token_path = token_path
+        log.info("Using token from: %s", self._token_path)
 
     # Abstract methods to implement
-    @abstractmethod
-    def get_client_specific_configs(self):
-        pass
-
     @abstractmethod
     def start(self):
         pass

dbc2val/dbcfeederlib/databrokerclientwrapper.py

@@ -1,7 +1,7 @@
 #!/usr/bin/env python3
 
 #################################################################################
-# Copyright (c) 2022 Contributors to the Eclipse Foundation
+# Copyright (c) 2022, 2023 Contributors to the Eclipse Foundation
 #
 # See the NOTICE file(s) distributed with this work for additional
 # information regarding copyright ownership.
@@ -48,19 +48,19 @@ def __init__(self, ip: str = "127.0.0.1", port: int = 55555,
                  token_path: str = "",
                  tls: bool = False):
         """
-        Init Databroker client wrapper, by default (for now) without TLS
+        Init Databroker client wrapper, by default with TLS
         """
+        super().__init__(ip, port, token_path, tls)
         self._grpc_client = None
         self._name_to_type: dict[str, DataType] = {}
         self._rpc_kwargs: Dict[str, str] = {}
         self._connected = False
         self._exit_stack = contextlib.ExitStack()
-        super().__init__(ip, port, token_path, tls)
         self._token = ""
 
-    def get_client_specific_configs(self):
+    def _do_init(self):
         """
-        Get client specific configs and env variables
+        Set up gRPC metadata for interaction for Databroker.
         """
 
         if os.environ.get("VEHICLEDATABROKER_DAPR_APP_ID"):

dbc2val/dbcfeederlib/dbcparser.py

@@ -21,49 +21,63 @@
 import logging
 import sys
 import os
-from typing import Set, Optional, Dict, cast, Tuple
+
 import cantools.database
 
+from types import MappingProxyType
+from typing import cast, Dict, Optional, List, Set, Tuple
+
+
 log = logging.getLogger(__name__)
 
 
 class DBCParser:
-    def __init__(self, dbcfile: str, use_strict_parsing: bool = True):
+
+    _dbc_file_encodings = MappingProxyType({
+                'dbc': 'cp1252',
+                'sym': 'cp1252'
+            })
+
+    def __init__(self,
+                 dbc_file_names: List[str],
+                 use_strict_parsing: bool = True,
+                 expect_extended_frame_ids: bool = False):
 
         first = True
-        found_names = set()
-        for name in dbcfile.split(","):
-            filename = name.strip()
-            if filename in found_names:
-                log.warning("The DBC file {} has already been read, ignoring it!".format(filename))
+        processed_files: Set[str] = set()
+        for filename in [name.strip() for name in dbc_file_names]:
+            if filename in processed_files:
+                log.warning("DBC file %s has already been read, ignoring it!", filename)
                 continue
-            found_names.add(filename)
+            processed_files.add(filename)
             if first:
-                log.info("Reading DBC file {} as first file".format(filename))
-                db = cantools.database.load_file(filename, strict=use_strict_parsing)
+                # by default, do not mask any bits of standard (11-bit) frame IDs
+                mask = 0b11111111111
+                if expect_extended_frame_ids:
+                    # mask 3 priority bits of extended (29-bit) frame IDs
+                    mask = 0b00011111111111111111111111111
+                log.info("Reading definitions from DBC file %s", filename)
+                database = cantools.database.load_file(filename, strict=use_strict_parsing, frame_id_mask=mask)
                 # load_file can return multiple types of databases, make sure we have CAN database
-                if isinstance(db, cantools.database.can.database.Database):
-                    self.db = cast(cantools.database.can.database.Database, db)
+                if isinstance(database, cantools.database.can.database.Database):
+                    self._db = cast(cantools.database.can.database.Database, database)
                     first = False
                 else:
-                    log.error("File is not a CAN database, likely a diagnostics database")
+                    log.error("File %s is not a CAN database, likely a diagnostics database", filename)
                     sys.exit(-1)
             else:
-                log.info("Adding definitions from {}".format(filename))
+                log.info("Adding definitions from DBC file %s", filename)
                 self._add_db_file(filename)
 
         # Init some dictionaries to speed up search
-        self.signal_to_canid: Dict[str, Optional[int]] = {}
-        self.canid_to_signals: Dict[int, Set[str]] = {}
+        self._signal_to_canid: Dict[str, Optional[int]] = {}
+        self._canid_to_signals: Dict[int, Set[str]] = {}
 
     def _determine_db_format_and_encoding(self, filename) -> Tuple[str, str]:
         db_format = os.path.splitext(filename)[1][1:].lower()
 
         try:
-            encoding = {
-                'dbc': 'cp1252',
-                'sym': 'cp1252'
-            }[db_format]
+            encoding = DBCParser._dbc_file_encodings[db_format]
         except KeyError:
             encoding = 'utf-8'
 
@@ -72,47 +86,47 @@ def _determine_db_format_and_encoding(self, filename) -> Tuple[str, str]:
     def _add_db_file(self, filename: str):
         db_format, encoding = self._determine_db_format_and_encoding(filename)
         if db_format == "arxml":
-            self.db.add_arxml_file(filename, encoding)
+            self._db.add_arxml_file(filename, encoding)
         elif db_format == "dbc":
-            self.db.add_dbc_file(filename, encoding)
+            self._db.add_dbc_file(filename, encoding)
         elif db_format == "kcd":
-            self.db.add_kcd_file(filename, encoding)
+            self._db.add_kcd_file(filename, encoding)
         elif db_format == "sym":
-            self.db.add_sym_file(filename, encoding)
+            self._db.add_sym_file(filename, encoding)
         else:
             log.warning("Cannot read CAN message definitions from file using unsupported format: %s", db_format)
 
     def get_canid_for_signal(self, sig_to_find: str) -> Optional[int]:
-        if sig_to_find in self.signal_to_canid:
-            return self.signal_to_canid[sig_to_find]
+        if sig_to_find in self._signal_to_canid:
+            return self._signal_to_canid[sig_to_find]
 
-        for msg in self.db.messages:
+        for msg in self._db.messages:
             for signal in msg.signals:
                 if signal.name == sig_to_find:
                     frame_id = msg.frame_id
-                    log.info(
-                        "Found signal in DBC file {} in CAN frame id 0x{:02x}".format(
-                            signal.name, frame_id
-                        )
-                    )
-                    self.signal_to_canid[sig_to_find] = frame_id
+                    log.debug("Found signal %s in CAN message with frame ID %#x", signal.name, frame_id)
+                    self._signal_to_canid[sig_to_find] = frame_id
                     return frame_id
-        log.warning("Signal {} not found in DBC file".format(sig_to_find))
-        self.signal_to_canid[sig_to_find] = None
+        log.warning("Signal %s not found in CAN message database", sig_to_find)
+        self._signal_to_canid[sig_to_find] = None
         return None
 
     def get_signals_for_canid(self, canid: int) -> Set[str]:
 
-        if canid in self.canid_to_signals:
-            return self.canid_to_signals[canid]
-
-        for msg in self.db.messages:
-            if canid == msg.frame_id:
-                names = set()
-                for signal in msg.signals:
-                    names.add(signal.name)
-                self.canid_to_signals[canid] = names
-                return names
-        log.warning(f"CAN id {canid} not found in DBC file")
-        self.canid_to_signals[canid] = set()
-        return set()
+        if canid in self._canid_to_signals:
+            return self._canid_to_signals[canid]
+
+        names: Set[str] = set()
+        message = self.get_message_for_canid(canid)
+        if message is not None:
+            for signal in message.signals:
+                names.add(signal.name)
+        self._canid_to_signals[canid] = names
+        return names
+
+    def get_message_for_canid(self, canid: int) -> Optional[cantools.database.Message]:
+        try:
+            return self._db.get_message_by_frame_id(canid)
+        except Exception:
+            log.debug("No DBC mapping registered for CAN frame id %#x", canid)
+            return None

dbc2val/dbcfeederlib/dbcreader.py

@@ -58,39 +58,44 @@ def start_listening(self, *args, **kwargs):
         rx_thread.start()
 
     def get_whitelist(self):
-        log.info("Generating CAN ID whitelist")
+        log.debug("Generating CAN ID whitelist")
         white_list = []
-        for entry in self.mapper.get_dbc2val_entries():
-            canid = self.dbc_parser.get_canid_for_signal(entry)
+        for signal_name in self.mapper.get_dbc2val_entries():
+            canid = self.dbc_parser.get_canid_for_signal(signal_name)
             if canid is not None and canid not in white_list:
-                log.info(f"Adding {entry} to white list, canid is {canid}")
+                log.debug("Adding CAN frame id %d of message containing signal %s to white list", canid, signal_name)
                 white_list.append(canid)
         return white_list
 
     def rx_worker(self):
         log.info("Starting Rx thread")
         while self.run:
             msg = self.canclient.recv(timeout=1)
-            log.debug("processing message from CAN bus")
             if msg and msg.get_arbitration_id() in self.canidwl:
+                log.debug("processing message with frame ID %#x from CAN bus", msg.get_arbitration_id())
                 try:
-                    decode = self.dbc_parser.db.decode_message(msg.get_arbitration_id(), msg.get_data())
-                    log.debug("Decoded message: %s", str(decode))
+                    message_def = self.dbc_parser.get_message_for_canid(msg.get_arbitration_id())
+                    if message_def is not None:
+                        decode = message_def.decode(data=msg.get_data())
+                    else:
+                        # no message definition found for frame ID
+                        continue
                 except Exception:
-                    log.warning(
-                        "Error Decoding: ID:{}".format(msg.get_arbitration_id()),
-                        exc_info=True,
-                    )
+                    log.warning("Error Decoding frame with ID: %#x", msg.get_arbitration_id(), exc_info=True)
                     continue
+
+                if log.isEnabledFor(logging.DEBUG):
+                    log.debug("Decoded message: %s", str(decode))
+
                 rx_time = time.time()
                 for k, v in decode.items():
                     vss_mappings = self.mapper.get_dbc2val_mappings(k)
                     for signal in vss_mappings:
                         if signal.time_condition_fulfilled(rx_time):
-                            log.debug(f"Queueing {signal.vss_name}, triggered by {k}, raw value {v} ")
+                            log.debug("Queueing %s, triggered by %s, raw value %s ", signal.vss_name, k, v)
                             self.queue.put(dbc2vssmapper.VSSObservation(k, signal.vss_name, v, rx_time))
                         else:
-                            log.debug(f"Ignoring {signal.vss_name}, triggered by {k}, raw value {v} ")
+                            log.debug("Ignoring %s, triggered by %s, raw value %s ", signal.vss_name, k, v)
         log.info("Stopped Rx thread")
 
     def stop(self):

dbc2val/dbcfeederlib/serverclientwrapper.py

@@ -1,7 +1,7 @@
 #!/usr/bin/env python3
 
 #################################################################################
-# Copyright (c) 2022 Contributors to the Eclipse Foundation
+# Copyright (c) 2022, 2023 Contributors to the Eclipse Foundation
 #
 # See the NOTICE file(s) distributed with this work for additional
 # information regarding copyright ownership.
@@ -42,11 +42,8 @@ def __init__(self, ip: str = "localhost", port: int = 8090,
         self._client_config["protocol"] = "ws"
         self._kuksa = None
 
-    def get_client_specific_configs(self):
-        """
-        Get client specific configs and env variables
-        """
-        log.debug("No additional configs for KUKSA.val server")
+    def _do_init(self):
+        log.debug("No additional initialization necessary for KUKSA.val server")
 
     def start(self):
         """

dbc2val/test/test_dbc/test_dbc.py

@@ -18,8 +18,11 @@
 # SPDX-License-Identifier: Apache-2.0
 ########################################################################
 
-from dbcfeederlib import dbcparser
 import os
+import pytest
+
+from dbcfeederlib import dbcparser
+from dbcfeeder import ServerType
 
 # read config only once
 test_path = os.path.dirname(os.path.abspath(__file__))
@@ -28,7 +31,7 @@
 
 def test_default_dbc():
 
-    parser3 = dbcparser.DBCParser(def_dbc)
+    parser3 = dbcparser.DBCParser([def_dbc])
     assert parser3.get_canid_for_signal('SteeringAngle129') == 297
     assert parser3.get_canid_for_signal('DI_uiSpeed') == 599
 
@@ -38,7 +41,7 @@ def test_splitted_dbc():
     This verifies that we can read a splitted DBC File.
     Difference compared to default is that 'SteeringAngle129' has been moved to test1_2.dbc
     """
-    parser3 = dbcparser.DBCParser(test_path + "/test1_1.dbc," + test_path + "/test1_2.dbc")
+    parser3 = dbcparser.DBCParser([test_path + "/test1_1.dbc", test_path + "/test1_2.dbc"])
     assert parser3.get_canid_for_signal('SteeringAngle129') == 297
     assert parser3.get_canid_for_signal('DI_uiSpeed') == 599
 
@@ -47,6 +50,6 @@ def test_duplicated_dbc():
     """
     Load original DBC multiple times
     """
-    parser3 = dbcparser.DBCParser(def_dbc + ',' + def_dbc)
+    parser3 = dbcparser.DBCParser([def_dbc, def_dbc])
     assert parser3.get_canid_for_signal('SteeringAngle129') == 297
     assert parser3.get_canid_for_signal('DI_uiSpeed') == 599

dbc2val/test/test_dbc/test_kcd.py

@@ -27,15 +27,15 @@
 
 def test_single_kcd():
 
-    parser = dbcparser.DBCParser(test_path + "/test1_1.kcd")
+    parser = dbcparser.DBCParser([test_path + "/test1_1.kcd"])
     assert parser.get_canid_for_signal('DI_bmsRequestInterfaceVersion') == 0x16
 
 
 def test_split_kcd():
     """
     This verifies that we can read multiple KCD files.
     """
-    parser = dbcparser.DBCParser(test_path + "/test1_1.kcd," + test_path + "/test1_2.kcd")
+    parser = dbcparser.DBCParser([test_path + "/test1_1.kcd", test_path + "/test1_2.kcd"])
     assert parser.get_canid_for_signal('DI_bmsRequestInterfaceVersion') == 0x16
     assert parser.get_canid_for_signal('SteeringAngle129') == 0x129
 
@@ -44,7 +44,7 @@ def test_mixed_file_types():
     """
     This verifies that we can read files of different type.
     """
-    parser = dbcparser.DBCParser(test_path + "/test1_1.kcd," + test_path + "/test1_2.dbc")
+    parser = dbcparser.DBCParser([test_path + "/test1_1.kcd", test_path + "/test1_2.dbc"])
     assert parser.get_canid_for_signal('DI_bmsRequestInterfaceVersion') == 0x16
     assert parser.get_canid_for_signal('SteeringAngle129') == 0x129
 
@@ -53,5 +53,5 @@ def test_duplicated_dbc():
     """
     Load original KCD multiple times
     """
-    parser = dbcparser.DBCParser(test_path + "/test1_1.kcd," + test_path + "/test1_1.kcd")
+    parser = dbcparser.DBCParser([test_path + "/test1_1.kcd", test_path + "/test1_1.kcd"])
     assert parser.get_canid_for_signal('DI_bmsRequestInterfaceVersion') == 0x16

dbc2val/test/test_example_mapping/test_example_mapping.py

@@ -25,7 +25,7 @@
 # read config only once
 test_path = os.path.dirname(os.path.abspath(__file__))
 mapping_path = test_path + "/../../mapping/vss_4.0/vss_dbc.json"
-parser = dbcparser.DBCParser(test_path + "/../../Model3CAN.dbc")
+parser = dbcparser.DBCParser([test_path + "/../../Model3CAN.dbc"])
 mapper: dbc2vssmapper.Mapper = dbc2vssmapper.Mapper(mapping_path, parser)