Sdl_evolution: [Accepted with Revisions] SDL 0266 - New vehicle data GearStatus

[theresalech]

Hello SDL community,

The review of the revised "SDL 0266 - New vehicle data GearStatus" proposal begins now and runs through January 14, 2020. The original review of this proposal took place December 4 - 10, 2019. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0266-New-vehicle-data-GearStatus.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/881

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
[email protected]

[joeljfischer]

1. Why are we creating a new enum GearPosition, when it basically just replicates the enum PRNDL? There doesn't seem to be a point to deprecating PRNDL and creating GearPosition except maybe that GearPosition has a better name? But I don't believe that's a good enough reason.

[atiwari9]

@joeljfischer

1. 
   I needed to add some new elements to the PRNDL enum which would have caused a revision in itself. So while we are doing that, there is an opportunity to rename the enum to represent it better as covers more than just PRNDL gear/drive positions(think both gearDriveMode and gearPosition).

I understand the aspect of liabilities associated with maintaining the code base, but in any case there will be new additions to the enum. Functionally it won't have an impact if the enum is not renamed, but since we strive to make the spec more readable with every iteration, this change does make sense.

[joeygrover]

@atiwari9 this is a breaking change, that's a big deal. It causes a great impact on the APIs that has to be maintained indefinitely, it is simply not worth it. there is no real opportunity here; it is an unnecessary change. If you need to add values to the PRNDL enum that's fine, but changing the enum name is not something that should be accepted or implemented.

[kshala-ford]

Without rating if the change is worth it, I want to note that the change is not a breaking change because:

1. the proposal only deprecates the enum PRNDL together with the params prndl in the vehicle data RPCs.

2. A new type GearStatus with new params gearStatus are proposed.

These two changes are logically related to the context of gears but they are not related to each other from an API perspective. A breaking change would be to instantly change prndl params to GearStatus which is not proposed!

[kshala-ford]

Oh and the proposed solution gives value because apps can read the automatic transmission position and the current gear of the gearbox.

[atiwari9]

@joeygrover - I am not very much adamant on renaming the enum, as i said it won't have any functional impact if we do not rename it. But i do have some discussion points:

PRNDL enum is only being used by vehicle data item prndl, with this proposal prndl is being deprecated i.e. it'd be removed with major version change. PRNDL enum is proposed to be discontinued with that change itself. Meanwhile new vehicle data item gearStatus and new enum gearPosition would be added to a minor version change to provide a transition time.

This is just to invoke a thought that we can make the name more relevant to the function they perform.

[joeygrover]

Yea sorry it's not breaking in the proposal yet. Changing names in a spec has to be done very carefully and warrant change because the mobile libraries will have to be maintained indefinitely to support both. Then there is the education to developers of when they should request one vehicle data over the other and when you can do that and when you can't. I do not see this as a case that warrants the change. It would be easier to add a new vehicle data item with a name like actualGearPosition and use the PRNDL enum. Also, proxy libraries could be changed to address such name changes or better reflection of status.

[joeljfischer]

There is a big difference between adding new values to an enum, and deprecating an entire enum, while replicating 90% of it with a new name. I'm strongly against the current state of the proposal and would very much prefer to see PRNDL extended instead.

We should be attempting to get the features we want while being very judicious in our alterations to the API because app devs need to always support old and new versions (which means either they need to do it themselves, or we need to do it for them).

[atiwari9]

@joeljfischer @joeygrover -

I believe your concerns are NOT just limited to enum but also to change the vehicle data name from prndl to gearStatus. Yes we'd need to support both of these at proxy side based on SDL Core version app connects to. But as long as there is proper documentation, this should not be a problem for app developer to understand, specially when it comes to supporting implementations with next major version change. If we agree to replace vehicle data item itself, then renaming enums appropriately only makes sense,

This is the idea of deprecation itself that we provide a transition time while removing support for older APIs. I agree that the transition time in car world is way larger for OEMs and it'd probably need maintaining both vehicle data items for considerable time. But that itself is inherent limitation of this approach. I believe this discussion is bigger than just one vehicle data item and should apply to any feature we plan to change/deprecate,

I am receptive of the concern of effort around maintaining both versions and that the effort could be utilized to address other areas of improvements and new feature additions. So can you please provide your summarized input on what is your suggestive change:

