betaflight / firmware-presets

Configuration Snippets for the Betaflight Flight Controller Firmware
GNU General Public License v3.0
103 stars 570 forks source link

Mutually exclusive option groups #461

Closed Stampede10343 closed 3 months ago

Stampede10343 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

limonspb commented 5 months ago

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 commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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

limonspb commented 4 months ago

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