minor param cleanup, move params into relevant module #8001
Conversation
dagar
force-pushed
the
pr-param_minor
branch
from
b202b90 to
b24f672
Compare
|
Finding params in the sensors module to force mark them active for QGC has been overdone. This is only required for a few cases where QGC will always look for the param, but it isn't read by default. Params used in the init script don't need to be forced here. 9766362 |
|
The PWM params are a much bigger mess I'll need to come back to. |
dagar
force-pushed
the
pr-param_minor
branch
from
ff3e631 to
88c18b5
Compare
|
@LorenzMeier I reverted the SYS_PARAM_VER delete and rebased on master. |
|
Merge after #8006. |
|
Going through the diff again I don't think this particular param cleanup requires restoring airframe defaults. |
src/drivers/mb12xx/parameters.c
Outdated
| ****************************************************************************/ | ||
|
|
||
| /** | ||
| * Maxbotix Soanr (mb12xx) |
src/modules/sensors/parameters.cpp
Outdated
|
|
||
| (void)param_find("SYS_PARAM_VER"); | ||
| (void)param_find("SYS_AUTOSTART"); | ||
| (void)param_find("SYS_AUTOCONFIG"); |
There was a problem hiding this comment.
sys_autoconfig is needed. Tested with SITL, then went to the Airframe tab
Did you test if sensor calibration is still ok?
There was a problem hiding this comment.
SYS_AUTOCONFIG is checked by rcS, and does the reset if it's set. In setups where rcS isn't used (currently linux, qurt, or custom nuttx) it isn't checked or used and therefore doesn't need to be set active for QGC.
I checked sensor calibration with QGC, both a completely fresh board, and one already calibrated.
There was a problem hiding this comment.
I know, but QGC will complain with an error that the param is not found. It's just a bad user experience.
The real solution would be to make the airframe configurable on these boards too, but until then I think we should keep this in.
There was a problem hiding this comment.
Right, makes sense for SYS_AUTOSTART and SYS_AUTOCONFIG.
I'm very interested in unifying the init across platforms.
src/modules/sensors/parameters.cpp
Outdated
| (void)param_find("UAVCAN_ENABLE"); | ||
| (void)param_find("SYS_MC_EST_GROUP"); | ||
|
|
||
| // Parameters controlling the on-board sensor thermal calibrator |
There was a problem hiding this comment.
these are needed, because they're only accessed when starting the temperature calibration.
There was a problem hiding this comment.
They still work for starting temperature calibration without being here. Why is it necessary to "param_find" them?
There was a problem hiding this comment.
Yes they still work, but QGC will not show the parameters. It will only show params that are accessed (via param_find) during bootup. If you remove them here, they're never accessed during normal bootups.
There was a problem hiding this comment.
I'm still not seeing the problem with SYS_CAL_GYRO, etc. They aren't touched outside of rcS.
https://github.com/PX4/Firmware/blob/master/ROMFS/px4fmu_common/init.d/rcS#L990
The init script runs, does a param compare, they're marked active and then synced to QGC. If you started through another route (custom init script, linux, or qurt) they aren't compared, and then not synced. Am I missing something?
There was a problem hiding this comment.
You're right about SYS_CAL_GYRO, but I'm refering to these:
- (void)param_find("SYS_CAL_TDEL");
- (void)param_find("SYS_CAL_TMAX");
- (void)param_find("SYS_CAL_TMIN");
They're not accessed in rcS
|
Sonar typo fixed and rebased on master. |
|
@dagar The basic logic here is that some of these might be read by the GCS and are nowhere referenced in firmware. So unless you cross-checked QGC for each of them (and potentially other scripts), we have no way of knowing what use cases we break. |
| * @min 0 | ||
| * @group System | ||
| */ | ||
| PARAM_DEFINE_INT32(SYS_PARAM_VER, 1); |
src/modules/sensors/parameters.cpp
Outdated
| (void)param_find("CAL_ACC2_ZOFF"); | ||
| (void)param_find("CAL_ACC2_ZSCALE"); | ||
|
|
||
| (void)param_find("SYS_PARAM_VER"); |
There was a problem hiding this comment.
This particular param is still needed - what I didn't understand when reading the diff is how you register all these in the system - did you add a new flag?
What we don't want is to transmit all params to the ground, even if they are unused, since that's taking way too long. Did you solve this in a different way I haven't seen yet?
|
I'm aware of the purpose of the param_find in sensors and interaction with QGC, but I missed a few like SYS_CAL_TMIN/TMAX/TDEL only used in events. SYS_PARAM_VER was originally removed because it isn't currently used by PX4/QGC master, but was readded after discussion in #8006. My goal here is to hide the useless parameters and improve the experience with slow/bad telemetry links. I encounter users and even developers that try to do things like enable a SF1XX (because the param is always there), but have no idea the driver isn't even in their firmware. I checked the param usage in QGC for the setup tabs. For sensors it only uses CAL_X_ID, and CAL_MAGX_ROT. Are you aware of a need for the others? The PWM and PWM_AUX parameters are either accessed by nuttx rc.interface, a pwm output modules, or in many cases not at all. Longer term I personally think we should separate params for user configuration and params generated by the system (calibration, time in air, etc), but that's a larger discussion/effort. For now limiting it to params that can even possibly be used by a particular setup (board + airframe) is still an improvement. |
dagar
dismissed stale reviews from bkueng and LorenzMeier
SYS_CAL params readded
|
Continued in #8869 |
Placing the param definition within the relevant module is a better user experience. When a module isn't built for a particular board the option to enable it shouldn't be either.