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 - 1E2 for scaled unitless values #894

Merged
merged 3 commits into from
May 1, 2024

Conversation

hamishwillee
Copy link
Contributor

This falls out of the discussion in mavlink/mavlink#2062 (comment)

GPS_RAW_INT.eph and .evp and same property names in HIL_GPS are unitless values, so correctly don't have units added.
However they are scaled by 100 and there is no way to know that programmatically. IN order to allow programmatic comparisons and conversions it is useful to supply this information.

This creates a unit 1E2 that indicates a unitless value scaled by 100.

FYI @shancock884

@peterbarker
Copy link
Contributor

Did you consider the pros/cons of adding a "mult" attribute instead? We do this in ArduPilot dataflash logs - carry both a unit and a multiplier in the message field definitions.

@hamishwillee
Copy link
Contributor Author

hamishwillee commented Dec 21, 2023

@peterbarker No I didn't. I quite like this idea though.

Would this be applied to all the existing units too? So for example units="degE7" would be come units="deg" mult="E7"?

Or would this apply only to unitless quantities?

@peterbarker
Copy link
Contributor

Would this be applied to all the existing units too? So for example units="degE7" would be come units="deg" mult="E7"?

You could certainly make that change; we try to use SI base units in ArduPilot logs for example. But given it might take a significant period of time for clients to understand the new attribute it might be worth delaying such a change until they've had a chance to catch up.

So introduce it in this one spot initially.

Or would this apply only to unitless quantities?

I'd suggest generally rather than just unitless quantities

@shancock884
Copy link
Contributor

shancock884 commented Dec 22, 2023

I agree its a much cleaner idea to have something like a "mult" attribute, rather than polluting the units attribute.
Given that the schema includes a few particularly funky units (like "m/s*5" and "deg/2"), I wondered if the debate had been had before?
Note that doing this would also remove the need for #885.

We would need to be clear which direction the gain goes (raw->engineering value or the opposite). For example if something like degE7 were to be changed, I think it maybe should be mult="1E-7" (i.e. minus not plus: raw*1E-7=deg).

As for changing of existing units, I think the particularly funky gained units (like "m/s*5" and "deg/2") would be good early candidates to be switched over.
In the longer term, if and when other things get changed, I would not suggest going to purely base types - for example altitude makes sense in m, but camera focal_length is more widely understood in mm. That way tools can render the value in whatever is the accepted norm for readability.

@peterbarker
Copy link
Contributor

peterbarker commented Dec 23, 2023 via email

@hamishwillee
Copy link
Contributor Author

hamishwillee commented Jan 4, 2024

OK, so this adds an attribute named multiplier that can be used in params and messages, which at this time takes just one value "1E7".

I'd like to have it the same way around as in ArduPilot dataflash logs - whichever that is :-)

@peterbarker I can adjust if you tell me which one you want? This uses hard coded specific multipliers for now - I guess we could use a pattern, but I think this is good for now.

FWIW I agree with the general sentiment to update these slowly and carefully, starting with the easy ones. I don't see it as too risky though because I suspect few are using these as anything other than "documentation".

@shancock884
Copy link
Contributor

OK, so this adds an attribute named multiplier that can be used in params and messages, which at this time takes just one value "1E7".

You mean "1E2" not "1E7"! (the commit is right)

I'd like to have it the same way around as in ArduPilot dataflash logs - whichever that is :-)

@peterbarker I can adjust if you tell me which one you want? This uses hard coded specific multipliers for now - I guess we could use a pattern, but I think this is good for now.

From what I see in the AP_Logger README file, the multipliers are specified the other way round. e.g. multipler 'G'='1e-7' is used for lat/lon integers.
Just to confuse things a little, the deprecated format identifiers are written the other way round, e.g. "int16_t * 100" means "real_value = raw_value / 100"!

I don't know if a pattern would be better to avoid having to update the schema for any new things?
Although I suppose most multipliers will end up being from a pretty common list.
Azimuth (PR#885) is a bit of an outlier, as I think it would need a value of either 0.708333333 or 1.411764706.

@hamishwillee
Copy link
Contributor Author

hamishwillee commented Feb 14, 2024

@peterbarker Can you please add github suggestions (or just edit) it the way you want it to be - or merge :-). NO point us playing Chinese whispers.

@peterbarker
Copy link
Contributor

@peterbarker I can adjust if you tell me which one you want? This uses hard coded specific multipliers for now - I guess we could use a pattern, but I think this is good for now.

From what I see in the AP_Logger README file, the multipliers are specified the other way round. e.g. multipler 'G'='1e-7' is used for lat/lon integers. Just to confuse things a little, the deprecated format identifiers are written the other way

OK, so my decade-old reasoning for this is that you can treat it exactly as an exponent. So a multiplier of 0 means 1e0 means 1. A multiplier of 2 is 1e2 is 100.

The letters are their offset from 1e**((letteroffset + 1) * -1)

i.e.
1e**((A-A+1)-1) == 1e-1 == 1/10
1e**((B-A+1)
-1) == 1e-2 == 1/100
1e**((C-A+1)-1) == 1e-3 == 1/1000
1e**((D-A+1)
-1) == 1e-4
1e**((E-A+1)-1) == 1e-5
1e**((F-A+1)
-1) == 1e-6
1e**((G-A+1)*-1) == 1e-7

... made sense to me at the time...

If we can make this the same as dataflash logs that's going to be very convenient :-)

Those legacy identifiers in the dataflash logs should be killed off. The only tooling I'd want to check is the rather atrophied Python log checker in the ArduPilot tree.

@shancock884
Copy link
Contributor

So, if I understand correctly, this is the way round we want to align...

    <xs:enumeration value="1E-2"/>        <!-- engineering value = raw value * 1E-2 -->

generator/mavschema.xsd Outdated Show resolved Hide resolved
@hamishwillee
Copy link
Contributor Author

@peterbarker What else does this need?

@peterbarker
Copy link
Contributor

@peterbarker What else does this need?

A rebase?

Any particular reason to limit this to unitless values?

@tridge
Copy link
Contributor

tridge commented Apr 29, 2024

needs manual rebase

@hamishwillee
Copy link
Contributor Author

Rebased.

Any particular reason to limit this to unitless values?

@peterbarker It isn't actually limited to unitless values by the XSD - for that we'd have had to either/or condition on the units field.

The reason for the title of unitless is that is why we need it - to indicate scaling in the cases where there are no units.

We did discuss whether it would be useful to use with with units as well, so for example replace the units degE7 with deg and a scaling factor of 1E-7. I'm not opposed to the idea but I thought it was you who suggested that initially we'd just use it for the unitless values and "see how we'll go".

Thoughts?

@peterbarker peterbarker merged commit c889cba into ArduPilot:master May 1, 2024
12 checks passed
@shancock884
Copy link
Contributor

Any particular reason to limit this to unitless values?

One example with a unit that I think it could be sensible to use it is on GPS_STATUS.satellite_azimuth, where the unit says it is in deg, but the description says it is really scaled such that a value of 255 means 360 deg.

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.

5 participants