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] SDL 0071 - SDL Remote Control Baseline (no zones, no driver/passenger, immediate control) #206

Closed
theresalech opened this issue Jun 28, 2017 · 23 comments

Comments

@theresalech
Copy link
Contributor

theresalech commented Jun 28, 2017

Hello SDL community,

The review of the revised proposal "SDL 0073 - SDL Remote Control Baseline (no zones, no driver/passenger, immediate control)" begins now and runs through July 18, 2017. The original review of "SDL Remote Control Baseline (no zones, no driver/passenger, immediate control)" occurred June 27 through July 11, 2017. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0071-remote-control-baseline.md

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

#206

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

The driver must exit current controlling application before using another application to control the same RC module. There is no indication of which application is currently control the RC module. Driver doesn't know which application to close. This makes application switching cumbersome.

It can only subscribe to all radio or climate control data, cannot subscribe to individual item change notifications.

These seem like quite serious limitations to me. I want to make sure that the current implementation will not prevent us from making updates that address these limitations.

@Jack-Byrne
Copy link
Contributor

Jack-Byrne commented Jun 30, 2017

Proposal says to remove:

app authorization (policy control, which app can access which remote control module(s) or which control items)

Having policy control seems like it should be a fundamental requirement for RPCs in SDL Core. Also allowing policy control via user consent might help alleviate the issues Joel referenced in his comment above.

@yang1070
Copy link
Contributor

yang1070 commented Jul 3, 2017

@joeljfischer Ford has a separate proposal that change the baseline's (current implementation's) SDL core behavior, which basically add a user/policy setting, that allows a second app control the same module. Depending on the setting (automatic allow, automatic deny, ask driver) the core can transfer the access right from the first app to the second app with or without asking the driver.

@JackLivio There are still policy control needed in this proposal, it is listed as "Basic app authorization support, the policy that control which app can access which type(s) of remote control module". Compared to the above mentioned full app authorization, the difference is in the basic version, an app can access all climate control modules or all radio control modules, in the full version an app can access exactly which climate control module and which control items in the module. I would like a full app policy control if it is possible in 4.4. But considering the full control relies on a good zone or location scheme to identify a module, which is not covered in the baseline, I left full policy control out.

@brandon-salahat-tm
Copy link
Contributor

Keep concept of driver vs passenger device, but treat all devices as driver's device, and only allow one device

Does this mean that this proposal still includes the driver/passenger system, but simply hard-codes all devices as a driver phone?

The driver must exit current controlling application before using another application to control the same RC module. There is no indication of which application is currently control the RC module. Driver doesn't know which application to close. This makes application switching cumbersome.

I agree with @joeljfischer that this seems like a major limitation for the baseline implementation. Does this mean that a user must completely close (ie exit) an application, or simple switch to another SDL RC app?

@joeygrover
Copy link
Member

Module Identification

Each RC module shall have a short name (or a label) in order to uniquely identify the module in case there are multiple modules of the same type. SDL does not standardize the name for the modules. The name may or may not related to the location of the module within the vehicle. It is the OEM's choice to name each individual module.

This will be a developer's nightmare if it is forced into a way to parse modules. If we accept this, we should be very strict on what it is used for and what OEM's/developers are not allowed to do. This inclusion runs a very high risk of breaking integrations if we don't. ie, if OEM X calls a module "Passenger Climate Control" while OEM Y calls it "Climate - P" it will be nearly impossible for a developer to understand how to parse this string for any real usefulness.

RC capabilities

  <struct name="RemoteControlCapabilities">
    <param name="climateControlCapabilities" type="ClimateControlCapabilities" mandatory="false" minsize="1" maxsize="100" array="true">
      <description>If included, the platform supports RC climate controls.</description >
    </param>

    <param name="radioControlCapabilities" type="RadioControlCapabilities" mandatory="false" minsize="1" maxsize="100" array="true">
      <description>If included, the platform supports RC radio controls.</description >
    </param>
  </struct>

I assume this will then just return all modules of that type in this call. This would imply that each module may have a different subset of capabilities supported. If one module supported setting 'x' and 'y' but another only supported 'x' and 'z', how would a developer pick which module to control? Would they just try to control both? Would one succeed while the other failed or would both fail?

Also should we include what buttons are supported from the 'buttonNames' enum that relate to RC? I would suggest just an array of ButtonName enums where their inclusion would imply their availability.

