Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for REAL type to PER codecs #107

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

gth828r
Copy link
Contributor

@gth828r gth828r commented Nov 1, 2023

  • For encode, support all special values and encode standard values as base 10 ISO 6093 NR3 for now.
  • For decode, support:
    • All special values
    • All binary encoded values (base 2, 8, or 16)
    • All base 10 encoded values (ISO 6093 NR1, NR2, and NR3)
  • Future work: Add support for encoding as binary, as well as NR1 and NR2. Selection needs to be based on some additional inputs.

Addresses #100

@gth828r
Copy link
Contributor Author

gth828r commented Nov 1, 2023

Two things I am not so sure about:

  1. Am I doing the alignment portion of this correctly? I believe UPER and APER REAL encoding is identical, and all I am doing for now is that when I create/consume the length parameter at the beginning of the value, I pass the alignment parameter as true for APER and false for UPER. Hopefully nothing else is needed.
  2. I added new unit tests directly in the common/encode/mod.rs and common/decode/mod.rs files, and there were no unit tests in those files before. If those should be moved elsewhere or restructured somehow, let me know.

I was not able to find a known working KPM serialization from srsRAN that includes REAL values, otherwise I would add that to the KPM test file.

Copy link
Collaborator

@gabhijit gabhijit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly nitpicks, nothing major. Just changing cause to Generic at most places and some other minor changes.

codecs/src/per/common/decode/decode_internal.rs Outdated Show resolved Hide resolved
codecs/src/per/common/decode/decode_internal.rs Outdated Show resolved Hide resolved
codecs/src/per/common/decode/decode_internal.rs Outdated Show resolved Hide resolved
codecs/src/per/common/decode/decode_internal.rs Outdated Show resolved Hide resolved
codecs/src/per/common/decode/decode_internal.rs Outdated Show resolved Hide resolved
codecs/src/per/common/decode/mod.rs Outdated Show resolved Hide resolved
codecs/src/per/common/decode/mod.rs Show resolved Hide resolved
@gabhijit
Copy link
Collaborator

gabhijit commented Nov 1, 2023

@gth828r Sorry I missed these questions

  1. Am I doing the alignment portion of this correctly? I believe UPER and APER REAL encoding is identical, and all I am doing for now is that when I create/consume the length parameter at the beginning of the value, I pass the alignment parameter as true for APER and false for UPER. Hopefully nothing else is needed.

Yes, passing true for aligned should work in most of the cases.This looks to be correct. TBH: I am never sure of an implementation unless there's a test data with which we can test. At-least for the aligned case we can test. For non-aligned, we may need to wait for someone needing to use REAL codec in that context and actually filing the bug if it is wrong! :-).

2. I added new unit tests directly in the `common/encode/mod.rs` and `common/decode/mod.rs` files, and there were no unit tests in those files before. If those should be moved elsewhere or restructured somehow, let me know.

This is fine. Well unit testing has not been as good for the codecs part in general, so if required we can revisit that. Since we are at it already what you can do is create a module called tests and move the tests there and that module is included with #[cfg(test)]. (edit: strike-out the comment)

@gth828r
Copy link
Contributor Author

gth828r commented Nov 1, 2023

I should be able to address these and update the PR tomorrow morning KST. Sorry for the delay!

@gabhijit
Copy link
Collaborator

gabhijit commented Nov 1, 2023

I should be able to address these and update the PR tomorrow morning KST. Sorry for the delay!

No issues, take your time! Thanks for the PR!

 * For encode, support all special values and encode standard values as
   base 10 ISO 6093 NR3 for now.
 * For decode, support:
   * All special values
   * All binary encoded values (base 2, 8, or 16)
   * All base 10 encoded values (ISO 6093 NR1, NR2, and NR3)
 * Future work: Add support for encoding as binary, as well as NR1 and
   NR2. Selection needs to be based on some additional inputs.

Addresses ystero-dev#100
@gth828r gth828r force-pushed the 100.real-type-codec-support branch from 490f55f to abe67fb Compare November 2, 2023 01:57
@gabhijit gabhijit self-requested a review November 2, 2023 03:17
@gabhijit gabhijit merged commit 09fa85d into ystero-dev:master Nov 2, 2023
2 checks passed
@gth828r gth828r deleted the 100.real-type-codec-support branch November 10, 2023 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants