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

APER support #125

Open
wants to merge 35 commits into
base: master
Choose a base branch
from
Open

APER support #125

wants to merge 35 commits into from

Conversation

ikarso
Copy link

@ikarso ikarso commented Oct 31, 2016

Most of the code are a copy of UPER. Decoding is verified against H225/h323.

@brchiu
Copy link
Contributor

brchiu commented Oct 31, 2016

Just wondering, is this pull request same as Aligned PER support part of #115 ?

@ikarso
Copy link
Author

ikarso commented Dec 2, 2016

Hi @brchiu
No, its two different implementations of APER support. Most of the code are probably the same i.e. a copy of UPER version with only a small change of call TYPE_[encode|decode]uper to TYPE[encode|decode]aper. I assume some of the basic function differ more for example per*.c and INTEGER.c.

From a encode/decode APER messages point of view #115 and #125 are hopefully the same :)

@mouse07410
Copy link

@ikarso and @persandstrom, I've noticed that this PR is not quite compatible with PR #129, as the latter introduces larger constraints than this one supports.

It seems that the best way to address this would be for @ikarso to update this PR to handle larger constraints.

Comments?

@mouse07410
Copy link

Also, @ikarso, shouldn't the command line flags be updated to accommodate APER vs UPER?

@mouse07410
Copy link

I've "made peace" between this PR and #129. It's in my fork now. Maybe more validation tests are needed to better exercise both APER and longer constraints, but I leave that part to you, @ikarso and @persandstrom.

@persandstrom
Copy link

Ok, thanks @mouse07410

@ikarso
Copy link
Author

ikarso commented Jan 29, 2017

@mouse07410 Do you mean in the converter-sample.c?
For now -iper/-oper do the unaligned version (as before) and -iaper/-oaper for the aligned.
Do you suggest replaceing -iper/-oper with -iuper/-ouper?

I'm not sure that the best path is to merge many PR. Of course i can merge #129 and #99 (from discussion on #115) if it would be helpful. But i rather see a new PR/branch/fork/.. with combined PRs than to extend #125 .
@vlm Can you give a hint what you prefer?

@mouse07410
Copy link

@mouse07410 Do you mean in the converter-sample.c?

No, I mean that both PRs (#125 and #129) are applied to my fork of asn1c, and the disagreements between them have been fixed.

For now -iper/-oper do the unaligned version (as before) and -iaper/-oaper for the aligned.
Do you suggest replaceing -iper/-oper with -iuper/-ouper?

No I do not, because I couldn't care less for this cosmetic issue.

I'm not sure that the best path is to merge many PR.

Well, right now #125 and #129 have been merged in my fork.

Of course i can merge #129 and #99 (from discussion on #115) if it would be helpful.

It would probably be nice, though if #99 matters, I'd rather see it on top of the already-working stuff (like my fork :).

But i rather see a new PR/branch/fork/.. with combined PRs than to extend #125 .

As I said, there is a new fork https://github.com/mouse07410/asn1c.git with most of the outstanding PRs applied and merged when there were inconsistencies between them. The fastest way to get #99 into a working asn1c code is IMHO to submit a PR there.

@vlm Can you give a hint what you prefer?

I know that I would prefer for @vlm to merge those PRs, so I don't need to maintain a fork. But since it doesn't seem to be coming any time soon, I took that upon myself.

You're welcome to either extend #125 with #99 (minding that in my fork #125 has been adjusted to accommodate for #129), or figure a better path if you think it exists.

mouse07410 referenced this pull request in mouse07410/asn1c Jan 31, 2017
@mouse07410
Copy link

@ikarso your update has been merged, thank you! Perhaps you could add a clean version of #99? ;-)

@mouse07410
Copy link

The merge has been completed. #99 is in the master branch of my fork, all the CI tests pass.

@sy2017
Copy link

sy2017 commented Feb 28, 2020

when i use the aper support code to decode a VisibleString like this 05 56 31 2E 30 30,code 05 is the size,i find the decode function OCTET_STRING_decode_aper using 7 bits (range_bits) as a boundary for each character ,i wonder if this treatment matchs the rule of APER,it should be 8bits as a boundary in a VisibleString for APER? @ikarso

@CLAassistant
Copy link

CLAassistant commented May 15, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ vlm
❌ ikarso
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

7 participants