diff --git a/python/canmodule_utils/cli.py b/python/canmodule_utils/cli.py index e64ece55..2e2ed214 100644 --- a/python/canmodule_utils/cli.py +++ b/python/canmodule_utils/cli.py @@ -95,11 +95,10 @@ def main(argv=None): set_items=args.set_items, ) validate_required_config(args.vendor, merged_config) + configuration = build_can_device_configuration(merged_config) except ValueError as error: raise SystemExit(str(error)) from error - configuration = build_can_device_configuration(merged_config) - if args.action == "dump": dump(args.vendor, configuration) elif args.action == "send": diff --git a/python/canmodule_utils/config.py b/python/canmodule_utils/config.py index 9a1ff111..c90cf06f 100644 --- a/python/canmodule_utils/config.py +++ b/python/canmodule_utils/config.py @@ -2,102 +2,32 @@ from canmodule import CanDeviceConfiguration - -UINT32_MAX = 0xFFFFFFFF NULL_STRINGS = {"null", "none"} -TRUE_STRINGS = {"true", "1", "yes", "on", "enable", "enabled"} -FALSE_STRINGS = {"false", "0", "no", "off", "disable", "disabled"} + +# Derive from the CanDeviceConfiguration bindings +VALID_CONFIG_KEYS = frozenset( + name + for name in dir(CanDeviceConfiguration) + if isinstance(getattr(CanDeviceConfiguration, name), property) +) def normalize_config_key(key): return key.replace("-", "_") -def parse_string(value): - if value is None: - return None - if not isinstance(value, str): - raise ValueError("expected a string") - return value - - -def parse_bool(value): - if value is None: - return None - if isinstance(value, bool): - return value - if isinstance(value, str): - normalized = value.strip().lower() - if normalized in NULL_STRINGS: - return None - if normalized in TRUE_STRINGS: - return True - if normalized in FALSE_STRINGS: - return False - raise ValueError(f"invalid boolean value '{value}'") - - -def parse_uint32(value): - if value is None: - return None - if isinstance(value, bool): - raise ValueError("expected an unsigned integer") - - try: - if isinstance(value, int): - parsed = value - elif isinstance(value, str): - normalized = value.strip().lower() - if normalized in NULL_STRINGS: - return None - parsed = int(normalized, 0) - else: - raise ValueError - except ValueError as error: - raise ValueError(f"invalid unsigned integer value '{value}'") from error - - if parsed < 0 or parsed > UINT32_MAX: - raise ValueError(f"unsigned integer value '{value}' is outside 0..0xFFFFFFFF") - return parsed - - -def parse_sent_acknowledgement(value): +def parse_config_value(key, value): + if key not in VALID_CONFIG_KEYS: + raise ValueError(f"Unknown configuration key '{key}'") if value is None: return None if isinstance(value, str) and value.strip().lower() in NULL_STRINGS: return None if isinstance(value, bool): - return 1 if value else 0 - if isinstance(value, str) and value.strip().lower() in TRUE_STRINGS | FALSE_STRINGS: - return 1 if parse_bool(value) else 0 - - parsed = parse_uint32(value) - if parsed not in (0, 1): - raise ValueError("sent_acknowledgement must be 0, 1, true, or false") - return parsed - - -CONFIG_FIELDS = { - "bus_name": parse_string, - "bus_number": parse_uint32, - "host": parse_string, - "bitrate": parse_uint32, - "enable_termination": parse_bool, - "high_speed": parse_bool, - "timeout": parse_uint32, - "vcan": parse_bool, - "sent_acknowledgement": parse_sent_acknowledgement, -} - - -def parse_config_value(key, value): - normalized_key = normalize_config_key(key) - if normalized_key not in CONFIG_FIELDS: - raise ValueError(f"Unknown configuration key '{key}'") - try: - return CONFIG_FIELDS[normalized_key](value) - except ValueError as error: - raise ValueError(f"Invalid value for '{normalized_key}': {error}") from error + return "true" if value else "false" + if isinstance(value, (str, int)): + return str(value) + raise ValueError(f"unsupported value type for '{key}': {type(value).__name__}") def parse_set_item(item): @@ -164,7 +94,4 @@ def validate_required_config(vendor, config): def build_can_device_configuration(config): - configuration = CanDeviceConfiguration() - for key, value in config.items(): - setattr(configuration, key, value) - return configuration + return CanDeviceConfiguration.from_map(config) diff --git a/src/include/CanDeviceConfiguration.h b/src/include/CanDeviceConfiguration.h index 12bc3b24..5b005601 100644 --- a/src/include/CanDeviceConfiguration.h +++ b/src/include/CanDeviceConfiguration.h @@ -7,6 +7,7 @@ #include #include #include +#include #include /** @@ -122,4 +123,38 @@ struct CanDeviceConfiguration { std::ostream& operator<<(std::ostream& os, const CanDeviceConfiguration& config) noexcept; +/** + * @brief A pointer to one of CanDeviceConfiguration's members + */ +using Member = + std::variant CanDeviceConfiguration::*, + std::optional CanDeviceConfiguration::*, + std::optional CanDeviceConfiguration::*>; + +/** + * @brief Describes a single configuration parameter + */ +struct FieldDescriptor { + std::string name; + Member member; +}; + +/** + * @brief Unique list defining the configuration fields + */ +inline const std::vector& fields() { + static const std::vector kFields = { + {"bus_name", &CanDeviceConfiguration::bus_name}, + {"bus_number", &CanDeviceConfiguration::bus_number}, + {"host", &CanDeviceConfiguration::host}, + {"bitrate", &CanDeviceConfiguration::bitrate}, + {"enable_termination", &CanDeviceConfiguration::enable_termination}, + {"high_speed", &CanDeviceConfiguration::high_speed}, + {"timeout", &CanDeviceConfiguration::timeout}, + {"vcan", &CanDeviceConfiguration::vcan}, + {"sent_acknowledgement", &CanDeviceConfiguration::sent_acknowledgement}, + }; + return kFields; +} + #endif // SRC_INCLUDE_CANDEVICECONFIGURATION_H_ diff --git a/src/main/CanDeviceConfiguration.cpp b/src/main/CanDeviceConfiguration.cpp index b2bcb6f8..b28233e5 100644 --- a/src/main/CanDeviceConfiguration.cpp +++ b/src/main/CanDeviceConfiguration.cpp @@ -9,7 +9,9 @@ #include #include #include +#include #include +#include #include namespace { @@ -71,35 +73,83 @@ bool to_bool(const std::string& key, const std::string& value) { key + "': expected 'true' or 'false'"); } +/** + * @brief Reads a field's value out of a configuration as a string + * + * @param config The configuration to read from. + * @param field The field to read. + * @return The value, or std::nullopt if the field is unset. + */ +std::optional value_as_string(const CanDeviceConfiguration& config, + const FieldDescriptor& field) { + return std::visit( + [&config](auto member) -> std::optional { + const auto& value = config.*member; + if (!value.has_value()) return std::nullopt; + + using ValueType = typename std::decay_t::value_type; + if constexpr (std::is_same_v) { + return value.value(); + } else if constexpr (std::is_same_v) { + return std::to_string(value.value()); + } else if constexpr (std::is_same_v) { + return value.value() ? "true" : "false"; + } + + return std::nullopt; + }, + field.member); +} + +/** + * @brief Sets a field's value on a configuration, parsing it from a string + * + * @param config The configuration to update. + * @param field The field to set. + * @param key The name of the parameter, used in errors. + * @param value The value to parse and assign. + * @throws std::invalid_argument if the value cannot be converted to the + * field's type. + */ +void assign_from_string(CanDeviceConfiguration& config, + const FieldDescriptor& field, const std::string& key, + const std::string& value) { + std::visit( + [&](auto member) { + using ValueType = + typename std::decay_t::value_type; + if constexpr (std::is_same_v) { + config.*member = value; + } else if constexpr (std::is_same_v) { + config.*member = to_uint32(key, value); + } else if constexpr (std::is_same_v) { + config.*member = to_bool(key, value); + } + }, + field.member); +} + } // namespace CanDeviceConfiguration CanDeviceConfiguration::from_map( const std::map& parameters) { CanDeviceConfiguration config; - for (const auto& [key, value] : parameters) { - if (key == "bus_name") { - config.bus_name = value; - } else if (key == "bus_number") { - config.bus_number = to_uint32(key, value); - } else if (key == "host") { - config.host = value; - } else if (key == "bitrate") { - config.bitrate = to_uint32(key, value); - } else if (key == "enable_termination") { - config.enable_termination = to_bool(key, value); - } else if (key == "high_speed") { - config.high_speed = to_bool(key, value); - } else if (key == "timeout") { - config.timeout = to_uint32(key, value); - } else if (key == "vcan") { - config.vcan = to_bool(key, value); - } else if (key == "sent_acknowledgement") { - config.sent_acknowledgement = to_uint32(key, value); - } else { + for (const auto& parameter : parameters) { + const std::string& key = parameter.first; + const std::string& value = parameter.second; + + const auto it = std::find_if( + // fields is a static const + fields().begin(), fields().end(), + [&key](const FieldDescriptor& field) { return field.name == key; }); + + if (it == fields().end()) { throw std::invalid_argument( "Unknown CAN device configuration parameter '" + key + "'"); } + + assign_from_string(config, *it, key, value); } return config; @@ -119,56 +169,23 @@ std::string CanDeviceConfiguration::to_string() const noexcept { std::ostringstream oss; bool first = true; - if (bus_name.has_value()) { - if (!first) oss << ", "; - oss << "bus_name=" << std::quoted(bus_name.value()); - first = false; - } - - if (bus_number.has_value()) { - if (!first) oss << ", "; - oss << "bus_number=" << bus_number.value(); - first = false; - } - - if (host.has_value()) { - if (!first) oss << ", "; - oss << "host=" << std::quoted(host.value()); - first = false; - } - - if (bitrate.has_value()) { - if (!first) oss << ", "; - oss << "bitrate=" << bitrate.value(); - first = false; - } - - if (enable_termination.has_value()) { - if (!first) oss << ", "; - oss << "enable_termination=" - << (enable_termination.value() ? "true" : "false"); - first = false; - } + for (const auto& field : fields()) { + const auto value = value_as_string(*this, field); + if (!value.has_value()) continue; - if (vcan.has_value()) { if (!first) oss << ", "; - oss << "vcan=" << (vcan.value() ? "true" : "false"); - first = false; - } - - if (timeout.has_value()) { - if (!first) oss << ", "; - oss << "timeout=" << timeout.value(); + oss << field.name << "="; + // Only add quotes if its a string + if (std::holds_alternative< + std::optional CanDeviceConfiguration::*>( + field.member)) { + oss << std::quoted(value.value()); + } else { + oss << value.value(); + } first = false; } - if (sent_acknowledgement.has_value()) { - if (!first) oss << ", "; - oss << "sent_acknowledgement=" << sent_acknowledgement.value(); - first = false; // NOLINT: Indeed, this is the last field, but we set first - // to false for consistency. - } - return oss.str(); } @@ -203,26 +220,11 @@ std::vector> CanDeviceConfiguration::set_parameters() const noexcept { std::vector> parameters; - if (bus_name.has_value()) - parameters.emplace_back("bus_name", bus_name.value()); - if (bus_number.has_value()) - parameters.emplace_back("bus_number", std::to_string(bus_number.value())); - if (host.has_value()) parameters.emplace_back("host", host.value()); - if (bitrate.has_value()) - parameters.emplace_back("bitrate", std::to_string(bitrate.value())); - if (enable_termination.has_value()) - parameters.emplace_back("enable_termination", - enable_termination.value() ? "true" : "false"); - if (high_speed.has_value()) - parameters.emplace_back("high_speed", - high_speed.value() ? "true" : "false"); - if (timeout.has_value()) - parameters.emplace_back("timeout", std::to_string(timeout.value())); - if (vcan.has_value()) - parameters.emplace_back("vcan", vcan.value() ? "true" : "false"); - if (sent_acknowledgement.has_value()) - parameters.emplace_back("sent_acknowledgement", - std::to_string(sent_acknowledgement.value())); + for (const auto& field : fields()) { + if (auto value = value_as_string(*this, field)) { + parameters.emplace_back(field.name, *value); + } + } return parameters; } diff --git a/test/python/test_canmodule_utils_config.py b/test/python/test_canmodule_utils_config.py index a8c964a5..68c46f9d 100644 --- a/test/python/test_canmodule_utils_config.py +++ b/test/python/test_canmodule_utils_config.py @@ -4,7 +4,6 @@ import pytest - ROOT = Path(__file__).resolve().parents[2] sys.path.append(str(ROOT / "python")) sys.path.append(str(ROOT / "build")) @@ -22,17 +21,17 @@ def write_json(tmp_path, data): def test_parse_set_item_accepts_uint32(): - assert config.parse_set_item("bitrate=500000") == ("bitrate", 500000) + assert config.parse_set_item("bitrate=500000") == ("bitrate", "500000") def test_parse_set_item_accepts_bool(): - assert config.parse_set_item("vcan=true") == ("vcan", True) + assert config.parse_set_item("vcan=true") == ("vcan", "true") def test_parse_set_item_normalizes_hyphenated_key(): assert config.parse_set_item("enable-termination=false") == ( "enable_termination", - False, + "false", ) @@ -51,20 +50,6 @@ def test_parse_set_item_rejects_comma_separated_values(): config.parse_set_item("bitrate=500000,vcan=false") -def test_parse_bool_rejects_invalid_values(): - with pytest.raises(ValueError, match="invalid boolean value"): - config.parse_bool("maybe") - - -def test_parse_uint32_rejects_negative_values(): - with pytest.raises(ValueError, match="outside 0..0xFFFFFFFF"): - config.parse_uint32("-1") - - -def test_parse_uint32_accepts_hexadecimal_values(): - assert config.parse_uint32("0x7A120") == 500000 - - def test_load_json_config_accepts_flat_object(tmp_path): path = write_json( tmp_path, @@ -78,9 +63,9 @@ def test_load_json_config_accepts_flat_object(tmp_path): assert config.load_json_config(path) == { "bus_name": "can0", - "bitrate": 500000, - "timeout": 100, - "vcan": False, + "bitrate": "500000", + "timeout": "100", + "vcan": "false", } @@ -115,8 +100,8 @@ def test_merge_precedence_is_json_then_set(tmp_path): assert merged == { "bus_name": "can0", - "bitrate": 125000, - "vcan": True, + "bitrate": "125000", + "vcan": "true", } @@ -173,12 +158,12 @@ def test_build_can_device_configuration_sets_all_supported_fields(): configuration = config.build_can_device_configuration( { "host": "192.168.1.20", - "bus_number": 0, - "bitrate": 125000, - "enable_termination": True, - "high_speed": False, - "timeout": 6000, - "sent_acknowledgement": 1, + "bus_number": "0", + "bitrate": "125000", + "enable_termination": "true", + "high_speed": "false", + "timeout": "6000", + "sent_acknowledgement": "1", } )