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

Move enums in description to real enums #1997

Closed
patrickelectric opened this issue May 25, 2023 · 6 comments · Fixed by #2039
Closed

Move enums in description to real enums #1997

patrickelectric opened this issue May 25, 2023 · 6 comments · Fixed by #2039

Comments

@patrickelectric
Copy link
Member

patrickelectric commented May 25, 2023

E.g: MAV_CMD_DO_CHANGE_SPEED, has on the first field:

<param index="1" label="Speed Type" minValue="0" maxValue="3" increment="1">Speed type (0=Airspeed, 1=Ground Speed, 2=Climb Speed, 3=Descent Speed)</param>

The speed type should be defined in an enum, and not being used in the description.

Doing so will empower the tools that use the specification to also provide possible valid values without parsing the description (that's just a text without xml validation).

@hamishwillee
Copy link
Collaborator

We often don't want to clutter the namespace if there are only a few values to use and they are unlikely to be extended. On the other hand, enums make it clear than you mean "this value" and prevents flight stacks from doing things like assuming values will not change and condition of greater-than.

I'd be happy with this. Do you want to PR it?

@hamishwillee
Copy link
Collaborator

@patrickelectric ^^^^ ?

@patrickelectric
Copy link
Member Author

Hi @hamishwillee, sorry, I'm in vacation, should be back on this before the end of the month.

@hamishwillee
Copy link
Collaborator

Cool, should be fixed by #2039

@patrickelectric
Copy link
Member Author

Hi @hamishwillee, sorry for not answering it before!

As said, make them as enum help us to have clear behavior and intention. Hiding enums under ranges makes it harder to define and make it clear for the user and tools how the values are used.

@hamishwillee
Copy link
Collaborator

No worries. All good. Merged!

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 a pull request may close this issue.

2 participants