proto3 support

[pbor]

This is an experimental first cut at adding proto3 support and I have not
tested it further than porting to proto3 the testcase. Before pushing
this further I would like to have some feedback on whether I am on
the right track.

As far as I understand protobuf-c already has pretty much everything
needed once it is built using a new version of protibuf itself.
The only missing thing is that in proto3 all fields are optional and
having to manually set has_foo is inconvenient.

This patch special cases the proto3 syntax files so that structs for the
bytes, enum and primitive fields do not emit the has_ field.

It also adds PROTOBUF_C_LABEL_NONE to the label enum that is used for
proto3 fields. When a fields has this label, the quantifier is not
consulted and instead the field is packed/unpacked depending on
whether it has a value different from NULL/0.

The patch changes the generated-code test to proto3 as a quick test,
clearly this should not be part of the final patch.

The patch also slightly refactors packing/unpacking optional fields
so that the internal functions take a boolean istead of a pointer to
a boolean: casting in the caller seems cleaner to me... this part
can be dropped or split out in its own preliminary patch.

[andygikling]

Ilya, Paolo,

What is your plan for this? I would love to see your Protobuf v3 support become mature because the Protobuf mainline is leaving microcontrollers in the dust...

Are you actively working on this? I'm going to try this code out for a bit - I'll let you know how it goes.
I'm still a little new to Protobuf but I really need Protobuf 3 on my microcontroller project(s).

Thanks,

~Andy

[pbor]

Andy, the patch should be working. Please let me know if it works for you or if you run into any problems.

I am willing to improve the patch, but I would like feedback from the maintainers

[andygikling]

Paolo,

Hey I'm having some trouble setting this up.
The directions say building protobuf-c is dependent on all sorts of Linux items such as autotools, autoconf, pkg-config etc... I've been able to follow the directions and now I have a libprotobuf output file and I've used protoc-c to generate code against my .proto file. All good so far.

However, my target is a small microcontroller with it's own C/C++ toolchain. So libprotobuf built in Ubuntu 16.04 x64 simply isn't going to link correctly when given to my microcontroller's toolchain.

I want to test this against IAR for an STM32 as well as a Microchip PIC32MZ and their xc32 free compiler. Can you give me any direction here? Basically I need to build the libprotobuf-c library with my microcontroller's toolchain...

Do we have any documentation on people able to do this? Seems silly they entire library would be dependent on Linux makefile configuration when people want to use this in embedded applications. Am I missing something? Thanks

[pbor]

Andy, I am not sure how to build on other toolchains, I guess this is orthogonal to this specific patch.

If I had to guess, I think the simplest way is to build the protoc-c compiler on a normal linux machine and generate the .c and .h file from the .proto on linux. Then on your microcontroller toolchain you only compile the generated .c and .h files and the protobuf-c.c and protobuf-c.h library files.

[andygikling]

Paolo,

I got it working. I was a little intimidated by the Linux autotools at first. But it just came down to building libprotobuf-c with my toolchain, and really that's just a single .c and .h file so, cool, I'm off to the races here.

I have tested one byte stream generated from Protobuf 3, C# in Windows and the data was correct on my microcontroller.

Let me test the link some more and I'll get back to you. I'm mostly interested in basic functionality so let me know if there are any specific things you want me to try out that are new in Protobuf 3. Thanks!

[pbor]

Great! I think the thing that needs more testing is the new proto3 "default" behavior, e.g. that booleans that are false, numbers that are 0, etc are not sent through the network

[andygikling]

Paolo,

I've been testing this for a bit and so far its working pretty well.

In my test setup I'm generating C# code from the official PB v3 release from google. I'm making a byte stream in C#, serializing it and reading it on a 32bit microcontroller with protobuf-c.

My main message has the following features:

1. Nested types under a single main message { }
2. Enums
3. All fields are not adorned with optional required etc, rather there is no keyword leading the type.
4. Int32, Double, string and bool all seem to work and initialize correctly to zero when I don't specify them before serialization.
5. There is no [ ] syntax in my proto file...

Again, these are my initial findings.

Perhaps you could provide me with a .proto file you think exercises the features of Protobuf 3 that you think should and should not work. I'd be happy to test this.

I think it would be really great if this project's wiki has a very clear table of features you can and cannot rely on in protobuf 3 with this project.

I know I was initially quite confused trying to understand how a C binding for Protobug would even work as well as what things are allowed. But at this point I'm rather pleased with this project. I think "repeated" is the last thing I really want to test before actually using this in a project.

Can we get a timeline for true, tested, Protobuf 3 support here everyone? I think this project could be very useful to people - we should try to get a baseline version that covers the important Protobuf 3 functionality,

[pbor]

Hi Andy, thanks a lot for the feedback. I do not have a proto with other things to test, but the google protobuf repo contains a conformance test they use for the languages that are included in their tree and it would be really cool to integrate that test.

Other than that I also really hope that your testing helps getting some traction on this issue: I have the exact same use case where one of the peers of the communication uses C# (that only supports v3) and the other uses C

[andygikling]

Paolo,

I'm not following your initial notes above about special casing "...proto3 syntax files so that structs for the bytes, enum and primitive fields do not emit the has_ field."

I might have spotted an issue with enums though. When I have an enum defined in my proto file, protobuf 3 forces me to start it's elements with index 0. Then, when I send a message from C# that has that enum in it, and it's value is set to the 0th element, protobuf-c unpack shows "has_myEnum" as being 0. But this is wrong, I did set the value - it's just set to the first item, 0, in the enum type. If I choose any other index in the enum and send that over the wire I see that has_myEnum correctly returns 1 though.

Is this desired? Or is this something that is wrong and should be corrected? Thank you for your time.

[pbor]

Hi Andy,

my understanding is that in proto3 setting something to 0 or not setting it are the same thing. It is up to the receiver of the message to handle the case.

[andygikling]

Paolo,

Earlier you mentioned you're using protobuf-c with this particular patch in a project correct? How then, based on your previous post, are using bools exactly? More generally, how do you deal with optional fields that need to be specifically asserted to the default (0, false, etc)? If you are using "has_" as a means to detect weather a value is available in any particular inbound message, you can't, for example, get set an optional bool back to false once it's been set true I think...

Say I have a setting on my microcontroller that is a bool type and it's a part of a message with several other data types. For example:

message {
int settingA = 2;
double settingB= 4;
bool settingC = 6;
}

When this message comes in on the micro I can use has_settingA, has_settingB, and has_settingC to detect if the message contains new data my microcontroller is supposed to consume. But the problem is "has_settingC" is always false when you try to assert "settingC = false" from C# and send it over the wire?

Overall things are working for me very well for me with this patch. But now I'm wondering, how are we supposed to deal with this scenario? Seems like either I'm totally missing something or we still have some work to do for this patch.

Your guidance, as always is much appreciated. Thanks!

[pbor]

You need to design your message differently and not rely on the ability to tell the difference between false and unset.

There are many ways to do that: you could use an additional boolean that representd "is set", you could use an enum with 3 values, you could make the boolean represent "toggle" instead of the value itself, etc.

