Fix (dis)arm reason enumeration #25766
Open
+40
−68
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ents and uORB that's the failing test because of which the (dis)arm reason can be wrong based on the message definition.
- removing deprecated ones - syncing used ones to the enum value events defined already - make naming more consistent
1. It was added because the action for ESCs not arming in time was lockdown and in this case it should "also" disarm (why not disarm directly in the first place?) see a7f2f29 2. but lockdown was also used such that motors don't spin in HITL which then caused auto disarm hence a workaround see 1ac3110 3. and in the big commander failsafe rewrite (#20172) the ESCs not arming case was treated as a failsafe that disarms and not using lockdown anymore 4. and then also the multicopter throw launch (#21170) started using lockdown to stop motors before throw which had to be excluded similar to the workaround in 2. So as a result the cases where lockdown is used now are HITL and throw launch, both don't want to cause an automatic disarm hence they are both exluded and the lockdown disarm should not be reachable and can be removed. Note: The case where you kill during a throw launch and get an automatic disarm is now correctly handled in my eyes. Before the disarm was skipped.
🔎 FLASH Analysispx4_fmu-v5x [Total VM Diff: -168 byte (-0.01 %)]px4_fmu-v6x [Total VM Diff: -168 byte (-0.01 %)]Updated: 2025-10-15T16:29:52 |
to solve: 'static_assert' with no message is a C++17 extension in Mac CI
|
Mac CI wanted a message for each static_cast |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Solved Problem
When analysing logs I found that the (dis)arm reason is reported wrong according to the message definition because the logged uORB message
vehicle_statusspecifically theARM_DISARM_REASON_...enum values are not in sync with the events enum defined in the json and autogenerated into thearm_disarm_reason_ttype.Solution
static_asserts for each enum value with the same name of both the uORB and event value. This does not force a new reason to be added to both places but at least checks the existing ones and gives a hint that the two have to match when people do a full text search.arm()ordisarm()and some of them are deprecated but were never removed:transition_to_standbyarming state machine leftoverfailure_detectorwas used for failure detection actions before Commander failsafe state machine rewrite #20172 and are now handled as failsafe actionsshutdownarming state machine leftoverunit_testarming state machine unit testing leftoverlockdownI made an extra commit because it's more complicated since it's still used but not really a usefully reachable code path since Commander failsafe state machine rewrite #20172 Please read the commit description for more details.lockdownsee last point of 2. and the in depth commit message.Changelog Entry
Test coverage
The static asserts are now passing (which they weren't after the first commit). The naming was consistently replaced. I didn't plan any additional testing. Please review.
Context
Related links, screenshot before/after, video