[Accepted] SDL 0037 - Expand Mobile putfile RPC about sdl_evolution HOT 16 CLOSED

Here are my concerns with this proposal:

• as discussed before, an OS issue on driver level where data gets corrupted needs to be fixed (which would make the proposal obsolete)

• adding a CRC param ONLY to the PutFile RPC would not be a complete solution - what about all the other RPCs that could get corrupted?

• defining behavior around an arbitrary parameter (e.g. "repeat transmission only 5 times") seems wrong

• if we think, we need to handle potential data corruption, I'd prefer to go with mentioned alternative - adding CRC to the SDL protocol layer. Could the impact be reduced by adding a new service type that adds additional CRC calculation while leaving existing service types untouched? Are there reserved bits available that could be used or would we have to extend the basic frame structure?

from sdl_evolution.

@jhodges55 Backward compatibility is not a concern, as the new parameter is optional, not required. Therefore, this is effectively adding a new API.

from sdl_evolution.

I have some concerns about this proposal.

1. This proposal introduces significant complexity to the mobile libraries. The mobile libraries now not only have to calculate the CRC checksum for each PutFile data packet sent, but they must also now store away the correlation ids of PutFiles sent along with which data was sent with them, so that if a PutFile response is returned with CORRUPTED_DATA, they may re-transmit. We currently do not have a setup for re-transmission, and that may take some time and complexity to set up. So this will not be an easy change on the mobile library side.

2. The proposal writes:

If the two checksums do not match, then the SDL core will know that the received data has been corrupted and will request the mobile library to resend the data along with the checksum. The SDL core will have a number limit for the same data packet retransmission. If the retry limit is reached, the SDL core will provide a response of “CORRUPTED_DATA” to the application. The app may have a logic resending same chunk of data or logging an error and giving up.

How will SDL Core request a retry compared to telling the mobile library not to retry with CORRUPTED_DATA?

3. CORRUPTED_DATA and INVALID_DATA seem very similar to me, especially if they're doing essentially the same thing (see 2). Was what is quoted above in (2) supposed to say that Core will request a retry with CORRUPTED_DATA and will state that no more retries will occur with INVALID_DATA? This proposal seems unclear on that point.

4. I am very confused if this is a protocol change or an RPC change. In some places it seems like the CRC is sent as an RPC parameter in PutFile, but later the proposal states:

Similar to the TCP design, the CRC checksum is placed at frame header (SmartDeviceLink Protocol level). The CRC checksum calculation is based on both the frame header and frame payload.

This is extremely confusing. If this is a protocol level change, then it has much deeper implications for how SDL works, how backward compatibility will work, and how this must be implemented.

Conclusion

Is the problem being addressed significant enough to warrant a change to SDL?

I'm not exactly sure how often this has been needed, or in what cases.

Does this proposal fit well with the feel and direction of SDL?

Due to the confusion listed above in (4), I would say no.

If you have used competitors with a similar feature, how do you feel that this proposal compares to those?

N/A

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

I carefully read this proposal.

Please state explicitly whether you believe that the proposal should be accepted into SDL.

I believe this proposal should not be accepted.

from sdl_evolution.

@joeljfischer thanks for your feedback, please see my comments below:

Concerns comment:

1. " The mobile libraries now not only have to calculate the CRC checksum for each PutFile data packet sent, but they must also now store away the correlation ids of PutFiles sent along with which data was sent with them"
   One question: Correlation ID is designed to map RPC request and response. Will mobile Lib maintain RPC request and correlation ID before it receives RPC response from SDL core? If we can maintain that RPC request, we might not need store away correlation ids. That request will retry until it receives " invalid_data" response.

2. Please check comment in 3

3. You are correct. SDL cores inform mobile lib to retry by sending "corrupted_data". "INVALID_DATA" should be sent if max retry number is reached.

4. In this proposal, there are two solutions. The first one "Expand Mobile putfile RPC" is RPC level change solution. The second solution "add checksum in SDL protocol"is listed at "Alternatives" section. The alter method is indeed a protocol level change. Additional checksum can provide robustness for all RPC communication. I am not sure about backward compatibility, but I guess it might require a major version change for SDL protocol.

Conclusion comment:

1. "Is the problem being addressed significant enough to warrant a change to SDL?"
   The problem we reported in this proposal is unreliable medium will have errors during transmission. It will impact every RPC. Among all operations, large file transfer through putfile RPC is most sensitive to this kind of error. Bluetooth is unreliable based on our test result. Research paper in this proposal also suggests Wi-Fi medium can be unreliable. Without any protection, it will limit SDL functionality in transferring large size of data (Binary file, Navigation Map file). Thus, I believe it needs a change for SDL to fulfill this need. And two solutions are proposed in this proposal.

from sdl_evolution.

Correlation ID is designed to map RPC request and response. Will mobile Lib maintain RPC request and correlation ID before it receives RPC response from SDL core? If we can maintain that RPC request, we might not need store away correlation ids. That request will retry until it receives " invalid_data" response.

This would technically work, but would require storing large amounts of data in RAM for an unknown period of time. It would be technically more difficult, but much more efficient, to store the correlation id and a reference to which data was sent with it, and then recreating the PutFile if necessary, since that will be the most uncommon use case. Which solution is used will require a bigger conversation.

