Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not arm above configurable threshold #13294

Conversation

haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Jan 13, 2024

Fixes #13290 where the quad did not disarm on user command.

In general it is our responsibility that users can fly safely.

Not sure how we should approach this issue but we need to provide safety first. We should set a safe value where advanced users can override this setting. Limiting 4K loop on F7X2 might be an alternative solution.

We must ensure that disarmament works promptly at every opportunity. That is the mean reason for this proposal.

Further investigation will be required to find the root cause.

See also

@haslinghuis haslinghuis added this to the 4.5 milestone Jan 13, 2024
@haslinghuis haslinghuis self-assigned this Jan 13, 2024
Copy link

Do you want to test this code? You can flash it directly from Betaflight Configurator:

  • Simply put #13294 (this pull request number) in the Select commit field of the Configurator firmware flasher tab (you need to Enable expert mode, Show release candidates and Development).

WARNING: It may be unstable. Use only for testing!

@blckmn
Copy link
Member

blckmn commented Jan 13, 2024

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> FAIL
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> FAIL
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

@mituritsyn
Copy link
Contributor

if it will be ever introduced - it should be configurable parameter

@ctzsnooze
Copy link
Member

ctzsnooze commented Jan 13, 2024

I'm not approving anything without a description where it now says "No description provided".
The description needs to make the case for the change, explain the reasons for it, and explain how to test it.
Agree that the value should be configurable.
I regularly fly F411's overclocked at 120Mhz at 8k4kthat are perfectly happy with CPU percentage 65% -75% without task over-run issues, including angle mode and GPS rescue..

@Justicefpv
Copy link

I have plenty of whoops running with over 50% CPU at 8k4k and no exceedingly late tasks. This PR appears to be hurried and has no supporting data.

@haslinghuis haslinghuis linked an issue Jan 13, 2024 that may be closed by this pull request
@KarateBrot
Copy link
Member

We should rather investigate why the issue appears even though the scheduler is not fully loaded. This just hides the issue at hand.

@haslinghuis
Copy link
Member Author

Open for any solution. We need more info for further investigation.

Betaflight should not allow arming when there is some condition allow disarming to have a delay as safety precaution.

@KarateBrot
Copy link
Member

KarateBrot commented Jan 13, 2024

We need to investigate what actually causes the issue and why the disarm is delayed. This seems to be a bug, maybe a racing condition of two tasks. Premature PRs which limit capable CPUs is not the answer. We should first check what the issue is and then open a PR to fix it.

@haslinghuis haslinghuis changed the title Do not arm above threshold Do not arm above configurable threshold Jan 13, 2024
@ctzsnooze
Copy link
Member

I'll close this in 24 hours unless there is supporting data

@haslinghuis
Copy link
Member Author

Closing now - as this should be fixed as stated before.

Not sure how tasks will help in this situation as the quad cannot be armed. Have asked for some blackbox logging in the issue.

Please continue investigation.

@haslinghuis haslinghuis deleted the disable-arm-on-high-cpu-load branch January 14, 2024 01:45
@ctzsnooze
Copy link
Member

Tasks tells us whether the issue relates to CPU load or not, by showing how many tasks are running late over the interval since the lasts tasks run.

If there is no significant difference in task over-runs between 8k8k and 8k4k, then it is unlikely that a CPU load/tasks over-run would be the cause.

On the other hand, if some specific task is running long, it may give us a clue as to what the problem is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

Delay on disarm
6 participants