Add support for the 'oneof' feature

[GoogleCodeExporter]

Protobuf 2.6 adds a 'oneof' keyword for expression union messages.

Perhaps this could finally give a good API for using union messages. Just have 
to figure out of how to express it in C, maybe map to union {}.

Original issue reported on code.google.com by Petteri.Aimonen on 3 Sep 2014 at 12:28

[GoogleCodeExporter]

At this moment oneof is still working in nanopb as all items were optional. Is 
it true?. At this moment I have no problems by encoding a oneof message with 
nanopb by setting only one field. Google Protocol Buffers in C++ seems to 
decode the information correctly.

Original comment by alvarolb@gmail.com on 10 Nov 2014 at 2:53

[GoogleCodeExporter]

Yes, they are compatible on the wire.

However the space savings could be very useful for the applications where 
nanopb is usually used.

Original comment by Petteri.Aimonen on 10 Nov 2014 at 3:18

[GoogleCodeExporter]

Ok, I understand. It will be a great advantage when defining messages with 
several fields, but only one can be initialized (is my case). A union in this 
case seems to be the direct approach, is it hard to implement?

I forgot to mention. Very good work with nanopb!

Original comment by alvarolb@gmail.com on 12 Nov 2014 at 8:37

[GoogleCodeExporter]

It shouldn't be too difficult to implement. Mostly needs changes to pb_common.c 
to not advance the pData for union fields. Not sure how to best indicate an 
union field.
One straightforward way would be to add PB_ATYPE_ONEOF.

pSize should probably point to a pb_size_t field common to the whole oneof, 
which will store the tag number of the actual field that was there.

Original comment by Petteri.Aimonen on 13 Nov 2014 at 12:00

[GoogleCodeExporter]

Maybe a problem is how to establish the decoding callbacks when you have in the 
same oneof strings and bytes, or submessages with repeated fields...

Original comment by alvarolb@gmail.com on 16 Nov 2014 at 2:30

[GoogleCodeExporter]

Not really. Each partial field in oneof will still have its own pb_field_t 
entry. So field->tag in a callback should tell the tag number of the field that 
was present.

Original comment by Petteri.Aimonen on 16 Nov 2014 at 3:44

[GoogleCodeExporter]

Hello Guys. How is it going with implementation of oneof -> union mapping? Does 
anyone working on that? 

I just discoverer NanoPB, great implementation! We are developing hard realtime 
systems and are very limited in what libraries to use. Code generated by NanoPB 
works prefect. We plan to use it the next version of our software. 

Oneof mapping to union could be a great feature. If you need help with 
implementation, please let me know.

best regards,
Alexey

Original comment by Alexey.Z...@gmail.com on 26 Nov 2014 at 1:50

[GoogleCodeExporter]

Help is always welcome :)

I'll probably get around to doing this some time in coming months, but cannot 
promise anything. If you decide to work on it, feel free to drop a note or ask 
questions (doing so in this issue report is fine).

As part of this I'll have to update the windows/mac os x binary build servers 
to protobuf 2.6; But that is something only I can do, and only matters for 
getting it into a release.

Original comment by Petteri.Aimonen on 26 Nov 2014 at 6:29

[GoogleCodeExporter]

This issue was updated by revision 7713d43bc3d4.

Original comment by Petteri.Aimonen on 4 Jan 2015 at 5:42

• Changed state: Started

[GoogleCodeExporter]

Newest git revision now has a (supposedly) working support for oneof fields.

It still needs more test cases before release.

Original comment by Petteri.Aimonen on 4 Jan 2015 at 5:42

[GoogleCodeExporter]

Good news!! I have tried the new code but the compiler didn't work for me. The 
error is the following:

Traceback (most recent call last):
  File "nanopb/generator/nanopb_generator.py", line 1335, in <module>
    main_cli()
  File "nanopb/generator/nanopb_generator.py", line 1285, in main_cli
    results = process_file(filename, None, options)
  File "nanopb/generator/nanopb_generator.py", line 1240, in process_file
    enums, messages, extensions = parse_file(fdesc, file_options)
  File "nanopb/generator/nanopb_generator.py", line 835, in parse_file
    messages.append(Message(names, message, message_options))
  File "nanopb/generator/nanopb_generator.py", line 669, in __init__
    self.oneofs[f.oneof_index].add_field(field)
  File "nanopb/generator/nanopb_generator.py", line 593, in add_field
    + " (field %s)" % field.fullname)
AttributeError: Field instance has no attribute 'fullname'

that is the following line:          

raise Exception("Callback fields inside of oneof are not supported"
                            + " (field %s)" % field.fullname)

If i remove the fullname, then the exception is the following:

