Feature: testing + can-parse improvment for CHECKFRAME operations

can-parse CHECKFRAME now works for single frame check and whole DB checks. The CppCAN::analysis namespace contains now is_frame_layout_ok which can return a diagnosis of the errors that have been identified. assert_frame_layout() which was declared but not defined has seen its signature changed and its definition created.

Finally, testing has been added with different example DBC files: both BigEndian and LittleEndian signals.

Author: xatavian

CMakeLists.txt

@@ -2,8 +2,9 @@ cmake_minimum_required(VERSION 3.7.0)
 
 project(CPP-CAN-Parser)
 
-
 include(CTest)
+include(GenerateExportHeader)
+
 
 set(CPPPARSER_INCLUDE_DIRECTORY 
 	${CMAKE_CURRENT_LIST_DIR}/include
@@ -12,7 +13,7 @@ set(CPPPARSER_INCLUDE_DIRECTORY
 set(CPPPARSER_SRC_FILES
 	src/models/CANDatabase.cpp
 	src/models/CANFrame.cpp
-  src/models/CANSignal.cpp
+  	src/models/CANSignal.cpp
 	src/parsing/DBCParser.cpp
 	src/parsing/ParsingUtils.cpp
 	src/parsing/Tokenizer.cpp
@@ -22,16 +23,51 @@ set(CPP_CAN_PARSER_COMPILATION_TYPE SHARED)
 if(CPP_CAN_PARSER_USE_STATIC)
 	set(CPP_CAN_PARSER_COMPILATION_TYPE STATIC)
 endif()
-	
-add_library(cpp-can-parser 
+
+set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS True)
+add_library(cpp-can-parser
 	${CPP_CAN_PARSER_COMPILATION_TYPE} 
 	${CPPPARSER_SRC_FILES})
 target_include_directories(cpp-can-parser 
-	PUBLIC ${CPPPARSER_INCLUDE_DIRECTORY})
+	PUBLIC ${CPPPARSER_INCLUDE_DIRECTORY}
+		   ${CMAKE_CURRENT_BINARY_DIR}/exports/)
+generate_export_header(cpp-can-parser
+	BASE_NAME cpp_can_parser
+	EXPORT_FILE_NAME ${CMAKE_CURRENT_BINARY_DIR}/exports/cpp_can_parser_export.h)
+install(FILES cpp-can-parser
+		${CMAKE_CURRENT_BINARY_DIR}/cpp_can_parser_export.h
+		DESTINATION ${CMAKE_CURRENT_BINARY_DIR}/include)
 
 add_executable(can-parse 
 	utils/can-parse/can-parse.cpp
 	utils/can-parse/print-frame.cpp
 	utils/can-parse/print-single-frame.cpp
 	utils/can-parse/check-frame.cpp)
-target_link_libraries(can-parse cpp-can-parser)
+target_link_libraries(can-parse cpp-can-parser)
+
+if(BUILD_TESTING)
+	file(COPY tests/dbc-files/
+		DESTINATION dbc-files/)
+	
+	add_executable(cpc-test-parsing
+		tests/test-parsing.cpp)
+	target_link_libraries(cpc-test-parsing PUBLIC cpp-can-parser)
+
+	add_test(NAME cpc-test-parsing
+			COMMAND cpc-test-parsing)
+
+	add_test(NAME cpc-checkframe-1
+			 COMMAND can-parse checkframe dbc-files/single-frame-1.dbc)
+
+	add_test(NAME cpc-checkframe-2
+			 COMMAND can-parse checkframe dbc-files/single-frame-2.dbc)
+
+	add_test(NAME cpc-checkframe-big-endian-1
+			 COMMAND can-parse checkframe 294 dbc-files/big-endian-1.dbc)
+
+	add_test(NAME cpc-checkframe-big-endian-2
+			 COMMAND can-parse checkframe 1807 dbc-files/big-endian-1.dbc)
+	
+	add_test(NAME cpc-checkframe-big-endian-3
+			 COMMAND can-parse checkframe 1800 dbc-files/big-endian-1.dbc)
+endif()

include/CANDatabase.h

@@ -6,6 +6,7 @@
 #include <exception>
 #include <map>
 
+#include "cpp_can_parser_export.h"
 #include "CANFrame.h"
 #include "CANDatabaseException.h"
 
@@ -23,13 +24,13 @@
  * If the database was parsed from a file, the filename() method can be used to
  * retrieve the name of the source file.
  */
-class CANDatabase {
+class CPP_CAN_PARSER_EXPORT CANDatabase {
 public:
   /**
    * @brief A parsing warning and its location
    */
   struct parsing_warning {
-    int line;
+    unsigned long long line;
     std::string description;
   };
 
@@ -73,7 +74,7 @@ class CANDatabase {
   /**
    * @brief Creates a CANDatabase object with no source file
    */
-  CANDatabase() = default;
+  CANDatabase();
 
   /**
    * @brief Creates a CANDatabase object that has been constructed from a file
@@ -85,23 +86,24 @@ class CANDatabase {
    * Creates a copy of the database: the individual frames are deep copied so there is no
    * shared memory betwwen the two databases.
    */
-  CANDatabase(const CANDatabase&) = default;
+  CANDatabase(const CANDatabase&);
 
   /**
    * @brief Makes a copy of the given database
    */
-  CANDatabase& operator=(const CANDatabase&) = default;
+  CANDatabase& operator=(const CANDatabase&);
 
   /**
    * @brief Moves a CANDatabase object. The CANFrame objects are NOT deep copied.
    */
-  CANDatabase(CANDatabase&&) = default;
+  CANDatabase(CANDatabase&&);
 
   /**
-   * @see CANDatabase(const CANDatabase&&)
+   * @see CANDatabase(CANDatabase&&)
    */
-  CANDatabase& operator=(CANDatabase&&) = default;
+  CANDatabase& operator=(CANDatabase&&);
 
+  ~CANDatabase();
 public:
   /**
    * @brief Get the frame with the given frame name
@@ -195,11 +197,8 @@ class CANDatabase {
   void removeFrame(const std::string& name);
 
 private:
-  std::string filename_;
-  container_type map_; // Index by CAN ID
-
-  std::map<unsigned long long, IDKey> intKeyIndex_;
-  std::map<std::string, IDKey> strKeyIndex_;
+  class CANDatabaseImpl;
+  CANDatabaseImpl* impl;
 };
 
 #endif

include/CANDatabaseAnalysis.h

@@ -7,14 +7,24 @@ namespace CppCAN {
 namespace analysis {
     /**
      * @brief Analyses the frame signals to see if some signals are overlapping
+     * @param src The CANFrame instance to inspect
      * @return true if no overlapping is detected, false otherwise
      */
     bool is_frame_layout_ok(const CANFrame& src);
 
+    /**
+     * @brief Overload of is_frame_layout_ok() that outputs a diagnosis of the
+     *        problematic signals if a layout error is detected.
+     * @param src The CANFrame instance to inspect
+     * @param diagnosis Filled with the names of all the overlapping signals
+     * @return true if no overlapping is detected, false otherwise
+     */
+    bool is_frame_layout_ok(const CANFrame& src, std::vector<std::string>& diagnosis);
+
     /**
      * @brief Like is_frame_layout_ok() but throws a CANDatabaseException if the layout is ill-formed
      */
-    void assert_frame_layout();
+    void assert_frame_layout(const CANFrame& src);
 }    
 } // namespace CppCAN
 

src/analysis/CANFrameAnalysis.cpp

@@ -1,22 +1,33 @@
 #include "CANDatabaseAnalysis.h"
+#include "CANDatabaseException.h"
 #include <algorithm>
 #include <cmath>
 #include <tuple>
+#include <set>
 
-// For each entry: (byte, lr start bit, lr end bit)
+/**
+ * For each entry: byte, [lr_start_bit, lr_end_bit)
+ * Few explanations about the attributes:
+ * lr_start_bit: Start bit position in the byte (included)
+ * lr_end_bit: End bit position in the byte (excluded)
+ */
 struct SignalRange {
     uint8_t byte;
-    uint8_t lr_start_bit;
-    uint8_t lr_end_bit;
+    char lr_start_bit;
+    char lr_end_bit;
 };
 
 using SignalRanges = std::vector<SignalRange>;
 
-/**
- * Few explanations about the attributes:
- * lr_start_bit: Start bit position reading from left to right (ie bit 7 of byte 1 is 0, bit 7 of byte 2 is 9, ...)
- * lr_end_bit: same but for the end bit.
- */
+static void
+convert_to_big_endian_layout(SignalRanges& src) {
+    // src is assumed to be LittleEndian
+    for(SignalRange& r : src) {
+        r.lr_start_bit = 7 - r.lr_start_bit;
+        r.lr_end_bit = 7 - r.lr_end_bit;
+    }
+}
+
 struct SignalLayoutEntry {
     SignalLayoutEntry() = delete;
     SignalLayoutEntry(const CANSignal* src, SignalRanges&& r)
@@ -35,52 +46,56 @@ struct SignalLayoutEntry {
 
 SignalRanges big_endian_ranges(const CANSignal& src) {
     SignalRanges result;
-
+    
     // For BigEndian signals, the start bit already represents the left mostbit 
-    // of the signal. Therefore, it is only required to transform it into a "human-readable"
-    // value, ie. when looking at the frame from left to right.
-    // -----------------               -----------------
-    // |*|*|*|*|*|*|*|*|               |*|*|*|*|*|*|*|*|
-    // -----------------               -----------------
-    //  0             7   instead of    7             0
-    //
-    // Example: Start bit 4 becomes LR start bit 3
-    unsigned bitsAnalyzed = 0;
-    bool is_start_byte = 0;
+    // -----------------  -----------------
+    // |*|*|*|*|*|*|*|*|  |*|*|*|*|*|*|*|*|
+    // -----------------  -----------------
+    //  7             0    15            8
+    
+    unsigned bitsLeft = src.length();
+    unsigned currentPos = src.start_bit();
 
-    for(unsigned current_byte = src.start_bit() / 8; bitsAnalyzed < src.length(); current_byte++, is_start_byte = false) {
-        unsigned lbit = is_start_byte ? (7 - src.start_bit() % 8) : 0;
-        unsigned rbit = std::min(7u, src.start_bit() - bitsAnalyzed);
+    for(unsigned current_byte = src.start_bit() / 8; bitsLeft > 0; current_byte++) {
+        char lbit = currentPos % 8;
+        char rbit = std::max<char>(-1, lbit - bitsLeft);
 
         // The static_cast are not "necessary" but it removes some warnings
         result.push_back({ static_cast<uint8_t>(current_byte), 
-                           static_cast<uint8_t>(lbit),
-                           static_cast<uint8_t>(rbit) });
+                           lbit, rbit });
+        
+        bitsLeft -= lbit - rbit;
+        currentPos += (lbit - rbit); 
     }
 
     return result;
 }
 
 SignalRanges little_endian_ranges(const CANSignal& src) {
+    // For LittleEndian signals, act like the bits are reversed in the byte:
+    // ----------------- -----------------
+    // |*|*|*|*|*|*|*|*| |*|*|*|*|*|*|*|*|
+    // ----------------- -----------------
+    //  0             7   8             15    
+    // 
+    // The signal can be found from the start bit + read to the right.
     SignalRanges result;
 
-    // For LittleEndian signals, the start bit represents the LSB of the signal
-    // which does not really represent anything in terms of layout. So we need
-    // to compute the ranges for each byte individually anyway.
-
+    if(src.length() == 0) // length is 0, we return an empty result.
+        return result;
+    
     unsigned bitsLeft = src.length();
-    bool is_start_byte = 0;
-
-    for(unsigned current_byte = src.start_bit() / 8; bitsLeft > 0; current_byte++, is_start_byte = false) {
-        unsigned lbit = 7 -( src.start_bit() + bitsLeft);
-        unsigned rbit = 7 - src.start_bit();
+    unsigned currentPos = src.start_bit();
+    for(unsigned current_byte = src.start_bit() / 8; bitsLeft > 0; current_byte++) {
+        char lbit = currentPos % 8;
+        char rbit = std::min<char>(lbit + bitsLeft, 8);
 
         // The static_cast are not "necessary" but it removes some warnings
         result.push_back({ static_cast<uint8_t>(current_byte), 
-                           static_cast<uint8_t>(lbit),
-                           static_cast<uint8_t>(rbit) });
+                           lbit, rbit });
 
-        bitsLeft -= (rbit - lbit);
+        bitsLeft -= rbit - lbit;
+        currentPos += rbit - lbit;
     }
 
     return result;
@@ -91,14 +106,14 @@ std::vector<SignalLayoutEntry> compute_layout(const CANFrame& src) {
 
     for(const auto& signal: src) {
         const CANSignal& sig = signal.second; 
-        int lr_start_bit, lr_end_bit;
 
         if(sig.endianness() == CANSignal::BigEndian) {
             auto ranges = big_endian_ranges(sig);
             result.emplace_back(&sig, std::move(ranges));
         }
         else {
             auto ranges = little_endian_ranges(sig);
+            convert_to_big_endian_layout(ranges);
             result.emplace_back(&sig, std::move(ranges));
         }
     }
@@ -118,7 +133,7 @@ bool overlap(const SignalLayoutEntry& e1, const SignalLayoutEntry& e2) {
       // ordered.first is the leftmost SignalRange in the byte
       // ordered.second is the rightmost SignalRange in the byte
       auto ordered = std::minmax(r1, r2, [](const SignalRange& r, const SignalRange& rr) {
-          return r.lr_start_bit < rr.lr_start_bit;
+          return r.lr_start_bit > rr.lr_start_bit;
       });
 
       // No overlapping if the last bit of the leftmost is before the first
@@ -135,13 +150,43 @@ bool CppCAN::analysis::is_frame_layout_ok(const CANFrame& src) {
     auto layout = compute_layout(src);
 
     for(size_t i = 0; i < layout.size(); i++) {
-        const SignalLayoutEntry& e = layout[i];
-
         for(size_t j = i + 1; j < layout.size(); j++) {
-            if(overlap(layout[i], layout[j]))
+            if(overlap(layout[i], layout[j])) {
                 return false;            
+            }
         }
     }
 
     return true;
+}
+
+bool CppCAN::analysis::is_frame_layout_ok(const CANFrame& src, std::vector<std::string>& diagnosis) {
+    auto layout = compute_layout(src);
+    diagnosis.clear();
+
+    std::set<size_t> diagnosis_indices;
+    auto report_issue = [&diagnosis, &diagnosis_indices](size_t idx, const CANSignal& sig) {
+        if(diagnosis_indices.count(idx) == 0) {
+            diagnosis_indices.insert(idx);
+            diagnosis.push_back(sig.name());
+        }
+    };
+
+    for(size_t i = 0; i < layout.size(); i++) {
+        for(size_t j = i + 1; j < layout.size(); j++) {
+            if(overlap(layout[i], layout[j])) {
+                report_issue(i, *layout[i].src_signal);
+                report_issue(j, *layout[j].src_signal);
+            }        
+        }
+    }
+
+    return diagnosis_indices.size() == 0;
+}
+
+void CppCAN::analysis::assert_frame_layout(const CANFrame& src) {
+    if(!is_frame_layout_ok(src)) {
+        std::string text = "assert_frame_layout() failed for frame \"" + src.name() + "\"";
+        throw CANDatabaseException(text);
+    }
 }