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

AP_Logger: fix locking issues, log wrapping and status messages #13178

Merged
merged 1 commit into from
Jan 13, 2020

Conversation

andyp1per
Copy link
Collaborator

@andyp1per andyp1per commented Jan 5, 2020

This is a follow-up to #13130
I was getting hangs in chip erase which lead to corrupted logs. This PR fixes this problem.
It also shortens some of the status messages so that they can be seen.

Tests done:

  • Start and stop logs using LOG_DISARMED, check logs list ok
  • Allow logs to wrap so that indexing no longer starts at 1, check logs list ok
  • Allow a single log to wrap itself, check listing works ok and log can be downloaded
  • Chip erase works and logging will work again afterwards
  • Pull the power during chip erase and make sure it restarts
  • Chip erase and then check that logs can be written once again without rebooting

All pass except the last one. So if you erase the chip you need to reboot to get logging to work again. This seems like a small inconvenience and nothing actually breaks in all these tests

@andyp1per andyp1per requested a review from tridge January 5, 2020 22:06
@andyp1per
Copy link
Collaborator Author

I think there may still be a problem if your flight is long enough to wrap a single log, but not proved this yet.

@andyp1per andyp1per changed the title AP_Logger: fix locking issues, uninitialized read and status messages AP_Logger: fix locking issues and status messages Jan 6, 2020
@andyp1per
Copy link
Collaborator Author

@rmackay9 FYI

@andyp1per
Copy link
Collaborator Author

andyp1per commented Jan 9, 2020

Ok, if I set log disarmed to 1 and just wait until the log wraps it's pretty easy to show the problems. I then end up with:

STABILIZE> Requesting log list
Log 65535  numLogs 3 lastLog 1 size 256
Log 0  numLogs 3 lastLog 1 size 0
Log 1  numLogs 3 lastLog 1 size 16711424

and maxproxy reports:

APM: No last page of log 65534 at top=8A21 or bot=8A20
APM: No last page of log 65535 at top=8A21 or bot=8A20

I will investigate more. Interestingly my checking code thinks everything is fine.

…length

account for erased partial sectors when looking at wrapped logs
@andyp1per
Copy link
Collaborator Author

Ok, I have it. We weren't accounting for the fact that when we wrap a log we erase blocks as we go, so there is a deadzone between the end of the last log and the start of the first log that is filled with 0xFFFF which confuses all of the logic around finding log numbers and pages. The solution is to make sure we skip this deadzone when looking for the start of the first log. In @tridge 's defense I think that the original code may have had some allowance for this, but in fixing some of the other issues I clearly undid that logic. The PR now works with wrapped files.

@andyp1per
Copy link
Collaborator Author

@tridge I'm pretty confident in this now. I have created multiple logs via LOG_DISARMED and rebooting and then continuously done "log list" from mavlink with this on. This has correctly detected the wrap and then also has correctly detected the number of logs (reducing) as they gradually get overwritten. Pretty happy with this change now. @rmackay9 FYI.

@andyp1per andyp1per changed the title AP_Logger: fix locking issues and status messages AP_Logger: fix locking issues, log wrapping and status messages Jan 11, 2020
@tridge tridge merged commit d167dac into ArduPilot:master Jan 13, 2020
@andyp1per andyp1per deleted the pr-flash-fix-fix branch January 14, 2020 08:09
@rmackay9
Copy link
Contributor

rmackay9 commented Feb 4, 2020

FYI, this is included in Copter-4.0.2-rc4

@rmackay9 rmackay9 moved this from PRs to 4.0.2-rc4 in Copter 4.0 backports Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants