-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
common: make PARACHUTE_ACTION enum consistent #1633
Conversation
I think that's more consistent and easier to grep this way.
@auturgy @peterbarker This would be good cleanup, but could be a breaking change - is it worth the risk from your perspective? |
FWIW
|
In my opinion it should be ok to break this because it's caught at compile time which is easy to fix. Plus, the data stays the same on the wire. Unless people disagree, then I'll close this. |
I don't disagree the intent, but don't think a cosmetic tidy up warrants breaking safety critical systems. |
Both the change and the risk are trivial. Personally I think it is worth making the change. I would object to the same change in MAV_COMPONENT, because there are so many instances of these things that the risk and effort are increased. |
I don't see the risk as trivial: this is implemented in a number of ways external to the flight controller and gcs. I agree the intent but introducing risk into a safety critical system for what is a cosmetic tidy up does not make sense to me. The benefit is trivial compared to the consequence. |
Just to understand: @auturgy what is your safety concern? This does not change the meaning or values on the wire. |
I understand that: but we don’t have visibility of how this is implemented in external micros, scripts, devices etc. If we can prove that this isn’t going to break anything anywhere, great. But I don’t think we can. We’d be relying on implementers to test and update all components and I don’t think we should expose our communities to that for a cosmetic change.
If this wasn’t for parachute I’d be less concerned, but when you need a chute to deploy, you need it to deploy.
… On 12 May 2021, at 5:38 pm, Julian Oes ***@***.***> wrote:
Just to understand: @auturgy what is your safety concern? This does not change the meaning or values on the wire.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
I think those concerns are fair and the risk / reward is not in favor of this change. I think we need to solve versioning in order to be able to make changes like this. |
I think that's more consistent and easier to grep this way.