[Accepted] SDL 0308 - Add a Reason Parameter to All Protocol NAKs

theresalech commented 4 years ago

Hello SDL community,

The review of "SDL 0308 - Add a Reason Parameter to All Protocol NAKs" begins now and runs through June 9, 2020. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0308-protocol-nak-reason.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

https://github.com/smartdevicelink/sdl_evolution/issues/1027

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

Is the problem being addressed significant enough to warrant a change to SDL?
Does this proposal fit well with the feel and direction of SDL?
If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study? Please state explicitly whether you believe that the proposal should be accepted into SDL.

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you, Theresa Lech

Program Manager - Livio theresa@livio.io

Sohei-Suzuki-Nexty commented 4 years ago

1. Why is this parameter type "string" rather than an enum with a defined reason? Does the OEM need to set the reason string? We are concerned that if the behavior needs to change depending on the parameters, it will be difficult to implement.

joeljfischer commented 4 years ago

1a.

Why is this parameter type "string" rather than an enum with a defined reason?

First, because the protocol spec doesn't have enums, so at best this would an informal enum. Second, because we may want to add additional strings in the future without updating the protocol_spec.

1b.

Does the OEM need to set the reason string?

No, as the proposal states, this will be handled by Core, not HMI:

update Core to provide strings for all known cases of NAKing a protocol spec request

If an OEM modifies Core, then they may need to add additional strings or modify these reasons.

1c.

We are concerned that if the behavior needs to change depending on the parameters, it will be difficult to implement.

I'm not sure what is meant here. What parameters? If the NAK is occurring, then Core should know the reason based on context. Passing an error message is standard practice in APIs when a failure occurs.

Shohei-Kawano commented 4 years ago

@joeljfischer -san Thanks your answer.

We expected the OEM to set the string. If that is the case, the strings may differ depending on the OEM, so I thought it would be confusing if there were logics that read the strings and separate the operations. However, if the string is set in Core, it will be a fixed string and our concern will be gone.

theresalech commented 4 years ago

The Steering Committee fully agreed to accept this proposal. It was noted that the comments on the review issue had been answered, and previously voice concerns had been addressed.

theresalech commented 4 years ago

Implementation Issues entered:

smartdevicelink / sdl_evolution

[Accepted] SDL 0308 - Add a Reason Parameter to All Protocol NAKs #1027