Redundancy in Getting Capabilities

  <function name="GetInteriorVehicleDataCapabilities" functionID="GetInteriorVehicleDataCapabilitiesID" messagetype="request">
    <description>Called to retrieve the available zones and supported control types</description>
    <param name="moduleTypes" type="ModuleType" array="true" mandatory="false" minsize="1" maxsize="1000">
        <description>If included, only the corresponding type of modules a will be sent back. If not included, all module types will be returned.</description>
    </param>
  </function>

  <function name="GetInteriorVehicleDataCapabilities" functionID="GetInteriorVehicleDataCapabilitiesID" messagetype="response">
    <param name="interiorVehicleDataCapabilities" type="RemoteControlCapabilities">
    </param>

This has changed from the previous proposal. Since there are no zones this extra RPC is actually not needed and we should utilize the previously accepted proposal #166. The reason the GetInteriorVehicleDataCapabilities RPC was introduced was to return the available modules that could be accessed within the supplied zone. Since that is no longer included, is there a reason to introduce this new RPC that duplicates a construct we've already accepted? I would move to include this in the GetSystemCapability and related RPCs/Enums.

Redundancy in Temperature Struct

  <struct name="Temperature">
    <param name="unit" type="TemperatureUnit">
      <description>Temperature Unit</description>
    </param>
    <param name="valueC" type="Float" minvalue="14.0" maxvalue="30.0" mandatory=”false”>
      <description>Temperature Value in Celsius, the value range is for setter only</description>
    </param>
    <param name="valueF" type="Float" minvalue="60.0" maxvalue="90.0" mandatory=”false”>
      <description>Temperature Value in Fahrenheit, the value range is for setter only</description>
    </param>
  </struct>

Why do we need both valueC and valueF? The unit should be the descriptor of a value that the module can use to do conversion if necessary. The min's and max's should also be rethought. If we were to ever reuse this struct it should be general enough to do so and not be limited to its initial integration purpose.

AppHMIType Limitations

<enum name="AppHMIType">
    <description>Enumeration listing possible app types.</description>
    ...
    <!-- new additions -->
    <element name="REMOTE_CONTROL" />
  </enum>

Why is there a new AppHMIType for remote control? This seems extremely limiting and hard to implement. For the example of an app wanting to control the radio tuner, they would be forced to register as REMOTE_CONTROL and not MEDIA however (to my knowledge) the system would expect MEDIA so it could be considered an audio source. In general, limiting an app's HMI type to REMOTE_CONTROL seems very wrong.

HMI_API

The HMI_API changes, while similar, should be defined as well since so much of this initial proposal is changing. eg InteriorZones still exists in the HMI_API

Conclusion

Overall I think this baseline set of features could work. However, I believe it is introducing a lot of challenges already solved in the previously submitted remote control proposal as well as new ones. Assuming we eventually agree on some way to use zones/addressable modules how do we move forward with systems that don't include that data? I stand by my assessment that this feature is incomplete without the inclusion of a way to address specific modules and control over those modules in relation to devices. I would rather see InteriorZones included in this feature with OEM's that do not wish to use the feature using only a single zone, than to not include it and move forward with a subpar feature.

@theresalech
Copy link
Contributor Author

theresalech commented Jul 6, 2017

The Steering Committee has voted to keep this proposal in review until next week to allow for additional discussion on the issue, specifically addressing details of what will be included in this implementation and concerns with module identification and RC capabilities. The proposal will be voted on during the next meeting on July 11, 2017 (2017-07-11).

@yang1070
Copy link
Contributor

@Toyota-BSalahat

The driver must exit current controlling application before using another application to control the same RC module. There is no indication of which application is currently control the RC module. Driver doesn't know which application to close. This makes application switching cumbersome.

I agree with @joeljfischer that this seems like a major limitation for the baseline implementation. Does this mean that a user must completely close (ie exit) an application, or simple switch to another SDL RC app?

This is the current implementation in RC branch, a user must completely close (ie exit) an application to release the remote control access right. We can definitely update it to satisfy the needs.

  • Let's consider the current resource management implementation (not specific to RC), basically an implicit rule says at a time only one application can have access to a resource, this applies to both the main HMI resource (except alert pop up and transit messages) and the audio streaming resource.
  • For main HMI resource control, considering regular non-media or media applications, the apps need to run in foreground (HMI level full) to have a "control" of the main HMI resource. If an app goes to background (HMI level BACKGROUND), it loses the control of main HMI.
  • For audio steaming control, a media app can streaming audio in foreground (FULL). When other non-media goes to foreground (FULL), it goes to LIMITED, and can still streaming audio. When another media app goes to foreground(FULL), it goes to BACKGROUND and loses the access to audio streaming.