The proto3 language guide ( https://developers.google.com/protocol-buffers/docs/proto3#default ) says:

"Note that for scalar message fields, once a message is parsed there's no way of telling whether a field was explicitly set to the default value (for example whether a boolean was set to false) or just not set at all: you should bear this in mind when defining your message types. For example, don't have a boolean that switches on some behaviour when set to false if you don't want that behaviour to also happen by default. Also note that if a scalar message field is set to its default, the value will not be serialized on the wire. "

[andygikling]

Ahh, very interesting. I can work with that - thank you for the reply.

The code is working very well on my end at this point then. What do we need to do to get these guys to pull it in? Do you know the repo owner?

[pbor]

I think the maintainers receive email for these comments on the PR, however can you add a comment on #192 saying that your tests were successful?

[edmonds]

Hi, Paolo:

I had a brief look at the patch and it looks like it's on the right track, but the modifications to t/test.proto are problematic. Could you leave t/test.proto in place so that it continues to test proto2, and add new test cases for proto3-specific functionality?

Thanks!

[pbor]

PR update to not touch test.proto, but instead add test-proto3.proto

Travis does not pass, but maybe it is because it is building against protobuf 2.6?

[coveralls]

Coverage increased (+0.2%) to 90.934% when pulling 29b2586 on pbor:wip/proto3 into 9c4c8f6 on protobuf-c:next.

[pbor]

PR updated with the following changes

• make and make check pass both when building against protobuf-2.6.0 and 3.0.X
• make distcheck requires 3.0.X
• travis config file updated to use 3.0.2 and now the checks pass

[pbor]

@edmonds , @lipnitsk sorry for pinging you, but any chance to have another look at this? The PR implements the previous requests by Robert.

[jwearing]

Hello,

I think I have found a bug in the enum generation for oneof fields. It seems to only occur when there are multiple oneofs in the same message. Here is a simple example:

message two_oneofs {
  oneof first_oneof {
    bool a = 10;
    bool b = 11;
  }

  oneof second_oneof {
    bool c = 20;
    bool d = 21;
  }
}

Compiles into:

typedef enum {
  TWO_ONEOFS__FIRST_ONEOF__NOT_SET = 0,
  TWO_ONEOFS__FIRST_ONEOF_A = 10,
  TWO_ONEOFS__FIRST_ONEOF_B = 11
    PROTOBUF_C__FORCE_ENUM_TO_BE_INT_SIZE(TWO_ONEOFS)
} TwoOneofs__FirstOneofCase;

typedef enum {
  TWO_ONEOFS__SECOND_ONEOF__NOT_SET = 0,
  TWO_ONEOFS__SECOND_ONEOF_C = 20
  TWO_ONEOFS__SECOND_ONEOF_D = 21
    PROTOBUF_C__FORCE_ENUM_TO_BE_INT_SIZE(TWO_ONEOFS)
} TwoOneofs__SecondOneofCase;

Note the lack of a comma after "20" in the second enum. When compiling, gcc gives this error:

In file included from two_oneofs.pb-c.c:9:0:
two_oneofs.pb-c.h:36:3: error: expected ‘,’ or ‘}’ before ‘TWO_ONEOFS__SECOND_ONEOF_D’
   TWO_ONEOFS__SECOND_ONEOF_D = 21
   ^

Also, the name of the last item is the same in both enums.

In file included from two_oneofs.pb-c.h:7:0,
                 from two_oneofs.pb-c.c:9:
two_oneofs.pb-c.h:37:5: error: redeclaration of enumerator ‘_TWO_ONEOFS_IS_INT_SIZE’
     PROTOBUF_C__FORCE_ENUM_TO_BE_INT_SIZE(TWO_ONEOFS)
     ^
two_oneofs.pb-c.h:30:5: note: previous definition of ‘_TWO_ONEOFS_IS_INT_SIZE’ was here
     PROTOBUF_C__FORCE_ENUM_TO_BE_INT_SIZE(TWO_ONEOFS)
     ^

It seems to work correctly on master.

[edmonds]

@pbor Thanks for pinging me. I've not had a chance to look at this but I'll try in the next week or so.

[pbor]

@jwearing that problem is present on the "next" branch, so it is not related to my patch. I think it was introduced in PR 220. Please file a sepaarate issue about it

[nacho]

@edmonds sorry to ping but any news on this?

[edmonds]

@pbor Thanks for implementing this, I've just merged this to next. Could you take a look at the FIXME on this line (c48d932#diff-9bf7bd796d81740df7bd0fb0bf6837a9R2168) and submit a fixup?