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

Log attitude information at full resolution #28008

Merged
merged 6 commits into from
Sep 10, 2024

Conversation

andyp1per
Copy link
Collaborator

@andyp1per andyp1per commented Sep 4, 2024

We are artificially quantizing the attitude information in the attitude log message. This makes it very hard to relate to the rate logging which is logged at full resolution. There is really no reason to quantize like this other than to save a little bit of log bandwidth.

Split from #27029

@andyp1per andyp1per changed the title Pr att fullres Log attitude information at full resolution Sep 4, 2024
@andyp1per andyp1per marked this pull request as ready for review September 4, 2024 12:09
@peterbarker
Copy link
Contributor

Need to divide the targets:
image

@peterbarker
Copy link
Contributor

We are artificially quantizing the attitude information in the attitude log message. This makes it very hard to relate to the rate logging which is logged at full resolution. There is really no reason to quantize like this other than to save a little bit of log bandwidth.

"a little bit of log bandwidth" sounds a little handy-wavey. Roughly speaking this will double the size of the message.

Which will move its proportion of a log from maybe 0.75% to 1.5%, based on the Plane.AUTOTEST log I'm looking at here.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to scale targets

libraries/AP_AHRS/AP_AHRS_Logging.cpp Outdated Show resolved Hide resolved
@andyp1per
Copy link
Collaborator Author

andyp1per commented Sep 5, 2024

Need to divide the targets:

Fixed. I actually think this is much cleaner now. Might even save a little flash.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fewer cd is good!

AntennaTracker/Log.cpp Outdated Show resolved Hide resolved
ArduCopter/Log.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like the change to log in floats.

But I keep finding these trivial cases where we are breaking ATT, one of the most important logging messages we have. Please check the log message is correct for all vehicles after your changes.

ArduPlane/Log.cpp Outdated Show resolved Hide resolved
ArduCopter/Log.cpp Outdated Show resolved Hide resolved
Rover/Log.cpp Show resolved Hide resolved
libraries/AP_AHRS/AP_AHRS_Logging.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Please state what testing you've given the various vehicles.

@andyp1per
Copy link
Collaborator Author

I have eyeballed logs from vehicles to check that desired and actual line up.

@tridge
Copy link
Contributor

tridge commented Sep 10, 2024

@andyp1per with the decision to add an ANG message, do you still want this change?

@rmackay9
Copy link
Contributor

I like this change

@peterbarker peterbarker merged commit 62806c6 into ArduPilot:master Sep 10, 2024
94 checks passed
@andyp1per andyp1per deleted the pr-att-fullres branch September 10, 2024 12:41
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.

6 participants