From 69074e33214f3df29c950ec802bfe43c2e2abfdd Mon Sep 17 00:00:00 2001 From: deeaitch Date: Thu, 1 Oct 2026 13:09:57 -0400 Subject: [PATCH] Fix #2 Preserve unsigned 64-bit decoded values --- README.md | 34 +++++++++++++++++++++ dbc_decoder.cpp | 19 +++++++----- dbc_decoder.h | 5 ++-- main.cpp | 5 ++-- tests/README.md | 3 +- tests/dbc_decoder_test.cpp | 61 ++++++++++++++++++++++++++++++++++++-- 6 files changed, 112 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index dde0824..4655cfd 100644 --- a/README.md +++ b/README.md @@ -414,6 +414,34 @@ Fields: --- +### Raw value representation and precision + +`DecodedSignalValue::rawValue` is a C++17 +`std::variant`. For a valid signal, signed definitions +produce the `int64_t` alternative and unsigned definitions produce the `uint64_t` +alternative. Raw values remain exact across their full 64-bit ranges. Check +`valid` before using the value; an invalid result's default value is not decoded data. + +Use `std::get` when the numeric type is known, or `std::visit` to handle both: + +```cpp +std::visit([] (auto raw) { std::cout << raw; }, decodedSignal.rawValue); +``` + +This replaces the previous `int64_t` field: consumers that read or print +`rawValue` directly must handle the variant. + +Physical conversion uses the active numeric type: +`physicalValue = static_cast(raw) * factor + offset`. +`physicalValue` remains a `double` and is approximate for large integers. +On usual IEEE 754 binary64 implementations, integers are exactly representable +through 2^53; above that, some integers round to the same floating-point value. +For example, unsigned `UINT64_MAX` remains exact in `rawValue`, but converts to +approximately 18446744073709551616 in binary64 before scaling. +Use `rawValue` whenever exact integer values are required. + +--- + ## `DecodedFrameValue` Represents one decoded frame result. @@ -839,3 +867,9 @@ DBC parser → DbcDatabase → DecodeDatabase → DbcDecoder ``` This gives FrameTap a much better foundation for real use, because both browsing and decoding are supported without forcing one representation to do the other's job. + +## 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, +physical conversion, numeric output, and invalid signal payloads. diff --git a/dbc_decoder.cpp b/dbc_decoder.cpp index 1490152..c1faaa1 100644 --- a/dbc_decoder.cpp +++ b/dbc_decoder.cpp @@ -55,10 +55,12 @@ DecodedFrameValue DbcDecoder::Decode (const DecodeDatabase &database, if (signal.valueType == ValueType::Signed) decoded.rawValue = SignExtend (unsignedValue, signal.length); else - decoded.rawValue = static_cast (unsignedValue); + decoded.rawValue = unsignedValue; decoded.physicalValue = - (static_cast (decoded.rawValue) * signal.factor) + signal.offset; + std::visit ([&signal] (auto rawValue) { + return (static_cast (rawValue) * signal.factor) + signal.offset; + }, decoded.rawValue); decoded.valid = true; result.signals.push_back (decoded); @@ -137,16 +139,17 @@ bool DbcDecoder::ExtractMotorola (const std::vector &data, } std::int64_t DbcDecoder::SignExtend (std::uint64_t value, std::uint32_t bitLength) { - if ((bitLength == 0U) || (bitLength >= 64U)) - return static_cast (value); - - const std::uint64_t signMask = (static_cast (1U) << (bitLength - 1U)); - const std::uint64_t valueMask = (static_cast (1U) << bitLength) - 1U; + if ((bitLength == 0U) || (bitLength > 64U)) + return 0; + const std::uint64_t signMask = std::uint64_t (1U) << (bitLength - 1U); + const std::uint64_t valueMask = (bitLength == 64U) + ? ~std::uint64_t (0U) : (std::uint64_t (1U) << bitLength) - 1U; value &= valueMask; if ((value & signMask) == 0U) return static_cast (value); - return static_cast (value | (~valueMask)); + // The complemented magnitude fits int64_t, including for INT64_MIN. + return -1 - static_cast ((~value) & valueMask); } diff --git a/dbc_decoder.h b/dbc_decoder.h index 2629a3c..6b7a3ef 100644 --- a/dbc_decoder.h +++ b/dbc_decoder.h @@ -14,6 +14,7 @@ #include #include +#include #include "decode_database.h" @@ -37,13 +38,13 @@ struct RawCanFrame { */ struct DecodedSignalValue { const DecodeSignal *definition; /**< Signal definition. */ - std::int64_t rawValue; /**< Extracted raw integer value. */ + std::variant rawValue; /**< Exact signed or unsigned raw value. */ double physicalValue; /**< Converted physical value. */ bool valid; /**< true if decoding succeeded. */ DecodedSignalValue() : definition (nullptr) - , rawValue (0) + , rawValue (std::uint64_t (0U)) , physicalValue (0.0) , valid (false) { } diff --git a/main.cpp b/main.cpp index aa70188..6656657 100644 --- a/main.cpp +++ b/main.cpp @@ -87,8 +87,9 @@ static void PrintDecodedFrame (const DecodedFrameValue &decoded) { continue; std::cout << " " << signal.definition->name - << " raw=" << signal.rawValue - << " physical=" << signal.physicalValue + << " raw="; + std::visit ([] (auto rawValue) { std::cout << rawValue; }, signal.rawValue); + std::cout << " physical=" << signal.physicalValue << " " << signal.definition->unit << "\n"; } diff --git a/tests/README.md b/tests/README.md index 47fd998..df46ee5 100644 --- a/tests/README.md +++ b/tests/README.md @@ -22,6 +22,7 @@ The executable returns a nonzero exit code if any case fails. Checks remain active in release builds and do not depend on `assert` or an external test framework. Coverage includes Motorola bit numbering within and across bytes, unaligned -signals, signed values, scaling, 64-bit extraction, invalid lengths, insufficient +signals, signed values, scaling, 64-bit extraction, signed/unsigned boundary values +(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. diff --git a/tests/dbc_decoder_test.cpp b/tests/dbc_decoder_test.cpp index 51f8450..b16be89 100644 --- a/tests/dbc_decoder_test.cpp +++ b/tests/dbc_decoder_test.cpp @@ -2,6 +2,7 @@ #include #include +#include struct DecodeCase { const char *name; @@ -83,15 +84,71 @@ int main() { (result.signals.size() == 1U) && (result.signals[0].definition == &database.frames[0].signals[0]) && (result.signals[0].valid == test.valid) && - (!test.valid || ((result.signals[0].rawValue == test.rawValue) && + (!test.valid || (((test.valueType == ValueType::Signed) + ? (std::get_if (&result.signals[0].rawValue) && + std::get (result.signals[0].rawValue) == test.rawValue) + : (std::get_if (&result.signals[0].rawValue) && + std::get (result.signals[0].rawValue) == + static_cast (test.rawValue))) && (result.signals[0].physicalValue == test.physicalValue))); if (!passed) { std::cerr << "FAIL: " << test.name << '\n'; ++failures; } } + std::size_t boundaryCases = 0U; + for (ByteOrder order : {ByteOrder::Intel, ByteOrder::Motorola}) { + for (ValueType type : {ValueType::Unsigned, ValueType::Signed}) { + for (std::uint64_t bits : {std::uint64_t (0U), std::uint64_t (0x7FFFFFFFFFFFFFFFULL), + std::uint64_t (0x8000000000000000ULL), + std::uint64_t (0x8000000000000001ULL), + std::uint64_t (0xFFFFFFFFFFFFFFFFULL)}) { + DecodeDatabase database; + DecodeFrame definition; + DecodeSignal signal; + signal.startBit = order == ByteOrder::Intel ? 0U : 7U; + signal.length = 64U; + signal.byteOrder = order; + signal.valueType = type; + signal.factor = 0.5; + signal.offset = 10.0; + definition.signals.push_back (signal); + database.frames.push_back (definition); + database.frameIndexByKey.emplace (FrameKey (0U, false), 0U); + RawCanFrame frame; + for (unsigned byte = 0U; byte < 8U; ++byte) { + const unsigned shift = (order == ByteOrder::Intel ? byte : 7U - byte) * 8U; + frame.data.push_back (static_cast (bits >> shift)); + } + const auto result = DbcDecoder().Decode (database, frame); + const std::int64_t signedExpected = bits <= 0x7FFFFFFFFFFFFFFFULL + ? static_cast (bits) + : std::numeric_limits::min() + + static_cast (bits - 0x8000000000000000ULL); + const auto &decoded = result.signals.at (0U); + const auto expected = type == ValueType::Unsigned + ? std::variant (bits) + : std::variant (signedExpected); + const double physical = (type == ValueType::Unsigned + ? static_cast (bits) : static_cast (signedExpected)) * 0.5 + 10.0; + std::ostringstream actualText; + std::ostringstream expectedText; + std::visit ([&actualText] (auto raw) { actualText << raw; }, decoded.rawValue); + if (type == ValueType::Unsigned) + expectedText << bits; + else + expectedText << signedExpected; + if (!decoded.valid || decoded.rawValue != expected || + decoded.physicalValue != physical || actualText.str() != expectedText.str()) { + std::cerr << "FAIL: 64-bit boundary " << bits << '\n'; + ++failures; + } + ++boundaryCases; + } + } + } if (failures != 0) return 1; - std::cout << "All " << sizeof (cases) / sizeof (cases[0]) << " decoder cases passed.\n"; + std::cout << "All " << sizeof (cases) / sizeof (cases[0]) + boundaryCases << " decoder cases passed.\n"; return 0; }