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:
https://github.com/smartdevicelink/sdl_evolution/pull/1106
Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:
https://github.com/smartdevicelink/sdl_evolution/issues/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:
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
[email protected]
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".
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.
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: https://github.com/smartdevicelink/sdl_evolution/pull/1106
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.
- 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. ThissystemHardwareVersionwould need to be added to this struct, and we would need to figure out something to do aboutsystemSoftwareVersionlike 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) {}
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.
Hello @Sohei-Suzuki-Nexty
@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?
Hello @Sohei-Suzuki-Nexty
The revision to add a new String param to the RegisterAppInterface response RPC is unnecessary. I will let @JackLivio comment on the HMI_API additions. The original proposal already includes the systemHardwareVersion string in the StartServiceACK to the RPC service. Since this must be implemented at the same time as the requested revisions it becomes redundant. Adding this info to the RegisterAppInterface response will only bloat the RPC further and create a duplication of the info.
The revision that is necessary is one that defines how the public API onVehicleTypeReceived &didReceiveVehicleType receives this string and the systemSoftwareVersion. Currently it only includes the VehicleType struct as others have mentioned. That can be accomplished in a few ways
boolean onVehicleTypeReceived(VehicleType type, String systemSoftwareVersion, String systemHardwareVersion);
SystemInfo) that is app library level only (not added to RPC Spec)class SystemInfo {
String systemSoftwareVersion,
String systemHardwareVersion
}
```java
boolean onVehicleTypeReceived(VehicleType type, SystemInfo systemInfo);
3. Add an API to retrieve these values from the `SdlManager` and the developer can access it themselves.
```java
SdlManager {
String getSystemHardwareVersion()
String getSystemSoftwareVersion()
}
Based on the naming of the current API method my preferences would be number 3, however this is very much open for discussion.
@joeygrover I think your 2 should be:
boolean onVehicleTypeReceived(VehicleType type, SystemInfo systemInfo);
EDIT: Submitted too soon. I also think that SystemInfo could contain a VehicleType, which would let it only pass one parameter.
I would prefer #1 among Joey's options myself.
Thank you, edited my code snippet to reflect the correct method signature.
If we go with SystemInfo that includes VehicleType we should rename the API callback for the developer; it doesn't align well.
Number 1 is my least favorite in terms of the Java Suite because each time an addition is made to the method signature it is technically a breaking change. So I prefer creating structs that can be modified if possible.
@joeygrover Regarding the point:
The revision to add a new String param to the RegisterAppInterface response RPC is unnecessary
I would like to notice this parameter is required to fulfill the item 5 in Flowcharts and sample code section of proposal:
If the StartServiceACK message does not have vehicle type ... \
We suppose Vehicle type info here is a combination of the following:
GetVehicleType data: make, model, modelYear, trimGetSystemInfo data: systemSoftwareVersion, systemHardwareVersionIn this case filtration logic that is going to be implemented in SDL mobile libraries will utilize the same set of parameters while making a decision regarding supportable system when sending EndService or UnregisterApp requests.
@joeygrover
Regarding HMI API Revisions:
I can confirm systemHardwareVersion should at least be added to the HMI API as a parameter for GetSystemInfo. The original proposal does not include a way for Core to retrieve this parameter from the HMI.
@dboltovskyi I do not believe there is a scenario where the information would not be available in time to send in the payload of the StartServiceACK but then available when the RAI response is sent. The only time that flow should be followed is when the version of Core and HMI do not include this proposal and therefore would not have the added systemHardwareVersion in the RAI response anyway.
@joeygrover actually, this is a scenario when mobile app is trying to connect to SDL using protocol version < 5. In that case, SDL can't provide any information in StartServiceAck because there is no bson payload, however it can provide all that information in RAI response. I believe there might be a case when mobile app does not have bson library support and uses older protocol version for a communication, however it may support feature like that, so providing information in RAI is the only way to work with such applications.
@AKalinich-Luxoft Adding the systemHardwareVersion to the RAI doesn't help in this case because protocol version < 5 is also going to be RPC version < 7.2 (or whatever). It's better to only have it in one place as we are adding this only for future versions, it doesn't assist us with backward compatibility.
The Steering Committee voted to keep these proposed revisions in review on 2021-01-19 to allow for conversation to continue on the open items.
@AKalinich-Luxoft Adding the
systemHardwareVersionto the RAI doesn't help in this case because protocol version < 5 is also going to be RPC version < 7.2 (or whatever). It's better to only have it in one place as we are adding this only for future versions, it doesn't assist us with backward compatibility.
@joeljfischer Do I understand correctly, if this proposal is actual only for RPC version > 7.1, then it seems we even don't need the case with the RAI response checking? Because for RPC version > 7.1 the Vehicle Type checking in StartServiceACK is enough, and we can just skip such checking wherever in StartServiceACK or RAI response for all RPC version < 7.2 and allow application works as usual in this case?
My second question, do we actually need systemHardware/SoftwareVersion information for purposes of enabling exclusive OEM support? I don't see in the proposal any specific cases where that would be needed. Based on this looks like the VehicleType information is primary and should be enough?
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.
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.
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.
@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?
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 tosystemHardwareVersion 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 keepsystemHardwareVersion and systemSoftwareVersionas isinStartServiceACKand get back to it for mobile implementation when these revisions are accepted. If you think,systemHardwareVersionblock any of the other items defined in the original proposal, please do let me know, we will discuss those issues.Regarding
systemHardwareVersionin 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?
@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 https://github.com/smartdevicelink/sdl_evolution/issues/1108#issuecomment-763107983 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.
@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.
@vladmu Regarding your addition, the requirement you refer to is
- 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 https://github.com/smartdevicelink/sdl_evolution/issues/1108#issuecomment-763608109
@vladmu Regarding your addition, the requirement you refer to is
- 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
systemHardwareVersionthe 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 https://github.com/smartdevicelink/sdl_evolution/issues/1108#issuecomment-760429319
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.
@joeljfischer thank you, I'd like just summarize the preferable options from our side: we prefer to support Option 2 from this comment https://github.com/smartdevicelink/sdl_evolution/issues/1108#issuecomment-764545279 including Option 2 from this comment https://github.com/smartdevicelink/sdl_evolution/issues/1108#issuecomment-763107983 and deprecating systemSoftwareVersion in RAIR in this revision or in a future separate proposal.
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 -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.
- We will not have systemHardwareVersion in the RAI response.
- 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.
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).
@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.
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:
VehicleData and SystemInfo parameters and fire onVehicleDataReceived event in StartServiceACK for the protocol version >= 5.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 https://github.com/smartdevicelink/sdl_evolution/issues/1108#issuecomment-763107983 and HMI_API changes already included in this revision.
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.
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.
@joeljfischer,
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?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.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) {}
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).
@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!
The proposal has been updated based on the Steering Committee's agreed-upon revisions. Comments have been left on implementation issues to reference revisions:
Most helpful comment
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.
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.