We can have a similar resource control rule for RC apps for app switching,

  • The simplest rule can be "only the foreground running app can perform remote control". This is just like how non-media apps control the main HMI. (In this case there is no need to differentiate a RC and a non-RC app) But what if the foreground app does not need RC at all, shall the 1st app lose the control right?

  • A better rule similar to audio streaming control rule can be "A foreground running RC app can perform remote control (obtain RC access right) until driver exit the app or launch another RC app that controls the same type of RC resources."
    However, this conflicts the need that a background app should also be able to perform remote control.

  • A even better solution allows the policy or the driver to control how app switching RC access right. I will update the proposal to include this solution. Basically, we add a RC setting, which has three options : automatic allow (RC access right transfer or switching), automatic deny, ask driver. Consider the case in which there is a running RC app that has the right to control a RC module types, another RC app wants to control the same RC module, depending on the setting, the system will do differently.

    1. If the setting is "automatic allow", the system revoke the access right of the 1st app, and allows the 2nd app to do the remote control. This is exactly how the system deal with media apps for audio streaming.
    1. If the setting is "automatic deny", the system keep the access right of the 1st app, and denies the 2nd app's RC request. The allows a background RC app keep controlling a RC module.
    1. If the setting is "ask driver", the system shows a pop up and asks the driver's decision.

@yang1070
Copy link
Contributor

@Toyota-BSalahat

Keep concept of driver vs passenger device, but treat all devices as driver's device, and only allow one device.

Does this mean that this proposal still includes the driver/passenger system, but simply hard-codes all devices as a driver phone?

Yes. Instead of start from scratch, this allows minimum change to the existing implementation.

@yang1070
Copy link
Contributor

@joeygrover

Module Identification

I will remove this string from the proposal. The result is how to ID a resource (resource level) if there are multiple resource of the same type is undefined. This leaves the room for later zone related proposals to address the issue (how to identify of a RC module).

RC capabilities

In the proposal both climateControlCapabilities and radioControlCapabilities has a name field, so the system shall says 'x' and 'y' belongs to module 'A', and 'x' and 'z' belongs to module 'B', even 'A' and 'B' are the same type, so the developer will know what are available to control, what are not available.

Redundancy in Getting Capabilities

Agree, I will update the proposal to remove the GetInteriorVehicleDataCapabilities API and re-use GetSystemCapability API and related RPCs/Enums.

Redundancy in Temperature Struct

Agree, I will update the proposal to keep only one copy of 'value' and remove the range for general usage.

AppHMIType Limitations

This is not a Limitation. AppHMIType is not exclusive. In the above example, the app shall register as both a REMOTE_CONTROL and a MEDIA app. This is already implemented. I don't know the exact reason to introduce this new type, but my guess it is used in several places (1) differentiate the app early so that the special logic of RC registration can be applied, (2) only RC app can use RC related RPCs, (3) when RC is disabled/enabled, the system need to know which apps need to be placed to NONE or unregistered notifications, and more.

I have no objection to remove it, considering the fact we have policy control of which app can control which RC module, but there maybe some side effect.

HMI_API

I will update the proposal to include it.

@yang1070
Copy link
Contributor

Also should we include what buttons are supported from the 'buttonNames' enum that relate to RC? I would suggest just an array of ButtonName enums where their inclusion would imply their availability.

agree.

@brandon-salahat-tm
Copy link
Contributor

Yes. Instead of start from scratch, this allows minimum change to the existing implementation.

@yang1070 from an engineering time perspective I understand the desire to do this, however I believe the goal of this proposal was to decouple the remote control functionality from the other features. Leaving the feature in, but disabled leaves the additional features tightly coupled to the remote control proposal. I think there is risk associated with approving disabled/unnecessary functionalities with this proposal, as @Toyota-Sbetts mentioned during the last meeting.

A even better solution allows the policy or the driver to control how app switching RC access right. I will update the proposal to include this solution. Basically, we add a RC setting, which has three options : automatic allow (RC access right transfer or switching), automatic deny, ask driver. Consider the case in which there is a running RC app that has the right to control a RC module types, another RC app wants to control the same RC module, depending on the setting, the system will do differently.

