[Accepted with Revisions] SDL 0263 - Extend SystemRequest With Optional Data For Proprietary Data Exchange

[theresalech]

Hello SDL community,

The review of the revised proposal "SDL 0263 - Extend SystemRequest With Optional Data For Proprietary Data Exchange" begins now and runs through January 21, 2020. The review of the original proposal took place November 18 - December 3, 2019. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0263-System-Request.md

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/871

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

[E-SAITO-TMC]

What percentage is the improvement of this proposal?

[yang1070]

@E-SAITO-TMC We haven't implemented the proposal yet, so we cannot measure the improvement. It depends on many factors, such as the hardware and how data is spread in the disk, But from the book Node.JS Design Patterns, it says : Accessing the RAM is in the order of nanoseconds ( 10e-9 seconds ),while accessing data on the disk or the network is in the order of milliseconds (10e-3 seconds).

[joeygrover]

Potential changes to the mobile proxy lib

Extract hybrid part of the message from SystemRequest RPC response message, if the library did not do that before.

I think the author should investigate if this is needed or not. Applying a statement about what "might" be needed leads to misunderstandings of proposal scope and therefore implementation timing. Proposals should be as exact as possible to avoid any assumptions, not be vague enough so that it can cover anything that comes up.

Since the impacted platforms includes the proxy library I would request the author investigates the following:

1. Are changes necessary in the iOS and Java Suite projects
2. If so, provide those changes in a revision to the proposal.
3. If they are not required, the impacted platforms should reflect that.

[Jack-Byrne]

@yang1070

Ill start my numbering at 3.

3.

When HMI receives a SystemRequest request, if optional parameter requestData exists, it shall ignore the fileName parameter.

fileName is mandatory in the HMI API for SystemRequest. What value will be sent for fileName when requestData exists in the message?

[Jack-Byrne]

@yang1070

4. What is the purpose of resultData in the SystemRequest response. There are no parameters included in the SystemRequest response, so there is no data sent back to Core in this response currently.

[yang1070]

@joeygrover I have investigated the java proxy code in SystemRequestResponse.java, currently it only handles result code and success flag, it does not handle bulkData or binaryData. In iOS library SmartDeviceLink/SDLProxy.m, systemRequestResponse message just get a log and discarded. so my answers to Q1 and Q2 : yes, both java and iOS proxy lib need update to handle the buldData - which is send in the hybrid part of the message, and is the base64 decoding of the resultData. @JackLivio Answer to Q3, fileName can be hard coded to any string, since it will be ignored when requestData exists. Answer to Q4, , resultData is the most important change of this proposal, it is more important then changing from file access to string parameter. The purpose is to provide the mobile app a way requesting OEM specific data from the head unit depending on the requestType and subRequestType in the system request. It is a totally new parameter. And shall be send as the bulkdata of a hybrid message. Previously usage is that core sends an onSystemRequest to mobile, mobile does some processing and sends a SystemRequest and gets a success or failure response. The new usage will be mobile sends a SystemRequest to core and core forwards it to HMI, and HMI does some processing, the result data will be included in the SystemRequest response directly.

[joeygrover]

yes, both java and iOS proxy lib need update to handle the buldData - which is send in the hybrid part of the message, and is the base64 decoding of the resultData.

Ok please be prepared to include these changes into the proposal revisions. Thank you!

[yang1070]

@joeygrover I will provide text descriptions of the change in the revisions, not the real code change.

[joeygrover]

@yang1070 I'm requesting code changes.

[yang1070]

@joeygrover I think the code changes shall belong to the implementation of a proposal. It is nice to have , but it is not a mandatory part of a proposal.

[joeygrover]

@yang1070 I understand your position, however, I am still requesting that code changes to the libraries be included in the proposal so there is no assumptions being made on how they will be changed. Thank you!

[TinaKleczka]

@joeygrover As part of the proposal, a high level description should be a sufficient amount of detail needed. The updated description will still need to be reviewed, understood and approved. There should be no misunderstanding of what needs to be implemented if the change been agreed upon and approved. I do not believe this requires actual code change at this time.

[joeygrover]

If there should be no misunderstanding of what needs to be implemented, then why can't it be included?

[joeygrover]

Also can you point me to where we introduced result data as being part of a hybrid message for SystemRequest response? The RPC spec does not contain any information that there would be binary data attached to that response or what its contents might be. So either we need to update the RPC spec to match where this was previously determined, or the actual RPC spec is an impacted platform as well for this proposal (not just the HMI API).

[TinaKleczka]

@yang1070 will be adding a description of what will be implemented, which will be clear for the implementation and review for this proposal. This will be potentially voted on tonight, does the code need to be there for the vote? When the proposal is reviewed, and if approved, we can certainly assure the code is updated with all the updated information. We also have great documentation here, were the PM stresses the concern of how the implementation needs to be. We can refer to all the history, as well as note it in the PR once the development work starts. We can discuss this in greater detail if not having the code is a major concern during our Steering Committee meeting this evening.

[yang1070]

Also can you point me to where we introduced result data as being part of a hybrid message for SystemRequest response? The RPC spec does not contain any information that there would be binary data attached to that response or what its contents might be. So either we need to update the RPC spec to match where this was previously determined, or the actual RPC spec is an impacted platform as well for this proposal (not just the HMI API).

the description of resultData clearly says SDL Core shall send decoded data to the mobile app in hybrid part of message.

<function name="SystemRequest" messagetype="response">
    ...
+   <param name="resultData" type="String" maxlength="65535" mandatory="false">
+       <description> Base64 encoded result data from the system to SDL Core. SDL Core shall send decoded data to the mobile app in hybrid part of message. </description>
+   </param>
</function>

It is also mentioned here

When SDL Core revives a SystemRequest response or OnSystemRequest notification from HMI, if the optional data/resultData parameter exists, SDL Core shall ignore the fileName parameter. Instead of reading data from the file, SDL Core shall base64 decode the string, and send the resulting data in hybrid part of the message to mobile.

Hybrid message or bulk data is part of protocol spec. RPC spec does not have bulk data as a parameter in any RPC, but there is a text description in some RPCs. Similar RPCs like PutFile, OnSystemRequest and OnAudioPassThrough, use the bulk data. I agree we need to update the RPC spec in the SystemRequest response section, to say binary data can be included in hybrid part of message in a response of the system request

[joeygrover]

@yang1070 thank you, that's what I was asking. While hybrid messages are part of the protocol spec, the contents of the bulk data is defined in the description of the RPC message itself. Otherwise there is no context to what that bulk data is or should be used for. So we should remove the line that states the RPC spec (mobile api) is not affected.

[yang1070]

So we should remove the line that states the RPC spec (mobile api) is not affected.

agree

[yang1070]

Although I can propose some code changes in the proposal, like updating the SystemRequestResponse.java, but since I’m not an iOS developer, i will not be able to propose any iOS code change, also I’m afraid of missing some necessary code changes in the proposal, I will leave implementation details up to the author(s) of the proxy libs for java and iOS.

The high level requirement to the mobile proxy is that (1) When the mobile proxy receives a SystemRequest response RPC from SDL core, it shall extract the bulk data from protocol message if the bulk data ever exists in the response. (2) The mobile proxy shall provide API of getBulkData/SetBulkData, just like in the java classes for OnSystemRequest and OnAudioPassThrou, so that mobile apps can call the API to read binary data for further processing.

[theresalech]

The Steering Committee voted to return this proposal for revisions. The author will revise the proposal to remove the line that states the RPC spec (mobile api) is not affected, and also add some proxy code changes (samples and snippets, not necessarily production level code) and implementation requirements as available.

There was also discussion about the level of detail required in a proposal with regards to code changes and implementation requirements, which will be a separate agenda item for the Steering Committee in future meetings.

[theresalech]

The author has revised this proposal per the Steering Committee's request, and the revised proposal is now under Steering Committee review. This review will take place until January 21, 2020.

[joeljfischer]

1. Currently the iOS proxy appears to discard all SystemRequestResponse RPCs.

// SDLProxy.m
- (void)handleSystemRequestResponse:(SDLRPCMessage *)message {
    SDLLogV(@"SystemRequestResponse to be discarded");
}

Additional details will be needed about changing this as well as the potential for changes to this method:

// SDLProxy.m
- (void)handleSystemRequestProprietary:(SDLOnSystemRequest *)request {

[Jack-Byrne]

2. Question for clarification, is this feature for only certain System Request types like OEM_SPECIFIC? The request type alters how information will be propagated through Core so impact to those different flows should be addressed. My main concern is with SystemRequests role in policy table updates.

[yang1070]

@joeljfischer The SDLLogV(@"SystemRequestResponse to be discarded"); need to be removed.

SDLSystemRequestResponse and SDLOnSystemRequest inherit from SDLRPCMessage so bulkData is already exposed (can be set/get and is nullable). SDLRPCMessage handles the parsing bulkData from SDL Core, Therefore there shall be no special code in the two handlers mention above.

@JackLivio Yes, the intention of this feature is to be used with System Request type OEM_SPECIFIC and subtypes. Other System Request types shall keep the existing logic.

[Jack-Byrne]

@yang1070 Thank you for the clarification. I would ask for a minor revision in the proposal to specifically mention the use of the RequestType enum, OEM_SPECIFIC. I think it would also be useful to include in the API descriptions that the new parameters will only work when the request type is set to OEM_SPECIFIC

[Jack-Byrne]

3. @yang1070 I think the description in the ini file should mention that the largest value for this configuration is 65535 because of the api maxlength value.

; The payload in SystemRequest/OnSystemRequest packet bigger than this value will be saved to a file when passing to HMI, ; payload smaller than this value will be based64 encoded and stored as a string parameter to HMI ; if not specified, this value will default to 0 MaximumBinaryPayloadSizeToString = 11520

[Jack-Byrne]

3. Continued: Was 65535 chosen as the max length because that is the largest max length of any string in the APIs? I am wondering if anyone knows of the limitations for this max length. Can we make it even larger?

[joeygrover]

4. Both the JavaScript suite and HMI (which includes both SDL HMI and Generic HMI)should be listed as an impacted platform.

5.

There is no parameter added to mobile API. However, the text description of SystemRequest response shall say that binary data can be included in the hybrid part of the message in the response of a system request.

Can this text be included with the xml example into the proposal so we know exactly what to include in the RPC spec?

6. Have we thought about adding the ability to send hybrid messages between Core/HMI instead of patching it in this way? Seems like we could potentially improve many RPCs and their performance if we could. Or are there other RPCs we would actually like to add similar changes to that could improve performance? For example, when sending many artworks, Core would first have to write them to disk, then the HMI would read them, and display them. However, if the image is dynamic and does not need to be cached, would it make sense to simply have the HMI use the raw data and not write to disk? Or write to disk after the image has been displayed. Maybe someone more versed with the HMI -> Core communication and capabilities could weigh in.

[yang1070]

2. Yes, we can update the proposal to specifically mention the use of the RequestType enum, OEM_SPECIFIC.
3. Yes, 65535 was chosen as the max length because it is the largest max length of string in the existing APIs. Not sure we make it larger or not. we can add a comments in ini file, that max length before base64 encoding is 49151. ( beacuae 49151/3*4=65534, which is length after encoding)
4. Yes.
5. Mobile API xml update

   <function name="SystemRequest" functionID="SystemRequestID" messagetype="response" since="3.0">
   <description>Binary data can be included in hybrid part of message for some responses when RequestType is OEM_SPECIFIC </description>
   ...
   </function>

6. In theory, all the RPC messages (request, response and notification) in the mobile API can support hybrid message. However, current, only the following 4 functions, OnSystemRequest, PutFile, SystemRequest, OnAudioPassThru use it. And they are using file tranfer as the method to pass file between SDL core and HMI. In the art work example, the HMI still needs to persist the art work binary data for the use case that user switch away from SDL App screen to any vehicle experience screen and come back to the app. The HMI need to restore the display without asking the app resend the RPC.

[Jack-Byrne]

6. @joeygrover That is an interesting idea but I think I would prefer the approach of adding binary data params to the HMI_API for different rpcs on a case by case basis. All data transferred between core and HMI gets turned into JSON so I don't think making a "hybrid" service wouldn't be more than attaching a binary data param to every message anyways.

But to expand on your artwork idea, that would allow Core and the HMI to run on separate file systems which could be useful for many reasons.

@yang1070 I believe Joey was suggesting that artwork would be transmitted as binary data to hmi while Core takes its time writing the image to disk. Once the image is saved to disk, core could still resume the binary image data without the app sending the rpc again.

[yang1070]

As Jack mentioned, All data transferred between core and HMI gets turned into JSON, that is the limitation and it is the reason why we have this proposal. If we have a way to include binary data directly in HMI API, that will solve a lot similar problems.

[joeljfischer]

1. My point here is that the proposal doesn't adequately speak about changes to the iOS platform, because it states, "therefore, there is no code change need." Because there are deeper changes necessary, I think that the authors need to do a deeper investigation into the changes that are necessary.

What I have noted above would change the behavior of the iOS library, and I have not done further investigation into changes that may be necessary to make this change work.

[yang1070]

@joeljfischer I have consulted our iOS developer Satbir Tanda in Palo alto, CA regarding the potential change of this proposal. He did the investigation. And advised there is no change needed unless we want to expose a new resultData instead of bulkData to SDLSystemRequestResponse. On the mobile side, the new function in this proposal is to parse the bulkdata so that developer can use it, and it is in class SDLRPCMessage, which is the existing code.

There is no change in the following method

// SDLProxy.m
- (void)handleSystemRequestProprietary:(SDLOnSystemRequest *)request {

And SDLLogV is just a logging function, remove it or update to proper log message is trivial

// SDLProxy.m
- (void)handleSystemRequestResponse:(SDLRPCMessage *)message {
    SDLLogV(@"SystemRequestResponse to be discarded");
}

[theresalech]

The Steering Committee voted to keep this proposal in review for another week, to allow members and the Project Maintainer additional time to review and provide comments on the review issue. It was noted that the Project Maintainer believes changes to the iOS library will be required for the proposal.

[joeljfischer]

Hi @yang1070, I would recommend that the methods I have mentioned be removed. I would also recommend that SystemRequest RPCs be made public. They are currently private; I'm not sure why. After further investigation I think the rest will work.

[yang1070]

@joeljfischer Which method need to be removed? Is it (void)handleSystemRequestResponse:(SDLRPCMessage *)message ? Could you please clarify?

[joeljfischer]

Correct, I mean - (void)handleSystemRequestResponse:(SDLRPCMessage *)message { and - (void)handleSystemRequestProprietary:(SDLOnSystemRequest *)request {

[yang1070]

@joeljfischer There are a lot of code within handleSystemRequestProprietary, we cannot remove it. For the other one, handleSystemRequestResponse, it does nothing except write to log, we can remove it and the place to called the function.

[theresalech]

The Steering Committee voted to keep this proposal in review, so that the PM could investigate the request to not remove handleSystemRequestProprietary as suggested by the author in this comment.

[joeljfischer]

@yang1070 That appears correct to me, I must have gotten mixed up earlier in the comment stream.

[theresalech]

A quorum was not present at the 2020-02-04 Steering Committee meeting, so a vote on this proposal could not take place. This proposal will remain in review until our next meeting on 2020-02-11.

[theresalech]

The Steering Committee voted to accept this proposal with the following revisions: make SystemRequest RPCs public, and remove handleSystemRequestResponse.

[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 enter issues in the respective repositories for implementation. Thanks!