Prearm check to validate backend read rates against scheduler loop rate #28672
Conversation
bugobliterator
force-pushed
the
pr-sched-rate-prearm-check
branch
3 times, most recently
from
fafeac5 to
6628454
Compare
|
| } | ||
| // 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.
Don't you have to cater for oversampling here? See get_gyro_rate_hz()
There was a problem hiding this comment.
Didn't we agree that the rate needs to be at least 2x the loop rate?
There was a problem hiding this comment.
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.
| @@ -75,6 +75,11 @@ class AP_InertialSensor_Invensensev3 : public AP_InertialSensor_Backend | |||
| bool accumulate_samples(const struct FIFOData *data, uint8_t n_samples); | |||
| bool accumulate_highres_samples(const struct FIFODataHighRes *data, uint8_t n_samples); | |||
|
|
|||
| // get the gyro backend rate in Hz at which the FIFO is being read | |||
| uint16_t get_gyro_backend_rate_hz() const override { | |||
There was a problem hiding this comment.
I think we should take the opportunity to come up with a better name than "backend". Perhaps "get_gyro_read_rate_hz()" at least conveys what is going on and why it might be important.
There was a problem hiding this comment.
That needs to be much larger and a separate change, I don't want to widen the scope of this PR.
There was a problem hiding this comment.
you could just remove the word "backend" from the method name maybe?
bugobliterator
force-pushed
the
pr-sched-rate-prearm-check
branch
from
6628454 to
023a3ad
Compare
| continue; | ||
| } | ||
| if (_backends[i]->get_gyro_backend_rate_hz() < (2*_loop_rate)) { | ||
| hal.util->snprintf(fail_msg, fail_msg_len, "Gyro %d backend rate %dHz < sched loop rate %dHz", |
There was a problem hiding this comment.
we could shorten the message by removing "Hz" or "rate"
|
@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. |
bugobliterator
force-pushed
the
pr-sched-rate-prearm-check
branch
from
023a3ad to
ff88246
Compare
peterbarker
force-pushed
the
pr-sched-rate-prearm-check
branch
from
ff88246 to
94c4ebc
Compare
|
I've rebased this on top of CI fixes and marking it for MergeOnCIPass. |
|
.... has to pass CI: |
tridge
force-pushed
the
pr-sched-rate-prearm-check
branch
from
94c4ebc to
d30d6f7
Compare
…t scheduler loop rate
bugobliterator
force-pushed
the
pr-sched-rate-prearm-check
branch
from
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 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.