• Option1-Change vehicle data item name from prndl to gearStatus & change enum from PRNDL to GearPosition (with new elements)
• Option2-Change vehicle data item name from prndl to gearStatus & extend enum PRNDL enum with new elements.
• Option3-Keep prndl vehicle data item & change type to struct GearStatus so as to accommodate gearDriveMode & actualGear. Extend prndl enum with new elements.

[joeljfischer]

But as long as there is proper documentation, this should not be a problem for app developer to understand, specially when it comes to supporting implementations with next major version change. If we agree to replace vehicle data item itself, then renaming enums appropriately only makes sense,

I strongly disagree with this. Developers don't want to have to create two separate but similar implementations to support multiple versions and we should avoid it wherever possible.

This is the idea of deprecation itself that we provide a transition time while removing support for older APIs. I agree that the transition time in car world is way larger for OEMs and it'd probably need maintaining both vehicle data items for considerable time. But that itself is inherent limitation of this approach. I believe this discussion is bigger than just one vehicle data item and should apply to any feature we plan to change/deprecate.

Yes, we've had these discussions many times in the past. Deprecations and even removals in the API are _very_ different from other APIs, partly because we have to support support two-way backward compatibility (Core at least must support old app libraries, and old app libraries must support old versions of Core / HMI). That, combined with the need to have backward compatibility for a decade or more due to the long lifecycles of vehicles, means that deprecations and removals are different, because they're continued to be supported due to backward compatibility concerns.

What that means is that deprecation / replace needs to be applied only in cases where we don't have any other good options. We've never had an instance where we've actually removed MOBILE_API APIs that I'm aware of, even across several major versions to this point.

With all of that said, I think it's clear that this is not a case where the scale of deprecation / replace in this proposal is necessary. I don't even think it's necessary at all.

I'm strongly against Option 1. It's simply unnecessary to do the scale of changes as presented. I'm okay with option 2 (assuming that by "change" you mean deprecate / replace, not actually "change"). I'm strongly against option 3 because it's a major version change (and the form of major version change is effectively un-implementable for the app libraries without breaking support for prndl with older head units).

I would propose an option 4. Option 4 is to keep the prndl vehicle data item, note that it refers to the gear's drive mode or actual state (depending on how it's currently implemented). Then add a new item for the other one, of type PRNDL and add the new items to the PRNDL enum. I'm most in favor of option 4 as it's purely additive and does not deprecate anything.

[joeygrover]

My concerns are added technical debt and developer burden with unnecessary changes. You are misunderstanding what I was saying. This is not a change I can accept in its current form.

The only option here is to leave prndl param named as is and PRNDL enum. If you want a second vehicle data item you can add a new actualGearStatus (or similar) param that uses PRNDL. This is in-place of creating the new struct GearStatus and it's param actualGear

<param name="prndl" type="PRNDL" mandatory="false" >
    <description>See "actualGearStatus for more detailed information.</description>
</param>
<param name="actualGearStatus" type="PRNDL" mandatory="false" >
    <description>Actual gear the vehicle is operating in. For use when "prndl" status is reported as an ambiguous status, eg "DRIVE" or "SPORT"</description>
</param>

[atiwari9]

Thanks for clarifying the deprecation process and i do agree that it creates extra work if we have to maintain some code for as long as 10 years. To satisfy the aim of minimum effort in long run, it does makes sense to add another vehicle data item and keep prndl as it is. But we should make the new vehicle data item scalable(keep type as struct) so that new params can be added to the vehicle data item without having to worry about maintenance part, So i propose to:

• Keep prndl vehicle data item as it is. (update description though)
• Add new vehicle data item gearStatus of type GearStatus struct
  
  
  
  • remove gearDriveMode from GearStatus struct as this would be read using prndl,
  
  
  • change actualGear param type to PRNDL
  
  
• Add new elements to PRNDL enum

Add to enum VehicleDataType:

<element name="VEHICLEDATA_GEARSTATUS" since="X.x"/>

Add new struct GearStatus:

<struct name="GearStatus" since="X.x">
    <param name="actualGear" type="PRNDL" mandatory="false">
        <description>Tells the actual gear selected by transmission</description>
    </param>
</struct>

Update enum PRNDL:

<enum name="PRNDL" since="x.x">
    <description>The selected gear.</description>
    <element name="PARK">
        <description>Parking</description>
    </element>
    <element name="REVERSE">
        <description>Reverse gear</description>
    </element>
    <element name="NEUTRAL">
        <description>No gear</description>
    </element>
    <element name="DRIVE">
        <description>Regular Drive mode</description>
    </element>
    <element name="SPORT">
        <description>Drive Sport mode</description>
    </element>
    <element name="LOWGEAR">
        <description>1st gear hold</description>
    </element>
    <element name="FIRST">
    </element>
    <element name="SECOND">
    </element>
    <element name="THIRD">
    </element>
    <element name="FOURTH">
    </element>
    <element name="FIFTH">
    </element>
    <element name="SIXTH">
    </element>
    <element name="SEVENTH">
    </element>
    <element name="EIGHTH">
    </element>
+   <element name="NINTH" since="X.x">
+   </element>
+   <element name="TENTH" since="X.x">
+   </element>
    <element name="UNKNOWN">
    </element>
    <element name="FAULT">
    </element>
</enum> 

Update following parameters in these function requests:

• SubscribeVehicleData
• UnsubscribeVehicleData
• GetVehicleData

<param name="gearStatus" type="Boolean" mandatory="false" since="X.x">
    <description>Provides granular details on selected gear/drive mode. See GearStatus</description>
</param>
<param name="prndl" type="Boolean" mandatory="false">
    <description>Provides gear drive mode information. Granular details are provided in gearStatus vehicle data.</description>
</param>

Update following parameters in these function responses:

• SubscribeVehicleData
• UnsubscribeVehicleData

<param name="gearStatus" type="VehicleDataResult" mandatory="false" since="X.x">
    <description>Provides granular details on selected gear/drive mode. See GearStatus</description>
</param>
<param name="prndl" type="VehicleDataResult" mandatory="false">
    <description>Provides gear drive mode information. Granular details are provided in gearStatus vehicle data.</description>
</param>

Update following parameters in these function responses:

• GetVehicleData
• OnVehicleData

<param name="gearStatus" type="GearStatus" mandatory="false" since="X.x">
    <description>Provides granular details on selected gear/drive mode. See GearStatus</description>
</param>
<param name="prndl" type="PRNDL" mandatory="false">
    <description>Provides gear drive mode information. Granular details are provided in gearStatus vehicle data.</description>
</param>

This way, if we do need to add some new param related to gear selection, we can simply add that to GearStatus struct.

[joeljfischer]

I don't think that we should add the GearStatus struct. It doesn't make sense to add a struct for only one parameter and don't know if we'll ever add more. Additionally, this isn't the current structure of the API, so this would add a very odd case. I would suggest Joey's comment as a better way to go.

[atiwari9]

@joeljfischer - I tend to disagree, we have an example where we didn't think that PRNDL would need more details. Had it been a structure already, we wouldn't need to add a new vehicle data item. While we are already deviating from the best practice of having the correct name representation, i am not in favor of keep adding new vehicle data items while we could have utilized existing ones.

Adding a new data item does add complexity in spec/server/permission groups etc. So when ever possible, we should consolidate like vehicle data items and switch to new additions when consolidation does not add value or does not make sense due to different kind of vehicle data items.

[kshala-ford]

We deprecated fuel level and migrated it into fuel type. Sure, fuel type already exist. In this case I agree to @atiwari9 if we would already have a struct we wouldn't be discussing about the deprecation. Having a struct allows adding new context related information at a later point. A new enum based vehicle data item limits us again and could make things worse if we add even more gear related information.

I support adding GearStatus with following params:

1. migrating prndl as the selectedGear indicating the driver selected gear status
2. adding actualGear as already suggested indicating the gear selected by the transmission
3. Gear type. In Europe manual transmission is still a thing. The market share is really high especially for medium and lower level vehicles. I don't have specific numbers but the number of vehicles with manual transmission is significant.

[joeygrover]

1. Why don't we just update the description of the 'prndl' variable instead to

<param name="prndl" type="VehicleDataResult" mandatory="false">
    <description>Provides selected gear drive mode information. Granular details are provided in gearStatus vehicle data.</description>
</param>

Changing a key adds so much extra complexity that is not needed. You can change the name in the mobile libraries if you want (getSelectedGear()), but changing the actual key for "clarity" does not work for our situation and is absolutely unnecessary.

3. Is Gear type new? Where would this go?

[theresalech]

The Steering Committee voted to return this proposal for revisions. The author will revise to deprecate the prndl vehicle data item and add a new vehicle data item gearStatus. This item will use a new struct, GearStatus, that includes the old prndl data but with the key selectedGear, but still using the PRNDL enum, a new item actualGear that uses the PRNDL enum, and finally a new data item transmissionType that uses a new enum TransmissionType that will have its values first defined by the author before the larger Steering Committee review.

