Skip to content
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

Conversation

Stampede10343
Copy link
Contributor

@Stampede10343 Stampede10343 commented May 8, 2024

Add support for mutually exclusive option groups. This uses the directive (Exclusive) that can optionally be added on OPTION 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.

@limonspb
Copy link
Member

limonspb commented May 8, 2024

Hi,
great job!

I think there is a philosophical problem here.
If we change the existing preset files, how would the current (10.10) or previous configurator versions react on the unknown #$ commands?

Ideally different configurator versions should look into different preset branches. Something to discuss maybe with @blckmn

@Stampede10343
Copy link
Contributor Author

Stampede10343 commented May 8, 2024

Hi, great job!

I think there is a philosophical problem here. If we change the existing preset files, how would the current (10.10) or previous configurator versions react on the unknown #$ commands?

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 index.json gets generated but it seems like mostly metadata. If we took the parsed output of the presets and either added them or had another presets.json file we saved, I believe it would cut down on some of the manual/duplicated string matching and parsing that happens in the configurator, and just read normal JSON data that's already in a nice to use format.

@limonspb
Copy link
Member

limonspb commented May 8, 2024

Hi, great job!
I think there is a philosophical problem here. If we change the existing preset files, how would the current (10.10) or previous configurator versions react on the unknown #$ commands?
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 index.json gets generated but it seems like mostly metadata. If we took the parsed output of the presets and either added them or had another presets.json file we saved, I believe it would cut down on some of the manual/duplicated string matching and parsing that happens in the configurator, and just read normal JSON data that's already in a nice to use format.

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?

@SupaflyFPV
Copy link
Contributor

If you can get it working in terms of compatibility etc, this is an awesome upgrade

@limonspb
Copy link
Member

limonspb commented May 8, 2024

If you can get it working in terms of compatibility etc, this is an awesome upgrade

Just don't tell @TehllamaFPV about it lol.

@Stampede10343
Copy link
Contributor Author

@limonspb I more meant outputting the presets as the parsed JSON, which the configurator could use to say preset.option_group[0].name. Instead of having to do lowercaseLine.includes(exlusiveDirective);. Have the index.json file be more like a API response, rather than doing line by line processing of the preset text file. Idk, probably outside the scope of this PR, but I figured it would make implementing additional preset related features easier on the configurator side, and this lib could do all the complicated parsing to leave the UI as simple as possible.

@limonspb
Copy link
Member

limonspb commented May 9, 2024

@limonspb I more meant outputting the presets as the parsed JSON, which the configurator could use to say preset.option_group[0].name. Instead of having to do lowercaseLine.includes(exlusiveDirective);. Have the index.json file be more like a API response, rather than doing line by line processing of the preset text file. Idk, probably outside the scope of this PR, but I figured it would make implementing additional preset related features easier on the configurator side, and this lib could do all the complicated parsing to leave the UI as simple as possible.

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

@TehllamaFPV
Copy link
Contributor

TehllamaFPV commented May 9, 2024

@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.
In the end, just having switch statement options would simplify a lot of stuff, but that mostly benefits the kind of idiot who has presets with 40+ options present on them, some of which would behave much better with exclusive, but in practice tend to overwrite data to the last checked box, and produce fairly respectable behavior given user expectations.

@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.

@sugaarK
Copy link
Member

sugaarK commented Jun 18, 2024

Hi, great job!
I think there is a philosophical problem here. If we change the existing preset files, how would the current (10.10) or previous configurator versions react on the unknown #$ commands?
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 index.json gets generated but it seems like mostly metadata. If we took the parsed output of the presets and either added them or had another presets.json file we saved, I believe it would cut down on some of the manual/duplicated string matching and parsing that happens in the configurator, and just read normal JSON data that's already in a nice to use format.

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?

this is a good idea. what do you think @Stampede10343 ?

@Stampede10343
Copy link
Contributor Author

Hi, great job!
I think there is a philosophical problem here. If we change the existing preset files, how would the current (10.10) or previous configurator versions react on the unknown #$ commands?
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 index.json gets generated but it seems like mostly metadata. If we took the parsed output of the presets and either added them or had another presets.json file we saved, I believe it would cut down on some of the manual/duplicated string matching and parsing that happens in the configurator, and just read normal JSON data that's already in a nice to use format.

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?

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

@Stampede10343
Copy link
Contributor Author

@limonspb @sugaarK
Okay I think I've got this PR as well as the configurator PR updated!

My fork's develop branch has the updated index if you need a preset repo to test with:
https://github.com/Stampede10343/firmware-presets/tree/develop

@Stampede10343 Stampede10343 force-pushed the feature/mutually-exclusive-option-groups branch from 964692f to b8c6092 Compare June 23, 2024 17:38
@sugaarK sugaarK requested review from limonspb and ctzsnooze June 25, 2024 06:22
@sugaarK sugaarK requested a review from KarateBrot June 25, 2024 06:22
@limonspb
Copy link
Member

This is great work @Stampede10343!
Give us some time to test, please.

README.md Outdated Show resolved Hide resolved
Co-authored-by: Mark Haslinghuis <[email protected]>
README.md Outdated Show resolved Hide resolved
Stampede10343 and others added 2 commits July 24, 2024 20:47
@haslinghuis haslinghuis merged commit 5b311f0 into betaflight:master Jul 25, 2024
1 check passed
@Stampede10343 Stampede10343 deleted the feature/mutually-exclusive-option-groups branch July 31, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants