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

User high resolution timer fix #20099

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

User high resolution timer fix #20099

wants to merge 2 commits into from

Conversation

pussuw
Copy link
Contributor

@pussuw pussuw commented Aug 23, 2022

Describe problem solved by this pull request

This fixes two crashes when using user hrt:

  1. Stack underflow, which occurs randomly due to exception stack overflow
  2. Crash due to illegal instruction when trying to disable global interrupts from user space.

Describe your solution

User land code does not need to disable system interrupts, especially here, as the atomic handling of hrt is handled by the kernel side code.

Test data / coverage

Unpublished RISC-V target for Microchip MPFS.

Additional context

RISC-V does not allow executing global interrupt disable/enable from user space. Doing so results in an illegal instruction trap.

@pussuw
Copy link
Contributor Author

pussuw commented Sep 12, 2022

@dagar I removed hrt_store_absolute_time entirely

@pussuw pussuw requested a review from dagar September 21, 2022 09:14
@dagar
Copy link
Member

dagar commented Sep 21, 2022

Can you fix the CI failure (style check)? https://github.com/PX4/PX4-Autopilot/actions/runs/3035286435/jobs/4885259187

Otherwise this looks good, thanks.

@pussuw
Copy link
Contributor Author

pussuw commented Sep 22, 2022

Can you fix the CI failure (style check)? https://github.com/PX4/PX4-Autopilot/actions/runs/3035286435/jobs/4885259187

Otherwise this looks good, thanks.

Oh I'm sorry, did not notice there was a styling issue.

WARN  [load_mon] usr_hrt low on stack! (96 bytes left)
Removed, as it is not needed
@pussuw
Copy link
Contributor Author

pussuw commented Oct 25, 2022

@dagar was there still something you wish for me to do? From my part this is ready to merge.

@pussuw
Copy link
Contributor Author

pussuw commented Jan 17, 2023

Hello again @dagar , this has been inactive for a while but the fix is valid and we have been running this patch for a while now so it is stable as well. If you wish I can also abandon this and we'll keep the change in our private repo only.

@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-maintainers-call-august-01-2023/33417/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin: Enhancement (improvement) 💡 Admin: Needs Review ✋ Architecture Maintainers call Add items that should be discussed in the weekly maintainers call!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants