Closed GoogleCodeExporter closed 9 years ago
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
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
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
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
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
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
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
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
This issue was updated by revision 7713d43bc3d4.
Original comment by Petteri.Aimonen
on 4 Jan 2015 at 5:42
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
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
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
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
@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
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
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
(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
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
@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
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
This issue was updated by revision e1c50496d9e7.
Original comment by Petteri.Aimonen
on 15 Jan 2015 at 4:58
Fix released in nanopb-0.3.2.
Original comment by Petteri.Aimonen
on 24 Jan 2015 at 3:53
Original issue reported on code.google.com by
Petteri.Aimonen
on 3 Sep 2014 at 12:28