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 RATE logging to attitude controller and log sample time #27996

Merged
merged 6 commits into from
Sep 10, 2024

Conversation

andyp1per
Copy link
Collaborator

@andyp1per andyp1per commented Sep 3, 2024

With RATE logging it is crucial that both the last gyro value used and the time that it was used be recorded in the log rather than whatever happens to be available at the time the log is written. In addition it makes no sense for RATE logging to be in AHRS, it really has very little to do with AHRS and everything to do with the rate and attitude controllers. This PR moves the logging functionality and records the gyro value used and timestamp at the appropriate point for logging. This is especially important when the rate loop because faster and concurrent.

Split from #27029

@andyp1per andyp1per force-pushed the pr-rate-log branch 3 times, most recently from 70e5ed1 to 76669b4 Compare September 4, 2024 12:03
@andyp1per andyp1per changed the title Pr rate log Move RATE logging to attitude controller and log sample time Sep 4, 2024
@andyp1per andyp1per marked this pull request as ready for review September 4, 2024 12:07
Copy link
Contributor

@lthall lthall left a comment

Choose a reason for hiding this comment

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

Great change but I would like to do the same thing with the ATT message.

Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

Should be a nice clean up.

libraries/AC_AttitudeControl/AC_AttitudeControl.cpp Outdated Show resolved Hide resolved
libraries/AC_AttitudeControl/AC_AttitudeControl.h Outdated Show resolved Hide resolved
libraries/AC_AttitudeControl/AC_AttitudeControl.h Outdated Show resolved Hide resolved
Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

I think this looks good. The only question is if any log tools have a problem with the log timestamp not matching the time the log was written. I don't think they should. Each message should still see a increasing time. Only between messages would time go backwards. The only other thing would be if we were to write the log twice in one loop for some reason then we would get two timestamps the same.

I guess we could get round this by adding a new timestamp rather than replacing the existing one, but that would make plotting in log review tools hard as there mostly hard coded to use TimeUS on the x axis.

Copy link
Contributor

@rmackay9 rmackay9 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

As discussed on the call, a few of us are worried about the TimeUS potentially going backwards which could confuse some log analysis tools but I think it's worth the risk and it seems important that we give the devs the data they need

@IamPete1 IamPete1 merged commit bb2249f into ArduPilot:master Sep 10, 2024
94 checks passed
@andyp1per andyp1per deleted the pr-rate-log branch September 10, 2024 15:33
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