mavlink altitude fix and general cleanup #9158
Conversation
dagar
force-pushed
the
pr-mavlink_altitude
branch
5 times, most recently
from
1f113d6
to
95f9ad8
Compare
| if (_global_pos_time != 0 && global_pos.terrain_alt_valid) { | ||
| msg.altitude_terrain = global_pos.terrain_alt; | ||
| msg.bottom_clearance = global_pos.alt - global_pos.terrain_alt; | ||
| if ((sensor.timestamp > 0) && (sensor_time > 0)) { |
There was a problem hiding this comment.
Checking for sensor.timestamp > 0 is enough (and then _sensor_time can be removed) and the added brackets make it less readable
There was a problem hiding this comment.
This is another one that's a bit awkward. In the nominal case we want to make sure everything is populated from both sensor_combined and vehicle_local_position. I'll look again.
| updated |= _gpos_sub->update(&gpos); | ||
| updated |= _lpos_sub->update(&lpos); | ||
| updated |= _home_sub->update(&home); | ||
| updated |= _sensor_sub->update(&sensor); |
There was a problem hiding this comment.
Can you move the sensor topic & update into the if else block, as to avoid copying the data when not needed?
There was a problem hiding this comment.
This was done to continue updating the alt from baro if everything else is lost. It's not ideal.
There was a problem hiding this comment.
Done, I made GLOBAL_POSITION_INT update a little more conservative and it should almost never actually need to copy sensor_combined unless the estimator is behaving weirdly.
| @@ -326,23 +326,22 @@ class MavlinkStreamHeartbeat : public MavlinkStream | |||
|
|
|||
| bool send(const hrt_abstime t) | |||
| { | |||
| struct vehicle_status_s status = {}; | |||
| vehicle_status_s status; | |||
|
|
|||
| /* always send the heartbeat, independent of the update status of the topics */ | |||
There was a problem hiding this comment.
This is not correct anymore. Please change it back to always send the heartbeat.
There was a problem hiding this comment.
I see now that the comment is wrong, but this is intentional. Commander publishes vehicle_status at >= 1 Hz. HEARTBEAT send will now sync up with commander publication and more importantly reflect if commander is actually alive.
There was a problem hiding this comment.
I see your reasoning, but I think it's more important to always send a heartbeat, even in the case commander is not running or misbehaving (or for some reasons the >1Hz assumption does not hold anymore). This makes link debugging and a lot easier.
There was a problem hiding this comment.
Fair enough, original HEARTBEAT restored.
| _raw_gyro_sub->update(&_raw_gyro_time, &_sensor_gyro); | ||
| bool updated = false; | ||
| updated |= _raw_accel_sub->update(&_raw_accel_time, &sensor_accel); | ||
| updated |= _raw_gyro_sub->update(&_raw_gyro_time, &sensor_gyro); | ||
|
|
||
| // send if raw_accel has been updated and use the newest gyro values we have |
dagar
force-pushed
the
pr-mavlink_altitude
branch
2 times, most recently
from
d955b63
to
cf33c17
Compare
dagar
dismissed
bkueng’s stale review
Most comments addressed, a couple points may need further discussion.
dagar
force-pushed
the
pr-mavlink_altitude
branch
from
cf33c17
to
9755eb3
Compare
| // always update monotonic altitude | ||
| bool sensor_updated = false; | ||
| sensor_combined_s sensor = {}; | ||
| _sensor_sub->update(&_sensor_time, &sensor); |
There was a problem hiding this comment.
_sensor_time is not required anymore since we don't check the return value.
dagar
force-pushed
the
pr-mavlink_altitude
branch
from
9755eb3
to
89eeb2d
Compare
dagar
force-pushed
the
pr-mavlink_altitude
branch
from
89eeb2d
to
56024c4
Compare
While testing #8771 I noticed the QGC altitude would stop updating in various situations if the global position validity was lost, even though the system still had a valid altitude.
I went through Mavlink altitude usage in VFD_HUD, GLOBAL_POSITION_INT, and ALTITUDE to make the usage consistent.
I also saw quite a few timestamps throughout the mavlink module that weren't actually used, so I removed them.