[Accepted with Revisions] SDL 0221 - Remote Control - Allow Multiple Modules per Module Type

[theresalech]

Hello SDL community,

The review of the revised proposal "SDL 0221 - Remote Control - Allow Multiple Modules per Module Type" begins now and runs through May 14, 2019. The original review took place April 10 - April 16, 2019. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0221-multiple-modules.md

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

#700

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. The moduleId also needs to affect the OnSystemCapabilityUpdated notification.
2. It strikes me as slightly odd to have parameters like share and location in the ModuleId struct when they have nothing to do with identifying the module. I would recommend renaming ModuleId to ModuleInfo, that would seem to describe it better.
3. It's mentioned in passing, but is HMI really an "exclusive" module type? It would seem to be shared to me.
4. Can shared not be inferred by the serviceArea? If it affects multiple seats, would it not be shared?
5. moduleId in ModuleData should have a max string length like id in ModuleId. These are meant to match, right?
6. The SeatLocation id parameter is pretty odd. Can you elaborate on why you did it the way you did instead of doing row, column, and level integer parameters?
7. Can the SeatLocationCapabilities be added to RemoteControlCapabilities instead of being a separate capability type to pull? They won't change after creation and anyone who's doing remote control will almost certainly want that data, right?

[Jack-Byrne]

8. Because Grid has defined a rowspan, colspan, and levelspan, it seems redundant to have a location param and a serviceArea param. Using the Grid struct, doesn't col/row define the "location" of the module? And colspan/rowspan defines the "service area"?

9. When an app receives the remote control capabilities, it shall remember the capabilities and it might show a map to the user indicating where a module is and a module's coverage so that the user can choose a module to control.

On the mobile device or on the head unit?

10. What does the moduleID.id look like? Simple counter? "Simple counter + module type"? UniqueHash?

11. In addition, this proposal deprecates the SupportedSeat Enumeration and parameter id in SeatControlData in order to get a uniformed solution.

We will still have to support the deprecated enums in core in the event an app is using ~5.1 or earlier. I propose we follow similar logic to the translation behavior of OK->PLAY_PAUSE.

In the SupportedSeat case, Core should translate DRIVER/FRONT_PASSENGER seats from mobile to the corresponding Grid location for the HMI.

12. Maybe related to Joel's SendLocation Updates #6? But why is SeatLocation needed when we have ModuleID to describe the area that a RC module is controlled from.

If SeatLocation is a must: I think ModuleID could be simplified to use an array of seatIDs for location/serviceArea instead of having those parameters also use the grid.

13. If the optional moduleId is not provided in a SetInteriorVehicleData request, SDL core shall forward the request as is (without moduleId) to HMI. HMI shall use the default id for this module type when processing the request.

Should Core populate this field for the HMI instead? Seems like an easy step if core can keep track of the default modules. This means less work for the HMI.

+    <param name="userLocation" type="Common.Grid" mandatory="false">
+      <description>Location of the user's seat. <description>
+   </param>

Should this use the "SeatLocation" struct instead of Grid?

15. SDL shall enforce permission control. It may or may not use the local policy table depending on the implementation. The permissions are given by the user/driver via HMI, and information is passed to sdl core.

Prompting a user on the permissions control should happen in one single batch or single prompt screen (especially if there is only one display). I don't think it would be a good experience to have the driver clicking "Accept" 10 times in a row to allow all of the modules at all of the module locations.

GetInteriorVehicleDataConsent should maybe be updated to have an array of moduleIDs to avoid this.

16. Granted permission is valid within ignition cycle. Reset after ignition, ...

I don't agree with this unless there is a better defined "permission granting" user experience. Even then, requiring user action to grant permissions every time would get quite annoying. I think permissions should be restored based on device MAC Address at least.

[yang1070]

@joeljfischer

1. The moduleId also needs to affect the OnSystemCapabilityUpdated notification.

agree, OnSystemCapabilityUpdated shall include remote control capabilities, which include XyzControlCapabilites.

