Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Accepted with Revisions] SDL 0262 - New vehicle data SeatOccupancy #870

Closed
theresalech opened this issue Nov 20, 2019 · 14 comments
Closed

Comments

@theresalech
Copy link
Contributor

Hello SDL community,

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

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0262-New-vehicle-data-SeatOccupancy.md

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

#870

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
Copy link
Contributor

This proposal seems generally good for me. I have a few small note: the SeatOccupancy parameter descriptions don't seem correct. Also optional arrays should have minSize=1 not 0.

@kshala-ford
Copy link
Contributor

I don't think this array should be minSize=1 because an empty array clearly specifies that no seat is occupied or belted. you couldn't differentiate from seats not known.

@atiwari9
Copy link
Contributor

atiwari9 commented Nov 25, 2019

Actually an empty array would mean that there are no seats. (hypothetical or in some universe where self driving cars don't offer seating :) ) , in that case how a result code VEHICLE_DATA_NOT_AVAILABLE different than and empty array? If there are other params in same vehicle data item, then i'd say to keep array min size as 0 to not affect overall result code. So i guess we can keep min size as 0 for scalability.

@kshala-ford
Copy link
Contributor

Thanks @atiwari9 I think I misunderstood the array in the first reading. Thought that only occupied or belted seats are included in the array.

To answer your question: My understanding is that VEHICLE_DATA_NOT_AVAILABLE for seats would mean that the seat status is not known or not implemented while an empty array is a clear statement of no seats available (at least with the given status information).

@atiwari9
Copy link
Contributor

Sure, So both reasons warrant to keep the minSize=0

@joeljfischer
Copy link
Contributor

👍

@joeljfischer
Copy link
Contributor

Regarding the parameter description that still needs updating for seatsOccupied and seatsBelted, what about something like "Array of seats containing their location and whether or not they are currently occupied (/ belted)."

@atiwari9
Copy link
Contributor

atiwari9 commented Dec 3, 2019

@joeljfischer - are you pointing to Struct for SeatOccupancy, is does have some description, did you mean to add the fact that the param is an array?

@joeljfischer
Copy link
Contributor

@atiwari9 The SeatOccupancy.seatsOccupied and belted parameter descriptions just say "true" or "false", but the actual parameters aren't Boolean, they're SeatStatus structs. I just think that the description is confusing.

@atiwari9
Copy link
Contributor

atiwari9 commented Dec 3, 2019

@joeljfischer - Got it, how about something like this then:

<struct name="SeatOccupancy">
	<param name="seatsOccupied" type="Common.SeatStatus" array="true" minsize="0" maxsize="100" mandatory="false">
		<description>Seat(s) status array containing location and whether the seat(s) is(are) occupied.</description>
	</param>
	<param name="seatsBelted" type="Common.SeatStatus" array="true" minsize="0" maxsize="100" mandatory="false">
		<description>Seat(s) status array containing location and whether the seat(s) is(are) belted.</description>
</struct>

@joeljfischer
Copy link
Contributor

joeljfischer commented Dec 3, 2019

@atiwari9 I think it's fine if you just use plural and remove the (s) and is. Singular is implied if there happens to only be one. e.g. Seat status array containing location and whether the seats are belted.

@theresalech theresalech changed the title [In Review] SDL 0262 - New vehicle data SeatOccupancy [Accepted with Revisions] SDL 0262 - New vehicle data SeatOccupancy Dec 4, 2019
@theresalech
Copy link
Contributor Author

The Steering Committee voted to accept this proposal with the following revision: update SeatOccupancy parameter description to "Seat status array containing the location and whether the seats are belted."

@smartdevicelink smartdevicelink locked as resolved and limited conversation to collaborators Dec 4, 2019
@theresalech
Copy link
Contributor Author

Proposal has been updated to reflect accepted revisions and implementation issues have been entered:

@theresalech
Copy link
Contributor Author

JavaScript Suite issue: smartdevicelink/sdl_javascript_suite#336

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants