-
-
Notifications
You must be signed in to change notification settings - Fork 582
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mutually exclusive option groups #461
Mutually exclusive option groups #461
Conversation
Hi, I think there is a philosophical problem here. Ideally different configurator versions should look into different preset branches. Something to discuss maybe with @blckmn |
Yeah.. I thought about that a little, it's definitely a good question and could lead to a bit of a maintenance nightmare if someone wants to update presets for a previous version of the configurator, although I'd imagine most people stick to the latest release or two. But if that were the case, you'd have to backport the preset changes/updates to another branch, as well as each BF folder, i.e. 4.4, 4.5, etc. In this case, I believe the changes to the configurator itself is backwards compatible, as it has to work with option groups that do, want the ability to check multiple options between the group but I'd need to do some more testing. I also wondered, why don't we generate a more consumer friendly output for the configurator? The |
I think for backwards compatibility the new syntax should be like that: #$ OPTION GROUP BEGIN: (Exclusive) My Group Then the old configurator will act like "(Exclusive) My Group" is just a group name. And new configurator could remove (Exclusive) from the name when it detects it, and make it exclusive. Sounds like a hack tho, but a neat one lol. About more readable output files, how would they be different than the preset files? We probably could unwrap the #$ include statements directly in preset repository, but what else? |
If you can get it working in terms of compatibility etc, this is an awesome upgrade |
Just don't tell @TehllamaFPV about it lol. |
@limonspb I more meant outputting the presets as the parsed JSON, which the configurator could use to say |
Oh I see what you mean. Yeah that would be sweet. The reason I did this originally is because initially presets planned as just simple CLI text with the filename. Then slowly realized we we need all the features. So basically poor planning |
@Stampede10343 Practically, that would be the direction I had been haggling for pushing towards a year or two ago where the eventual end capability would be direct entry of numbers or strings that would enable parametrically set values (e.g. input KV of motors or AUW) to feed in as tuning parameters and add logic to. @limonspb I'll ask you this very specific question: when was the last time you saw somebody ask 'what PIDs are you running' in response to a video or basically any social media content? It's been 18mo or more for me, instead the questions is always 'which preset should I use to get my stuff to fly like that'. In the engineering sense, Minimum Viable Product has been illustrated so comprehensively that your thing is everywhere for a very good reason. |
this is a good idea. what do you think @Stampede10343 ? |
Yeah I can look to change to that format. I'll see if I can get it updated this evening |
@limonspb @sugaarK My fork's develop branch has the updated index if you need a preset repo to test with: |
964692f
to
b8c6092
Compare
This is great work @Stampede10343! |
Co-authored-by: Mark Haslinghuis <[email protected]>
Use consistent uppercase syntax Co-authored-by: Mark Haslinghuis <[email protected]>
Add support for mutually exclusive option groups. This uses the directive
(Exclusive)
that can optionally be added onOPTION GROUP
, .e.g.#$ OPTION GROUP BEGIN (Exclusive): My Group of Options that should only have one checked at once:
This allows preset authors to write presets that enforce only one of several options to be selected at once, which is helpful when applying multiple would produce unintended results.
See this PR for Configurator updates to support the new directive.
Let me know if I should break the changes to the presets into its own PR.