2. It strikes me as slightly odd to have parameters like share and location in the ModuleId struct when they have nothing to do with identifying the module. I would recommend renaming ModuleId to ModuleInfo, that would seem to describe it better.

agree, originally we named it as ModuleDescription

3. It's mentioned in passing, but is HMI really an "exclusive" module type? It would seem to be shared to me.

shared is a property of the module, the value can be true or false. SDL does not define it. The description "exclusive" is just an example in the sense that HMI is driver's resource. Of course, if it is allowed to be used by multiple users, it is a shared.

4. Can shared not be inferred by the serviceArea? If it affects multiple seats, would it not be shared?

I'm thinking the use case that there is only one module, which covers the whole area of the vesicle, but I want it is controlled by only one user. Having two separate properties will have more flexibility to achieve the goal.

5. moduleId in ModuleData should have a max string length like id in ModuleId. These are meant to match, right?

agree

6. The SeatLocation id parameter is pretty odd. Can you elaborate on why you did it the way you did instead of doing row, column, and level integer parameters?

This id might need to be presented to the end user. To the end user, a short name "1A", "2B" is easier to use than "row=1, column=0, level=0" or,"row=2, column=2, level=0" . And it is similar to the seat number in an airplane or a train. Certainly, the id can be any meaningful string, but in this way the seat id can be inferred by row, column, and level

7. Can the SeatLocationCapabilities be added to RemoteControlCapabilities instead of being a separate capability type to pull? They won't change after creation and anyone who's doing remote control will almost certainly want that data, right?

Correct. It is possible and acceptable.
SeatLocationCapabilities is static. RemoteControlCapabilities might be semi-static. I prefer to separate them. It can be used by other features besides remote control. For example, to indicated the location of a display monitor, and to calculate the seat distance between a use and the display for reachability.

[joeygrover]

| 2. Yes the original naming will be very confusing because the variable name of moduleId is used with a type of String in one place, while in others it is the ModuleId struct. I'm fine with either alternatives suggested.
| 4. I think the variable name is the confusing issue. The boolean value is whether or not two sources can control the module simultaneously. Service area is simply where a user can be located and still control the module. I would suggest renaming this param to something like allowsMultipleAccess, simultaniousControlEnabled or something similar that better describes its purpose.
| 6. I think we need to make a hard definition for this if that is really how it is going to be used. If the id param is mandatory, then OEMs and app developers need to know what acceptable values are and what they can expect.

| 11. Deprecating SupportedSeat is a big deal. A conversion will have to be provided between the enum and grids. OEMs and developers need to know exactly what should be used and can be expected.
|16. Absolutely agree here. Any pop up that happens every time a user gets in their vehicle is a complete annoyance and will give SDL a bad reputation. There should be better controls here; possibly add a Always take this action sort of consent.

| 17. The id string in the ModuleId struct is a bit strange. Is it intended that this id could be parsed to find the module? If so the exact structure needs to be adhered. Could it not simply be a UUID?

| 18. In the ModuleId struct, if a serviceArea is not included, it should be assumed only the overlap of its location to seat locations will be covered.

| 19. I understand the author does not want to get into defining policies for this feature but I feel as they are a heavy part of RemoteControl and should be defined in a way that can be implemented into the Core and policy server repos for all members.

| 20.

HMI is responsible for choosing a default moduleId from its supported list per module type if it is not included in the mobile request.

The app developer needs some guarantee on what module will be controlled. Leaving it up to the HMI is yet another thing the app developer has to plan for between OEMs and we should not be putting that burden on them if we can avoid it.

| 21. On the idea of ReleaseInteriorVehicleDataModule, how long does an app get to maintain control of a module even if it isn't actually using it? Could an app simply grab access on connect and never give it up? Is there a way for an app to request the module be released?

[yang1070]

8. Because Grid has defined a rowspan, colspan, and levelspan, it seems redundant to have a location param and a serviceArea param. Using the Grid struct, doesn't col/row define the "location" of the module? And colspan/rowspan defines the "service area"?

The span is greater than zero. If the location is always the starting corner of the serviceArea, then col/row of a serviceArea is also the location. However, that may not be the case. It may be in the middle or some where else in the area. The location is a nice to have parameter to indicate the exact physical place of a module. Without it, the proposal can still work.

9. When an app receives the remote control capabilities, it shall remember the capabilities and it might show a map to the user indicating where a module is and a module's coverage so that the user can choose a module to control.

On the mobile device or on the head unit?

It is on the mobile device when user uses the app.

10. What does the moduleID.id look like? Simple counter? "Simple counter + module type"? UniqueHash?

This id string is used internally by software only. For each module type, the id must be unique. But it is preferred to be unique across all module types. I don't have a preference of how it is defined. SDL can define it or leave it to OEM to define it.

11. In addition, this proposal deprecates the SupportedSeat Enumeration and parameter id in SeatControlData in order to get a uniformed solution.

We will still have to support the deprecated enums in core in the event an app is using ~5.1 or earlier. I propose we follow similar logic to the translation behavior of OK->PLAY_PAUSE.

In the SupportedSeat case, Core should translate DRIVER/FRONT_PASSENGER seats from mobile to the corresponding Grid location for the HMI.

Agree, for transition time in 5.x, SDL can still support deprecates. But it is hard for sdl to do a string mapping or a grid mapping, unless the id string defined in capabilities is the same as those in enums.

12. Maybe related to Joel's SendLocation Updates #6? But why is SeatLocation needed when we have ModuleID to describe the area that a RC module is controlled from.

If SeatLocation is a must: I think ModuleID could be simplified to use an array of seatIDs for location/serviceArea instead of having those parameters also use the grid.

SeatLocation is needed for permission control. It is OK for serviceArea to say it covers a list of seats. I think it is two ways to define a parameter. I don't agree one way is simpler than the other. Suppose the mobile app wants to show the module coverage, it needs to convert the list of seat back to some grid for visual representation.

13. If the optional moduleId is not provided in a SetInteriorVehicleData request, SDL core shall forward the request as is (without moduleId) to HMI. HMI shall use the default id for this module type when processing the request.

Should Core populate this field for the HMI instead? Seems like an easy step if core can keep track of the default modules. This means less work for the HMI.

The problem is SDL may not know which one is the default if there are multiples. If SDL knows, it can do it easily.

14

+    <param name="userLocation" type="Common.Grid" mandatory="false">
+      <description>Location of the user's seat. <description>
+   </param>

Should this use the "SeatLocation" struct instead of Grid?

Good catch. yes, it shall be SeatLocation.

15. SDL shall enforce permission control. It may or may not use the local policy table depending on the implementation. The permissions are given by the user/driver via HMI, and information is passed to sdl core.

Prompting a user on the permissions control should happen in one single batch or single prompt screen (especially if there is only one display). I don't think it would be a good experience to have the driver clicking "Accept" 10 times in a row to allow all of the modules at all of the module locations.

GetInteriorVehicleDataConsent should maybe be updated to have an array of moduleIDs to avoid this.

That's a good suggestion. I can update the proposal to make it an array.

16. Granted permission is valid within ignition cycle. Reset after ignition, ...

I don't agree with this unless there is a better defined "permission granting" user experience. Even then, requiring user action to grant permissions every time would get quite annoying. I think permissions should be restored based on device MAC Address at least.

It is annoying to ask driver every time.
There shall be some kind of cache, a user (device ID or MAC address) + location (a seat) + module id + yes/no result of user granted permission (not auto granted permission) can be cashed for certain time. But we need a way to clear the cache when user need it.

[yang1070]

| 2. Yes the original naming will be very confusing because the variable name of moduleId is used with a type of String in one place, while in others it is the ModuleId struct. I'm fine with either alternatives suggested.

ModuleInfo sounds good to me. I will update struct ModuleId to ModuleInfo

| 4. I think the variable name is the confusing issue. The boolean value is whether or not two sources can control the module simultaneously. Service area is simply where a user can be located and still control the module. I would suggest renaming this param to something like allowsMultipleAccess, simultaniousControlEnabled or something similar that better describes its purpose.

allowsMultipleAccess sounds good to me. So shared will be renamed.

| 6. I think we need to make a hard definition for this if that is really how it is going to be used. If the id param is mandatory, then OEMs and app developers need to know what acceptable values are and what they can expect.

The id string is coming from the capabilites. capabilites shall include both id and grid. Mobile app can use any string as id of the seat.
If an id is not published in capabilites, it is not a valid id.

| 11. Deprecating SupportedSeat is a big deal. A conversion will have to be provided between the enum and grids. OEMs and developers need to know exactly what should be used and can be expected.

It is not easy for SDL to do the conversion. Some vehicles have driver seat on the left, others have drive seat on the right. And we define the grid start from front-left.

|16. Absolutely agree here. Any pop up that happens every time a user gets in their vehicle is a complete annoyance and will give SDL a bad reputation. There should be better controls here; possibly add a Always take this action sort of consent.

As I replyed, there shall be some kind of cache.

| 17. The id string in the ModuleId struct is a bit strange. Is it intended that this id could be parsed to find the module? If so the exact structure needs to be adhered. Could it not simply be a UUID?

Yes, the intention is to used this id string to get the module from table look up (not by parsing the characters), the module data info includes (lcation, coverage, etc. that are defined in new "ModuleInfo", and data in the exsiting "xyzControlData". a UUID will work.

| 18. In the ModuleId struct, if a serviceArea is not included, it should be assumed only the overlap of its location to seat locations will be covered.

That's a good suggestion.
I would say even it is an optional parameter, when intergating with HMI, HMI shall always include it.

| 19. I understand the author does not want to get into defining policies for this feature but I feel as they are a heavy part of RemoteControl and should be defined in a way that can be implemented into the Core and policy server repos for all members.

The section of "How to use the new parameter for permission control" is a kind of policy need to be done by Core. I don't know what kind of policy it needs beside it.

| 20.

HMI is responsible for choosing a default moduleId from its supported list per module type if it is not included in the mobile request.

The app developer needs some guarantee on what module will be controlled. Leaving it up to the HMI is yet another thing the app developer has to plan for between OEMs and we should not be putting that burden on them if we can avoid it.

The new mobile apps shall always include the "id" on platforms that support multiple modules. The default shall only be used when the id is missing, which is for old apps compatibilty.

| 21. On the idea of ReleaseInteriorVehicleDataModule, how long does an app get to maintain control of a module even if it isn't actually using it? Could an app simply grab access on connect and never give it up? Is there a way for an app to request the module be released?

It is up to the app to send a ReleaseInteriorVehicleDataModule. It is possible that an app never gives it up while keep running. if remote control setting is "auto allow" or "ask driver", even the app does not release, SDL can still release the module and give it to a new app/user.

[theresalech]

The Steering Committee voted to return this proposal for revisions, so that the author can make updates based on the agreed upon comments left on the review issue (1, 2, 4, 5, 11, 12, 14, 15, 16, 18), as well as use grid location for seatLocation id and allow app developers to translate to human readable names with regards to comment 6. There was also discussion in the meeting about how app developers store data and if human readable names might be usable for voice assistants. This can be discussed further in future meetings and possibly warrant a new separate proposal for the enhancement.

[theresalech]

The author has revised this proposal to include the Steering Committee's requested revisions, as listed in this comment. The revised proposal is now in review until 2019-05-14.

[Jack-Byrne]

If the RC setting is auto deny (always no), then only the first user can control the module. Whoever requests a free shared module can lock the module and use it until the user frees the module.

I wanted to clarify that this is referring to OnRemoteControlSettings rpc and the accessMode parameter.

Also by adding GetInteriorVehicleDataConsent to the mobile api does that mean that an app cannot gain control of a module without sending this RPC first? If yes then I think this would be a breaking change to how RC apps get access to control an RC module.

[yang1070]

@JackLivio
Yes, the auto allow/auto deny/ask driver is the value of accessMode parameter in OnRemoteControlSettings RPC.

An app is still able/possible to control a module by sending a ButtonPress request or SetInteriorVehicleData request without sending a GetInteriorVehicleDataConsent first. There is no breaking change regarding this.

[yang1070]

Some comments from email, posted here

If a serviceArea is not included, it is assumed the serviceArea is the same as location. If both are not included, it is assumed that they cover the whole area of the vehicle.

Q: What value of location is assumed if serviceArea exists, but location does not?
A: If location does not exist, it does not have a default value. It shall be update to "If a serviceArea is not included, it is assumed the serviceArea is the same as location. If both are not included, it is assumed the serviceArea covers the whole area of the vehicle."

Q: Does allowMultipleAccess relate to module or module type?
A: allowMultipleAllow is defined per module, not per module type.

If a mobile app uses the SupportedSeat and no moduleId in a RPC request, SDL shall forward the request as is to HMI, HMI shall automatically convert seat id=DRIVER to the moduleId that corresponds to the driver’s seat module, and seat id=FRONT_PASSENGER to the moduleId that corresponds to the front passenger's seat.

Q: What behavior is expected from SDL and HMI in the following cases

• mobile app sends both parameters id and moduleId, both parameters are correct
• mobile app sends both parameters id and moduleId, id has correct value, moduleId has incorrect value
• mobile app sends both parameters id and moduleId, id has incorrect value, moduleId has correct value

A: If both moduleId and id are included, moduleId has a higher priority than id. Because we depreciate id, and we want new app to use new the parameter moduleId. Old app will have an id but not a moduleId.
SDL process moduleId first, if it is invalid, the request is invalid, if it is valid, use it and ignore the id parameter. If moduleId is not included, SDL process id. If id is not valid, the request is not valid. Otherwise, SDL pass it to HMI. HMI convert the id to correct moduleId.

This proposal proposes to publish the row number, column number and level number in a new SeatLocationCapability, which also includes all the seats installed and their locations.

Q: Should row number, column number and level number defined in SeatLocationCapability be used to validate parameters location and serviceArea from ModuleInfo which used in all RC capabilities?
The same question regarding grid parameter in SeatLocation?
A: when HMI publish Capabilities, it shall make sure that the parameters in grid are in correct ranges regarding col/row/level . It is a good suggestions that SDL validate the parameters, but I think it might be optimal.

[yang1070]

more questions from Luxoft email:
Q: How does SDL recognize value of parameter userLocation in case app doesn’t send GetInteriorVehicleDataConsent, but sends SetInteriorVehicleData or ButtonPress in allowMode = ASK_DRIVER?
Q: How can SDL get user location in case an app doesn’t send GetInteriorVehicleDataConsent? Should userLocation be added to SetInteriorVehicleData, ButtonPress requests?
Q; What SDL behavior is expected in case mobile app sends GetInteriorVehicleDataConsent without userLocation parameter?

A;
The userLocation can be missing (not included in the RPC or not send the RPC) as long as it is defined as not mandatory. It better to have a default value. How about the default user location is front passenger seat or the driver seat?

[theresalech]

The Steering Committee voted to accept this proposal with the following revisions:

• The default userLocation should be DRIVER to ensure backwards compatibility
• Add new non-mandatory userLocation parameter to SetGlobalProperties RPC instead of including userLocation as a parameter in each Remote Control request, such as GetInteriorVehicleDataConsent, SetInteriorVehicleData`, etc.

[theresalech]

@yang1070 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 update issues in the respective repositories for implementation. Thanks!

[theresalech]

Proposal updated to reflect revisions, and implementation issues have been entered:
Core
RPC
iOS
Java Suite
sdl_hmi