AP_Arming: turn ARMING_CHECK into ARMING_SKIPCHK #31568
Conversation
tpwrules
requested review from
Williangalvani,
peterbarker,
rmackay9 and
tridge
|
I like this idea! Not totally convinced about the name, but the current behaviour is definitely sub-optimal |
peterbarker
left a comment
peterbarker
left a comment
There was a problem hiding this comment.
I'm excited for this one!
| uint32_t(AP_Arming::Check::VOLTAGE) | | ||
| uint32_t(AP_Arming::Check::BATTERY)) |
There was a problem hiding this comment.
We should actually have a closer look at these - they may be historical and no-longer-needed
You can arm Copter without RC. Adding RC in here means you can see an RC and then lose it and still be able to arm. I don't know if that's desired behaviour
The voltage/battery checks are weird, too, as there shouldn't be a battery monitor set up by default.
There was a problem hiding this comment.
I would be happy to let Sub to default to all checks enabled if Willian approves.
On the other hand, doing that does break half the tests.
There was a problem hiding this comment.
The voltage/battery checks are weird, too, as there shouldn't be a battery monitor set up by default
We could use battery bit to allow user to skip arming voltage check (but maybe not low/critical) ex. for performing a lot of short missions on one battery.
You can arm Copter without RC. Adding RC in here means you can see an RC and then lose it and still be able to arm. I don't know if that's desired behaviour
That may be the case for some users ex. performing test hover prior to auto mission without RC control. This should would require 0-ing RC input state if it isn't seen when disarmed.
|
This fixes #15326 |
IamPete1
left a comment
IamPete1
left a comment
There was a problem hiding this comment.
I would be quite tempted to just skip the conversion and make people turn skip checks again manually if they really want to.
| VOLTAGE = (1U << 7), | ||
| BATTERY = (1U << 8), |
There was a problem hiding this comment.
Now is the time to consolidate these if were doing param conversion anyway. Its not clear to me why we need both VOLTAGE and BATTERY.
| BATTERY = (1U << 8), | ||
| AIRSPEED = (1U << 9), | ||
| LOGGING = (1U << 10), | ||
| SWITCH = (1U << 11), |
| AIRSPEED = (1U << 9), | ||
| LOGGING = (1U << 10), | ||
| SWITCH = (1U << 11), | ||
| GPS_CONFIG = (1U << 12), |
There was a problem hiding this comment.
Do we need both GPS_CONFIG and GPS as separate items?
| enum class Check { | ||
| ALL = (1U << 0), | ||
| // 0 used to be ALL, though it could be reused as the SKIPCHK conversion | ||
| // never sets it. check_mask would also need updating. |
There was a problem hiding this comment.
I guess we could shuffle everything down one. We only have a couple more before we run into float transport issues.
|
As discussed Tridge and I think we should document that setting the param to -1 disables/skips all checks |
tridge
left a comment
tridge
left a comment
There was a problem hiding this comment.
if they have ARMING_CHECK=0 and upgrade, then ARMING_SKIPCHK should be updated to -1
Was prototyped many years ago but never defined or called.
Was prototyped many years ago but never defined or called.
This is never called as the main `wscript` does not recurse into this directory until build time. Even if it were called, it would update the environment for all builds instead of just replay. Even if it did update the environment, nobody reads that variable so there's no effect.
Now that we call the AP_Arming singleton's initialization method in AP_Vehicle, we have to ensure an AP_Arming exists for all vehicles. Otherwise we get a segfault. This is nicer than disabling AP_ARMING_ENABLED for replay and causing a full recompile. Also nicer than checking that the singleton is not nullptr as this is zero overhead for all other vehicles.
This conversion was dropped for other vehicles in late 2024. This conversion is in 4.0.0.
This conversion was dropped for other vehicles in late 2024. This conversion is in 4.0.0.
The mentioned bug must have been fixed as the test works now without doing any fiddling. The code never worked properly anyway, `1 ^ 25` is nonsense and presumably should have been `1 << 25`.
In some cases the index can be more than a `uint8_t` so just use the underlying type in the `ConversionInfo`. This is a no compiler output change.
This lowers the effort required to turn off just one arming check. Previously, a user had to disable the ALL bit and enable every check except the undesired one. Now they can just disable that one directly. Hopefully this will result in less vehicles with no arming checks whatsoever, presuming only one check is giving the user grief. This, as a side effect, removes the difference between the ALL bit set and all non-ALL bits set (e.g. the latter disables IMU heater checks). It also ensures the user will get any new arming checks even if they have skipped one. People who need to disable all current and future checks for e.g. bench testing can still do this efficiently by setting the parameter to `-1`, leveraging that this sets all bits in 2s complement arithmetic. A parameter conversion is included that skips no checks if the old ALL bit is set; otherwise it migrates the user's selected checks. If no checks were enabled, it disables all current and future checks.
This lowers the effort required to turn off just one arming check. Previously, a user had to disable the ALL bit and enable every check except the undesired one. Now they can just disable that one directly. Hopefully this will result in less vehicles with no arming checks whatsoever, presuming only one check is giving the user grief. This, as a side effect, removes the difference between the ALL bit set and all non-ALL bits set (e.g. the latter disables IMU heater checks). It also ensures the user will get any new arming checks even if they have skipped one. People who need to disable all current and future checks for e.g. bench testing can still do this efficiently by setting the parameter to `-1`, leveraging that this sets all bits in 2s complement arithmetic. A parameter conversion is included that skips no checks if the old ALL bit is set; otherwise it migrates the user's selected checks. If no checks were enabled, it disables all current and future checks.
This lowers the effort required to turn off just one arming check. Previously, a user had to disable the ALL bit and enable every check except the undesired one. Now they can just disable that one directly. Hopefully this will result in less vehicles with no arming checks whatsoever, presuming only one check is giving the user grief. This, as a side effect, removes the difference between the ALL bit set and all non-ALL bits set (e.g. the latter disables IMU heater checks). It also ensures the user will get any new arming checks even if they have skipped one. People who need to disable all current and future checks for e.g. bench testing can still do this efficiently by setting the parameter to `-1`, leveraging that this sets all bits in 2s complement arithmetic. A parameter conversion is included that skips no checks if the old ALL bit is set; otherwise it migrates the user's selected checks. If no checks were enabled, it disables all current and future checks.
This lowers the effort required to turn off just one arming check. Previously, a user had to disable the ALL bit and enable every check except the undesired one. Now they can just disable that one directly. Hopefully this will result in less vehicles with no arming checks whatsoever, presuming only one check is giving the user grief. This, as a side effect, removes the difference between the ALL bit set and all non-ALL bits set (e.g. the latter disables IMU heater checks). It also ensures the user will get any new arming checks even if they have skipped one. People who need to disable all current and future checks for e.g. bench testing can still do this efficiently by setting the parameter to `-1`, leveraging that this sets all bits in 2s complement arithmetic. A parameter conversion is included that skips no checks if the old ALL bit is set; otherwise it migrates the user's selected checks. If no checks were enabled, it disables all current and future checks.
This lowers the effort required to turn off just one arming check. Previously, a user had to disable the ALL bit and enable every check except the undesired one. Now they can just disable that one directly. Hopefully this will result in less vehicles with no arming checks whatsoever, presuming only one check is giving the user grief. This, as a side effect, removes the difference between the ALL bit set and all non-ALL bits set (e.g. the latter disables IMU heater checks). It also ensures the user will get any new arming checks even if they have skipped one. People who need to disable all current and future checks for e.g. bench testing can still do this efficiently by setting the parameter to `-1`, leveraging that this sets all bits in 2s complement arithmetic. A parameter conversion is included that skips no checks if the old ALL bit is set; otherwise it migrates the user's selected checks. If no checks were enabled, it disables all current and future checks.
This lowers the effort required to turn off just one arming check. Previously, a user had to disable the ALL bit and enable every check except the undesired one. Now they can just disable that one directly. Hopefully this will result in less vehicles with no arming checks whatsoever, presuming only one check is giving the user grief. This, as a side effect, removes the difference between the ALL bit set and all non-ALL bits set (e.g. the latter disables IMU heater checks). It also ensures the user will get any new arming checks even if they have skipped one. People who need to disable all current and future checks for e.g. bench testing can still do this efficiently by setting the parameter to `-1`, leveraging that this sets all bits in 2s complement arithmetic. A parameter conversion is included that skips no checks if the old ALL bit is set; otherwise it migrates the user's selected checks. If no checks were enabled, it disables all current and future checks.
AIRSPEED check was inexplicably disabled before as well, but it's compiled out so re-enabling it doesn't matter.
The meaning is the same as before, for better or worse.
These conversions were done by spirit rather than literally. Note that the webots python params get several more checks enabled but the one disabled now matches the comment.
The meaning is the same as before, for better or worse. Some files only for older firmwares were not touched.
This also probably actually makes this parameter effective by removing the ALL bit.
tpwrules
force-pushed
the
pr/arming-skipchk
branch
from
94f3af1 to
f498b13
Compare
|
Everything addressed except for the potential changes regarding the bits themselves. |
|
This is great @tpwrules - is there a SKIPCHK for scripted arming checks? i.e. when disabling any other specific checks, can I also check a bit to disable all scripted arming checks? |
This should be covered by the AuxAuth bit (17), just like before. |
|
Regarding the name, how about ARMING_BPASS_CHK |
9 participants
This lowers the effort required to turn off just one arming check. Previously, a user had to disable the ALL bit and enable every check except the undesired one. Now they can just disable that one directly. Hopefully this will result in less vehicles with no arming checks whatsoever, presuming only one check is giving the user grief.
This, as a side effect, removes the difference between the ALL bit set and all non-ALL bits set (e.g. the latter disables IMU heater checks). It also ensures the user will get any new arming checks even if they have skipped one.
Developers who need to disable all current and future checks for e.g. bench testing can still do this efficiently by setting the parameter to
-1, leveraging that this sets all bits in 2s complement arithmetic.But we will not obviously document this for users, it should be an annoyance!A parameter conversion is included that skips no checks if the old ALL bit is set; otherwise it migrates the user's selected checks. If no checks were enabled, it disables all current and future checks.
Tested on the bench that it behaves logically. Fortunately the actual arming logic change is pretty trivial. I will write a wiki update for this change as well. Please see commit messages for various details and comments on the conversions I did in tree. Also did parameter conversion testing with the following commands:
python3 Tools/autotest/test_param_upgrade.py --vehicle arducopter --vehicle arduplane --vehicle ardusub --vehicle ardurover --vehicle blimp --param "ARMING_CHECK=3241->ARMING_SKIPCHK=0"python3 Tools/autotest/test_param_upgrade.py --vehicle arducopter --vehicle arduplane --vehicle ardusub --vehicle ardurover --vehicle blimp --param "ARMING_CHECK=12->ARMING_SKIPCHK=2097138"python3 Tools/autotest/test_param_upgrade.py --vehicle arducopter --vehicle arduplane --vehicle ardusub --vehicle ardurover --vehicle blimp --param "ARMING_CHECK=1->ARMING_SKIPCHK=0"python3 Tools/autotest/test_param_upgrade.py --vehicle arducopter --vehicle arduplane --vehicle ardusub --vehicle ardurover --vehicle blimp --param "ARMING_CHECK=0->ARMING_SKIPCHK=-1"Is it possible we could just drop the conversion (or re-enable all checks ifdecided not toARMING_CHECKwas 0) and make people fix their vehicles? :)