[Accepted with Revisions] SDL 0255 Revisions - Enhance BodyInformation vehicle data

[theresalech]

Hello SDL community,

The review of "Revise 0255-Enhance BodyInformation vehicle data" begins now and runs through June 2, 2020.

This will be a review of proposed revisions to a previously accepted but not yet implemented proposal, SDL 0255.

The pull request outlining the revisions under review is available here:

#1005

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

#1022

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

[joeljfischer]

1. Updates to DoorStatusType enum:

I have issues with this update due to overlapping enum states. e.g. LOCKED / UNLOCKED vs. CLOSED. I believe that previously CLOSED was intended to be an equivalent to LOCKED if the item was UNLOCKED. Secondly, AJAR vs. OPEN / REMOVED / PRESENT.

If you're going to add all these items, I think you need to write some documentation on when values should be present. e.g. "LOCKED / UNLOCKED should be present instead of CLOSED when the door / window / roof is closed and has a locking mechanism. REMOVED should be used instead of AJAR or OPEN when the entire module can be removed from the vehicle..." This is just a partial suggestion.

Alternatively, you could add additional non-mandatory parameters for things like REMOVED / PRESENT and LOCKED / UNLOCKED.

I don't think that OPEN is necessary at all given AJAR and state.

[atiwari9]

@joeljfischer

First, yes it makes sense to elaborate on conditions between 'LOCKED/UNLOCKED' and 'CLOSED'. May be it was just in my head but yeah it should be documented. Similarly for 'AJAR' vs 'OPEN', this depends on the opening mechanism of the module, we can document this as well along with guidelines on how to use 'REMOVED/PRESENT' the way i did for roof. #ToUpdate

Secondly, we should not add dedicated params for these, that'd just created rigidity for such modules and possibly potential to duplicate the params in some instances.

Thanks for the suggestion, agreed to add above details for HMI guidelines.

[joeljfischer]

1. Having documentation will be very helpful in understanding these different values.
2. I'm still conflicted over this. Having in the same enum three different value sets for basically "open vs. closed" seems wrong to me. I understand the impetus to only have one parameter and to encapsulate different "types" of "open vs. closed" there, but I feel that there must be a better way to do it.

Perhaps it would be better to leave CLOSED / LOCKED / AJAR as is, but to add a new parameter for context? That would contain values like REMOVABLE / CONVERTIBLE / LOCKABLE in an array of those enum values?

[atiwari9]

@joeljfischer

2. Are you suggesting to keep DoorStatusType enum as it is and add a new enum for above mentioned states? If so, that'd need new params as well in respective RoofStatus and DoorStatus enums. That brings back the point of redundancy. While it may seem like DoorStatusType enum has a lot states, but all of these can be logically defined as we agreed in point 1.

[joeljfischer]

2. I disagree that simple documentation will solve this problem, because it's too confusing for HMI and app developers and too easy to get wrong. I think you're misunderstanding my point. I was saying to keep the original 3 DoorStatusType enums and to add a different enum for roof context to the RoofStatus struct. I don't see why anything would affect the DoorStatus struct.

Added:

<enum name="RoofOpenType">
  <element name="REMOVABLE">
     <documentation>The entire roof can be removed.</documentation
   </element>
  <element name="CONVERTIBLE">
    <documentation>The entire roof can be moved.</documentation
  </element>
  <element name="OPENABLE">
    <documentation>A part of the roof can be opened / closed (e.g. moonroof / sunroof).</documentation
  </element>
</enum>

Alternate, instead of the above, use a RoofType that describes the actual type of roof (e.g. soft-top, hard-top, T-top, moonroof, etc.).

Added:

<struct name="RoofStatus" since="X.x">
	<description>
		Describes the status of a parameter of roof/convertible roof/sunroof/moonroof etc.
		If roof is open, state will determine percentage of roof open.
	</description>
	<param name="location" type="Grid" mandatory="true"/>
	<param name="status" type="DoorStatusType" mandatory="true"/>
+       <param name="openType" type="RoofOpenType" mandatory="true"/>
	<param name="state" type="WindowState" mandatory="false"/>
</struct>

Then we would be able to use the DoorStatusType enum as-is without creating values that are the same with different terminology.

Examples:

1. Roof is REMOVABLE, status is AJAR and state is 100%. Roof is removed.
2. Roof is REMOVABLE, status is CLOSED. Roof is present.
3. Roof is CONVERTIBLE, status is AJAR and state shows how far open it is.
4. Roof is CONVERTIBLE, status is CLOSED. Roof is fully closed.
5. Roof is OPENABLE, same as above 3/4, but for moonroof / sunroof.

Does that make sense?

EDIT: Added examples and documentation

[atiwari9]

@joeljfischer

2. I see, but we'd need the same for Doors as well. Those can also be removed. In addition, e.g. for removable roof we'd say the status is AJAR when roof is removed and CLOSED when it is not removed. That is also not super intuitive. But it does provide additional information on roof/door.

So back to same question, since we'd need to add additional param in both DoorStatus and RoofStatus, along with new enum RoofOpenType or RoofType(i like this one better between two but this does not work for doors), does it provide a good trade-off over original approach(with some sophisticated documentation)

[joeljfischer]

I see, I don't think it was clear in the revision proposal that you sought removable behavior in the door as well. That may complicate things. Would merely adding the REMOVED enum value work?

Then it would look like this:

• Convertible roof - location grid would span entire rows and columns and roof status could be CLOSED or AJAR with corresponding state.
• Sunroof/Moonroof - location grid would span just actual location of sunroof/moonroof. status could be CLOSED or AJAR with corresponding state.
• Entire roof - location grid would span entire rows and columns and roof status would be REMOVED or CLOSED/LOCKED. state can be omitted.

The downside is that you don't know if it's removable / convertible / moon-roof if the status is CLOSED. That's what I was trying to work-around with the RoofType enum.

[theresalech]

The Steering Committee voted to accept this proposal with revisions. The author will revise the proposal to specify required updates to the HMI Integration Guidelines to describe each condition, and also update the proposal to remove PRESENT, OPEN, and UNLOCKED (as these can be implied by CLOSED and AJAR) and to use the logic as outlined in this comment.

[theresalech]

@atiwari9 please advise when your PR has been updated to reflect the agreed upon revisions. I'll then merge the PR so the proposal is up to date, and leave comments on the implementation issues to reference these updates. Thanks!

[theresalech]

Author has updated PR with agreed upon revisions, PR has been merged, and comments have been left on implementation issues to note these updates:

• iOS
• Java Suite
• RPC Spec
• Core
• SDL HMI