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 0293 Revisions - Enable OEM exclusive apps support #1108

Closed
jordynmackool opened this issue Dec 16, 2020 · 44 comments

Comments

@jordynmackool
Copy link
Contributor

jordynmackool commented Dec 16, 2020

Hello SDL community,

The review of "Revise SDL-0293 Enable OEM exclusive apps support" began on December 15, 2020, and continues through January 26, 2021.

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

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

#1106

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

#1108

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,
Jordyn Mackool

Program Manager - Livio
jordyn@livio.io

@jordynmackool jordynmackool changed the title SDL 0293 Revisions - Enable OEM exclusive apps support [In Review] SDL 0293 Revisions - Enable OEM exclusive apps support Dec 16, 2020
@vladmu
Copy link
Contributor

vladmu commented Dec 17, 2020

Hello SDL community,

I have a point regarding the proposed changes because I see some data inconsistency in the way proposed here. The systemHardwareVersion parameter looks very similar to the systemSoftwareVersion which was not placed in the VehicleType structure but is a separate parameter in the RegisterAppInterfaceResponse and the GetSystemInfo HMI's function. So my concern is that in the proposed revision the systemHardwareVersion is a part of the VehicleType and is not implemented in the same way as the systemSoftwareVersion, otherwise, we need the last one to be implemented in the same way as the systemHardwareVersion proposed here and put it into the VehicleType structure also to match "send it to the mobile criteria".

@vladmu
Copy link
Contributor

vladmu commented Dec 22, 2020

Hello SDL community,

The comment above is not actual anymore because the latest commit contains described changes.

@jordynmackool
Copy link
Contributor Author

The author has added additional commits to the pull request which changes what was under review. Since changes were made to the PR, we must immediately withdraw this and lock it. We will discuss if we can bring it back into review during the next Steering Committee Meeting.

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Dec 22, 2020
@jordynmackool jordynmackool changed the title [In Review] SDL 0293 Revisions - Enable OEM exclusive apps support [Withdrawn] SDL 0293 Revisions - Enable OEM exclusive apps support Dec 22, 2020
@jordynmackool
Copy link
Contributor Author

jordynmackool commented Jan 13, 2021

Luxoft the authoring company of the PR under review has confirmed that the current state of the revisions PR is ready for review and changes will not be made to the PR during this review period.

The review of "Revise SDL-0293 Enable OEM exclusive apps support" begins now and continues through January 19, 2021.

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

@jordynmackool jordynmackool changed the title [Withdrawn] SDL 0293 Revisions - Enable OEM exclusive apps support [In Review] SDL 0293 Revisions - Enable OEM exclusive apps support Jan 13, 2021
@smartdevicelink smartdevicelink unlocked this conversation Jan 13, 2021
@joeljfischer
Copy link
Contributor

1. These additions don't allow the app libraries to pass the system hardware or software version to the developer because the type passed back is the RPC struct VehicleType. This systemHardwareVersion would need to be added to this struct, and we would need to figure out something to do about systemSoftwareVersion like a deprecate / replace.

@vladmu
Copy link
Contributor

vladmu commented Jan 14, 2021

  1. These additions don't allow the app libraries to pass the system hardware or software version to the developer because the type passed back is the RPC struct VehicleType. This systemHardwareVersion would need to be added to this struct, and we would need to figure out something to do about systemSoftwareVersion like a deprecate / replace.

It looks logical that the software or hardware version is not a part of the vehicle type but the vehicle system information. I rather propose to create a SystemInfo struct for that purpose, join both versions into it and pass back 2 RPC structs. If we say about the proposal implementation, the packet has the information separately for each field in tags, it is not the RPC struct, but the RAI response already has the VehicleType struct without the systemSoftwareVersion, thus it looks normal to have systemHardwareVersion near that. No any problems to pass 2 additional parameters into onVehicleTypeReceived event instead of the one VehicleType. As an example, you could check this draft PR of the JS suite how it looks like. There the onVehicleTypeReceived is defined as:

onVehicleTypeReceived (vehicleType, systemSoftwareVersion, systemHardwareVersion) {}

@Sohei-Suzuki-Nexty
Copy link

2.We would like to ask a question for clarification. Is it correct that systemHardwareVersion is the version of the HU?
We would like you to clarify what kind of value is placed in what format.

@ashwink11
Copy link
Contributor

Hello @Sohei-Suzuki-Nexty
2. yes, systemHardwareVersion is the version of the HU. This value will be communicated with mobile apps using a string format.

@Sohei-Suzuki-Nexty
Copy link

