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

Rename BRD_SAFETYENABLE to BRD_SAFETY_START #9307

Closed
wants to merge 1 commit into from

Conversation

Pedals2Paddles
Copy link
Contributor

This parameter name is extremely misleading. This does not enable or disable the safety. It is setting the startup state. So it should reflect that in the name. I'm not at home right now, so I don't have the ability to do more than this. If more is required, I can do later.

This is in response to several issues over the last few days that revolve around misunderstanding the function of this parameter (myself included). The parameter name and display name were both contradictory to the actual function.

This parameter name is extremely misleading.  This does not enable or disable the safety.  It is setting the startup state. So it should reflect that in the name.
@MidwestAire
Copy link
Member

This PR references issue #9304 but it does not address the base flaw in the code logic.

@Pedals2Paddles
Copy link
Contributor Author

Update. Not actually a code logic issue. The issue is handling of brd_safetyoption has not yet been implemented in ChibiOS. And that therefore allowed the hardware safety switch to be active in flight despite the option bit not being set, leading to the incident. The code does exist and does work in nuttx still.

@rmackay9
Copy link
Contributor

I'm not sure renaming the parameter is such a good idea. People are familiar with the existing name and I'm not sure the new name really makes it more likely that a user will understand what it's for.

@MidwestAire
Copy link
Member

Many users, @bnsgeyer included, assume that BRD_SAFETYENABLE = 0 totally disables the safety circuit. That's why I questioned the logic. For the computer programmer the logic might seem sensible, for the average user it's not. At the moment I don't think we can do anything with this. If the switch just did a disarm instead of I/O shutdown it would be different. But we do need to work on a fix for heli to lock out ActiveWhenArmed at the minimum, as we can't ever allow I/O shutdown on helicopters from armed state.

@OXINARF OXINARF added the Library label Sep 4, 2018
@OXINARF
Copy link
Member

OXINARF commented Sep 4, 2018

I think the question is: is there an actual valid reason for someone to have an active/working safety switch connected, but start it in unsafe state?

If there is, then code should stay as is and param name changed. We should also update documentation (both in parameters and wiki) to make clear that disabling the safety switch is done via BRD_SAFETYOPTIONS.

If there isn't, then keep the name (since, as @rmackay9 points out, many people are familiar with it), but change the code to actually disable the safety switch (despite the value of BRD_SAFETYOPTIONS) instead of only setting the initial state.

@MidwestAire
Copy link
Member

My opinion is that since we have the problem patched that caused it to randomly enable that it should stay as-is going into 3.6 for as few changes as possible. And maybe look at making any future changes in master. As @tridge pointed out, making BRD_SAFETYENABLE disable the switch totally is quite hard to do because of the hardware state when the I/O initializes. Even if it is disabled via the SAFETYENABLE setting an I/O reset in flight will still result in it being active. The patch tridge did addresses that.

So if anything, for any future changes in master the SAFETYENABLE param should probably be eliminated and make more options for the SAFETYOPTION setting to set its initial state and active states with a bitmask, so it's all in one setting.

@Pedals2Paddles
Copy link
Contributor Author

I agree with all of the above. Closing.

@Pedals2Paddles Pedals2Paddles deleted the Pedals2Paddles-patch-1 branch September 4, 2018 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants