From 9062a531f32124f0fd9b171402347308fcfcf3e4 Mon Sep 17 00:00:00 2001 From: David Given Date: Thu, 21 Aug 2025 00:53:50 +0200 Subject: [PATCH] Migrate the 40track etc extension configs to actual options. Add the ability to have --group=value options to make this cleaner. --- build.py | 18 ++-- doc/disk-40track_drive.md | 2 +- doc/disk-apple2_drive.md | 2 +- doc/disk-shugart_drive.md | 2 +- lib/config/config.cc | 145 ++++++++++++----------------- lib/config/config.h | 22 ++++- lib/config/config.proto | 3 +- lib/config/flags.cc | 3 +- src/formats/40track_drive.textpb | 22 ----- src/formats/_global_options.textpb | 77 +++++++++++++++ src/formats/apple2_drive.textpb | 29 ------ src/formats/build.py | 4 +- src/formats/shugart_drive.textpb | 22 ----- tests/options.cc | 12 +-- 14 files changed, 175 insertions(+), 188 deletions(-) delete mode 100644 src/formats/40track_drive.textpb create mode 100644 src/formats/_global_options.textpb delete mode 100644 src/formats/apple2_drive.textpb delete mode 100644 src/formats/shugart_drive.textpb diff --git a/build.py b/build.py index 9e61d692..38b0c9cb 100644 --- a/build.py +++ b/build.py @@ -28,7 +28,7 @@ else: ("acorndfs", "", "--200"), ("agat", "", ""), ("amiga", "", ""), - ("apple2", "", "--140 -c 40track_drive"), + ("apple2", "", "--140 --drivetype=40"), ("atarist", "", "--360"), ("atarist", "", "--370"), ("atarist", "", "--400"), @@ -38,17 +38,17 @@ else: ("atarist", "", "--800"), ("atarist", "", "--820"), ("bk", "", ""), - ("brother", "", "--120 -c 40track_drive"), + ("brother", "", "--120 --drivetype=40"), ("brother", "", "--240"), ( "commodore", "scripts/commodore1541_test.textpb", - "--171 -c 40track_drive", + "--171 --drivetype=40", ), ( "commodore", "scripts/commodore1541_test.textpb", - "--192 -c 40track_drive", + "--192 --drivetype=40", ), ("commodore", "", "--800"), ("commodore", "", "--1620"), @@ -60,17 +60,17 @@ else: ("ibm", "", "--1232"), ("ibm", "", "--1440"), ("ibm", "", "--1680"), - ("ibm", "", "--180 -c 40track_drive"), - ("ibm", "", "--160 -c 40track_drive"), - ("ibm", "", "--320 -c 40track_drive"), - ("ibm", "", "--360 -c 40track_drive"), + ("ibm", "", "--180 --drivetype=40"), + ("ibm", "", "--160 --drivetype=40"), + ("ibm", "", "--320 --drivetype=40"), + ("ibm", "", "--360 --drivetype=40"), ("ibm", "", "--720_96"), ("ibm", "", "--720_135"), ("mac", "scripts/mac400_test.textpb", "--400"), ("mac", "scripts/mac800_test.textpb", "--800"), ("n88basic", "", ""), ("rx50", "", ""), - ("tartu", "", "--390 -c 40track_drive"), + ("tartu", "", "--390 --drivetype=40"), ("tartu", "", "--780"), ("tids990", "", ""), ("victor9k", "", "--612"), diff --git a/doc/disk-40track_drive.md b/doc/disk-40track_drive.md index 39b65558..203628f8 100644 --- a/doc/disk-40track_drive.md +++ b/doc/disk-40track_drive.md @@ -10,6 +10,6 @@ no way to detect this automatically. For example: ``` -fluxengine read -c ibm --180 40track_drive +fluxengine read ibm --180 40track_drive ``` diff --git a/doc/disk-apple2_drive.md b/doc/disk-apple2_drive.md index 6fa952e4..e631d138 100644 --- a/doc/disk-apple2_drive.md +++ b/doc/disk-apple2_drive.md @@ -11,6 +11,6 @@ connector. For example: ``` -fluxengine read -c apple2 --160 apple2_drive +fluxengine read apple2 --160 apple2_drive ``` diff --git a/doc/disk-shugart_drive.md b/doc/disk-shugart_drive.md index a1f8685a..086e1d39 100644 --- a/doc/disk-shugart_drive.md +++ b/doc/disk-shugart_drive.md @@ -10,6 +10,6 @@ on Greaseweazle hardware. For example: ``` -fluxengine read -c ibm --720 shugart_drive +fluxengine read ibm --720 shugart_drive ``` diff --git a/lib/config/config.cc b/lib/config/config.cc index 3a96df88..cec45b20 100644 --- a/lib/config/config.cc +++ b/lib/config/config.cc @@ -5,6 +5,7 @@ #include "lib/core/utils.h" #include #include +#include static Config config; @@ -181,35 +182,8 @@ ConfigProto* Config::combined() { _combinedConfig = _baseConfig; - /* First apply any standalone options. */ - - std::set options = _appliedOptions; - for (const auto& option : _baseConfig.option()) - { - if (options.find(option.name()) != options.end()) - { - _combinedConfig.MergeFrom(option.config()); - options.erase(option.name()); - } - } - - /* Then apply any group options. */ - - for (auto& group : _baseConfig.option_group()) - { - const OptionProto* selectedOption = &*group.option().begin(); - - for (auto& option : group.option()) - { - if (options.find(option.name()) != options.end()) - { - selectedOption = &option; - options.erase(option.name()); - } - } - - _combinedConfig.MergeFrom(selectedOption->config()); - } + for (const auto& optionInfo : _appliedOptions) + _combinedConfig.MergeFrom(optionInfo.option->config()); /* Add in the user overrides. */ @@ -241,51 +215,27 @@ std::vector Config::validate() { std::vector results; - std::set optionNames = _appliedOptions; - std::set appliedOptions; - for (const auto& option : _baseConfig.option()) - { - if (optionNames.find(option.name()) != optionNames.end()) + /* Ensure that only one item in each group is set. */ + + std::map optionsByGroup; + for (auto [group, option, hasArgument] : _appliedOptions) + if (group) { - appliedOptions.insert(&option); - optionNames.erase(option.name()); + auto& o = optionsByGroup[group]; + if (o) + results.push_back( + fmt::format("multiple mutually exclusive values set for " + "group '{}': valid values are: {}", + group->comment(), + fmt::join(std::views::transform( + group->option(), &OptionProto::name), + ", "))); + o = option; } - } - - /* Then apply any group options. */ - - for (auto& group : _baseConfig.option_group()) - { - int count = 0; - - for (auto& option : group.option()) - { - if (optionNames.find(option.name()) != optionNames.end()) - { - optionNames.erase(option.name()); - appliedOptions.insert(&option); - - count++; - if (count == 2) - results.push_back( - fmt::format("multiple mutually exclusive options set " - "for group '{}'", - group.comment())); - } - } - } - - /* Check for unknown options. */ - - if (!optionNames.empty()) - { - for (auto& name : optionNames) - results.push_back(fmt::format("'{}' is not a known option", name)); - } /* Check option requirements. */ - for (auto& option : appliedOptions) + for (auto [group, option, hasArgument] : _appliedOptions) { try { @@ -360,11 +310,12 @@ void Config::readBaseConfig(std::string data) error("couldn't load external config proto"); } -const OptionProto& Config::findOption(const std::string& optionName) +Config::OptionInfo Config::findOption( + const std::string& name, const std::string value) { const OptionProto* found = nullptr; - auto searchOptionList = [&](auto& optionList) + auto searchOptionList = [&](auto& optionList, const std::string& optionName) { for (const auto& option : optionList) { @@ -377,17 +328,39 @@ const OptionProto& Config::findOption(const std::string& optionName) return false; }; - if (searchOptionList(base()->option())) - return *found; + /* First look for any group names which match. */ + + if (!value.empty()) + for (const auto& optionGroup : base()->option_group()) + if (optionGroup.name() == name) + { + /* The option must therefore be one of these. */ + + if (searchOptionList(optionGroup.option(), value)) + return {&optionGroup, found, true}; + + throw OptionNotFoundException(fmt::format( + "value {} is not valid for option {}; valid values are: {}", + value, + name, + fmt::join(std::views::transform( + optionGroup.option(), &OptionProto::name), + ", "))); + } + + /* Now search for individual options. */ + + if (searchOptionList(base()->option(), name)) + return {nullptr, found, false}; for (const auto& optionGroup : base()->option_group()) { - if (searchOptionList(optionGroup.option())) - return *found; + if (optionGroup.name().empty()) + if (searchOptionList(optionGroup.option(), name)) + return {nullptr, found, false}; } - throw OptionNotFoundException( - fmt::format("option {} not found", optionName)); + throw OptionNotFoundException(fmt::format("option {} not found", name)); } void Config::checkOptionValid(const OptionProto& option) @@ -445,22 +418,20 @@ bool Config::isOptionValid(const OptionProto& option) } } -bool Config::isOptionValid(std::string option) -{ - return isOptionValid(findOption(option)); -} - -void Config::applyOption(const OptionProto& option) +void Config::applyOption(const OptionInfo& optionInfo) { + auto* option = optionInfo.option; log(OptionLogMessage{ - option.has_message() ? option.message() : option.comment()}); + option->has_message() ? option->message() : option->comment()}); - _appliedOptions.insert(option.name()); + _appliedOptions.insert(optionInfo); } -void Config::applyOption(std::string option) +bool Config::applyOption(const std::string& name, const std::string value) { - applyOption(findOption(option)); + auto optionInfo = findOption(name, value); + applyOption(optionInfo); + return optionInfo.usesValue; } void Config::clearOptions() diff --git a/lib/config/config.h b/lib/config/config.h index 5025347e..e3026336 100644 --- a/lib/config/config.h +++ b/lib/config/config.h @@ -66,6 +66,18 @@ struct FluxConstructor class Config { +private: + struct OptionInfo + { + bool operator==(const OptionInfo& other) const = default; + std::strong_ordering operator<=>( + const OptionInfo& other) const = default; + + const OptionGroupProto* group; + const OptionProto* option; + bool usesValue; + }; + public: /* Direct access to the various proto layers. */ @@ -124,12 +136,12 @@ public: /* Option management: look up an option by name, determine whether an option * is valid, and apply an option. */ - const OptionProto& findOption(const std::string& option); + OptionInfo findOption( + const std::string& name, const std::string value = ""); void checkOptionValid(const OptionProto& option); bool isOptionValid(const OptionProto& option); - bool isOptionValid(std::string option); - void applyOption(const OptionProto& option); - void applyOption(std::string option); + void applyOption(const OptionInfo& optionInfo); + bool applyOption(const std::string& name, const std::string value = ""); void clearOptions(); /* Adjust overall inputs and outputs. */ @@ -165,7 +177,7 @@ private: ConfigProto _baseConfig; ConfigProto _overridesConfig; ConfigProto _combinedConfig; - std::set _appliedOptions; + std::set _appliedOptions; bool _configValid; FluxSourceProto _verificationFluxSourceProto; diff --git a/lib/config/config.proto b/lib/config/config.proto index e98c85ec..9db8f972 100644 --- a/lib/config/config.proto +++ b/lib/config/config.proto @@ -73,5 +73,6 @@ message OptionProto message OptionGroupProto { optional string comment = 1 [(help) = "help text for option group"]; - repeated OptionProto option = 2; + optional string name = 2 [(help) = "option group name"]; + repeated OptionProto option = 3; } diff --git a/lib/config/flags.cc b/lib/config/flags.cc index f474673b..f61b0dac 100644 --- a/lib/config/flags.cc +++ b/lib/config/flags.cc @@ -158,7 +158,7 @@ std::vector FlagGroup::parseFlagsWithFilenames(int argc, index += usesthat; } else - globalConfig().applyOption(path); + usesthat = globalConfig().applyOption(path, value); } else error("unrecognised flag '-{}'; try --help", key); @@ -197,6 +197,7 @@ void FlagGroup::parseFlagsWithConfigFiles(int argc, const char* argv[], const std::map& configFiles) { + globalConfig().readBaseConfigFile("_global_options"); FlagGroup({this, &configGroup}).parseFlags(argc, argv); } diff --git a/src/formats/40track_drive.textpb b/src/formats/40track_drive.textpb deleted file mode 100644 index 39b16f95..00000000 --- a/src/formats/40track_drive.textpb +++ /dev/null @@ -1,22 +0,0 @@ -comment: 'Adjust configuration for a 40-track drive' -is_extension: true - -documentation: -<<< -This is an extension profile; adding this to the command line will configure -FluxEngine to read from 40-track, 48tpi 5.25" drives. You have to tell it because there is -no way to detect this automatically. - -For example: - -``` -fluxengine read ibm --180 40track_drive -``` ->>> - -drive { - tracks: "c0-40h0-1" - drive_type: DRIVETYPE_40TRACK -} - - diff --git a/src/formats/_global_options.textpb b/src/formats/_global_options.textpb new file mode 100644 index 00000000..7619f5da --- /dev/null +++ b/src/formats/_global_options.textpb @@ -0,0 +1,77 @@ +comment: 'Options which can be applied everywhere.' +is_extension: true + +option_group { + comment: "Drive type" + name: "drivetype" + + option { + name: "80" + comment: '80 track drive' + set_by_default: true + + config { + } + } + + option { + name: "40" + comment: '40 track drive' + + config { + drive { + tracks: "c0-40h0-1" + drive_type: DRIVETYPE_40TRACK + } + } + } + + option { + name: "160" + comment: '160 track Apple II drive' + + config { + drive { + tracks: "c0-159h0" + drive_type: DRIVETYPE_APPLE2 + } + } + } +} + +option_group { + comment: 'Bus interface' + name: "bus" + + option { + name: "pc" + comment: 'PC drive interface' + set_by_default: true + } + + option { + name: "shugart" + comment: 'Shugart bus interface (only on Greaseweazle)' + + config { + usb { + greaseweazle { + bus_type: SHUGART + } + } + } + } + + option { + name: "appleii" + comment: 'Apple II bus interface (only on Greaseweazle)' + + config { + usb { + greaseweazle { + bus_type: APPLE2 + } + } + } + } +} diff --git a/src/formats/apple2_drive.textpb b/src/formats/apple2_drive.textpb deleted file mode 100644 index 64532bcc..00000000 --- a/src/formats/apple2_drive.textpb +++ /dev/null @@ -1,29 +0,0 @@ -comment: 'Adjust configuration for a 40-track Apple II drive' -is_extension: true - -documentation: -<<< -This is an extension profile; adding this to the command line will configure -FluxEngine to adjust the pinout and track spacing to work with an Apple II -drive. This only works on Greaseweazle hardware and requires a custom -connector. - -For example: - -``` -fluxengine read apple2 --160 apple2_drive -``` ->>> - -usb { - greaseweazle { - bus_type: APPLE2 - } -} - -drive { - tracks: "c0-159h0" - drive_type: DRIVETYPE_APPLE2 -} - - diff --git a/src/formats/build.py b/src/formats/build.py index b4689560..99f0591e 100644 --- a/src/formats/build.py +++ b/src/formats/build.py @@ -3,14 +3,13 @@ from build.c import cxxlibrary from scripts.build import protoencode formats = [ - "40track_drive", + "_global_options", "acornadfs", "acorndfs", "aeslanier", "agat", "amiga", "ampro", - "apple2_drive", "apple2", "atarist", "bk", @@ -33,7 +32,6 @@ formats = [ "psos", "rolandd20", "rx50", - "shugart_drive", "smaky6", "tartu", "ti99", diff --git a/src/formats/shugart_drive.textpb b/src/formats/shugart_drive.textpb deleted file mode 100644 index 8734c2fc..00000000 --- a/src/formats/shugart_drive.textpb +++ /dev/null @@ -1,22 +0,0 @@ -comment: 'Adjust configuration for a Shugart drive' -is_extension: true - -documentation: -<<< -This is an extension profile; adding this to the command line will configure -FluxEngine to adjust the pinout to work with a Shugart drive. This only works -on Greaseweazle hardware. - -For example: - -``` -fluxengine read ibm --720 shugart_drive -``` ->>> - -usb { - greaseweazle { - bus_type: SHUGART - } -} - diff --git a/tests/options.cc b/tests/options.cc index 547e9021..37f8af64 100644 --- a/tests/options.cc +++ b/tests/options.cc @@ -55,14 +55,14 @@ static void test_option_validity() } )M"); - AssertThat( - globalConfig().isOptionValid(globalConfig().findOption("option1")), + AssertThat(globalConfig().isOptionValid( + *globalConfig().findOption("option1").option), Equals(true)); - AssertThat( - globalConfig().isOptionValid(globalConfig().findOption("option2")), + AssertThat(globalConfig().isOptionValid( + *globalConfig().findOption("option2").option), Equals(false)); - AssertThat( - globalConfig().isOptionValid(globalConfig().findOption("option3")), + AssertThat(globalConfig().isOptionValid( + *globalConfig().findOption("option3").option), Equals(true)); }