ArduPilot / ardupilot

ArduPlane, ArduCopter, ArduRover, ArduSub source
http://ardupilot.org/
GNU General Public License v3.0
10.26k stars 16.87k forks source link

NavEKF3_core::setup_core: is beacon buffer length setted wrong? #26833

Open luweiagi opened 3 months ago

luweiagi commented 3 months ago

Here we set beacon buffer length as imu_buffer_length+1: https://github.com/ArduPilot/ardupilot/blob/d0a7e70d0f3e7c14f0b8d90f67652a79b5a4ee13/libraries/AP_NavEKF3/AP_NavEKF3_core.cpp#L140 image

But, at https://github.com/ArduPilot/ardupilot/blob/d0a7e70d0f3e7c14f0b8d90f67652a79b5a4ee13/libraries/AP_NavEKF3/AP_NavEKF3_core.cpp#L96 image We said that obs_buffer_length should not longer than the IMU buffer because we can't process data faster than the EKF prediction rate even if obs sensor reading-data speed is faster than the IMU update speed.

So, why we set beacon buffer length as imu_buffer_length+1, which >imu_buffer_length? Can it be changed as: image ?

rmackay9 commented 3 months ago

Hi @luweiagi,

Thanks for your ongoing bug search, it's very much appreciated.

I think we support multiple beacons that could theoretically each provide an update at a high rate, but I'm also pretty sure that we don't allow the total number of updates from all beacons to be more than 50hz (total, e.g. if 5 beacons then each can only update at 10hz maximum) so I agree that it is probably fine to reduce the size of the rngBcn.storedRange array to be smaller. On the other hand, do we really want to risk making it smaller just to save a few bytes of RAM?

luweiagi commented 3 months ago

@rmackay9 Thanks for reply! No, it's not necessary. I only made a logical judgment.

peterbarker commented 1 week ago

OK, sounds like this was sorted. Obviously the +1 could have done with a comment!

Closing this now.