@ashwink11 -san
2.Thank you for your reply.
We would like to clarify a little more, but what version of HU do you specifically have in mind?
For example, is it a prototype, mass-produced version, or is it something like a variation of a HU for some vehicle?
If not, can you tell me what value you are expecting?

@ashwink11
Copy link
Contributor

Hello @Sohei-Suzuki-Nexty
2. SystemHarwareVersion is similar to an already available parameter called systemSoftwareVersion. Both are of type String. It is up to OEM how they version their software and hardware systems.

@vladmu
Copy link
Contributor

vladmu commented Jan 21, 2021

As long as this revision was not accepted yet, we are OK with preparing of our deliverables without systemHardwareVersion parameter in rai response.
When this revision will be clarified and accepted by SDLC, the updated implementation will be provided.

It is more critical for the mobile part to clarify both systemHardwareVersion and systemSoftwareVersion parameters placement. The proposal describes that both parameters must exist in the payload of StartServiceACK. Still, it doesn't describe how those parameters should be passed back to the app developer, and the VehicleType is defined as an only parameter, at the same time both parameters are not a part of this struct. So according to the proposal, we can't omit both parameters in the implementation, and we will be blocked until the revisions made. It seems this also blocks this proposal to be a part of the upcoming release. An acceptable workaround in mobile view is just to remove systemHardwareVersion and systemSoftwareVersion parameters from the current proposal, it will unblock all parts, then another proposal could be created for those parameters.

@ashwink11
Copy link
Contributor

Hi @vladmu ,
My apologies if we missed something in the original SDL-0293 proposal, However, I believe that proposal has a lot of changes based on Vehicle Type info, and it's not limited to systemHardwareVersion and systemSoftwareVersion. The majority of the proposal defines the requirement of supported vehicle list, logic to filter apps, conveying vehicle type info to apps domain, and also we defined behavior of the apps for old SDL systems. These items are not affected by the system Hardware version and should still function in absence of this info. I believe we should implement the original proposal and deliver those for the upcoming release. We can keep systemHardwareVersion and systemSoftwareVersion as is in StartServiceACK and get back to it for mobile implementation when these revisions are accepted. If you think, systemHardwareVersion block any of the other items defined in the original proposal, please do let me know, we will discuss those issues.

Regarding systemHardwareVersion in the RAI response, I think we can remove it.

@Sohei-Suzuki-Nexty
Copy link

@ashwink11 -san
2.Thank you for your reply.
The parameters of systemHarwareVersion and systemSoftwareVersion are for judging whether the OEM exclusive apps is the target of operation or not,and are not used for other purposes, so does it mean that the OEM can set them on their own?

@vladmu
Copy link
Contributor

vladmu commented Jan 21, 2021

Hi @vladmu ,
My apologies if we missed something in the original SDL-0293 proposal, However, I believe that proposal has a lot of changes based on Vehicle Type info, and it's not limited to systemHardwareVersion and systemSoftwareVersion. The majority of the proposal defines the requirement of supported vehicle list, logic to filter apps, conveying vehicle type info to apps domain, and also we defined behavior of the apps for old SDL systems. These items are not affected by the system Hardware version and should still function in absence of this info. I believe we should implement the original proposal and deliver those for the upcoming release. We can keep systemHardwareVersion and systemSoftwareVersion as is in StartServiceACK and get back to it for mobile implementation when these revisions are accepted. If you think, systemHardwareVersion block any of the other items defined in the original proposal, please do let me know, we will discuss those issues.

Regarding systemHardwareVersion in the RAI response, I think we can remove it.

Hi @ashwink11, thank you for your reply. I'm not blaming the proposal, and I just highlight the facts that block the implementation. Firstly, if we will skip systemHardwareVersion, PMs could reasonably ask us on the code's review where the parameter is and why it is missed in the implementation when described in the proposal. The secondary is that the usage of those parameters is not clear, as described by me above. Do I understand correctly to reach the original proposal requirements from the mobile view we will receive but ignore systemHardwareVersion and systemSoftwareVersion in StartServiceACK, and for RAI we will receive only systemSoftwareVersion in the response that also be just ignored because the only parameter that is passed back is VehicleType?

@ghost
Copy link

ghost commented Jan 21, 2021

@vladmu I understand your concern that finalizing the proposal is impossible because the proposal doesn't specify how to fetch systemHardwareVersion. You are correct in the assumption that systemSoftwareVersion and systemHardwareVersion isn't being used yet as criteria but could possibly be used in future by future proposals.

Let's just get through the options we have from the technical point of view:

Option 1: Remove systemHardwareVersion and systemSoftwareVersion from this proposal. It's probably the easiest solution but has the downside if the information is needed in future. There's clearly the possibility to add both in a new proposal for a future SDL release.

Option 2: Extend HMI API to provide systemHardwareVersion and have both parameters in StartServiceACK, use any way suggested here #1108 (comment) to provide the information to the app. Apps could use the information in any way and future proposals will have it easy to add criteria as information is already available. Ford prefers this solution for this proposal if the review process doesn't delay the proposal implementation and SDL release timing.

Option 3: Option 2 + extend RAI response. If software version becomes a requirement in future proposals, then the criteria would be used in the protocol level, before you would register the app. Adding it to RAI doesn't make sense as it would be too late to react. Anything the proposal does with RAI is for backward compatibility. Also the information seems redundant and the library should instead provide vehicle information and version to the app using the notification.

With regards to the SDLC process we have following options:

Option a: Agree on an option from above and accept the proposal with revisions. The revisions would be whatever option we choose from above. This accelerates the implementation and wouldn't risk the SDL release timing further.

Option b: Agree on an option from above and return the proposal with revisions. The revisions would be whatever option we choose from above. In this case I would suggest allowing the developer to continue work on the PR but review and merging requires proposal acceptance.

Option c: Reject/Withdraw the proposal revision and recreate another revision if the changes are too big. From our point of view there's no reason to follow this option. I don't think any of the options are so big that the proposal revision must be withdrawn.

@vladmu
Copy link
Contributor

vladmu commented Jan 21, 2021

@kshala-ford thank you for your reply.
We understand that systemSoftwareVersion and systemHardwareVersion could possibly be used in the future by future proposals. And we are OK if those parameters will be present in StartServiceACK and/or in RegisterAppInterfaceResponse. But their usage should be formalized in the proposal, and if no processing is needed in the mobile library at this time, it would be good to mention that.
Short addition, regarding Options 2 and 3, it seems option 2 could not be reached without 3 in order of current proposal requirements because this states the onVehicleTypeReceived function in all libraries as a point where the developer could respond true or false. Thus we could not use different options for each case (without systemHardwareVersion in the RAI response but including that in StartServiceACK), and both VehicleType and SystemInfo should be present in StartServiceACK and RegisterAppInterfaceResponse. The current proposed revision exactly covers this by adding systemHardwareVersion near systemSoftwareVersion in the RAI response.

@ghost
Copy link

ghost commented Jan 21, 2021

@vladmu Regarding your addition, the requirement you refer to is

  1. The developer will receive vehicle type information from either the protocol StartServiceACK or from the RegisterAppInterfaceResponse. The information is then sent to the SDLManagerDelegate / SdlManagerListener for the developer to handle.

If you include the following two points in the proposal you will see that this is rather a description that data could be used from StartServiceACK or from RAI repsonse whereby StartServiceACK is preferred. The third point in the proposal describes backwards compatibility using any information available in RAI response. Now for systemHardwareVersion the third point will never apply because the third point only applies to SDL versions prior to this proposal. Hence, adding it to the RAI response is not necessary. This is what @joeljfischer tried to explain in his comment here #1108 (comment)

@vladmu
Copy link
Contributor

vladmu commented Jan 21, 2021

@vladmu Regarding your addition, the requirement you refer to is

  1. The developer will receive vehicle type information from either the protocol StartServiceACK or from the RegisterAppInterfaceResponse. The information is then sent to the SDLManagerDelegate / SdlManagerListener for the developer to handle.

If you include the following two points in the proposal you will see that this is rather a description that data could be used from StartServiceACK or from RAI repsonse whereby StartServiceACK is preferred. The third point in the proposal describes backwards compatibility using any information available in RAI response. Now for systemHardwareVersion the third point will never apply because the third point only applies to SDL versions prior to this proposal. Hence, adding it to the RAI response is not necessary. This is what @joeljfischer tried to explain in his comment here #1108 (comment)

The explanation is clear, for backward compatibility systemHardwareVersion could be optional in the onVehicleTypeReceived of course and be absent in the RAI response, but if we talk about future purposes for usage such fields in StartServiceACK, why it could not be an argument to put the systemHardwareVersion near the systemSoftwareVersion for the further compatibility? It seems the library should stay consistent in both directions.
Btw we have no objections regarding any of the options proposed by @joeygrover in #1108 (comment) as it just extends our proposals in #1108 (comment)

@joeljfischer
Copy link
Contributor

The explanation is clear, for backward compatibility systemHardwareVersion could be optional in the onVehicleTypeReceived of course and be absent in the RAI response, but if we talk about future purposes for usage such fields in StartServiceACK, why it could not be an argument to put the systemHardwareVersion near the systemSoftwareVersion for the further compatibility?It seems the library should stay consistent in both directions.

There's no "further compatibility" gained by placing a new parameter in two places. In fact, we could deprecate the systemSoftwareVersion and vehicle information in the RAIR to make it clearer that as of the time this proposal is implemented, the vehicle type information should be retrieved only through the protocol message.

@vladmu
Copy link
Contributor

vladmu commented Jan 21, 2021

@joeljfischer thank you, I'd like just summarize the preferable options from our side: we prefer to support Option 2 from this comment #1108 (comment) including Option 2 from this comment #1108 (comment) and deprecating systemSoftwareVersion in RAIR in this revision or in a future separate proposal.

@ashwink11
Copy link
Contributor

Hello,
Ford and Luxsoft teams had an offline discussion regarding these revisions. We agreed on the below items.
1. We will not have systemHardwareVersion in the RAI response.
2. To communicate system info to app-level, we could proceed with option 2 from the suggestion made in comment here .
Please let us know if you have any suggestions.

@ashwink11
Copy link
Contributor

@ashwink11 -san
2.Thank you for your reply.
The parameters of systemHarwareVersion and systemSoftwareVersion are for judging whether the OEM exclusive apps is the target of operation or not,and are not used for other purposes, so does it mean that the OEM can set them on their own?

Hello @Sohei-Suzuki-Nexty, systemHarwareVersion and systemSoftwareVersion are the params sent to apps. Apps can use this info as they need. SDL libraries do not use this info to filter apps. Since these params is provided to determine the version of the OEM's SDL enabled system, OEM can set them on their own.

@joeljfischer
Copy link
Contributor

  1. We will not have systemHardwareVersion in the RAI response.
  2. To communicate system info to app-level, we could proceed with option 2 from the suggestion made in comment here .
    Please let us know if you have any suggestions.

I'm okay with this. However, I would also like to see RAIR. vehicleType and RAIR.systemHardwareVersion deprecated.

@mrapitis
Copy link
Contributor

mrapitis commented Jan 25, 2021

If we deprecate the fields in the RAI response, it would be good to explicitly mention in the proposal that the fields will continue to be sent by SDL core if the RPC version is less than the next major version (8.0?) to protect mobile library implementations in the field that have not been updated yet (this scenario is an old mobile library connecting to a new core implementation).

@Sohei-Suzuki-Nexty
Copy link

@ashwink11 -san
2.

Hello @Sohei-Suzuki-Nexty, systemHarwareVersion and systemSoftwareVersion are the params sent to apps. Apps can use this info as they need. SDL libraries do not use this info to filter apps. Since these params is provided to determine the version of the OEM's SDL enabled system, OEM can set them on their own.

I understand the parameters of systemHarwareVersion and systemSoftwareVersion.
Thank you for the explanation.

@vladmu
Copy link
Contributor

vladmu commented Jan 26, 2021

If we understand correctly all conversations above, the deprecation of VehicleType and systemSoftwareVersion in RAIR is optional from the mobile view because the processing of those data will be skipped if the data is already received in StartServiceACK.

The mobile library implementation should:

  • expect VehicleData and SystemInfo parameters and fire onVehicleDataReceived event in StartServiceACK for the protocol version >= 5.
  • in RAI response processing, check if VehicleData and SystemInfo are already received in StartServiceACK and only if no, try to get the required data from the response and fire onVehicleDataReceived event. The only time for this case is when Core and HMI's version does not include this proposal. In this case, the deprecated vehicleType and systemSoftwareVersion parameters will be present in the RAIR, and the systemHardwareVersion will be absent.

It would be good to reflect all described above alongside the new SystemInfo structure and parameter in onVehicleDataReceived event proposed in option 2 in #1108 (comment) and HMI_API changes already included in this revision.

@joeljfischer
Copy link
Contributor

If we deprecate the fields in the RAI response, it would be good to explicitly mention in the proposal that the fields will continue to be sent by SDL core if the RPC version is less than the next major version (8.0?) to protect mobile library implementations in the field that have not been updated yet (this scenario is an old mobile library connecting to a new core implementation).

The versioning system of Core would handle this. An old mobile library version that connects with a newer head unit will use the RPC spec version that the mobile library supports, in which the parameters would not be deprecated. There should be no issue here.

If we understand correctly all conversations above, the deprecation of VehicleType and systemSoftwareVersion in RAIR is optional from the mobile view because the processing of those data will be skipped if the data is already received in StartServiceACK.

Correct, it is optional, I simply think it's confusing, especially to newer members, if there are two ways of passing the same data and neither is deprecated. The rest of your mobile library implementation appears to be correct.

@joeljfischer
Copy link
Contributor

Agreed upon items for revision:
1. Agree that the systemHardwareVersion should not be added to the RegisterAppInterface response.

2. Update the onVehicleTypeReceived callback to include a new param of a new class type SystemInfo.

3. Deprecate the VehicleType and systemSoftwareVersion params as to not cause confusion with the newly added params to the StartServiceACK for the RPC service.

<function name="RegisterAppInterface" functionID="RegisterAppInterfaceID" messagetype="response" since="1.0">
    <param name="vehicleType" type="VehicleType" mandatory="false" since="x.x" deprecated="true">
        <description>Specifies the vehicle's type. See VehicleType.</description>
        <history>
            <param name="vehicleType" type="VehicleType" mandatory="false" since="2.0" until="x.x" />
        </history>
    </param>
    <param name="systemSoftwareVersion" type="String" maxlength="100" mandatory="false" platform="documentation" since="x.x" deprecated="true">
        <description>The software version of the system that implements the SmartDeviceLink core.</description>
        <history>
           <param name="systemSoftwareVersion" type="String" maxlength="100" mandatory="false" platform="documentation" since="3.0" until="x.x">
        </history>
    </param>
</function>

Final item for approval:
The PM would like to update the method signature and class structure further to the following:

class SystemInfo {
    VehicleType vehicleType
    String systemSoftwareVersion,
    String systemHardwareVersion
}

boolean onSystemInfoReceived(SystemInfo systemInfo);
@interface SDLSystemInfo

@property (strong, nonatomic, readonly) SDLVehicleType *vehicleType;
@property (strong, nonatomic, readonly) NSString *systemSoftwareVersion;
@property (strong, nonatomic, readonly) NSString *systemHardwareVersion;

@end

- (BOOL)didReceiveSystemInfo:(SDLSystemInfo *)systemInfo;

and equivalent for the JavaScript Suite library.

It provides a cleaner API with a more intuitive name. It allows for additional params to be added in the future without making breaking changes.

@vladmu
Copy link
Contributor

vladmu commented Jan 26, 2021

@joeljfischer,

  1. Is the SystemInfo a new RPC struct or just a class somewhere in the library? If the last one, where is the best place for this according to the current folder structure? If the first one, should it be added into MOBILE_API also?
  2. Final items don't contain the JS snippet, just some words about equivalent, if the SystemInfo is just a class, it would be good to define the snippet for JS also, for clear understanding, eg. should it contain set*/get* methods for each class field, etc.

@joeygrover
Copy link
Member

joeygrover commented Jan 26, 2021

SystemInfo is just a new class, it is not an RPC. I don't believe it is important to determine folder structure at this time since it can likely be located in a number of places. The PM will determine this during implementation if it is agreed to.

The new SystemInfo classes should have getters and setters, just a very basic class to hold this data. With how basic this class is it should be pretty clear from the comments to imagine the code but here's what it would likely look like for the JS suite:

class SystemInfo {
   
    /**
     * Initializes an instance of SystemInfo.
     * @class
     * @param {VehicleType} vehicleType 
     * @param {String} systemSoftwareVersion 
     * @param {String} systemHardwareVersion 
     */
    constructor (vehicleType = null, systemSoftwareVersion = null, systemHardwareVersion = null) {
        this._vehicleType = vehicleType;
        this._systemSoftwareVersion = systemSoftwareVersion;
        this._systemHardwareVersion = systemHardwareVersion;
    }
}
 

//API callback
onSystemInfoReceived (sdlManager, systemInfo) {}

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Jan 27, 2021
@jordynmackool
Copy link
Contributor Author

jordynmackool commented Jan 27, 2021

The Steering Committee voted on 2021-01-26 to accept this revised proposal with the revisions stated in this comment (Items 1, 2, 3 and the noted "Final item for approval”: update the method signature and class structure).

@jordynmackool jordynmackool changed the title [In Review] SDL 0293 Revisions - Enable OEM exclusive apps support [Accepted with Revisions] SDL 0293 Revisions - Enable OEM exclusive apps support Jan 27, 2021
@jordynmackool
Copy link
Contributor Author

@LitvinenkoIra 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 comment on the already created issues for the impacted repositories. Thanks!

@jordynmackool
Copy link
Contributor Author

The proposal has been updated based on the Steering Committee's agreed-upon revisions. Comments have been left on implementation issues to reference revisions:

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

10 participants