ystero-dev / hampi

Rust ASN.1 Toolkit
Other
43 stars 18 forks source link

Codec support for `REAL` type #100

Closed gabhijit closed 8 months ago

gabhijit commented 11 months ago

98 Implements support for REAL ASN.1 type in the compiler. The current codec support for the REAL type is just a todo!. Proper codec support for REAL type should be added inasn-codecs and asn-codecs-derive

gth828r commented 9 months ago

I have come to a point where I may need this in the near term. I have some cycles to work on it this week and submit a PR, but I may need some guidance to complete it in time. Do you have any pointers or suggestions on where to start?

gth828r commented 9 months ago

It looks like X.691 (PER encoding spec) references X.690 (BER/CER/DER encoding spec) in order to describe how to encode things. ASN.1 REAL values have some special values (often represented by keywords) which can be used as set values within the ASN.1 specification files:

Values in the specification can also be presented in the following forms:

The X.691 (PER) spec makes the following statement:

If the base of the abstract value is 10, then the base of the encoded value shall be 10, and if the base of the abstract value is 2 the base of the encoded value shall be 2.

I'm not sure if that means that only bases 10 and 2 are supported.

For my immediate needs, I do not need to create values in the asn.1 specification, and I only need to support standard base 10 values which are determined by the running application (i.e., not from the spec). I am not sure if the code generator currently supports any of these other values, but perhaps what I could do is create an enum for REAL values that get passed into the encoder/decoder functions so we can support handling of the special cases in the future. For now, they can be marked as todo!().

gth828r commented 9 months ago

ASN.1 REAL values have some special values (often represented by keywords) which can be used as set values within the ASN.1 specification files:

  • PLUS-INFINITY
  • MINUS-INFINITY
  • NOT-A-NUMBER
  • -0

These values are all supported by special f64 constants in standard Rust. We do not need to do anything special for these.

gabhijit commented 8 months ago

I have come to a point where I may need this in the near term. I have some cycles to work on it this week and submit a PR, but I may need some guidance to complete it in time. Do you have any pointers or suggestions on where to start?

@gth828r : Thanks for looking at it. You might want to start by implementing the codec support by starting at the encoder.

And similarly you can find the implementation for the decoder. I believe e2ap uses aper so best is to implement it for aper first (and if you believe the implementation for the uper part will be identical, then implement it inside the common encoder module and then call that in the above encode_real function). Hope, you should be able to start here.

gth828r commented 8 months ago

And similarly you can find the implementation for the decoder. I believe e2ap uses aper so best is to implement it for aper first (and if you believe the implementation for the uper part will be identical, then implement it inside the common encoder module and then call that in the above encode_real function). Hope, you should be able to start here.

Thank you, I have found those parts and started on it. While creating a unit test, I found a KPM indication message provided by an application called srsRAN which I believe to be valid, however I cannot deserialize the message. It appears that srsRAN does not include extension fields for the MeasurementLabel type (https://github.com/gabhijit/hampi/blob/master/examples/specs/e2sm/E2SM-KPM.asn#L55).

This is causing the deserialization to fail, because the current deserializer is looking for 24 optional flags (including the 3 fields after the extension marker) in the sequence header, but the serialized message (which does not serialized the extension fields) only includes 21 optional flags in its header. I do not know the spec very well yet, but is that an issue with the deserializer?

If so, I can open up a new issue. I will try to read the spec to help determine what the expected behavior is.

gabhijit commented 8 months ago
  • {314159, 10, -5}

  • {mantissa 314159, base 10, exponent -5}

  • 3.14159

Supporting this would require the support in the parser and later on in the resolver and generator module. I believe currently only the last form is supported. The above two forms can be supported by adding the support in the parse_setish_value inside the parser module in asn1-compiler.

This might be little involved as we may have to define a ResolvedReal type as an Enum of the two types one an f64 and other a struct (I am not sure whether the ResolvedReal type is defined yet. And finally when we want to write a codec, depending we might have to use some struct attrs that can be used in codecs. I believe this part is not implemented as yet as well.

Not sure how much useful above comment is, but hopefully should help in taking a look at the code.

gabhijit commented 8 months ago

This is causing the deserialization to fail, because the current deserializer is looking for 24 optional flags (including the 3 fields after the extension marker) in the sequence header, but the serialized message (which does not serialized the extension fields) only includes 21 optional flags in its header. I do not know the spec very well yet, but is that an issue with the deserializer?

Likely this is a bug in the decode implementation. Yes extension markers and fields beyond extension marker are not supported properly, but if they are not used, the encoded message should get decoded properly. It might be a good idea to create a separate issue for this - including the ASN. 1 definition (link like above is fine) and generated Rust structure and an encoded value that fails to get decoded. I will take a look at this bug separately.

gth828r commented 8 months ago

Created https://github.com/gabhijit/hampi/issues/105 to track the new issue.

gth828r commented 8 months ago

After looking at this for a bit, I think I will create something like the following:

The reasoning for this is that we can't take shortcuts for the decoder (i.e., be liberal in what you accept), but we can for the encoder. I think it is more important to get something functional now and improve performance later. If we choose to go with this approach, we should create a separate issue to add binary encoding support for the encoder. If we later choose to add support all forms of setting values in the ASN.1 spec file like we described above, then we would need to support all types of encoding. For now, we can pick and choose since we only need to encode an inputted f64.

gabhijit commented 8 months ago

@gth828r : Here are a couple of suggestions I have. Generally, most implementations would use the same form say NR3 and decimal encoded values. It's perhaps alright to have the first implementation to just support that part both on the encoders and the decoders side.

The reason being, without a proper value encoded by some third party (to which we do not have the source code available), trying to support that will be more work with very little benefit (you cannot realistically test it).

So you might want to start with what the srsRAN project is using for now for the encoded and decoder part. Later support for more values can be added.

Having said so, if you think supporting most decoders is not much of more work than supporting one decoder, feel free to go ahead and implement those.

Makes sense?

gth828r commented 8 months ago

Unfortunately, it looks like the srsRAN KPM implementation doesn't actually send any metrics as REAL values. So in the interim, I was thinking of using some example values I found in this document. It is possible that those examples are not exactly in PER encoding, but the actual REAL payload is the same between PER and the simpler encodings.

My plan is to make some more basic unit tests for each supported decoder, and also verify that the encoder matches. If that ends up being too much of a pain, I will fall back to what you are suggesting and support a single decoder.

gth828r commented 8 months ago

I have something that I think is mostly ready to go, but for the unit tests I mentioned above, they might be a bit out of place. I've run out of time to finish things up today, but I'll try to open a PR tomorrow if I can.

gth828r commented 8 months ago

Opened PR #107 for this.

gabhijit commented 8 months ago

This is available in crate version 0.6.0. Thanks @gth828r for the PR!!