With that said, this proposal seems incomplete without a full discussion of how this is intended to be accomplished within the mobile libraries. It feels very much like a "this is the concept, we'll figure out how the mobile libraries will work later," which is a significant amount of complexity that hasn't been decided upon and is being committed to without being discussed.

2 & 3. You are correct. SDL cores inform mobile lib to retry by sending "corrupted_data". "INVALID_DATA" should be sent if max retry number is reached.

Okay, the proposal is very unclear on this. I was mostly just guessing on how I thought it might work.

The second solution "add checksum in SDL protocol"is listed at "Alternatives" section. The alter method is indeed a protocol level change.

I somehow missed that part was in alternatives. Apologies.

from sdl_evolution.

@joeljfischer

This would technically work, but would require storing large amounts of data in RAM for an unknown period of time

Thanks for your clarification. I see the reason why we need to maintain correlation id.

It feels very much like a "this is the concept, we'll figure out how the mobile libraries will work later,"

I might not have a full scope of mobile libraries implementation details. please share more implementation details concerns, that will be very beneficial. In the meantime, we also have a protocol level change solution in proposol. I just want to check community's opinion before we get more details into each solution.

from sdl_evolution.

@mfoedisch
Thanks for your comments.

as discussed before, an OS issue on driver level where data gets corrupted needs to be fixed (which would make the proposal obsolete)

Agreed. but data corruption issue might not only be caused by OS and driver issue.

adding a CRC param ONLY to the PutFile RPC would not be a complete solution - what about all the other RPCs that could get corrupted?

So alter solution is also proposed to cover all RPCs.

defining behavior around an arbitrary parameter (e.g. "repeat transmission only 5 times") seems wrong

could you be more specific on this one?

if we think, we need to handle potential data corruption, I'd prefer to go with mentioned alternative - adding CRC to the SDL protocol layer. Could the impact be reduced by adding a new service type that adds additional CRC calculation while leaving existing service types untouched? Are there reserved bits available that could be used or would we have to extend the basic frame structure?

In SDL protocol, I didn't find 32bit length long reserved bits that we can directly use to place CRC code.
We might have to extend basic frame structure for RPC service binary header like below?

RPC Payload

Binary header

from sdl_evolution.

Hi @mfoedisch, I'm afraid I must disagree with you on most of your comments here.

adding a CRC param ONLY to the PutFile RPC would not be a complete solution - what about all the other RPCs that could get corrupted?

There are no other RPCs that can really compare with PutFile for sheer size. RegisterAppInterface is fairly large, but nowhere near the size of a PutFile. Others may correct me if I'm wrong, but I think this issue is only really appearing in very large file transfers.

defining behavior around an arbitrary parameter (e.g. "repeat transmission only 5 times") seems wrong

I think this is fairly common practice in most network use cases. Eventually things time out by an "arbitrary parameter".

if we think, we need to handle potential data corruption, I'd prefer to go with mentioned alternative - adding CRC to the SDL protocol layer. Could the impact be reduced by adding a new service type that adds additional CRC calculation while leaving existing service types untouched? Are there reserved bits available that could be used or would we have to extend the basic frame structure?

I very much do not like this option. Bumping the protocol for something that isn't really an issue with the protocol itself, and only appears to be an issue with a single RPC? I think that alterations to the protocol itself should be done extraordinarily carefully and only after much thought and careful design. We've talked about this internally before, but we can only bump the protocol so many times due to the length of the version parameter in the spec. With that said, it is possible to extend the protocol to a version 5 and do all of this, but it seems like unnecessary overkill for consistency's sake, and will slow down the entire protocol.

from sdl_evolution.

The Steering Committee has decided to extend this review, as they were not able to accept this proposal as written. The Ford team will re-write this proposal to add clarity and address the concerns expressed in the issue comments and during the Steering Committee meeting. The Steering Committee will vote on the revised proposal during the next meeting on April 4, 2017.

from sdl_evolution.

Can the revised proposal also address the concern raised in a couple of the early comments on backwards compatibility. For organizations that have already implemented the SDL Core, new HS applications which utilize this updated API may not work with the previous SDL Core. Our recommendation is to add a new API instead of updating an existing API.

from sdl_evolution.

@joeljfischer Thank you for clarifying. This addresses our concern.

from sdl_evolution.

If I understand this correctly, because there is no version bump, the best practice for an developer will be to always assume that CRC is supported and to request that option always -- there is no way to know a priori if CRC is supported.

The developer then will see two identical success results for different cases:

• the putfile succeeded with CRC on a CRC-providing implementation. The data is verifiably copied.
• the putfile "succeeded" without CRC on an old implementation. The data may or may not have been copied correctly

It seems like it might be useful to discriminate between these cases, if only for test purposes. Could different info fields be returned in these two cases: “File Downloaded with CRC Check” vs "File Downloaded" for an older implementation?

from sdl_evolution.

@jhludwig There is a version bump, because according to semantic versioning any API addition is a minor version bump. therefore, the developer can use the syncMsgVersion property of the RAIR to know if the CRC will be supported by the head unit. This probably isn't needed or practical for production cases, but for test uses, it should be just fine. I would be interested to hear others' opinions on that.

from sdl_evolution.

from sdl_evolution.

The Steering Committee has agreed to accept this proposal, given the revisions to add clarity and address initial concerns (outlined here).

from sdl_evolution.

Issues entered:
Core
iOS
Android
RPC

from sdl_evolution.