search

osrf / dynamic_message_introspection

Apache License 2.0
33 stars 19 forks source link

Add support for C++ messages, make conversion symmetrical, and refactor #15

Closed christophebedard closed 2 years ago

christophebedard commented 2 years ago

See https://discourse.ros.org/t/ros-2-type-support-introspection-and-conversion-of-c-c-messages-to-from-yaml/22295

Signed-off-by: Christophe Bedard bedard.christophe@gmail.com

gbiggs commented 2 years ago

I don't quite understand the inclusion of both C and C++ namespaces, when there doesn't appear to be much difference between the types used in each.

christophebedard commented 2 years ago

I don't quite understand the inclusion of both C and C++ namespaces, when there doesn't appear to be much difference between the types used in each.

The underlying functions do things differently: the message (bytes) <--> YAML conversion isn't the same for C and C++ messages. That's why I used two different namespaces with roughly the same function names. Then, on top of this, I wanted to provide extern "C" functions so that C messages could be converted from C directly without the caller needing C++. std::string being used in the extern "C" functions is clearly an oversight (showing that I don't test this "functionality"), so I'll fix it.

Does this make sense?

christophebedard commented 2 years ago

std::string being used in the extern "C" functions is clearly an oversight (showing that I don't test this "functionality"), so I'll fix it.

Well it seems like I didn't really think it through back then. Actually supporting using dynmsg from C would require a few more changes, so I just removed most of the extern "C" functions: 75fd6b88dbcb2cc5110e72f25a827bfe8e8597f1. We can add this C functionality later on in another PR if it's actually needed.

gbiggs commented 2 years ago

I'm seeing a huge number of test failures. @christophebedard could you check them out?

christophebedard commented 2 years ago

I'm seeing a huge number of test failures. @christophebedard could you check them out?

@gbiggs they were due to the change & workaround I used, namely removing the type/value split in the YAML structure and working around yaml-cpp's limitation with int8_t/uint8_t. I updated the tests: abe0bb21513ee709a07653fb5e4635d00a87e8ca

gbiggs commented 2 years ago

This looks good to me. In it goes!

Thanks for the contribution!