Exception: Callback fields inside of oneof are not supported (field     
pb_callback_t bytes;)

Does it means that bytes cannot be part of a oneof structure?

Original comment by alvarolb@gmail.com on 5 Jan 2015 at 12:04

[GoogleCodeExporter]

Bytes fields can be inside oneof, as long as you give a max_size for them.

Callback fields (pb_callback_t) cannot be, because they wouldn't really work 
for decoding (you'd have to know beforehand which callback to set, because only 
one can be set for unions).

Original comment by Petteri.Aimonen on 5 Jan 2015 at 6:13

[GoogleCodeExporter]

I have a .proto file using oneof that was working fine with the latest stable 
nanopb version, and also with google 2.6.0. The newest git version raises the 
exception commented above. Does it means that the newest nanopb will not be 
fully compatible with oneof feature?

Original comment by alvarolb@gmail.com on 5 Jan 2015 at 10:46

[GoogleCodeExporter]

@alvarolb: I'll add a setting for generating oneofs as regular fields (like 
nanopb 0.3.1 does). The issue is not about being "not compatible", just that 
callback fields inside C union are not possible.

Original comment by Petteri.Aimonen on 5 Jan 2015 at 10:51

[GoogleCodeExporter]

Then this problem is what I commented in #5 :)

Maybe all callbacks from a oneof structure can be wrapped inside an structure 
inside the union, so all the decoding callbacks can be set...

Original comment by alvarolb@gmail.com on 6 Jan 2015 at 12:15

[GoogleCodeExporter]

Ah, yeah, didn't understand it back then. Even if one wraps the callbacks 
inside the union, a field could overwrite them and then another field would 
call an overwritten callback (i.e. a corrupted message would trigger security 
bug).

Besides, I think most common use case is to have submessages inside the oneof.

Original comment by Petteri.Aimonen on 6 Jan 2015 at 7:00

[GoogleCodeExporter]

(Note that Google's protobuf does not have 'callback' type fields, so that is 
why this problem doesn't occur there. I guess we'll just have to accept the 
fact that only static/pointer fields can be placed inside union.)

Original comment by Petteri.Aimonen on 6 Jan 2015 at 7:09

[GoogleCodeExporter]

Wow, you already implemented Oneofs, great!

I tried to generate code for following message:
message SampleMessage {
  required int32 frameCounter = 1;
  oneof test_oneof {
     int32  iVal = 2;
     double dVal = 3;
     bool   bVal = 4;      
  }
}

But got following error:
Traceback (most recent call last):
  File "/home/alexey/workspace/nanopb/generator/nanopb_generator.py", line 1333, in <module>
    main_plugin()
  File "/home/alexey/workspace/nanopb/generator/nanopb_generator.py", line 1318, in main_plugin
    results = process_file(filename, fdesc, options)
  File "/home/alexey/workspace/nanopb/generator/nanopb_generator.py", line 1257, in process_file
    messages, extensions, options))
  File "/home/alexey/workspace/nanopb/generator/nanopb_generator.py", line 1039, in generate_source
    worst_field = str(field.struct_name) + '.' + str(field.name)
AttributeError: OneOf instance has no attribute 'struct_name'
--nanopb_out: protoc-gen-nanopb: Plugin failed with status code 1.

Looks like struct_name is missing in OneOf class.

best regards,
Alexey

Original comment by Alexey.Z...@gmail.com on 7 Jan 2015 at 12:23

[GoogleCodeExporter]

@Alexey.Zakharov: Thanks, should be fixed now.

Remaining issues before release:
- Add option to generate oneofs as regular fields.
- Releasing of memory for pointer type fields.
- Avoiding memory leaks when overwriting pointer type fields inside unions.
- Add oneofs to atleast mem_release and alltypes test cases.

Original comment by Petteri.Aimonen on 7 Jan 2015 at 5:01

[GoogleCodeExporter]

Pushed a few new fixes:
- Added option 'no_unions' to generate oneofs as regular fields like in 0.3.1.
- Memory release for pointers inside oneofs.
- Added oneofs to AllTypes test case

Still needed:
- Avoiding memory leaks when overwriting pointer type fields inside unions.
- Add oneofs to mem_release test case.

Original comment by Petteri.Aimonen on 11 Jan 2015 at 5:51

[GoogleCodeExporter]

This issue was updated by revision e1c50496d9e7.

Original comment by Petteri.Aimonen on 15 Jan 2015 at 4:58

• Changed state: FixedInGit

[GoogleCodeExporter]

Fix released in nanopb-0.3.2.

Original comment by Petteri.Aimonen on 24 Jan 2015 at 3:53

• Changed state: FixReleased