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

MavSchema: Allow 64-bit enum values to be specified #915

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

shancock884
Copy link
Contributor

Within the Mavlink schema, enum values are currently limited to 10 decimal, 8 hex, or 32 binary digits.
However, we already have two fields which are bitmasks and use uint64_t type (AUTOPILOT_VERSION.capabilities and GENERATOR_STATUS.status). This character limit means that you can't currently specify a value using (most of) the higher 32-bits.
I came across this while trying to build a test case for #895.

My proposal is to extend the character limit to 20 decimal, 16 hex or 64 binary digits to allow for this.
@hamishwillee - does this make sense?

I did wonder if it might be useful to be allowed to specify values as powers of 2 (e.g. "2^0", "2^4", "2^55") to be more readable as the numbers get bigger, but as this would require work on the generators, so have not proposed it.
I also wondered if the test.xml file in Mavlink repo should include at least one enum and bitmask, if it is supposed to be used a nice little test file cover major functionality?

@hamishwillee
Copy link
Contributor

@shancock884 Thanks very much for this. I think it makes sense but I don't have merge rights in this repo.

@peterbarker @amilcarlucas Can you please review this.

I did wonder if it might be useful to be allowed to specify values as powers of 2 (e.g. "2^0", "2^4", "2^55") to be more readable as the numbers get bigger, but as this would require work on the generators, so have not proposed it.

It would be useful for bitmask readability. I'd like it, but IMO this would be hard to get in. I suspect you could do this easily enough for mavgen by converting the values in the parser phase so that all the generators get a converted value. But there are more than just the generators in this repo to think about. @peterbarker might have other thoughts.

I also wondered if the test.xml file in Mavlink repo should include at least one enum and bitmask, if it is supposed to be used a nice little test file cover major functionality?

Absolutely, or a new test file and test. Again, discussion with Peter about what test makes sense would be good.

@peterbarker peterbarker merged commit f7a2f29 into ArduPilot:master Feb 7, 2024
12 checks passed
@peterbarker
Copy link
Contributor

I like all of those ideas.

@shancock884
Copy link
Contributor Author

Regarding the "2^n" idea, mavparse.py can almost cope with it already, as it does an "eval" on the value string.
The issue though is that "^" means bitwise-not, and not raise-to-power in Python expressions.
So either, mavparse would need to string replace "^" => "**" first, or the schema get defined such that "**" is used (i.e. directly use Python expression syntax).

@shancock884 shancock884 deleted the mavschema-64-bit-bitmask branch February 7, 2024 16:27
@amilcarlucas
Copy link
Contributor

I vote for the simpler ** Syntax

@hamishwillee
Copy link
Contributor

FWIW More than happy for you to try it but:

  • consider restricting to bitmasks
  • allow either format
  • we will need to have a period where other generators have a chance to catch up before we modify the XML.

@shancock884
Copy link
Contributor Author

shancock884 commented Feb 10, 2024

I like all of those ideas.

I vote for the simpler ** Syntax

FWIW More than happy for you to try it but:

As there seemed to be some enthusiasm for the power-of-2 idea, I've created PR #920!

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.

4 participants