OpenCyphal / nunavut

Generate code from DSDL using PyDSDL and Jinja2
https://nunavut.readthedocs.io/
Other
39 stars 24 forks source link

Generate aliases that resolve to the latest minor version of the data type #193

Open Cherish-forever opened 3 years ago

Cherish-forever commented 3 years ago

I think nunavut generate source file without version, so that user application will don`t care about protocol version. user application include by different protocol version header file to change protocol version. application code will use the variable or struct or api without version.

pavel-kirienko commented 3 years ago

My own experience also indicates that it would be valuable to be able to omit the minor version number in some cases. Dropping the major version may cause nasty consequences since there is no wire compatibility guarantee across different major versions, but the minor one is usually safe to omit making the implementation default to the latest one. This idea does not need to be limited to C only, of course.

However, if we did it the way you described, C/C++ applications that leverage different minor versions of a data type would violate the ODR since there would be different definitions under the same name available in different files. A robust solution is to make Nunavut emit an alias for every major version of every data type that simply resolves to the latest minor version; such alias necessarily has to be placed in a separate file.

For example, if you have Type.1.1, Type.1.2, and Type.2.0, Nunavut would produce the following outputs (assume C in this case but it holds for other targets):

@thirtytwobits do you support this? @Cherish-forever would you like to work on this?

Cherish-forever commented 3 years ago

@pavel-kirienko Keep major is better.

pavel-kirienko commented 3 years ago

@thirtytwobits if you greenlighted this we could ask @Cherish-forever to help us implement it.

thirtytwobits commented 3 years ago

@Cherish-forever and @pavel-kirienko , yeah. I think this makes a lot of sense.

pavel-kirienko commented 3 years ago

So @Cherish-forever would you like to work on this?

silverv commented 2 years ago

Just to clarify, work for this is done in Python but needs to be implemented here for C and C++.

pavel-kirienko commented 1 year ago

@thirtytwobits How would you go about implementing this for C++? We need to extend the list of data types for which we generate outputs with aliases. Given a list of DSDL types that contains ns.Type.1.0, we will need to generate not only ns/Type_1_0.hpp but also ns/Type_1.hpp. This differs from the implementation for Python because in Python you can define the aliases inside __init__.py by providing a custom Namespace.j2, which is a lot easier. Does it look to you that implementing this for C++ requires modifications to the Nunavut core?

This issue is kinda important right now because the high-level interfaces in libcyphal are going to depend on DSDL-generated types, and we don't want them to depend on a specific minor version (at least not always).

thirtytwobits commented 1 year ago

We could overload the namespace concept such that, when enabled in C++ generation, we generate ns/Type_{major}.h that is simply using directives:

// Example of Type_1.h

#include "ns/Type_1_1.h"

namespace ns
{
using Type_1 = ns::Type_1_1;
}

Is this what we're looking for here?

pavel-kirienko commented 1 year ago

Yes.

thirtytwobits commented 1 year ago

(for C++ and C this should be the same. I'll discuss c++ here)

In src/nunavut/lang/properties.yaml under the cpp configuration you'll see two keys:

    namespace_file_stem: _namespace_
    has_standard_namespace_files: false

Setting has_standard_namespace_files to true will cause the generator to fail because there is no C++ template for this. If you touch src/nunavut/lang/cpp/templates/Namespace.j2 and try again it will work by emitting _namespace_.hpp files for each folder it creates. You could then develop this template (and rename the stem... let's just call it MajorTypes) to have aliases for all major types in a given namespace:

    namespace_file_stem: MajorType
    has_standard_namespace_files: true
// example of uavcan/diagnostic/MajorTypes.hpp

#include "uavcan/diagnostic/Record_1_1.hpp"
#include "uavcan/diagnostic/Severity_1_0.hpp"

namespace uavcan
{
namespace diagnostic
{
using Record_1 = Record_1_1;
using Severity_1 = Severity_1_0;
}
}
// example use:
#include "uavcan/diagnostic/MajorTypes.hpp"

...
auto latest_record = new uavcan::diagnostic::Record_1();

This is a bit ugly, I admit. The alternative would require changes to the core to enable a "generate latest minor of major only" where the generator itself would only emit (for our example) uavcan/diagnostic/Record_1.hpp and uavcan/diagnostic/Severity_1.hpp under the uavcan/diagnostic namespace. The types in here would be named by their major and minor version but would come with the alias suggested by the header name.

Thoughts?

pavel-kirienko commented 1 year ago

This is a bit ugly, I admit. The alternative would require changes to the core to enable a "generate latest minor of major only"

I think this is superior to the namespace header (MajorTypes.hpp) approach because, for large namespaces, dependency of the namespace header on all types in its namespace may be a problem.