-
Notifications
You must be signed in to change notification settings - Fork 19k
Prearm check to validate backend read rates against scheduler loop rate #28672
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
Prearm check to validate backend read rates against scheduler loop rate #28672
Conversation
fafeac5
to
6628454
Compare
|
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.
Getting there
} | ||
// if gyro backend rate hz is 0, that means we are polling sensor at the sample rate or higher | ||
if (_backends[i]->get_gyro_backend_rate_hz() == 0) { | ||
if (_gyro_raw_sample_rates[i] < _loop_rate) { |
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.
Don't you have to cater for oversampling here? See get_gyro_rate_hz()
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.
Didn't we agree that the rate needs to be at least 2x the loop rate?
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.
Don't you have to cater for oversampling here? See get_gyro_rate_hz()
No, oversampling is only set by drivers that use FIFO, and hence will be setting backend_rate_hz and we will be using that for comparision
Didn't we agree that the rate needs to be at least 2x the loop rate?
Done.
6628454
to
023a3ad
Compare
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.
It looks good to me but I'll leave it to Andy and Sid to sort out naming
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.
looks good to me, thanks @bugobliterator !
@andyp1per I'd like to get your comments on this if possible |
@andyp1per looking for your approval on this one. Seems like a sensible sanity check. |
023a3ad
to
ff88246
Compare
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.
Looks good now - thanks @bugobliterator !
ff88246
to
94c4ebc
Compare
I've rebased this on top of CI fixes and marking it for MergeOnCIPass. |
.... has to pass CI:
|
94c4ebc
to
d30d6f7
Compare
d30d6f7
to
70db719
Compare
Seems like SITL was failing the prearm because the second IMU gyro rate is 760 < 2*400. Pixhawk1 and other boards using LSM9DS0 will have the same issue. @tridge |
This PR is linked from our 4.6.0 issues list so I wonder if we want to try again to get this in? |
5939782
to
dd644b3
Compare
dd644b3
to
7d16d55
Compare
…t scheduler loop rate
7d16d55
to
75ca7e5
Compare
@bugobliterator I updated the fn to avoid the multiple fn calls, and shorted the msg so it fits in the message buffer, tested on a Pixhawk1, behaves as expected |
@bugobliterator VectorNavEAHRS test is failing - it produces very low gyro rates. It is flyable on fixed wing but cannot be used for notch filtering. I think we will need an exception for external AHRS IMUs |
OK, new real failure on EAHRS systems; we can consume gyro data from these systems and we're failing the new test:
|
} | ||
const auto rate_hz = _backends[i]->get_gyro_backend_rate_hz(); | ||
if (rate_hz < threshold) { | ||
hal.util->snprintf(fail_msg, fail_msg_len, "Gyro %d rate %dHz < loop ratex1.8 %dHz", |
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.
hal.util->snprintf(fail_msg, fail_msg_len, "Gyro %d rate %dHz < loop ratex1.8 %dHz", | |
hal.util->snprintf(fail_msg, fail_msg_len, "Gyro%d-rate=%dHz < loop rate*1.8=%dHz", |
@bugobliterator ping on the test fail with vectornav? see above comments |
3223ad4
to
bec18df
Compare
this is included in 4.6.0-beta6 |
This PR adds a prearm check that ensures that we are reading gyro data atleast as fast as we are running our main loop, where all controllers run at. We do this for all the INS sensors that are enabled.
This avoids a situation where user might have set SCHED LOOP RATE faster than gyro poll rate or primary gyro with appropriate backend rate but forgot to do so for the rest.