[Accepted] SDL 0064 - Choice-VR optional

theresalech commented 7 years ago

Hello SDL community,

The review of the revised proposal "SDL 0064 - Choice-VR optional" begins now and runs through April 24, 2018. The original review of "Choice-VR optional" took place May 24 - June 7, 2017. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0064-choice-vr-optional.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/189

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

joeygrover commented 7 years ago

This would be the first time we have moved from a mandatory parameter to an optional one. We would have to ensure we fully understand the impact of doing so.
Since older version of the spec will always still require the vrCommands param, what would you suggest the default value be in the case the app developer didn't provide anything?

kshala-ford commented 7 years ago

@joeygrover

The impact is quite simple.

existing apps would continue to work with no issues on any head unit
today existing apps use numbers (1, 2, 3 ...) as placeholders when creating a choiceset which will never be performed in VR mode.
existing head units with mandatory vr commands would reply with INVALID_DATA if an app doesn't provide vr commands.
head units that don't provide VR capabilities at all would just ignore the vr portion (and reply with a warning)

As mentioned in the proposal it is very important to know the version number of the APIs that introduce optional VR commands.

theresalech commented 7 years ago

Because Livio and Ford are not in line with how the libraries will handle the switch, we will look to the Steering Committee to provide input and agree on how to make breaking changes to the RPC spec and API. The Steering Committee will vote on this issue during the June 6, 2017 (2017-06-06) Meeting.

theresalech commented 7 years ago

The Steering Committee voted to defer this proposal in the meeting on May 31, 2017 (2017-05-31). Here is the rationale: We need a separate proposal on how to handle breaking changes in the RPC Spec. Once that proposal has been entered and voted upon, we can vote on this proposal, as it includes a breaking change of moving a parameter from Mandatory to Optional.

theresalech commented 6 years ago

Now that SDL 0089 has been accepted, the Steering Committee has voted to return this proposal for revisions. The author will update the proposal based on the accepted version of SDL 0089, once SDL 0089 has been implemented.

theresalech commented 6 years ago

The author has updated this proposal per the requested revisions, and those revisions are now ready for Steering Committee review. This revised proposal will be in review until April 24, 2018.

joeljfischer commented 6 years ago

I have a few notes:

The current info for a mixed vrCommand result is: info = "Some choices don't contain VR commands." I would prefer to see something slightly more descriptive, like info = "Some choices don't contain VR commands. Either all or none must have voice commands."
This may be a more major version for the SDKs because they will have to auto-fill the vrCommands if they are connected to a head unit that requires them but the app is running a proxy version that does not.

kshala-ford commented 6 years ago

The current info for a mixed vrCommand result is: info = "Some choices don't contain VR commands." I would prefer to see something slightly more descriptive, like info = "Some choices don't contain VR commands. Either all or none must have voice commands."

:thumbsup:

This may be a more major version for the SDKs because they will have to auto-fill the vrCommands if they are connected to a head unit that requires them but the app is running a proxy version that does not.

I'm afraid I don't understand what you mean with "more major vesion". You mean more effort to implement? Let me try to explain how I would expect the SDK to work with this change:

In the proxy layer we must make Choice.vrCommands nullable and mention in the code comments of the property "Starting with RPC spec version X.Y vr Commands is optional but <X.Y it's mandatory".

In the management layer I would expect the choice set manager to use the auto-generated choiceID as a VR command placeholder (auto-fill) if the app is connected to a head unit that doesn't support optional VR commands. Still these choices must not be used for VR sessions. This way the app developer wouldn't need to care about it.

In fact since Ford's SYNC3 head unit v2.3 I can send choices without .vrCommands and I receive SUCCESS. I think that's the MsgVersion 4.2... That's a decent amount of vehicles already supporting it. Therefore my suggestion for the choice set manager would be the following (I guess I should copy that to the acutal manager proposal):

The choice set manager should create a single choice set as a place holder (the placeholder choice) for keyboard presentation. (This is required anyway because .interactionChoiceSetIDList is mandatory. We should propose to make it optional for 5.0 as well)
The placeholder choice should be created after the app was launched. This is the case if the app entered an HMI level that allows CreateIntearctionChoiceSet for the first time after registration.
The placeholder choice should contain a reserved .choiceID and .text when being sent by the manager.
If the head unit replies with SUCCESS the manager should remember that and not auto-fill .vrCommands for non-voice choices created by the app later on.
If the head unit replies with INVALID_DATA (happens within <50ms) the manager should repeat sending the placeholder choice but this time using .vrCommands set to ["\(.choiceID)"]. The manager should remember that auto-fill is required for non-voice choices.

Sounds like reverse engineering... but it would make it possible to allow non-voice choices on existing head units.

joeljfischer commented 6 years ago

This may be a more major version for the SDKs because they will have to auto-fill the vrCommands if they are connected to a head unit that requires them but the app is running a proxy version that does not.

I'm afraid I don't understand what you mean with "more major version". You mean more effort to implement?

Sorry, that was bad terminology. More work to be done than the very minor proxy work the proposal states, not a major version change. I suppose its true if there's no choice set manager, but that component will make for more work.

As far as the rest of the flow goes, that sounds about right to me.

theresalech commented 6 years ago

The Steering Committee agreed to accept this proposal and also agreed that the error for mixed vrCommand should be more descriptive, like info = "Some choices don't contain VR commands. Either all or none must have voice commands."

theresalech commented 6 years ago

Issues Entered RPC Spec Android iOS Core

smartdevicelink / sdl_evolution

[Accepted] SDL 0064 - Choice-VR optional #189