From 5788c8aa338aebedd6dde081bf14c04070a37ad9 Mon Sep 17 00:00:00 2001 From: Tiago Lourinho Date: Wed, 2 Sep 2026 10:31:41 +0200 Subject: [PATCH 1/4] added constructor to can device configuration --- cmake/gtest.cmake | 1 + src/include/CanDeviceConfiguration.h | 14 ++++ src/main/CanDeviceConfiguration.cpp | 101 +++++++++++++++++++++++ src/main/CanVendorAnagate.cpp | 8 +- src/main/CanVendorSocketCan.cpp | 2 +- test/cpp/CanDeviceConfiguration_test.cpp | 95 +++++++++++++++++++++ test/cpp/CanDevice_test.cpp | 16 ++-- 7 files changed, 228 insertions(+), 9 deletions(-) create mode 100644 test/cpp/CanDeviceConfiguration_test.cpp diff --git a/cmake/gtest.cmake b/cmake/gtest.cmake index 3b0b8dfd..107c1559 100644 --- a/cmake/gtest.cmake +++ b/cmake/gtest.cmake @@ -12,6 +12,7 @@ FetchContent_MakeAvailable(googletest) # Add test files set(TEST_SOURCES test/cpp/CanDevice_test.cpp + test/cpp/CanDeviceConfiguration_test.cpp test/cpp/CanFrame_test.cpp test/cpp/LogIt_test.cpp test/cpp/CanVersion_test.cpp diff --git a/src/include/CanDeviceConfiguration.h b/src/include/CanDeviceConfiguration.h index 93731a99..7550a0f0 100644 --- a/src/include/CanDeviceConfiguration.h +++ b/src/include/CanDeviceConfiguration.h @@ -3,6 +3,7 @@ #include #include +#include #include #include @@ -15,6 +16,19 @@ * depending on the type of CAN device being used. */ struct CanDeviceConfiguration { + CanDeviceConfiguration() = default; + + /** + * @brief Constructs a configuration from a map of string parameters, where + * each key must be the name of one of the parameters below + * + * @param parameters The configuration parameters, indexed by name. + * @throws std::invalid_argument if a key is not a configuration parameter or + * if a value cannot be converted to the type of its parameter. + */ + explicit CanDeviceConfiguration( + const std::map& parameters); + /** * @brief The name of the CAN bus. * diff --git a/src/main/CanDeviceConfiguration.cpp b/src/main/CanDeviceConfiguration.cpp index 207c75f6..465dd24e 100644 --- a/src/main/CanDeviceConfiguration.cpp +++ b/src/main/CanDeviceConfiguration.cpp @@ -1,10 +1,111 @@ #include "CanDeviceConfiguration.h" +#include +#include #include #include +#include +#include #include +#include #include +namespace { + +/** + * @brief Converts a string to an unsigned 32 bits integer. + * + * @param key The name of the parameter being converted, used in the error. + * @param value The value to convert, a decimal number without sign. + * @return The converted value. + * @throws std::invalid_argument if the value is not an unsigned 32 bits + * integer. + */ +uint32_t to_uint32(const std::string& key, const std::string& value) { + const bool is_decimal = + !value.empty() && + std::all_of(value.begin(), value.end(), [](unsigned char character) { + return std::isdigit(character) != 0; + }); + + if (is_decimal) { + try { + const uint64_t parsed = std::stoull(value); + + if (parsed <= std::numeric_limits::max()) { + return static_cast(parsed); + } + } catch (const std::out_of_range&) { + // Reported below, together with the values that are not decimal. + } + } + + throw std::invalid_argument("Invalid value '" + value + + "' for CAN device " + "configuration parameter '" + + key + "': expected an unsigned 32 bits integer"); +} + +/** + * @brief Converts a string to a boolean. + * + * @param key The name of the parameter being converted, used in the error. + * @param value The value to convert, either "true" or "false". + * @return The converted value. + * @throws std::invalid_argument if the value is not a boolean. + */ +bool to_bool(const std::string& key, const std::string& value) { + if (value == "true") { + return true; + } + + if (value == "false") { + return false; + } + + throw std::invalid_argument("Invalid value '" + value + + "' for CAN device " + "configuration parameter '" + + key + "': expected 'true' or 'false'"); +} + +} // namespace + +/** + * @brief Constructs a configuration from a map of string parameters. + * + * @param parameters The configuration parameters, indexed by name. + * @throws std::invalid_argument if a key is not a configuration parameter or if + * a value cannot be converted to the type of its parameter. + */ +CanDeviceConfiguration::CanDeviceConfiguration( + const std::map& parameters) { + for (const auto& [key, value] : parameters) { + if (key == "bus_name") { + bus_name = value; + } else if (key == "bus_number") { + bus_number = to_uint32(key, value); + } else if (key == "host") { + host = value; + } else if (key == "bitrate") { + bitrate = to_uint32(key, value); + } else if (key == "enable_termination") { + enable_termination = to_bool(key, value); + } else if (key == "high_speed") { + high_speed = to_bool(key, value); + } else if (key == "timeout") { + timeout = to_uint32(key, value); + } else if (key == "vcan") { + vcan = to_bool(key, value); + } else if (key == "sent_acknowledgement") { + sent_acknowledgement = to_uint32(key, value); + } else { + throw std::invalid_argument( + "Unknown CAN device configuration parameter '" + key + "'"); + } + } +} + /** * @brief Converts the CanDeviceConfiguration object to a string representation. * diff --git a/src/main/CanVendorAnagate.cpp b/src/main/CanVendorAnagate.cpp index 744f853e..40af348d 100644 --- a/src/main/CanVendorAnagate.cpp +++ b/src/main/CanVendorAnagate.cpp @@ -62,8 +62,12 @@ void anagate_receive(AnaInt32 nIdentifier, const char* pcBuffer, */ CanVendorAnagate::CanVendorAnagate(const CanDeviceArguments& args) : CanDevice("anagate", args) { - if (!args.config.bus_number.has_value() || !args.config.host.has_value()) { - throw std::invalid_argument("Missing required configuration parameters"); + if (!args.config.host.has_value()) { + throw std::invalid_argument("Missing required host"); + } + + if (!args.config.bus_number.has_value()) { + throw std::invalid_argument("Missing required bus number"); } } diff --git a/src/main/CanVendorSocketCan.cpp b/src/main/CanVendorSocketCan.cpp index 9e52b166..1078c4ee 100644 --- a/src/main/CanVendorSocketCan.cpp +++ b/src/main/CanVendorSocketCan.cpp @@ -32,7 +32,7 @@ constexpr auto LIBSOCKETCAN_SUCCESS = 0; CanVendorSocketCan::CanVendorSocketCan(const CanDeviceArguments& args) : CanDevice("socketcan", args) { if (!args.config.bus_name.has_value()) { - throw std::invalid_argument("Missing required configuration parameters"); + throw std::invalid_argument("Missing required bus name"); } } /** diff --git a/test/cpp/CanDeviceConfiguration_test.cpp b/test/cpp/CanDeviceConfiguration_test.cpp new file mode 100644 index 00000000..7481ccb8 --- /dev/null +++ b/test/cpp/CanDeviceConfiguration_test.cpp @@ -0,0 +1,95 @@ +#include "CanDeviceConfiguration.h" + +#include + +#include +#include +#include +#include +#include + +class CanDeviceConfigurationTest : public ::testing::Test {}; + +TEST_F(CanDeviceConfigurationTest, DefaultConstructorLeavesEverythingUnset) { + const CanDeviceConfiguration config; + + ASSERT_FALSE(config.bus_name.has_value()); + ASSERT_FALSE(config.bus_number.has_value()); + ASSERT_FALSE(config.host.has_value()); + ASSERT_FALSE(config.bitrate.has_value()); + ASSERT_FALSE(config.enable_termination.has_value()); + ASSERT_FALSE(config.high_speed.has_value()); + ASSERT_FALSE(config.timeout.has_value()); + ASSERT_FALSE(config.vcan.has_value()); + ASSERT_FALSE(config.sent_acknowledgement.has_value()); +} + +TEST_F(CanDeviceConfigurationTest, AssignsEveryParameterOfTheMap) { + const CanDeviceConfiguration config{{ + {"bus_name", "can0"}, + {"bus_number", "2"}, + {"host", "127.0.0.1"}, + {"bitrate", "125000"}, + {"enable_termination", "true"}, + {"high_speed", "false"}, + {"timeout", "6000"}, + {"vcan", "true"}, + {"sent_acknowledgement", "1"}, + }}; + + ASSERT_EQ(config.bus_name.value(), "can0"); + ASSERT_EQ(config.bus_number.value(), 2); + ASSERT_EQ(config.host.value(), "127.0.0.1"); + ASSERT_EQ(config.bitrate.value(), 125000); + ASSERT_TRUE(config.enable_termination.value()); + ASSERT_FALSE(config.high_speed.value()); + ASSERT_EQ(config.timeout.value(), 6000); + ASSERT_TRUE(config.vcan.value()); + ASSERT_EQ(config.sent_acknowledgement.value(), 1); +} + +TEST_F(CanDeviceConfigurationTest, LeavesTheAbsentParametersUnset) { + const CanDeviceConfiguration config{{{"bus_name", "can0"}}}; + + ASSERT_EQ(config.bus_name.value(), "can0"); + ASSERT_FALSE(config.bus_number.has_value()); + ASSERT_FALSE(config.host.has_value()); + ASSERT_FALSE(config.bitrate.has_value()); + ASSERT_FALSE(config.enable_termination.has_value()); + ASSERT_FALSE(config.high_speed.has_value()); + ASSERT_FALSE(config.timeout.has_value()); + ASSERT_FALSE(config.vcan.has_value()); + ASSERT_FALSE(config.sent_acknowledgement.has_value()); +} + +TEST_F(CanDeviceConfigurationTest, RejectsUnknownParameters) { + ASSERT_THROW(CanDeviceConfiguration({{"buss_name", "can0"}}), + std::invalid_argument); + ASSERT_THROW(CanDeviceConfiguration({{"vendor", "socketcan"}}), + std::invalid_argument); + ASSERT_THROW(CanDeviceConfiguration({{"", ""}}), std::invalid_argument); +} + +TEST_F(CanDeviceConfigurationTest, RejectsValuesOfTheWrongType) { + const std::vector> invalid_values = { + {"bus_number", ""}, + {"bus_number", "-1"}, + {"bus_number", "+1"}, + {"bus_number", "1.5"}, + {"bus_number", "12a"}, + {"bus_number", "0x2"}, + {"bus_number", " 2 "}, + {"bitrate", "4294967296"}, + {"bitrate", "99999999999999999999"}, + {"enable_termination", ""}, + {"enable_termination", "yes"}, + {"high_speed", "TRUE"}, + {"vcan", "1"}, + {"sent_acknowledgement", "abc"}, + }; + + for (const auto& [key, value] : invalid_values) { + ASSERT_THROW(CanDeviceConfiguration({{key, value}}), std::invalid_argument) + << "Expected '" << value << "' to be invalid for '" << key << "'"; + } +} diff --git a/test/cpp/CanDevice_test.cpp b/test/cpp/CanDevice_test.cpp index c94512aa..5f4cc0db 100644 --- a/test/cpp/CanDevice_test.cpp +++ b/test/cpp/CanDevice_test.cpp @@ -40,7 +40,8 @@ TEST_F(CanDeviceTest, CreationLoopbackDevice) { auto dummy_cb_ = [](const CanFrame& frame) { return; }; auto myDevice = CanDevice::create( "loopback", - CanDeviceArguments{CanDeviceConfiguration{"dummy"}, dummy_cb_}); + CanDeviceArguments{CanDeviceConfiguration{{{"bus_name", "dummy"}}}, + dummy_cb_}); ASSERT_NE(myDevice, nullptr); ASSERT_EQ(myDevice->vendor_name(), "loopback"); ASSERT_EQ(myDevice->args().config.bus_name.value(), "dummy"); @@ -56,7 +57,8 @@ TEST_F(CanDeviceTest, LoopbackDeviceMessageTransmission) { }; auto myDevice = CanDevice::create( "loopback", - CanDeviceArguments{CanDeviceConfiguration{"dummy"}, dummy_cb_}); + CanDeviceArguments{CanDeviceConfiguration{{{"bus_name", "dummy"}}}, + dummy_cb_}); for (uint32_t i = 0; i < 10; ++i) { outFrames.push_back(CanFrame{i}); @@ -86,8 +88,9 @@ TEST_F(CanDeviceTest, OnErrorCallbackIsInvoked) { }; TestableCanDevice device{ - "test", CanDeviceArguments{CanDeviceConfiguration{"dummy"}, - [](const CanFrame&) {}, on_error_cb}}; + "test", + CanDeviceArguments{CanDeviceConfiguration{{{"bus_name", "dummy"}}}, + [](const CanFrame&) {}, on_error_cb}}; device.notify_error(CanReturnCode::disconnected); ASSERT_TRUE(called); @@ -108,8 +111,9 @@ TEST_F(CanDeviceTest, ThrowingCallbacksAreHandled) { }; TestableCanDevice device{ - "test", CanDeviceArguments{CanDeviceConfiguration{"dummy"}, receiver_cb, - on_error_cb}}; + "test", + CanDeviceArguments{CanDeviceConfiguration{{{"bus_name", "dummy"}}}, + receiver_cb, on_error_cb}}; ASSERT_NO_THROW(device.received(CanFrame{0})); ASSERT_TRUE(receiver_called); From 2606f1f683aacf2e35e7dfb233c3b1e5f702da1b Mon Sep 17 00:00:00 2001 From: Tiago Lourinho Date: Thu, 3 Sep 2026 13:20:04 +0200 Subject: [PATCH 2/4] added python bind --- src/python/CanModule.cpp | 4 ++++ test/python/test_common.py | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/python/CanModule.cpp b/src/python/CanModule.cpp index bdbe963e..1de13cb6 100644 --- a/src/python/CanModule.cpp +++ b/src/python/CanModule.cpp @@ -2,6 +2,8 @@ #include #include +#include +#include #include #include "CanDevice.h" @@ -77,6 +79,8 @@ PYBIND11_MODULE(canmodule, m) { py::class_(m, "CanDeviceConfiguration") .def(py::init<>()) + .def(py::init&>(), + py::arg("parameters")) .def_readwrite("bus_name", &CanDeviceConfiguration::bus_name) .def_readwrite("bus_number", &CanDeviceConfiguration::bus_number) .def_readwrite("host", &CanDeviceConfiguration::host) diff --git a/test/python/test_common.py b/test/python/test_common.py index ba153828..088d6b21 100644 --- a/test/python/test_common.py +++ b/test/python/test_common.py @@ -65,6 +65,24 @@ def test_loopback_multiple_messages(): assert received_frames[1].message() == ["W", "o", "r", "l", "d"] +def test_configuration_from_parameters(): + config = CanDeviceConfiguration( + {"bus_name": "can0", "bitrate": "500000", "vcan": "true"} + ) + assert config.bus_name == "can0" + assert config.bitrate == 500000 + assert config.vcan is True + assert config.host is None + + +def test_configuration_from_parameters_unknown_key(): + with pytest.raises(ValueError) as e: + CanDeviceConfiguration({"not_a_parameter": "value"}) + assert ( + str(e.value) == "Unknown CAN device configuration parameter 'not_a_parameter'" + ) + + def test_loopback_construction_empty_callback(): myDevice = CanDevice.create( "loopback", CanDeviceArguments(CanDeviceConfiguration()) From 061ea3e03f59861395527050e5a62038d4c7eca6 Mon Sep 17 00:00:00 2001 From: Tiago Lourinho Date: Fri, 4 Sep 2026 08:40:24 +0200 Subject: [PATCH 3/4] added back explicit positional constructor --- src/include/CanDeviceConfiguration.h | 16 ++++++++- src/main/CanDeviceConfiguration.cpp | 17 +++++++++ test/cpp/CanDeviceConfiguration_test.cpp | 46 +++++++++++++++++++++--- test/cpp/CanDevice_test.cpp | 16 ++++----- 4 files changed, 79 insertions(+), 16 deletions(-) diff --git a/src/include/CanDeviceConfiguration.h b/src/include/CanDeviceConfiguration.h index 7550a0f0..4d8052c7 100644 --- a/src/include/CanDeviceConfiguration.h +++ b/src/include/CanDeviceConfiguration.h @@ -16,7 +16,21 @@ * depending on the type of CAN device being used. */ struct CanDeviceConfiguration { - CanDeviceConfiguration() = default; + /** + * @brief Constructs a configuration from positional parameters, in the + * same order as the fields below. Only a leading subset needs to be + * provided; the remaining fields are left unset. + */ + explicit CanDeviceConfiguration( + std::optional bus_name = std::nullopt, + std::optional bus_number = std::nullopt, + std::optional host = std::nullopt, + std::optional bitrate = std::nullopt, + std::optional enable_termination = std::nullopt, + std::optional high_speed = std::nullopt, + std::optional timeout = std::nullopt, + std::optional vcan = std::nullopt, + std::optional sent_acknowledgement = std::nullopt); /** * @brief Constructs a configuration from a map of string parameters, where diff --git a/src/main/CanDeviceConfiguration.cpp b/src/main/CanDeviceConfiguration.cpp index 465dd24e..c26cfb1e 100644 --- a/src/main/CanDeviceConfiguration.cpp +++ b/src/main/CanDeviceConfiguration.cpp @@ -9,6 +9,7 @@ #include #include #include +#include namespace { @@ -106,6 +107,22 @@ CanDeviceConfiguration::CanDeviceConfiguration( } } +CanDeviceConfiguration::CanDeviceConfiguration( + std::optional bus_name, std::optional bus_number, + std::optional host, std::optional bitrate, + std::optional enable_termination, std::optional high_speed, + std::optional timeout, std::optional vcan, + std::optional sent_acknowledgement) + : bus_name(std::move(bus_name)), + bus_number(bus_number), + host(std::move(host)), + bitrate(bitrate), + enable_termination(enable_termination), + high_speed(high_speed), + timeout(timeout), + vcan(vcan), + sent_acknowledgement(sent_acknowledgement) {} + /** * @brief Converts the CanDeviceConfiguration object to a string representation. * diff --git a/test/cpp/CanDeviceConfiguration_test.cpp b/test/cpp/CanDeviceConfiguration_test.cpp index 7481ccb8..112e4aa7 100644 --- a/test/cpp/CanDeviceConfiguration_test.cpp +++ b/test/cpp/CanDeviceConfiguration_test.cpp @@ -49,7 +49,8 @@ TEST_F(CanDeviceConfigurationTest, AssignsEveryParameterOfTheMap) { } TEST_F(CanDeviceConfigurationTest, LeavesTheAbsentParametersUnset) { - const CanDeviceConfiguration config{{{"bus_name", "can0"}}}; + const CanDeviceConfiguration config{ + std::map{{"bus_name", "can0"}}}; ASSERT_EQ(config.bus_name.value(), "can0"); ASSERT_FALSE(config.bus_number.has_value()); @@ -62,12 +63,45 @@ TEST_F(CanDeviceConfigurationTest, LeavesTheAbsentParametersUnset) { ASSERT_FALSE(config.sent_acknowledgement.has_value()); } +TEST_F(CanDeviceConfigurationTest, AssignsPositionalParametersInOrder) { + const CanDeviceConfiguration config{"can0", 2, "127.0.0.1", 125000, true, + false, 6000, true, 1}; + + ASSERT_EQ(config.bus_name.value(), "can0"); + ASSERT_EQ(config.bus_number.value(), 2); + ASSERT_EQ(config.host.value(), "127.0.0.1"); + ASSERT_EQ(config.bitrate.value(), 125000); + ASSERT_TRUE(config.enable_termination.value()); + ASSERT_FALSE(config.high_speed.value()); + ASSERT_EQ(config.timeout.value(), 6000); + ASSERT_TRUE(config.vcan.value()); + ASSERT_EQ(config.sent_acknowledgement.value(), 1); +} + +TEST_F(CanDeviceConfigurationTest, LeavesTrailingPositionalParametersUnset) { + const CanDeviceConfiguration config{"can0", 2}; + + ASSERT_EQ(config.bus_name.value(), "can0"); + ASSERT_EQ(config.bus_number.value(), 2); + ASSERT_FALSE(config.host.has_value()); + ASSERT_FALSE(config.bitrate.has_value()); + ASSERT_FALSE(config.enable_termination.has_value()); + ASSERT_FALSE(config.high_speed.has_value()); + ASSERT_FALSE(config.timeout.has_value()); + ASSERT_FALSE(config.vcan.has_value()); + ASSERT_FALSE(config.sent_acknowledgement.has_value()); +} + TEST_F(CanDeviceConfigurationTest, RejectsUnknownParameters) { - ASSERT_THROW(CanDeviceConfiguration({{"buss_name", "can0"}}), + ASSERT_THROW(CanDeviceConfiguration( + std::map{{"buss_name", "can0"}}), std::invalid_argument); - ASSERT_THROW(CanDeviceConfiguration({{"vendor", "socketcan"}}), + ASSERT_THROW(CanDeviceConfiguration( + std::map{{"vendor", "socketcan"}}), std::invalid_argument); - ASSERT_THROW(CanDeviceConfiguration({{"", ""}}), std::invalid_argument); + ASSERT_THROW( + CanDeviceConfiguration(std::map{{"", ""}}), + std::invalid_argument); } TEST_F(CanDeviceConfigurationTest, RejectsValuesOfTheWrongType) { @@ -89,7 +123,9 @@ TEST_F(CanDeviceConfigurationTest, RejectsValuesOfTheWrongType) { }; for (const auto& [key, value] : invalid_values) { - ASSERT_THROW(CanDeviceConfiguration({{key, value}}), std::invalid_argument) + ASSERT_THROW(CanDeviceConfiguration( + std::map{{key, value}}), + std::invalid_argument) << "Expected '" << value << "' to be invalid for '" << key << "'"; } } diff --git a/test/cpp/CanDevice_test.cpp b/test/cpp/CanDevice_test.cpp index 5f4cc0db..c94512aa 100644 --- a/test/cpp/CanDevice_test.cpp +++ b/test/cpp/CanDevice_test.cpp @@ -40,8 +40,7 @@ TEST_F(CanDeviceTest, CreationLoopbackDevice) { auto dummy_cb_ = [](const CanFrame& frame) { return; }; auto myDevice = CanDevice::create( "loopback", - CanDeviceArguments{CanDeviceConfiguration{{{"bus_name", "dummy"}}}, - dummy_cb_}); + CanDeviceArguments{CanDeviceConfiguration{"dummy"}, dummy_cb_}); ASSERT_NE(myDevice, nullptr); ASSERT_EQ(myDevice->vendor_name(), "loopback"); ASSERT_EQ(myDevice->args().config.bus_name.value(), "dummy"); @@ -57,8 +56,7 @@ TEST_F(CanDeviceTest, LoopbackDeviceMessageTransmission) { }; auto myDevice = CanDevice::create( "loopback", - CanDeviceArguments{CanDeviceConfiguration{{{"bus_name", "dummy"}}}, - dummy_cb_}); + CanDeviceArguments{CanDeviceConfiguration{"dummy"}, dummy_cb_}); for (uint32_t i = 0; i < 10; ++i) { outFrames.push_back(CanFrame{i}); @@ -88,9 +86,8 @@ TEST_F(CanDeviceTest, OnErrorCallbackIsInvoked) { }; TestableCanDevice device{ - "test", - CanDeviceArguments{CanDeviceConfiguration{{{"bus_name", "dummy"}}}, - [](const CanFrame&) {}, on_error_cb}}; + "test", CanDeviceArguments{CanDeviceConfiguration{"dummy"}, + [](const CanFrame&) {}, on_error_cb}}; device.notify_error(CanReturnCode::disconnected); ASSERT_TRUE(called); @@ -111,9 +108,8 @@ TEST_F(CanDeviceTest, ThrowingCallbacksAreHandled) { }; TestableCanDevice device{ - "test", - CanDeviceArguments{CanDeviceConfiguration{{{"bus_name", "dummy"}}}, - receiver_cb, on_error_cb}}; + "test", CanDeviceArguments{CanDeviceConfiguration{"dummy"}, receiver_cb, + on_error_cb}}; ASSERT_NO_THROW(device.received(CanFrame{0})); ASSERT_TRUE(receiver_called); From d62a804bce740b0c230553fc6036eb4851f0085e Mon Sep 17 00:00:00 2001 From: Tiago Lourinho Date: Fri, 4 Sep 2026 09:21:48 +0200 Subject: [PATCH 4/4] added factory for map --- src/include/CanDeviceConfiguration.h | 23 ++---------- src/main/CanDeviceConfiguration.cpp | 48 +++++++----------------- src/python/CanModule.cpp | 4 +- test/cpp/CanDeviceConfiguration_test.cpp | 24 +++++------- test/python/test_common.py | 4 +- 5 files changed, 32 insertions(+), 71 deletions(-) diff --git a/src/include/CanDeviceConfiguration.h b/src/include/CanDeviceConfiguration.h index 4d8052c7..be194608 100644 --- a/src/include/CanDeviceConfiguration.h +++ b/src/include/CanDeviceConfiguration.h @@ -17,30 +17,15 @@ */ struct CanDeviceConfiguration { /** - * @brief Constructs a configuration from positional parameters, in the - * same order as the fields below. Only a leading subset needs to be - * provided; the remaining fields are left unset. - */ - explicit CanDeviceConfiguration( - std::optional bus_name = std::nullopt, - std::optional bus_number = std::nullopt, - std::optional host = std::nullopt, - std::optional bitrate = std::nullopt, - std::optional enable_termination = std::nullopt, - std::optional high_speed = std::nullopt, - std::optional timeout = std::nullopt, - std::optional vcan = std::nullopt, - std::optional sent_acknowledgement = std::nullopt); - - /** - * @brief Constructs a configuration from a map of string parameters, where - * each key must be the name of one of the parameters below + * @brief Builds a configuration from a map of string parameters, where + * each key must be the name of one of the parameters below. * * @param parameters The configuration parameters, indexed by name. + * @return The parsed configuration. * @throws std::invalid_argument if a key is not a configuration parameter or * if a value cannot be converted to the type of its parameter. */ - explicit CanDeviceConfiguration( + static CanDeviceConfiguration from_map( const std::map& parameters); /** diff --git a/src/main/CanDeviceConfiguration.cpp b/src/main/CanDeviceConfiguration.cpp index c26cfb1e..5244e1bd 100644 --- a/src/main/CanDeviceConfiguration.cpp +++ b/src/main/CanDeviceConfiguration.cpp @@ -9,7 +9,6 @@ #include #include #include -#include namespace { @@ -72,56 +71,37 @@ bool to_bool(const std::string& key, const std::string& value) { } // namespace -/** - * @brief Constructs a configuration from a map of string parameters. - * - * @param parameters The configuration parameters, indexed by name. - * @throws std::invalid_argument if a key is not a configuration parameter or if - * a value cannot be converted to the type of its parameter. - */ -CanDeviceConfiguration::CanDeviceConfiguration( +CanDeviceConfiguration CanDeviceConfiguration::from_map( const std::map& parameters) { + CanDeviceConfiguration config; + for (const auto& [key, value] : parameters) { if (key == "bus_name") { - bus_name = value; + config.bus_name = value; } else if (key == "bus_number") { - bus_number = to_uint32(key, value); + config.bus_number = to_uint32(key, value); } else if (key == "host") { - host = value; + config.host = value; } else if (key == "bitrate") { - bitrate = to_uint32(key, value); + config.bitrate = to_uint32(key, value); } else if (key == "enable_termination") { - enable_termination = to_bool(key, value); + config.enable_termination = to_bool(key, value); } else if (key == "high_speed") { - high_speed = to_bool(key, value); + config.high_speed = to_bool(key, value); } else if (key == "timeout") { - timeout = to_uint32(key, value); + config.timeout = to_uint32(key, value); } else if (key == "vcan") { - vcan = to_bool(key, value); + config.vcan = to_bool(key, value); } else if (key == "sent_acknowledgement") { - sent_acknowledgement = to_uint32(key, value); + config.sent_acknowledgement = to_uint32(key, value); } else { throw std::invalid_argument( "Unknown CAN device configuration parameter '" + key + "'"); } } -} -CanDeviceConfiguration::CanDeviceConfiguration( - std::optional bus_name, std::optional bus_number, - std::optional host, std::optional bitrate, - std::optional enable_termination, std::optional high_speed, - std::optional timeout, std::optional vcan, - std::optional sent_acknowledgement) - : bus_name(std::move(bus_name)), - bus_number(bus_number), - host(std::move(host)), - bitrate(bitrate), - enable_termination(enable_termination), - high_speed(high_speed), - timeout(timeout), - vcan(vcan), - sent_acknowledgement(sent_acknowledgement) {} + return config; +} /** * @brief Converts the CanDeviceConfiguration object to a string representation. diff --git a/src/python/CanModule.cpp b/src/python/CanModule.cpp index 1de13cb6..a951b581 100644 --- a/src/python/CanModule.cpp +++ b/src/python/CanModule.cpp @@ -79,8 +79,8 @@ PYBIND11_MODULE(canmodule, m) { py::class_(m, "CanDeviceConfiguration") .def(py::init<>()) - .def(py::init&>(), - py::arg("parameters")) + .def_static("from_map", &CanDeviceConfiguration::from_map, + py::arg("parameters")) .def_readwrite("bus_name", &CanDeviceConfiguration::bus_name) .def_readwrite("bus_number", &CanDeviceConfiguration::bus_number) .def_readwrite("host", &CanDeviceConfiguration::host) diff --git a/test/cpp/CanDeviceConfiguration_test.cpp b/test/cpp/CanDeviceConfiguration_test.cpp index 112e4aa7..f2fc19fc 100644 --- a/test/cpp/CanDeviceConfiguration_test.cpp +++ b/test/cpp/CanDeviceConfiguration_test.cpp @@ -25,7 +25,7 @@ TEST_F(CanDeviceConfigurationTest, DefaultConstructorLeavesEverythingUnset) { } TEST_F(CanDeviceConfigurationTest, AssignsEveryParameterOfTheMap) { - const CanDeviceConfiguration config{{ + const CanDeviceConfiguration config = CanDeviceConfiguration::from_map({ {"bus_name", "can0"}, {"bus_number", "2"}, {"host", "127.0.0.1"}, @@ -35,7 +35,7 @@ TEST_F(CanDeviceConfigurationTest, AssignsEveryParameterOfTheMap) { {"timeout", "6000"}, {"vcan", "true"}, {"sent_acknowledgement", "1"}, - }}; + }); ASSERT_EQ(config.bus_name.value(), "can0"); ASSERT_EQ(config.bus_number.value(), 2); @@ -49,8 +49,8 @@ TEST_F(CanDeviceConfigurationTest, AssignsEveryParameterOfTheMap) { } TEST_F(CanDeviceConfigurationTest, LeavesTheAbsentParametersUnset) { - const CanDeviceConfiguration config{ - std::map{{"bus_name", "can0"}}}; + const CanDeviceConfiguration config = + CanDeviceConfiguration::from_map({{"bus_name", "can0"}}); ASSERT_EQ(config.bus_name.value(), "can0"); ASSERT_FALSE(config.bus_number.has_value()); @@ -63,7 +63,7 @@ TEST_F(CanDeviceConfigurationTest, LeavesTheAbsentParametersUnset) { ASSERT_FALSE(config.sent_acknowledgement.has_value()); } -TEST_F(CanDeviceConfigurationTest, AssignsPositionalParametersInOrder) { +TEST_F(CanDeviceConfigurationTest, AssignsEveryParameterPositionally) { const CanDeviceConfiguration config{"can0", 2, "127.0.0.1", 125000, true, false, 6000, true, 1}; @@ -93,15 +93,12 @@ TEST_F(CanDeviceConfigurationTest, LeavesTrailingPositionalParametersUnset) { } TEST_F(CanDeviceConfigurationTest, RejectsUnknownParameters) { - ASSERT_THROW(CanDeviceConfiguration( - std::map{{"buss_name", "can0"}}), + ASSERT_THROW(CanDeviceConfiguration::from_map({{"buss_name", "can0"}}), std::invalid_argument); - ASSERT_THROW(CanDeviceConfiguration( - std::map{{"vendor", "socketcan"}}), + ASSERT_THROW(CanDeviceConfiguration::from_map({{"vendor", "socketcan"}}), + std::invalid_argument); + ASSERT_THROW(CanDeviceConfiguration::from_map({{"", ""}}), std::invalid_argument); - ASSERT_THROW( - CanDeviceConfiguration(std::map{{"", ""}}), - std::invalid_argument); } TEST_F(CanDeviceConfigurationTest, RejectsValuesOfTheWrongType) { @@ -123,8 +120,7 @@ TEST_F(CanDeviceConfigurationTest, RejectsValuesOfTheWrongType) { }; for (const auto& [key, value] : invalid_values) { - ASSERT_THROW(CanDeviceConfiguration( - std::map{{key, value}}), + ASSERT_THROW(CanDeviceConfiguration::from_map({{key, value}}), std::invalid_argument) << "Expected '" << value << "' to be invalid for '" << key << "'"; } diff --git a/test/python/test_common.py b/test/python/test_common.py index 088d6b21..81472fbb 100644 --- a/test/python/test_common.py +++ b/test/python/test_common.py @@ -66,7 +66,7 @@ def test_loopback_multiple_messages(): def test_configuration_from_parameters(): - config = CanDeviceConfiguration( + config = CanDeviceConfiguration.from_map( {"bus_name": "can0", "bitrate": "500000", "vcan": "true"} ) assert config.bus_name == "can0" @@ -77,7 +77,7 @@ def test_configuration_from_parameters(): def test_configuration_from_parameters_unknown_key(): with pytest.raises(ValueError) as e: - CanDeviceConfiguration({"not_a_parameter": "value"}) + CanDeviceConfiguration.from_map({"not_a_parameter": "value"}) assert ( str(e.value) == "Unknown CAN device configuration parameter 'not_a_parameter'" )