-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
HRT: Create new separate call for atomic HRT elapsed time calculation #11248
Conversation
This call rarely needs to be truly atomic and the involved CPU overhead in making it atomic was unnecessary and introduces a lot of IRQ jitter with no value-add. The call has been moved to be non-atomic and the codebase will be inspected and changed in follow-up commits for the few instances where it is truly needed.
This ensures we get absolutely accurate timing.
On first pass I didn't find any critical code really requiring atomic operation - but we need to review this carefully. Search for
|
The actual implementation is not atomic, as the value on the application layer would be limited.
This reduces interrupt load significantly.
This reduces interrupt load significantly.
@PX4/testflights Please clock up a couple hours on this. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the canges I've been meaning to do as well, thanks for looking into it.
I see a problematic call on the IO:
fmu_data_received_time
is read inmixer.cpp
, but it's written from an IRQ context inregisters_set
(registers.c
).
Tested on pixhawk 4 and pixhawk 2.1 |
This leads to less jitter in the benchmark
@bkueng I changed to the atomic call on IO. |
This allows the compiler to optimize better without loosing any performance / accuracy.
This is required as we might be in interrupt context on this bare-metal target.
6193c13
to
17ea90b
Compare
tested on pixhawk v4 pro pixhawk 4 mini v5 pixhawk race v4 |
This call rarely needs to be truly atomic and the involved CPU overhead in making it atomic was unnecessary and introduces a lot of IRQ jitter with no value-add. The call has been moved to be non-atomic and the codebase will be inspected and changed in follow-up commits for the few instances where it is truly needed.
Fixes #11245.
After fix (more CPU idle = better):
Current: