Update niRFmxNR to 23.8

[alenkani]

What does this Pull Request accomplish?
Updates niRFmxNR support to 23.8 API.

This adds the following:

Attributes
new property added :-
BANDWIDTH_PART_DC_LOCATION_KNOWN

Enums Values
name change -FREQUENCY_RANGE_RANGE2 to FREQUENCY_RANGE_RANGE2_1
new Enum
FREQUENCY_RANGE_RANGE2_2
SSB_PATTERN_CASE_F
SSB_PATTERN_CASE_G

Why should this Pull Request be merged?
https://dev.azure.com/ni/DevCentral/_workitems/edit/2458476

https://dev.azure.com/ni/DevCentral/_workitems/edit/2527171

What testing has been done?
Locally built grpc-device.

[reckenro]

You'll need to update

grpc-device/source/tests/system/nirfmxnr_driver_api_tests.cpp

Line 662 in 7d4dbb1

EXPECT_SUCCESS(session, client::set_attribute_i32(stub(), session, "", NIRFMXNR_ATTRIBUTE_FREQUENCY_RANGE, NIRFMXNR_INT32_FREQUENCY_RANGE_RANGE2));

to point to your new enum value name.

[alenkani]

NIRFMXNR_INT32_FREQUENCY_RANGE_RANGE2

update the test
EXPECT_SUCCESS(session, client::set_attribute_i32(stub(), session, "", NIRFMXNR_ATTRIBUTE_FREQUENCY_RANGE, NIRFMXNR_INT32_FREQUENCY_RANGE_RANGE2_1));

[alenkani]

@astarche i saw that you have tagged this PR as source breaking , we have same concerns as well , is there way we can support obsolete APIs or its always going be source breaking in future as well
@ThangamV-NI

[astarche]

@alenkani @ThangamV-NI @reckenro

What are you planning to do in other languages? If you're worried about it, I'm surprised to see that the C header broke source compatibility. It's so easy to alias a #define and it doesn't really have downsides. Maybe there's a problem with CVI or your codegen?

In protobuf you can have enum aliases. That's the easiest option. To do that, you just need to include both RANGE2 and RANGE2_1 in your enums.py. Aliases are discouraged in protobuf, but we allow it in grpc-device and these are already an "attribute values" enum, which has aliases.

There is a grpc mechanism to mark stuff as deprecated. We don't have a way to do that in grpc-device but it should be easy to add.

It's also possible to version the enum itself, though we don't have a mechanism to do it in grpc-device and it would be harder to add. I'm also not certain it preserves source compatibility.

But it could look something like:

enum MyEnum {
RANGE1 = 0;
RANGE2 = 1;
option deprecated = true;
}

enum MyEnum2 {
RANGE1 = 0;
RANGE2_1 = 1;
RANGE2_2 = 2;
}

message MyMessage {
oneof my_enum_enum {
MyEnum my_enum = 1; [deprecated = true]
MyEnum my_enum_2 = 2; // or whatever the next available slot is
}
}

Maybe @jasonmreding @cumitche or @bkeryan have more developed ideas for deprecating in proto. In grpc-device we pretty much do whatever the C header does.

[jasonmreding]

@alenkani @ThangamV-NI @reckenro

What are you planning to do in other languages? If you're worried about it, I'm surprised to see that the C header broke source compatibility. It's so easy to alias a #define and it doesn't really have downsides. Maybe there's a problem with CVI or your codegen?

In protobuf you can have enum aliases. That's the easiest option. To do that, you just need to include both RANGE2 and RANGE2_1 in your enums.py. Aliases are discouraged in protobuf, but we allow it in grpc-device and these are already an "attribute values" enum, which has aliases.

There is a grpc mechanism to mark stuff as deprecated. We don't have a way to do that in grpc-device but it should be easy to add.

It's also possible to version the enum itself, though we don't have a mechanism to do it in grpc-device and it would be harder to add. I'm also not certain it preserves source compatibility.

But it could look something like:

enum MyEnum {
RANGE1 = 0;
RANGE2 = 1;
option deprecated = true;
}

enum MyEnum2 {
RANGE1 = 0;
RANGE2_1 = 1;
RANGE2_2 = 2;
}

message MyMessage {
oneof my_enum_enum {
MyEnum my_enum = 1; [deprecated = true]
MyEnum my_enum_2 = 2; // or whatever the next available slot is
}
}

Maybe @jasonmreding @cumitche or @bkeryan have more developed ideas for deprecating in proto. In grpc-device we pretty much do whatever the C header does.

We don't really have any official guidance on deprecation other than this which just states mark things deprecated instead of reserved. The only other guidance is just don't break compatibility. Marking it deprecated will at least communicate that it shouldn't be used, but we haven't really defined an obsolescence policy for going from deprecated to obsolete or no longer supported. The purist policy is it would only be removed once a new major version of the API was introduced. Also, the deprecated tag is language specific as to whether it actually does anything in the generated code to warn the user about the deprecation.

I doubt enum aliasing works with the latest grpc-labview code generation now that it generated typedef enums, but I could be wrong. I doubt grpc-labview can consume most of the grpc-device proto files as they currently exist, so maybe this isn't a reason to reject this as a solution, especially if we're already using that proto feature elsewhere.

As far as defining a new enum and moving things into a oneof, there are all sorts of caveats and opinions on this, especially as it applies to individual languages. While I believe it would maintain wire compatibility, it seems like there are a number of cases where it would break source compatibility. Here is some further reading:

• Protobuf language guide
• A more in depth exploration of oneof compatibility issues
• Lacking any NI specific guidance, I usually fall back to Google AIPs as the gold standard, in this case AIP-180.

Is there a specific reason for breaking compatibility in this case or was it just an oversight?

[astarche]

Yeah, let's scrap the oneof/v2 idea. You could do a v2 on the rpc route without the oneof weirdness but let's not do that either.

I doubt enum aliasing works with the latest grpc-labview code generation now that it generated typedef enums [...]

That's a good example of why allow_aliases is not preferred in general. But in this case, we're already in an enum with aliases. So I think that's going to be our best option.

The more obvious option is to add all the fields as new values:

enum MyEnum {
RANGE1 = 0;
RANGE2 = 1; [deprecated = true]
RANGE2_1 = 2;
RANGE2_2 = 3;
}

I think that would be the default recommendation for grpc services in general. For grpc-device, we try to match the C enum values, for better-or-worse. We do support having different values via generate-mappings.

If this was a standalone enum, I might prefer that to using aliases. But this is a big attribute values enum that already has a ton of aliases. So I think it probably won't work and there's not much benefit anyway.

@jasonmreding @alenkani

[alenkani]

@astarche The generated enums.py by scrapigen doesn't include aliases for range even if its supported ,is there way to do that or its future scope to update the scrapigen
@ThangamV-NI

[alenkani]

@astarche while reviewing the proto file, I noticed that enum names are duplicated for the same set of attributes. For instance, consider the attribute:
enum AcpAveragingEnabled {
ACP_AVERAGING_ENABLED_FALSE = 0;
ACP_AVERAGING_ENABLED_TRUE = 1;
}
These enum names are also repeated inside:
enum NiRFmxNRInt32AttributeValues {
option allow_alias = true;

NIRFMXNR_INT32_ACP_AVERAGING_ENABLED_FALSE = 0;
NIRFMXNR_INT32_ACP_AVERAGING_ENABLED_TRUE = 1;
}
Is there a specific reason for this ?

[astarche]

@alenkani

I noticed that enum names are duplicated for the same set of attributes.

This is coming from the driver API not grpc-device. But it's pretty common. You have to look at the methods that use the enums/attributes and how they map to the driver API. Usually this means that there's both a "generic" attribute accessor and a "specific" attribute accessor for the same attribute.

In this case that might be:

// Specific accessor
SetAcpAveragingEnabled(ACP_AVERAGING_ENABLED_FALSE);

// Generic accessor
SetI32Atribute(ACP_AVERAGING_ENABLED, ACP_AVERAGING_ENABLED_FALSE);

In C ACP_AVERAGING_ENABLED_FALSE is a #define not a true enum. So it can be used in both callsites without anything special. protobuf does not have a #define concept so we need to separately define enums for the set of valid values for each method. For SetI32Attribute, that is all the possible values of all "enum" attributes (assuming all enums are i32s).

EDIT: that's a high level "why" it's like that. At an implementation level, the "attribute values" enums are generated by looking at all enums that are referenced by an attribute. The "top level" enums are added if there's at least one function that references it directly. So, when you see that some enums are "top level", some are "attribute values", and some are "both". The difference is how they're referenced by functions and/or attributes.

[astarche]

@alenkani

The generated enums.py by scrapigen doesn't include aliases for range even if its supported ,is there way to do that or its future scope to update the scrapigen

I'm not sure what you are saying here. In your original PR it looks like the change to RANGE_2_1 was implemented as a rename (not an alias) at all levels: C API, RF json, grpc-device metadata. Did you do something to try to make it include aliases that failed? If so, can you post a PR?

If you update the Enums.json to include the alias, I'm not certain what scrapigen would do. It might work or might require a minor change to the RFmx specific code that translates RF json. I don't know if that's considered "valid" RFmx metadata. But if you care about preserving compatibility in this way (in all languages: C API, etc), then it seems like you need some way to represent it. OR don't rename enum values.

You can also write a custom transform in scrapigen to add the value: https://github.com/ni/grpc-device-scrapigen/blob/main/src/scrapigen/rfmxnr/transformenums.py

I believe that would work, or at least it will almost work. I don't know of a case that is exactly like this, but there are definitely similar cases.