search

PX4 / PX4-Autopilot

PX4 Autopilot Software
https://px4.io
BSD 3-Clause "New" or "Revised" License
8.32k stars 13.43k forks source link

Several SENS_* params could be not marked as used on start #18110

Closed lukegluke closed 3 years ago

lukegluke commented 3 years ago

It could lead ground station to parameters rereading due to parameters cache match failing, because of Parameter in cache but not on vehicle componentId:Name 1 "SENS_GPS_MASK" or Parameter missing from cache "SENS_GPS_MASK" (depending on state).

I've encountered to issue only with SENS_GPS_*, but in fact it is also could be related for example to SENS_MAG_* (I suppose other sensors start faster, so no issue).

Source of problem: parameters SENSGPS marked as used on first VehicleGPSPosition creation in InitializeVehicleGPSPosition(), but only if orb sensor_gps exists, while it could take a while for sensor_gps to be advertise. https://github.com/PX4/PX4-Autopilot/blob/f9d8c613b048f58eb3110e9af13cb3a89c4c866f/src/modules/sensors/sensors.cpp#L493-L497 So parameters SENSGPS could be not marked as used if vehicle connected to ground station before first gps data parsed, or marked as used - if connected after.

@dagar why several sensors objects are created dynamically? To save RAM if unused? https://github.com/PX4/PX4-Autopilot/blob/f9d8c613b048f58eb3110e9af13cb3a89c4c866f/src/modules/sensors/sensors.cpp#L174-L178

dagar commented 3 years ago

The idea was to only pay for what you actually have and use so that we can scale down and use the sensors hub on things like CAN nodes.

For mag we could use the existence of magnetometer calibration to know we should start.

Let me see what to do about GPS.

lukegluke commented 3 years ago

For mag we could use the existence of magnetometer calibration to know we should start.

or checking SYS_HAS_MAG is enought. Maybe there is a mechanism to check gps module is started? Maybe check perf exists by name? Maybe also add SYS_HAS_GPS? Or in any way donew VehicleGPSPosition() and others and delete them immediately to mark params used?

UPD: Another idea to just call _report_gps_pos_pub.advertise() in gps constructor. Looks like easiest and forward solution, no changes in sensors needed. I've tested it - it works. Can something be influenced by empty advertised topic?

lukegluke commented 3 years ago

Another idea to just call _report_gps_pos_pub.advertise() in gps constructor.

Oh, I see that there also could be gps sensors on uavcan, so it also will demand _report_gps_pos_pub.advertise() by default. But almost all default boards use uavcan, so with this modification creating VehicleGPSPosition dynamically get profit only in several case of custom boards without uavcan. So instead of getting rid of dynamic VehicleGPSPosition, just always marking parameters as used will be better solution?

dagar commented 3 years ago

I was overthinking this, let's just respect SYS_HAS_MAG, SYS_HAS_BARO, etc.

I'll introduce a new parameter SYS_HAS_GPS and then make the sensors change.

dagar commented 3 years ago

See https://github.com/PX4/PX4-Autopilot/pull/18113

dagar commented 3 years ago

Another idea to just call _report_gps_pos_pub.advertise() in gps constructor.

Oh, I see that there also could be gps sensors on uavcan, so it also will demand _report_gps_pos_pub.advertise() by default. But almost all default boards use uavcan, so with this modification creating VehicleGPSPosition dynamically get profit only in several case of custom boards without uavcan. So instead of getting rid of dynamic VehicleGPSPosition, just always marking parameters as used will be better solution?

You're right that most boards will have this by default. So with https://github.com/PX4/PX4-Autopilot/pull/18113 that will be the case everywhere, but we can opt out of it as needed per board by updating SYS_HAS_GPS, SYS_HAS_MAG, etc.

If the PR fixes the problem I'll do a backport for v1.12.2.