Comments (16)
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.
-
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.
-
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
?
-
CORRUPTED_DATA
andINVALID_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 withCORRUPTED_DATA
and will state that no more retries will occur withINVALID_DATA
? This proposal seems unclear on that point. -
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:
-
" 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. -
Please check comment in 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.
-
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:
- "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.
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 |
JSON Data |
Binary header
Byte 1 | Byte 2 | Byte 3 | Byte 4 | |
---|---|---|---|---|
RPC Type | RPC Function ID | |||
Correlation ID | ||||
JSON Size | ||||
CRC32 checksum |
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.
Related Issues (20)
- [Withdrawn] SDL 0330 - App Service Subscription Resumption HOT 8
- [Accepted] Revise SDL-0296 Possibility to update video streaming capabilities during ignition cycle HOT 9
- [Withdrawn] Revise SDL 0280 - Adding new parameter of requiresAudioSupport and BluetoothDeviceAddress HOT 14
- [Withdrawn] SDL 0331 - Add new SDL System Structure using Mediation Application for middle/low-end class model of Powered Two Wheeler and low-cost vehicle models HOT 22
- [Accepted with Revisions] SDL 0332 - Additional Video Streaming Capabilities Validation HOT 8
- [Accepted] SDL 0333 - Handle Scenario Where no Valid Cert is Available HOT 18
- [Accepted with Revisions] SDL 0334 - Transform SetDisplayLayout requests into UI.Show for HMIs HOT 5
- [Accepted with Revisions] SDL 0335 - Limit TextField Length According to HMI Capabilities HOT 16
- [Accepted] SDL 0336 - Strict Versioning for Outgoing Core Messages HOT 16
- [Accepted] SDL 0337 - Reject PROPRIETARY/HTTP SystemRequests when PTU is not in progress HOT 3
- [Accepted] SDL 0338 - Remove Unused Files From SDL Core HOT 1
- [Accepted] SDL 0338 Revisions - Remove Unused Files From SDL Core HOT 1
- [Accepted] SDL 0339 - Replace `thread::Thread` implementation with `std::thread` from C++11 HOT 8
- [Accepted] SDL 0340 - ATF Selenium Support HOT 10
- [Accepted] SDL 0341 - Add Generic HMI Plugin Support HOT 26
- [Accepted] SDL 0342 - Remove Policy Mode Option From SDL Core HOT 6
- [Withdrawn] SDL 0343 - Add Notifications for Required App Support HOT 31
- [Accepted] SDL 0338 Revisions - Remove Unused Files From SDL Core HOT 1
- [Accepted] SDL 0344 - Use Promises in Server Logic of SDL Policy Server HOT 2
- [Accepted] SDL 0345 - Android 12 Issues HOT 26
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from sdl_evolution.