[theresalech]

The author has revised this proposal to incorporate the requested revisions from the Steering Committee. The revised proposal is now in review and will be voted upon during the 2020-01-14 meeting.

[atiwari9]

1. First off, i want to correct the param selectedGear -> gearDriveMode.

   gearDriveMode makes more sense to represent _PRNDL/PRNDS_ rather than selectedGear which can easily be confused with actualGear(gear selected by transmission control module).

[joeljfischer]

@atiwari9 I think that gearDriveMode is less descriptive and more easily confused with actualGear than selectedGear is. I would think that gearDriveMode is the actual gear in use.

[joeygrover]

I agree with @joeljfischer. selectedGear and actualGear are a good pair. I instantly think the selectedGear is one selected by the user, while the actualGear is the one the system has chosen. I think any confusion could be addressed in the descriptions of the params. The name gearDriveMode is confusing and I don't believe the change to be for the better.

The current state of the proposal I do agree with it.

[atiwari9]

@joeljfischer - i'd disagree, actualGear and seletedGear have more chance of being confused with one another rather than gearDriveMode and actualGear.

It needs to be a _DriveMode_ to represent Park, Drive, Reverse, LowGear etc. But just saying _DriveMode_ could be confused with driving conditions like Snow, Mud, OffRoad etc.

[joeljfischer]

@atiwari9 In America at least, nobody that I know thinks of those as "drive modes." They're "gears." I strongly disagree with your naming.

[atiwari9]

@joeygrover - selectedGear could also be confused by gear selected by transmission. If it was userSelectedGear it'd be unambiguous.

@joeljfischer - Well, _DriveMode_ is not that alien of a term that you'd think, I can say that Ford/Lincoln cars use that, a quick example here

But a quick search provided other OEMs also using the drive modes like these:

Volvo

Kia

Toyota

[joeygrover]

@atiwari9 your example video is an example that the naming you suggest is not equal to what it means.

_DriveMode_ is Normal, Conserve, Slippery, etc. This is how the transmission will move through gears, distribute power, etc. It is a modification on the drive gear selection.

_DriveMode_ is NOT the gear name such as Drive, Park, Reverse, etc.

Again I will state the name should stay and strongly disagree with gearDriveMode naming as it is misleading. The description of the param is all that is necessary to clarify that it is user selected.

[atiwari9]

@joeygrover - Exactly the same point i specified in earlier comment , that's why i said gearDriveMode instead of just driveMode.

Just saying selectedGear is also not clear enough for me as it could mean that transmissions selected the gear which in turn is same as actualGear. It'd be better if we can call it say userSelectedGear.

So userSelectedGear or gearDriveMode are the options which represent this param better IMO.

[JackLivio]

Hi Ankur, I think updating the language in the descriptions might help a bit with avoiding confusion?

<struct name="GearStatus">
    <param name="selectedGear" type="Common.PRNDL" mandatory="false">
        <description>Gear position selected by the user ie Park, Drive, Reverse</description>
    </param>
    <param name="actualGear" type="Common.PRNDL" mandatory="false">
        <description>Actual Gear in use by the transmission</description>
    </param>
    <param name="transmissionType" type="Common.TransmissionType" mandatory="false">
        <description>Tells the transmission type</description>
    </param>
</struct>

[atiwari9]

@JackLivio - I welcome this suggestion to updated the description, but that alone would not be sufficient when say someone is coding for these values in the app etc. selectedGear and actualGear are too close in the contextual meaning here. It seems like PM team are inclined towards selectedGear option, then i'd say let's just change it to userSelectedGear. After all, we are trying to reduce the burden on the developers and making the params self explanatory is really important in that.

[theresalech]

The Steering Committee voted to accept this proposal with the following revisions: use userSelectedGear in place of selectedGear and use the descriptions for GearStatus included in this comment.

[theresalech]

@atiwari9 Please advise when a new PR has been entered to update the proposal to reflect the agreed upon revisions. I'll then merge the PR so the proposal is up to date, and enter issues in the respective repositories for implementation. Thanks!

[theresalech]

Proposal has been updated to reflect agreed upon revisions, and implementation issues have been entered:

• RPC
• Core
• HMI
• iOS
• Java Suite
• Server

[theresalech]

JavaScript Suite Issue: https://github.com/smartdevicelink/sdl_javascript_suite/issues/255