From 8bf5dcff301921c72a6637892fa9196d834aa9a8 Mon Sep 17 00:00:00 2001 From: deeaitch Date: Wed, 7 Oct 2026 18:59:18 -0400 Subject: [PATCH] Closes #12 --- changelog.md | 8 +++ core/dbc_parser.h | 2 + core/dbc_statement_handler.cpp | 9 ++- tests/README.md | 3 + tests/dbc_parser_test.cpp | 107 ++++++++++++++++++++++++++++++--- 5 files changed, 120 insertions(+), 9 deletions(-) diff --git a/changelog.md b/changelog.md index d4729bb..4b7f339 100644 --- a/changelog.md +++ b/changelog.md @@ -24,6 +24,14 @@ existing Git tag; subsequent changes are listed as unreleased. - Overlapping unconditional signals are now rejected during parsing. +#### Fixed + +- Reject duplicate frames by normalized CAN ID and frame type, including + equivalent extended ID encodings, and duplicate signal names within a frame. + Errors report the file path and line of the repeated definition. Standard and + extended frames may share a numeric ID; separate frames may reuse signal names. + Added regression coverage and updated parser documentation. + ### 2026-10-06 #### Added diff --git a/core/dbc_parser.h b/core/dbc_parser.h index fcf3322..8cfaac2 100644 --- a/core/dbc_parser.h +++ b/core/dbc_parser.h @@ -39,6 +39,8 @@ public: * @return Parsed database. * @throws std::runtime_error on file or parse errors. * Line parse errors include the file path and one-based line number. + * Duplicate normalized CAN IDs of the same frame type and duplicate signal + * names within a frame are rejected at the repeated definition's line. */ DbcDatabase ParseFile(const std::string &filePath) const; diff --git a/core/dbc_statement_handler.cpp b/core/dbc_statement_handler.cpp index 44d8cc2..d4a4ab5 100644 --- a/core/dbc_statement_handler.cpp +++ b/core/dbc_statement_handler.cpp @@ -340,7 +340,11 @@ namespace { }// namespace void DbcFrameHandler::Handle(const std::string &line, DbcParseContext &context) const { - context.database.frames.push_back(ParseFrameLine(line)); + FrameInfo frame = ParseFrameLine(line); + if (FindFrameById(context.database, frame.canId, frame.isExtended) != nullptr) + throw std::runtime_error("Duplicate " + std::string(frame.isExtended ? "extended" : "standard") + + " frame CAN ID: " + std::to_string(frame.canId)); + context.database.frames.push_back(std::move(frame)); context.currentFrame = &context.database.frames.back(); context.signalLines.emplace_back(); } @@ -349,6 +353,9 @@ void DbcSignalHandler::Handle(const std::string &line, DbcParseContext &context) if (context.currentFrame == nullptr) throw std::runtime_error("Signal found before any frame definition."); SignalInfo signal = ParseSignalLine(line); + if (FindSignalByName(*context.currentFrame, signal.name) != nullptr) + throw std::runtime_error("Duplicate signal name '" + signal.name + + "' in frame '" + context.currentFrame->name + "'."); const std::uint64_t payloadBits = static_cast(context.currentFrame->dlc) * 8U; std::uint64_t bit = signal.startBit; for (std::uint32_t index = 0U; index < signal.length; ++index) { diff --git a/tests/README.md b/tests/README.md index 799c761..ec060ab 100644 --- a/tests/README.md +++ b/tests/README.md @@ -47,6 +47,9 @@ found; insufficient payloads are checked through signal validity. 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. +Duplicate definitions are rejected at their source lines, including equivalent +extended ID encodings and repeated signal names within a frame. Standard and +extended frames may share a numeric ID, and separate frames may reuse signal names. Signal tests cover complete syntax and numeric validation, scientific notation, finite physical parameters and ordered ranges, spaces and tabs, receiver lists, unsupported multiplexing, and Intel/Motorola declared-payload boundaries. diff --git a/tests/dbc_parser_test.cpp b/tests/dbc_parser_test.cpp index dbea4e0..ae7414e 100644 --- a/tests/dbc_parser_test.cpp +++ b/tests/dbc_parser_test.cpp @@ -42,8 +42,8 @@ public: std::string SignalLine(const std::string &start, const std::string &length, const std::string &order, const std::string &factor, const std::string &offset, const std::string &minimum, - const std::string &maximum) { - return "SG_ Value : " + start + "|" + length + "@" + order + + const std::string &maximum, const std::string &name = "Value") { + return "SG_ " + name + " : " + start + "|" + length + "@" + order + " (" + factor + "," + offset + ") [" + minimum + "|" + maximum + "] \"\" ECU"; } @@ -67,6 +67,9 @@ private Q_SLOTS: void multiplexingBoundaries(); void validFrames(); void invalidFrames(); + void duplicateFrames(); + void duplicateSignals(); + void distinctDefinitions(); void integration(); void invalidSignals(); void validSignals(); @@ -134,6 +137,93 @@ void DbcParserTest::invalidFrames() { } } +void DbcParserTest::duplicateFrames() { + Fixture fixture; + struct DuplicateCase { + const char *firstId; + const char *duplicateId; + const char *message; + }; + const DuplicateCase cases[] = { + {"1", "1", "Duplicate standard frame CAN ID: 1"}, + {"2147483649", "2147483649", "Duplicate extended frame CAN ID: 1"}, + {"2048", "2147485696", "Duplicate extended frame CAN ID: 2048"}, + {"2147485696", "2048", "Duplicate extended frame CAN ID: 2048"}, + {"536870911", "2684354559", "Duplicate extended frame CAN ID: 536870911"} + }; + const DbcParser parser; + for (const DuplicateCase &test : cases) { + fixture.Write("BO_ " + std::string(test.firstId) + " First: 8 ECU\n" + "BO_ 2 Other: 8 ECU\n\n" + " BO_\t" + test.duplicateId + "\tSecond: 8 ECU\n"); + try { + parser.ParseFile(fixture.path.string()); + CHECK(false, "Duplicate frame must be rejected"); + } catch (const std::runtime_error &error) { + QCOMPARE(std::string(error.what()), fixture.path.string() + ":4: " + test.message); + } + } + fixture.Write("BO_ 1 First: 8 ECU\n"); + QCOMPARE(parser.ParseFile(fixture.path.string()).frames.size(), std::size_t(1U)); +} + +void DbcParserTest::duplicateSignals() { + Fixture fixture; + const std::string first = "SG_ State : 0|8@1+ (1,0) [0|255] \"\" ECU\n"; + const std::string duplicates[] = { + first, + " SG_\tState : 16|8@1+ (2,1) [0|511] \"V\" ECU\n", + "SG_ State m1 : 16|8@1+ (1,0) [0|255] \"\" ECU\n" + }; + for (const std::string &duplicate : duplicates) { + fixture.Write("BO_ 1 Frame: 8 ECU\n" + first + + "SG_ Other : 8|8@1+ (1,0) [0|255] \"\" ECU\n\n" + duplicate); + try { + DbcParser().ParseFile(fixture.path.string()); + CHECK(false, "Duplicate signal must be rejected"); + } catch (const std::runtime_error &error) { + QCOMPARE(std::string(error.what()), fixture.path.string() + + ":5: Duplicate signal name 'State' in frame 'Frame'."); + } + } +} + +void DbcParserTest::distinctDefinitions() { + Fixture fixture; + fixture.Write("BO_ 1 Shared: 1 ECU\n" + "SG_ State : 0|8@1+ (1,0) [0|255] \"\" ECU\n" + "BO_ 2147483649 Shared: 1 ECU\n" + "SG_ State : 0|8@1+ (1,0) [0|255] \"\" ECU\n" + "BO_ 2 Shared: 2 ECU\n" + "SG_ State : 0|8@1+ (1,0) [0|255] \"\" ECU\n" + "SG_ state : 8|8@1+ (1,0) [0|255] \"\" ECU\n" + "CM_ BO_ 1 \"Standard\";\n" + "CM_ BO_ 2147483649 \"Extended\";\n" + "CM_ SG_ 1 State \"Standard state\";\n" + "CM_ SG_ 2147483649 State \"Extended state\";\n"); + const DbcDatabase parsed = DbcParser().ParseFile(fixture.path.string()); + QCOMPARE(parsed.frames.size(), std::size_t(3U)); + QCOMPARE(parsed.frames[0].canId, parsed.frames[1].canId); + CHECK(!parsed.frames[0].isExtended && parsed.frames[1].isExtended, + "Standard and extended frames with the same numeric ID remain distinct"); + QCOMPARE(parsed.frames[0].comment, std::string("Standard")); + QCOMPARE(parsed.frames[1].comment, std::string("Extended")); + QCOMPARE(parsed.frames[0].signals[0].comment, std::string("Standard state")); + QCOMPARE(parsed.frames[1].signals[0].comment, std::string("Extended state")); + QCOMPARE(parsed.frames[2].signals.size(), std::size_t(2U)); + const DecodeDatabase runtime = DbcDecodeBuilder().Build(parsed); + RawCanFrame raw; + raw.canId = 1U; + raw.data = {7U}; + const DecodedFrameValue standard = DbcDecoder().Decode(runtime, raw); + QVERIFY(standard.valid); + QCOMPARE(standard.definition->comment, std::string("Standard")); + raw.isExtended = true; + const DecodedFrameValue extended = DbcDecoder().Decode(runtime, raw); + QVERIFY(extended.valid); + QCOMPARE(extended.definition->comment, std::string("Extended")); +} + void DbcParserTest::integration() { Fixture fixture; fixture.Write( @@ -324,7 +414,7 @@ void DbcParserTest::validSignals() { const DbcParser signalParser; fixture.Write("BO_ 1 Frame: 8 ECU\n" + SignalLine("0", "8", "1+", "0", "0", "0", "0") + "\n" + - SignalLine("8", "8", "1-", "-1", "0", "-255", "0") + "\n"); + SignalLine("8", "8", "1-", "-1", "0", "-255", "0", "Negative") + "\n"); const DbcDatabase factors = signalParser.ParseFile(fixture.path.string()); CHECK(factors.frames[0].signals.size() == 2U && factors.frames[0].signals[0].factor == 0.0 && @@ -506,17 +596,18 @@ void DbcParserTest::invalidValueDescriptions() { } } for (const std::string suffix: { - "BO_ 1 Other: 8 ECU\n", // Ambiguous normalized frame reference. - "SG_ State : 0|8@1+ (1,0) [0|255] \"\" ECU\n",// Ambiguous signal reference. + "BO_ 1 Other: 8 ECU\n", // Duplicate frame definition. + "SG_ State : 0|8@1+ (1,0) [0|255] \"\" ECU\n",// Duplicate signal definition. ""}) { fixture.Write("VAL_ 1 " + std::string(suffix.empty() ? "Missing" : "State") + " 0 \"Description\";\n" + definitions + suffix); try { DbcParser().ParseFile(fixture.path.string()); - CHECK(false, "Deferred missing or ambiguous reference"); + CHECK(false, "Missing reference or duplicate definition"); } catch (const std::runtime_error &error) { - CHECK(std::string(error.what()).find(fixture.path.string() + ":1: ") == 0, - "Deferred reference retains source location"); + const std::string line = suffix.empty() ? "1" : "4"; + CHECK(std::string(error.what()).find(fixture.path.string() + ":" + line + ": ") == 0, + "Error retains the reference or duplicate definition source location"); } } for (const std::string number: {"+1", "1x", "0x10", "1.5", "1e2", "--1",