Sdl_evolution: [Accepted] SDL 0052 - Constructed Payloads

[theresalech]

Hello SDL community,

The review of the revised proposal "SDL 0052 - Constructed Payloads" begins now and runs through May 9, 2017. The original review of "Constructed Payloads" occurred April 18 through April 25, 2017. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0052-constructed-payloads.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/157

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
[email protected]

[joeljfischer]

I think there are a few points to be addressed:

1. We still have to support previous versions, so we would essentially have two totally separate protocol packet parsing paths? Say that 10 times fast.

2. In your example you place video frame size / frame rate in the Start Service negotiation. But wouldn't this be better as an RPC pair? Would this proposal include these items as concretely accepted changes to the protocol spec?

3. You say:

   The definitions of the payloads will be defined by the protocol spec. However, all pieces of the payload will be considered optional. This allows us to add additional tags to the to the payloads without having to up the spec.

   But adding new tags still requires upping the spec, even if they're optional. All API additions are minor version changes. Since we don't understand minor changes, wouldn't we still have to up the spec for versioning purposes? e.g. To say, v.5.2.0 of the iOS library supports v.5 of the protocol spec loses some semantic meaning when we add new APIs without upping the spec version number.

4. Why is the tag section 2-128 bytes, yet the length is 2-129 bytes? Additionally, why was 128 / 129 chosen? How large are we expecting these tags or lengths to be? Is this a raw value? Unsigned, I would assume?

5. We should probably enforce a String type. I would think ASCII would be best for our purposes, though it is less future-proof. But do people really want to put emoji in their tag payloads? 😝

6. Is subtags a part of this proposal or an alternative? I believe the proposal should argue for one way or the other, with the alternative listed in the appropriate section, the way it currently is, if it's ever to be accepted, it would _have_ to be accepted with revision removing the optional section tag or removing it altogether.

7. The subtags section is pretty tough to parse, can you provide more detail? e.g. Explaining how subtags are recognized by the parser within the payload (as opposed to being actual data) would be helpful.

[joeygrover]

1. Yes. We would say anything pre v5 protocol uses previous parsing method. That code already exists, so it's just refactoring into the ability to handle this.
2. This was merely an example, however, I believe capabilities would be best for RPCs, however, when actually trying to start a service I believe this would be the best place to negotiate so we don't have to mix RPCs with other services. Again, this was just an example and not tied to this proposal.
3. This is something that can be changed when accepting this proposal. If the steering committee thinks it is unwise, it can be changed before final approval. I believe the mobile and core libraries will always have to be prepared to deal with situations where certain tags don't exist or aren't sent, so in doing so, they will be able to handle adding new tags gracefully. Since we don't have a minor version in the protocol, we would quickly run out of versions if we did tie every addition/change directly to the protocol version. We could implement a new "control packet version" but that doesn't sound great.

4. This was a typo, but also there is a clarification is needed. Each item in the payload would have a tag, length, and data section. The tag and length items are special in terms of of being of at least 2 bytes, and no more than 129 bytes. The first byte of each of these items is for the length of the data for that particular item. Byte 2 to 129 will be the actual value of the item. A picture is helpful here.
   

5. I'm fine with ASCII.

6. & 7. I think subtag should be part of this proposal. Each control packet has the necessary context to know which tag might have subtags and can parse as needed. If the steering committee doesn't agree, this obviously could be removed before final approval.

[shoamano83]

Hi @joeygrover, a quick question. I understand that "video frame resolution" is just an example and is not included in this proposal. Do you plan to create another proposal for it?
(I know this question is not tied to the proposal itself, please excuse me for that.)

[shoamano83]

Generally I'm positive to the idea. There are some cases where we want to convey additional information when starting a video or audio streaming, such as video resolution. If we use a RPC messaging for them, SDL proxy and core need to handle two sequential events (i.e. an RPC message then StartService). So to me, extending StartService message looks simpler. The downside compared to adding new RPC is that we need a bump in protocol version.

Regarding the structure, is there any specific reason that we need to invent our own format rather than using an open serialization format? I think using an open format has many benefits; we can reuse existing implementations as long as the license permits, many developers should be familiar with the format, etc.
For example, I think we can use protobuf (https://github.com/google/protobuf). It's cross-platform, it's BSD 3-clause license, and it supports integers/float/double/bool/string (ASCII and UTF-8)/enums/nested types/raw bytes so I think everything we need is already there.

[joeygrover]

Hi @shoamano83, I don't think this proposal should include the actual use cases. I believe the stated use cases are necessary though, so if this gets accepted I will open another proposal that includes a first pass at constructed payload implementations.

Overall I'm not opposed to using protocol buffers, it is a robust solution. My intent was to keep this portion of the project lightweight and simple. This solution makes sure we don't abuse the ability to send information through control packets. It could be a very slippery-slope if we decide one day that we no longer want to use RPC messages and only want a new protocol buffer service, then we end up with doubling the size of our libraries to accommodate legacy implementations; though that's just opinion.

I also always hesitate to bring in 3rd party libraries unless their benefits far outweighs their risks. Essentially any 3rd party library that we bring in we are saying we will support for the lifetime of this project. Furthermore, we are currently in the process of getting some language clarified in the SDLC bylaws that states we actually can't bring in any 3rd party libraries.

[shoamano83]

Hi @joeygrover, thank you for your reply.

I don't think this proposal should include the actual use cases.

I understand and agree.

This solution makes sure we don't abuse the ability to send information through control packets. It could be a very slippery-slope if we decide one day that we no longer want to use RPC messages and only want a new protocol buffer service, then we end up with doubling the size of our libraries to accommodate legacy implementations; though that's just opinion.

I understand your concern, but I don't think we need to worry about it. Control Frames have meaning by their own (like "Start Service") so they are not suitable for generic usage like RPC.

Furthermore, we are currently in the process of getting some language clarified in the SDLC bylaws that states we actually can't bring in any 3rd party libraries.

I don't know about the 3rd-party library issue you mentioned. But even if we cannot use a 3rd-party library, I think we can still pick up an open format and implement it (or subset of it) by our own.
What I wanted to say was that we should consider using open serializer/deserializer format (like protobuf, JSON, BSON, ...) first, and only if none of them suits our needs, then we should invent our own. For me, the proposed format looked similar to protobuf, so I just wondered why we need to create our own format from scratch.

Thanks.

[theresalech]

@shoamano83 and @joeygrover please see below for legal information on using third-party content within SDL, per the Contributors License Agreement (attached).

We allow contributions subject to the BSD 3-clause license (Paragraph D), because that’s the license we’re using on the overall project code. Fortunately, protobuf is BSD 3-clause, so incorporating this into the SDL project is OK, provided we maintain the original protobuf BSD 3-clause license (with the author/copyright date) in the protobuf source code.

SDLC Contributor License Agreement 2-23-17.docx

[kshala-ford]

To my understanding this proposal is about allowing packets payload to be (re)structured. The proposed solution is just one option of how to (re)structure the payload.

shoamano83 wrote:

What I wanted to say was that we should consider using open serializer/deserializer format (like protobuf, JSON, BSON, ...) first, and only if none of them suits our needs, then we should invent our own. For me, the proposed format looked similar to protobuf, so I just wondered why we need to create our own format from scratch.

I agree to @shoamano83 to consider the use of other well-known formats. Those formats (and their specification) should be examined in terms of how they fit our needs/wishes. Furthermore existing implementations (and their license) should be reviewed of how they fit into our software and how they match our license agreement. If none of them fit our needs then I would advise to _reinvent the wheel_.

Personally I would prefer BSON rather than protobuf as BSON it's much more lightweight and doesn't require schemas. BSON is much closer to a common CPU (little endian) and would beform very efficiently. At the end it specifies the use of data types like IEEE754 floating point and UTF8 (which I highly recommend instead of ASCII).

I don't want to advertise BSON. I just want to point out that there are formats out there where people already thought out and solved many things.

[madhuy]

Attaching some of the alternative open formats with its Pros and cons.

[joeljfischer]

@madhuy I think regarding BSON, the license would depend on the library, as the protocol itself is under CC Zero, which is essentially public domain (https://creativecommons.org/publicdomain/zero/1.0/).

[joeygrover]

We should also consider the actual size of data we are sending across for each of the solutions. JSON, for example, obviously has a lot of overhead.

If we were to implement our own library/code to handle one of the formats the benefits would also be reduced for that format. We would have to implement and maintain our own version of that format anyways.

[theresalech]

The Steering Committee has decided they'd like to move forward with using BSON as opposed to a custom solution for the addition of constructed payloads for non-RPC type packets. Reasons for using BSON are included within the comments on this review, but overall, the Steering Committee agreed that BSON is relatively easy to adopt and already has a mature spec.

They've decided that this proposal should be deferred and revised to describe using BSON. Once the proposal has been revised, it will be reviewed again by the Steering Committee.

[shoamano83]

Hi @joeygrover,
Thanks for updating the proposal. In the example, I think the field value should be 4 bytes (i.e. '\x00\x04\x00\x00').
I had a quick try with Python:

>>> import bson
>>> dump = bson.dumps({"MTU":1024})
>>> dump
'\x0e\x00\x00\x00\x10MTU\x00\x00\x04\x00\x00\x00'
>>> example = '\x0E\x00\x00\x00\x10MTU\x00\x04\x00\x00\x00\x00\x04\x00\x00\x00'
>>> output = bson.loads(example)
>>> output
{u'MTU': 4}
>>> example2 = '\x0E\x00\x00\x00\x10MTU\x00\x00\x04\x00\x00\x00'
>>> output2 = bson.loads(example2)
>>> output2
{u'MTU': 1024}

Other than the field value, it looks fine to me.

[theresalech]

This proposal has been revised to include BSON, as requested by the Steering Committee, and has moved into review. The Steering Committee will vote on this proposal during the May 9, 2017 meeting.

[joeljfischer]

I have no opposition to this proposal other than the obvious note that this would likely mean we need to either 1. fork a BSON library on iOS, Android, and C++ for our own use, or 2. create such from scratch. Any fork would likely require retaining the license and to provide attribution, and to require any implementer of SDL to provide attribution as well.

[theresalech]

As this proposal has now been revised to include BSON, the Steering Committee has agreed to accept this proposal.

[theresalech]

Issues Entered:
Android
iOS
Core
Protocol