I think this idea is in the right direction, but I think for the sake of the core RC feature we maybe should go with "Always Allow". In the future proposal for Driver/Passenger mode, I think we could flesh out this behavior a lot more. My initial thoughts on this, is that option 3 would need to be able to be customized on the HMI (ie, some OEMs may not be comfortable doing a popup for driver distraction reasons). I also think it may be beneficial for an app to be able to specify whether or not it wants to lock an RC resource in the first place.
I think something like "Always Allow" would serve the needs of this core functionality proposal, however.

@yang1070
Copy link
Contributor

however I believe the goal of this proposal was to decouple the remote control functionality from the other features. Leaving the feature in, but disabled leaves the additional features tightly coupled to the remote control proposal.

Agree. I will update the proposal to complete remove driver/passenger concept.

I think SDL can provide APIs for vehicle HMI to change RC settings. OEM (not SDL) can choose to lock the setting as "always allow", thus does not need a HMI pop up

@takaharuueno
Copy link

takaharuueno commented Jul 11, 2017

I have some elementary question and comment on this proposal.

  1. Can we add/remove/edit the element name in the <enum name="ButtonName"> ?
    For example, OEM A does not have an Eject button. OEM B has more than 7 buttons in their system (In proposal 6 buttons are defined in the Button Name).

  2. Some of Value Range like a Current Cabin Temperature, it depends on OEMs.

@joeygrover
Copy link
Member

@yang1070

RC capabilities
In the proposal both climateControlCapabilities and radioControlCapabilities has a name field, so the system shall says 'x' and 'y' belongs to module 'A', and 'x' and 'z' belongs to module 'B', even 'A' and 'B' are the same type, so the developer will know what are available to control, what are not available.

But how can a developer only target module's that support 'y'? I thought we agreed the "name" field for a module shouldn't be used to parse out modules as it was not an adequate solution. This seems to contradict that.

@takaharuueno

  1. The <enum name="ButtonName"> should include all SDL supported elements; it should not be modified based on what an OEM supports. I refered to adding a new capability struct for supported buttons that would contain an array of these elements that are supported by that specific head unit. Something similar to:
<struct name="ButtonControlCapabilities">
    <param name="buttonsSupported" type="ButtonName" mandatory="false" array="true">
      <description>If included, the platform supports RC button controls with the included button names.</description >
    </param>
  1. I believe @yang1070 agreed to this in a previous comment:

Redundancy in Temperature Struct
Agree, I will update the proposal to keep only one copy of 'value' and remove the range for general usage.

@Toyota-Sbetts
Copy link
Contributor

@yang1070 In addition to @Toyota-BSalahat's notes, there were some additional items mentioned to be kept and disabled:

Keep but disable OnDeviceRankChanged notifications
Keep but disable OnReverseAppsAllowing API

Can you confirm if with the removal of the driver/passenger system if these will also be removed?
Overall we have the same concern for any item (feature, RPC, etc.) that is left in but disabled; it will be tightly coupled and therefore more difficult to change based on the additional proposals.
We hope that the intent of this proposal is just the baseline RC feature and only what is necessary to support it so we can review and discuss other features in their own proposal.

@yang1070
Copy link
Contributor

yang1070 commented Jul 11, 2017

Can you confirm if with the removal of the driver/passenger system if these will also be removed?

@Toyota-Sbetts Yes. It will be removed. I have a PR pending to update the proposal.

@yang1070
Copy link
Contributor

Some of Value Range like a Current Cabin Temperature, it depends on OEMs.

@takaharuueno The value range in the Temperature stuct will be removed.

@yang1070
Copy link
Contributor

@joeygrover Assume there is only one RC module per type, we can remove the moduleName string, and leave if for future proposals.

@theresalech
Copy link
Contributor Author

This proposal has been revised to include the feedback provided on this review issue and also the feedback provided during the Steering Committee meeting on July 11, 2017. It is now in review until July 18, 2017 (2017-07-18).

@theresalech
Copy link
Contributor Author

Not all revisions were included at the time of my last comment. All revisions have now been included and the complete revised proposal is in review.

@theresalech theresalech changed the title [In Review] SDL 0071 - SDL Remote Control Baseline (no zones, no driver/passenger, immediate control) [Accepted] SDL 0071 - SDL Remote Control Baseline (no zones, no driver/passenger, immediate control) Jul 19, 2017
@theresalech
Copy link
Contributor Author

Now that the requested revisions have been incorporated, the Steering Committee has agreed to accept this proposal.

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Jul 19, 2017
@theresalech
Copy link
Contributor Author

theresalech commented Jul 19, 2017

Issues Entered:
Core
iOS
Android
RPC

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

8 participants