Sdl_evolution: [Accepted with Revisions] SDL 0261 - New vehicle data WindowStatus

[theresalech]

Hello SDL community,

The review of "SDL 0261 - New vehicle data WindowStatus" begins now and runs through December 3, 2019. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0261-New-vehicle-data-WindowStatus.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/869

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. Non-mandatory arrays should have minSize=1.
2. I think GateWindowStatus.gateType needs updating for the grid.
3. I think the WindowState enums need better definition, like "0% open", "1-10% open", "10-49% open", "50-90% open", "90-99% open", and "100% open". Or, use an integer to represent the percentage open instead of using an enum.

[kshala-ford]

Just some thoughts to this proposal and the feedback.

1. There could be cases where a non-mandatory array can be an empty array. See soft button array in Show. Also while omitted parameter means "windows is undefined/unknown", an empty array could indicate "there are no windows"...

it's a theoretical thought ... every car has windows ... but just to give an example of minSize=0

1. I kind of agree but I'm afraid we can't provide an integer unless it jumps from 0% to 10% to 50% etc.

2. With regards to implementing the RPC code generator I would suggest to remove internal_name as long as they are not mandatory in the SDL implementation. Rationale: The generator scripts will create enum elements based on the internal name. I would like the libraries to provide WindowState.CLOSED or WindowState.closed instead of WindowState.wpClosed` etc.

[atiwari9]

1. @kshala-ford - What'd be the different between and empty array response and result code VEHICLE_DATA_NOT_AVAILABLE in case there are no supported windows.

2. @joeljfischer - After agreed upon revisions for Enhance BodyInformation vehicle data, i believe we can update this proposal to cover door and window status in one param itself since both of these would be covered by Grid.

So following changes would be made to MOBILE/HMI API:

Add to enum VehicleDataType:

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

Add new struct WindowStatus:

<struct name="WindowStatus" since="X.x">
    <description>Describes the status of a window of a door/liftgate etc.</description>
    <param name="location" type="Grid" mandatory="true"/>
    <param name="state" type="WindowState" mandatory="true"/>           
</struct>

Add new enum WindowState:

<enum name="WindowState" since="X.x">
    <description>Reflects the postion of window.</description>
    <element name="UNDEFINED">
    </element>
    <element name="CLOSED">
    </element>
    <element name="BARELY_OPEN">
    </element>
    <element name="NEARLY_HALF_OPEN">
    </element>
    <element name="NEARLY_FULL_OPEN">
    </element>
    <element name="OPEN">
    </element>
    <element name="UNUSED">
    </element>
</enum>

Add the following parameter to these function requests:

• SubscribeVehicleData
• UnsubscribeVehicleData
• GetVehicleData

<param name="windowStatus" type="Boolean" mandatory="false" since="X.x">
    <description>See WindowStatus</description>
</param>

Add the following parameter to these function responses:

• SubscribeVehicleData
• UnsubscribeVehicleData

<param name="windowStatus" type="VehicleDataResult" mandatory="false" since="X.x">
    <description>See WindowStatus</description>
</param>

Add the following parameter to these function responses:

• GetVehicleData
• OnVehicleData

<param name="windowStatus" type="WindowStatus" array="true" minsize="0" maxsize="100" mandatory="false">
    <description>See WindowStatus</description>
</param>

1. @joeljfischer / @kshala-ford - Not all OEMs might have same mapping for window related signals, so i did not want to tie a specific %age number to enums. e.g. if an OEM has window signal sent for 0%, 1-5% , 6-40%, 41-80%, 81-95%, 95-99%, 100% then suggested enum values in previous comment do not directly apply. I personally would love to have a raw %age point as the position value but did not propose it for the same reason as stated earlier.

2. @kshala-ford - Ok, #ToUpdate . I have removed that from proposed changes in point 2 above,

[joeljfischer]

1. I agree that the minsize should be 0.
2. The only thing we lose by combining door and gate status is that the developer no longer knows if it's a door or gate. However, I think this is okay because the developer can just correlate it with the BodyInformation proposal.
3. I understand. I don't like it, but I understand. I think the enums are missing some cases however.

<enum name="WindowState" since="X.x">
    <description>Reflects the postion of window.</description>
    <element name="UNDEFINED">
    </element>
    <element name="CLOSED">
    </element>
    <element name="SLIGHTLY_OPEN">
    </element>
    <element name="LESS_THAN_HALF_OPEN">
    </element>
    <element name="MORE_THAN_HALF_OPEN">
    </element>
    <element name="LESS_THAN_FULL_OPEN">
    </element>
    <element name="OPEN">
    </element>
</enum>

I'm not sure what UNDEFINED would mean here, but I think it's useful in case something went wrong with the internal system. I don't think UNUSED is necessary. That's handled outside of the enum. I changed the enums to be usable by OEMs that support only two states (CLOSED / OPEN), 3 states (CLOSED, LESS_THAN_FULL_OPEN, OPEN), and more states than that. I think that an enum description and / or element subscriptions should be added to describe how the enum would be used in these various cases.

[atiwari9]

@joeljfischer

1. Adding to your point, any grid location outside of values defined in SeatLocationCapability would be gates.

2. This looks mostly OK to me as well, we should also include element HALF_OPEN as well. So the enum WindowState would be:

<enum name="WindowState" since="X.x">
    <description>Reflects the postion of window.</description>
    <element name="UNDEFINED">
    </element>
    <element name="CLOSED">
    </element>
    <element name="SLIGHTLY_OPEN">
    </element>
    <element name="LESS_THAN_HALF_OPEN">
    </element>
+   <element name="HALF_OPEN">
+   </element>
    <element name="MORE_THAN_HALF_OPEN">
    </element>
    <element name="LESS_THAN_FULL_OPEN">
    </element>
    <element name="OPEN">
    </element>
</enum>

[joeljfischer]

@atiwari9 On 3, I see what you're saying, but I disagree with the need for another enum. I don't see a need for 49.999% and less, 50%, and 50.001% and more. I would suggest changing MORE_THAN_HALF_OPEN to HALF_OPEN_OR_MORE.

[joeygrover]

I would also suggest we put these percentages into the descriptions of the elements. If we don't put guidelines on what they mean, each implementation could be different.

[atiwari9]

@joeljfischer - In that case HALF_OPEN_OR_MORE is better.

@joeygrover - Added description as well with percentage points.

<enum name="WindowState" since="X.x">
    <description>Reflects the postion of window.</description>
    <element name="UNDEFINED">
        <description>
            Window position is undefined
        </description>
    </element>
    <element name="CLOSED">
        <description>
            Window position is 0% open.
        </description>
    </element>
    <element name="SLIGHTLY_OPEN">
        <description>
            Window position is 1%-10% open.
        </description>
    </element>
    <element name="LESS_THAN_HALF_OPEN">
        <description>
            Window position is 11%-49% open.
        </description>
    </element>
    <element name="HALF_OPEN_OR_MORE">
        <description>
            Window position is 50%-89% open.
        </description>
    </element>
    <element name="LESS_THAN_FULL_OPEN">
        <description>
            Window position is 90%-99% open.
        </description>
    </element>
    <element name="OPEN">
        <description>
            Window position is 100% open.
        </description>
    </element>
</enum>

[theresalech]

During the Steering Committee meeting, the author and project maintainer discussed possible issues with using an enum for WindowState, including the situation where an OEM doesn't support granular window positions and therefore couldn't use the enum. There was conversation about the OEM sending min and max values to be averaged, or values and deviations for developers to use, as well as the request for other SDLC Members to provide their input on which implementation of this feature would work best for them. Ultimately, the Steering Committee voted to keep this proposal in review so that an alternative to the WindowState enum could be discussed and determined.

[joeljfischer]

Based on our discussion, here's what I would suggest.

Either (1):

<struct name="WindowState" since="X.X">
  <param name="approximateLocation" type="Integer" minValue="0" maxValue="100">
    <description>The approximate percentage that the window is open - 0 being fully closed, 100 being fully open</description>
  </param>
  <param name="deviation" type="Integer" minValue="0" maxValue="100">
    <description>The percentage deviation of the approximateLocation. e.g. If the approximateLocation is 50 and the deviation is 10, then the window's location is somewhere between 40 and 60.</description>
  </param>
</struct>

Or (2):

<struct name="WindowState" since="X.X">
  <description>The window state provides a minimum and maximum position the window could be in. The average of the min and max is an approximation of the window's location. e.g. If the minValue is 40 and the maxValue is 60, then a developer could display the window at 50% in their UI.</description>
  <param name="minLocation" type="Integer" minValue="0" maxValue="100">
    <description>The lowest percentage position the window could be in.</description>
  </param>
  <param name="maxLocation" type="Integer" minValue="0" maxValue="100">
    <description>The highest percentage position the window could be in.</description>
  </param>
</struct>

[kshala-ford]

Personally, I would lean towards option 1. I believe app developers are mostly interested into the location even if it's only approximated. With option 2 the app developer would need to calculate the Average between min and max which doesn't sound really convenient to me...

[joeljfischer]

@kshala-ford Both provide the exact same information. Option 1 simply provides the average, then the developer would have to calculate the min and max. Option 2 is the opposite. Either should work. I tend to agree that the average is more important information, and so is perhaps slightly preferred.

[kshala-ford]

@joeljfischer that was my point for choosing option 1. Though both options provide the same information I believe the format of option 1 might be more interesting for most developers and the few others would convert to min/max.

[atiwari9]

@kshala-ford @joeljfischer - Thanks for your inputs, i read @joeljfischer 's comment during my vacation and i too felt like Option 1 is the way to go for same reasons discussed above. However, i'd like to change the name of param from approximateLocation to approximatePosition as that is better understood IMO.

<struct name="WindowState" since="X.X">
  <param name="approximatePosition" type="Integer" minValue="0" maxValue="100">
    <description>The approximate percentage that the window is open - 0 being fully closed, 100 being fully open</description>
  </param>
  <param name="deviation" type="Integer" minValue="0" maxValue="100">
    <description>The percentage deviation of the approximatePosition. e.g. If the approximatePositionis 50 and the deviation is 10, then the window's location is somewhere between 40 and 60.</description>
  </param>
</struct>

[theresalech]

The Steering Committee voted to accept this proposal with the revision to WindowStatus described 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
• SDL HMI
• Core
• iOS
• Java Suite
• Server

[theresalech]

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