From 3bd9339baff9698574669af24a2be4c495aea2ee Mon Sep 17 00:00:00 2001 From: deeaitch Date: Thu, 1 Oct 2026 13:23:01 -0400 Subject: [PATCH] Closes #3 Validate DBC frame definitions and report error locations --- README.md | 33 +++++++++++++- dbc_parser.cpp | 95 ++++++++++++++++++++++++--------------- dbc_parser.h | 1 + tests/README.md | 19 ++++++++ tests/dbc_parser_test.cpp | 94 ++++++++++++++++++++++++++++++++++++++ tests/dbc_parser_test.pro | 7 +++ 6 files changed, 213 insertions(+), 36 deletions(-) create mode 100644 tests/dbc_parser_test.cpp create mode 100644 tests/dbc_parser_test.pro diff --git a/README.md b/README.md index 4655cfd..736b118 100644 --- a/README.md +++ b/README.md @@ -505,6 +505,36 @@ Parsed fields: --- +### Frame validation and parse errors + +`BO_` definitions require a decimal CAN ID, a nonempty single-token frame name, +a colon, a decimal payload length, and a transmitter token. Spaces or tabs may +separate fields, and whitespace before the colon is accepted. Additional trailing +fields are rejected. + +Numeric fields must contain decimal digits only; signs, hexadecimal notation, +fractional values, suffixes, and overflow are rejected. CAN IDs may contain the +29-bit identifier plus the optional DBC extended flag (bit 31). Bits 29 and 30 +are rejected rather than silently discarded. For compatibility, unflagged IDs +above `0x7FF` are still treated as extended. + +The payload length must fit `uint8_t` (`0..255`). This is a storage-range check, +not CAN or CAN FD protocol validation; no new 8-byte or 64-byte limit is imposed. + +Parsing stops at the first detected error. Exceptions raised while parsing a +line are reported as `std::runtime_error` with the supplied file path and the +one-based physical line number, including blank lines: + +```text +example.dbc:3: Invalid CAN ID: invalid +``` + +This change validates frame definitions. Signal syntax and unsupported DBC +constructs retain their existing behavior; comprehensive validation is still +planned. File-open errors contain the path but have no line number. + +--- + ## Signal definition Example: @@ -871,5 +901,6 @@ This gives FrameTap a much better foundation for real use, because both browsing ## Regression tests See [tests/README.md](tests/README.md) for compiler and qmake commands. -Tests cover Intel and Motorola extraction, signed and unsigned 64-bit boundaries, +Tests cover frame validation and error locations, Intel and Motorola extraction, +signed and unsigned 64-bit boundaries, physical conversion, numeric output, and invalid signal payloads. diff --git a/dbc_parser.cpp b/dbc_parser.cpp index 6519ff8..edb1141 100644 --- a/dbc_parser.cpp +++ b/dbc_parser.cpp @@ -15,8 +15,21 @@ #include #include #include +#include +#include namespace { + std::uint32_t ParseDecimal (const std::string &text, + std::uint32_t maximum, + const char *field) { + std::uint32_t value = 0U; + const auto parsed = std::from_chars (text.data(), text.data() + text.size(), value); + if (text.empty() || parsed.ec != std::errc() || + parsed.ptr != text.data() + text.size() || value > maximum) + throw std::runtime_error (std::string ("Invalid ") + field + ": " + text); + return value; + } + /** * @brief Remove leading and trailing spaces. * @param text Input text. @@ -45,31 +58,39 @@ DbcDatabase DbcParser::ParseFile (const std::string &filePath) const { DbcDatabase database; FrameInfo *currentFrame = nullptr; + std::size_t lineNumber = 0U; std::string line; while (std::getline (input, line)) { + ++lineNumber; line = Trim (line); if (line.empty()) continue; - if (IsFrameLine (line)) { - FrameInfo frame = ParseFrameLine (line); - database.frames.push_back (frame); - currentFrame = &database.frames.back(); - } else if (IsSignalLine (line)) { - if (currentFrame == nullptr) - throw std::runtime_error ("Signal found before any frame definition."); + try { + if (IsFrameLine (line)) { + FrameInfo frame = ParseFrameLine (line); + database.frames.push_back (frame); + currentFrame = &database.frames.back(); + } else if (IsSignalLine (line)) { + if (currentFrame == nullptr) + throw std::runtime_error ("Signal found before any frame definition."); - SignalInfo signal = ParseSignalLine (line); - currentFrame->signals.push_back (signal); - } else if (IsCommentLine (line)) - ParseCommentLine (line, database); + SignalInfo signal = ParseSignalLine (line); + currentFrame->signals.push_back (signal); + } else if (IsCommentLine (line)) + ParseCommentLine (line, database); + } catch (const std::exception &error) { + throw std::runtime_error (filePath + ":" + std::to_string (lineNumber) + + ": " + error.what()); + } } return database; } bool DbcParser::IsFrameLine (const std::string &line) { - return (line.size() >= 4U) && (line.compare (0U, 4U, "BO_ ") == 0); + return (line.compare (0U, 3U, "BO_") == 0) && + ((line.size() == 3U) || std::isspace (static_cast (line[3U]))); } bool DbcParser::IsSignalLine (const std::string &line) { @@ -146,33 +167,37 @@ FrameInfo DbcParser::ParseFrameLine (const std::string &line) { */ std::istringstream stream (line); - std::string token; + std::string keyword; + std::string idText; + if (!(stream >> keyword >> idText) || keyword != "BO_") + throw std::runtime_error ("Missing CAN ID in frame definition."); + + const std::uint32_t rawCanId = ParseDecimal ( + idText, std::numeric_limits::max(), "CAN ID"); + // Only bit 31 may accompany the actual 29-bit identifier. + if ((rawCanId & 0x60000000U) != 0U) + throw std::runtime_error ("CAN ID exceeds the 29-bit identifier range."); + + std::string nameText; + if (!std::getline (stream, nameText, ':') || stream.eof()) + throw std::runtime_error ("Missing ':' in frame definition."); + std::istringstream nameStream (nameText); FrameInfo frame; - std::uint32_t rawCanId = 0U; + std::string extra; + if (!(nameStream >> frame.name) || (nameStream >> extra)) + throw std::runtime_error ("Missing or invalid frame name."); - stream >> token; - if (token != "BO_") - throw std::runtime_error ("Invalid frame line: " + line); + std::string lengthText; + if (!(stream >> lengthText)) + throw std::runtime_error ("Missing payload length in frame definition."); + frame.dlc = static_cast (ParseDecimal ( + lengthText, std::numeric_limits::max(), "payload length")); + if (!(stream >> frame.transmitter)) + throw std::runtime_error ("Missing transmitter in frame definition."); + if (stream >> extra) + throw std::runtime_error ("Unexpected trailing field in frame definition: " + extra); - stream >> rawCanId; NormalizeCanId (rawCanId, frame.canId, frame.isExtended); - - stream >> token; - if (token.empty()) - throw std::runtime_error ("Missing frame name: " + line); - - if (token[token.size() - 1U] == ':') - token.erase (token.size() - 1U, 1U); - - frame.name = token; - - { - unsigned int dlcValue = 0U; - stream >> dlcValue; - frame.dlc = static_cast (dlcValue); - } - - stream >> frame.transmitter; frame.pgn = TryExtractPgn (frame.canId, frame.isExtended, frame.hasPgn); return frame; diff --git a/dbc_parser.h b/dbc_parser.h index 4d621aa..dc3f360 100644 --- a/dbc_parser.h +++ b/dbc_parser.h @@ -39,6 +39,7 @@ class DbcParser { * @param filePath Path to input file. * @return Parsed database. * @throws std::runtime_error on file or parse errors. + * Line parse errors include the file path and one-based line number. */ DbcDatabase ParseFile (const std::string &filePath) const; diff --git a/tests/README.md b/tests/README.md index df46ee5..bd5a2de 100644 --- a/tests/README.md +++ b/tests/README.md @@ -26,3 +26,22 @@ signals, signed values, scaling, 64-bit extraction, signed/unsigned boundary val (including `INT64_MIN`, `INT64_MAX`, and `UINT64_MAX`), numeric output, invalid lengths, insufficient payloads, and Intel regression cases. Frame validity means the definition was found; insufficient payloads are checked through signal validity. + +## Parser regression tests + +Run from the repository root: + +```sh +g++ -std=c++17 -Wall -Wextra -Wpedantic -Werror -I. \ + tests/dbc_parser_test.cpp dbc_parser.cpp -o /tmp/dbc_parser_test +/tmp/dbc_parser_test +``` + +For qmake, use `tests/dbc_parser_test.pro` in a separate build directory, +following the decoder instructions above, then run `./dbc_parser_test`. + +Parser tests cover standard and extended CAN IDs, whitespace, payload storage +boundaries, missing fields, malformed and overflowing numbers, reserved ID bits, +error paths and line numbers, and integration with signals and comments. +Temporary DBC fixtures are created in the system temporary directory and removed +automatically. diff --git a/tests/dbc_parser_test.cpp b/tests/dbc_parser_test.cpp new file mode 100644 index 0000000..2a30ab0 --- /dev/null +++ b/tests/dbc_parser_test.cpp @@ -0,0 +1,94 @@ +#include "dbc_parser.h" + +#include +#include +#include +#include +#include + +struct Fixture { + std::filesystem::path path = std::filesystem::temp_directory_path() / + ("dbc_parser_test_" + std::to_string ( + std::chrono::steady_clock::now().time_since_epoch().count()) + ".dbc"); + ~Fixture() { std::error_code error; std::filesystem::remove (path, error); } + void Write (const std::string &text) const { + std::ofstream output (path); + output << text; + if (!output) + throw std::runtime_error ("Cannot write test fixture."); + } +}; + +int main() { + Fixture fixture; + int failures = 0; + std::size_t count = 0U; + const auto check = [&] (bool passed, const std::string &name) { + ++count; + if (!passed) { ++failures; std::cerr << "FAIL: " << name << '\n'; } + }; + struct ValidCase { const char *line; std::uint32_t id; bool extended; unsigned length; }; + const ValidCase valid[] = { + {"BO_ 0 Frame: 0 ECU", 0U, false, 0U}, + {"BO_ 2047 Frame: 8 ECU", 2047U, false, 8U}, + {"BO_ 2048 Frame: 64 ECU", 2048U, true, 64U}, + {"BO_ 536870911 Frame: 255 ECU", 0x1FFFFFFFU, true, 255U}, + {"BO_ 2147483649 Frame: 8 ECU", 1U, true, 8U}, + {"BO_ 2684354559 Frame: 8 ECU", 0x1FFFFFFFU, true, 8U}, + {" BO_\t256\tFrame :\t8\tECU \r", 256U, false, 8U} + }; + for (const auto &test : valid) { + fixture.Write (std::string (test.line) + '\n'); + try { + const auto database = DbcParser().ParseFile (fixture.path.string()); + check (database.frames.size() == 1U && database.frames[0].name == "Frame" && + database.frames[0].canId == test.id && + database.frames[0].isExtended == test.extended && + database.frames[0].dlc == test.length && + database.frames[0].transmitter == "ECU", test.line); + } catch (const std::exception &error) { check (false, error.what()); } + } + const char *invalid[] = { + "BO_ invalid", "BO_", "BO_ 1", "BO_ 1 : 8 ECU", "BO_ 1 Frame 8 ECU", + "BO_ 1 Frame:", "BO_ 1 Frame: 8", "BO_ 1 Frame: 8 ECU extra", + "BO_ 1 Two Names: 8 ECU", "BO_ -1 Frame: 8 ECU", "BO_ +1 Frame: 8 ECU", + "BO_ 1x Frame: 8 ECU", "BO_ 0x123 Frame: 8 ECU", "BO_ 1.5 Frame: 8 ECU", + "BO_ 4294967296 Frame: 8 ECU", "BO_ 999999999999999999999 Frame: 8 ECU", + "BO_ 536870912 Frame: 8 ECU", "BO_ 1073741824 Frame: 8 ECU", + "BO_ 2684354560 Frame: 8 ECU", "BO_ 4294967295 Frame: 8 ECU", + "BO_ 1 Frame: -1 ECU", "BO_ 1 Frame: +8 ECU", "BO_ 1 Frame: 8x ECU", + "BO_ 1 Frame: 8.5 ECU", "BO_ 1 Frame: 256 ECU", + "BO_ 1 Frame: 4294967296 ECU", "BO_\tinvalid" + }; + for (const auto *line : invalid) { + fixture.Write (std::string ("VERSION \"test\"\n\n") + line + '\n'); + try { + DbcParser().ParseFile (fixture.path.string()); + check (false, line); + } catch (const std::runtime_error &error) { + const std::string prefix = fixture.path.string() + ":3: "; + check (std::string (error.what()).find (prefix) == 0U && + std::string (error.what()).size() > prefix.size(), line); + } + } + fixture.Write ( + "BO_TX_BU_ 256 : ECU;\nBO_ 256 Frame: 8 ECU\n" + " SG_ Speed : 0|16@1+ (0.5,1) [0|100] \"rpm\" ECU\n" + "CM_ BO_ 256 \"Frame comment\";\nCM_ SG_ 256 Speed \"Signal comment\";\n" + "BO_ 257 Second: 8 ECU\n"); + const auto database = DbcParser().ParseFile (fixture.path.string()); + check (database.frames.size() == 2U && database.frames[0].signals.size() == 1U && + database.frames[0].comment == "Frame comment" && + database.frames[0].signals[0].comment == "Signal comment", "Integration"); + fixture.Write ("\nSG_ Speed : invalid\n"); + try { + DbcParser().ParseFile (fixture.path.string()); + check (false, "Signal error location"); + } catch (const std::runtime_error &error) { + check (std::string (error.what()).find (fixture.path.string() + ":2: ") == 0U, + "Signal error location"); + } + if (failures != 0) return 1; + std::cout << "All " << count << " parser cases passed.\n"; + return 0; +} diff --git a/tests/dbc_parser_test.pro b/tests/dbc_parser_test.pro new file mode 100644 index 0000000..6b6970f --- /dev/null +++ b/tests/dbc_parser_test.pro @@ -0,0 +1,7 @@ +TEMPLATE = app +TARGET = dbc_parser_test +CONFIG += console c++17 +CONFIG -= app_bundle qt +INCLUDEPATH += .. +SOURCES += dbc_parser_test.cpp ../dbc_parser.cpp +HEADERS += ../dbc_parser.h ../dbc_database.h ../frame_info.h ../signal_info.h