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 enum values to be powers of 2 #920

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

shancock884
Copy link
Contributor

@shancock884 shancock884 commented Feb 10, 2024

This is a follow-up from comments in #915.
It allows use of powers of 2 to specify enum values - which would be most useful for big bitmasks.
Example syntax is: <entry value="2**7" name="BIT7" />

@hamishwillee in #915 (comment) wrote:

FWIW More than happy for you to try it but:

  • consider restricting to bitmasks

I couldn't see how this was possible, as this is part of the rule for the 'value' field of enum 'entry' tag. Although I suspect xsd syntax can do many things that I don't know about!

  • allow either format

The syntax allows the user to use any of: decimal, hex, binary, power of 2.

  • we will need to have a period where other generators have a chance to catch up before we modify the XML.

No XML has been changed.

Note that the comment next to the binary rule has also been corrected from 'base 1' to 'base 2'.

Testing

I tried out the full variety of enum value syntax with Mavgen, and it works without any modifications, as it already does an 'eval' on the value. e.g.:

      <entry value="1" name="BIT0" />
      <entry value="2**4" name="BIT4" />
      <entry value="0b000100000000" name="BIT8" />
      <entry value="0x10000" name="BIT16" />
      <entry value="0b1000000000000000000000000000000000000000000000000000000000000" name="BIT60" />
      <entry value="2305843009213693952" name="BIT61" />
      <entry value="2**62" name="BIT62" />
      <entry value="0x8000000000000000" name="BIT63" />

I was planning to include this in the Wireshark generator test I am building for the fix to issue: #895 (of which the above is an extract). This would mean it will be tested as part of pymavlink CI.

@shancock884 shancock884 marked this pull request as ready for review February 10, 2024 19:31
@peterbarker
Copy link
Contributor

peterbarker commented Feb 10, 2024 via email

@hamishwillee
Copy link
Contributor

hamishwillee commented Feb 15, 2024

Firstly, thanks @shancock884 !

My general preference would have been to use the ^ symbol, as that is by far the most common operator used for indicating exponent. I guess you used the ** operator because it is the exponent symbol in Python?

That makes sense for the mavgen parser because I assume it evaluates this value in the parsing stage and stores it as the full number. So when code is generated, the output is already converted - right?

I am not sure we can make that assumption for other parsers, but in any case, as we discussed last night in the dev call they need to be updated anyway. I have emailed the GCS group for discussion and pinged a few of the other generators https://mavlink.io/en/#external-generatorslanguages - you can see the ones I added comments for as linked issues

@hamishwillee
Copy link
Contributor

FYI, from OllieW:

MANY THX for informing me, this is really much appreciated.
I too would have preferred the more common '^' over '**', but I think it's anyway a very good addition, making things easier. Very cool.

@shancock884
Copy link
Contributor Author

My general preference would have been to use the ^ symbol, as that is by far the most common operator used for indicating exponent. I guess you used the ** operator because it is the exponent symbol in Python?

I also would have preferred '^' visually, but as '^' is bitwise-not in Python (and Javascript), parsers that simply do 'eval' on the string would end up with the wrong number (e.g. 2^3 => 1) and it may not be obvious that something has gone wrong, whereas using '**' may well work without (or with easy) modifications in parsers written in Python and JS.

That makes sense for the mavgen parser because I assume it evaluates this value in the parsing stage and stores it as the full number. So when code is generated, the output is already converted - right?

Correct.

@hamishwillee
Copy link
Contributor

@shancock884 Thanks for the clarifications.

While XML is supposed to be language agnostic I think using ** is probably the lowest risk and cost option. It still is obviously not multiplication, and it is likely to loudly break other parsers that don't understand it rather than silently do so.

@hamishwillee
Copy link
Contributor

@peterbarker What do you think of this? Can we get it merged?

@peterbarker
Copy link
Contributor

peterbarker commented Feb 21, 2024 via email

@tridge tridge merged commit f4e45e4 into ArduPilot:master Feb 21, 2024
12 checks passed
@shancock884 shancock884 deleted the mavschema-value-power-of-2 branch February 21, 2024 08:56
@hamishwillee
Copy link
Contributor

Thanks very much. Good to see.

@ThomasDebrunner
Copy link

Implemented in libmav here: Auterion/libmav#42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants