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

Merge APER implementation from mouse07410's repository #226

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

Conversation

brchiu
Copy link
Contributor

@brchiu brchiu commented Oct 22, 2017

Merge APER implementation from @mouse07410's repository.

Due to there is no APER encoder and decoder for OPEN TYPE, so s1ap-dump built is still not able to decode S1AP messages.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 71.03% when pulling bdcf1a5 on brchiu:merge_mouse07410_aper_implementation into 9f470d6 on vlm:master.

@brchiu brchiu force-pushed the merge_mouse07410_aper_implementation branch from bdcf1a5 to e129848 Compare October 24, 2017 07:32
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 71.03% when pulling e129848 on brchiu:merge_mouse07410_aper_implementation into cb59ce6 on vlm:master.

@brchiu brchiu force-pushed the merge_mouse07410_aper_implementation branch 2 times, most recently from 3c016d1 to 70dcada Compare October 26, 2017 08:22
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 71.068% when pulling 70dcada on brchiu:merge_mouse07410_aper_implementation into 5e73c50 on vlm:master.

@brchiu
Copy link
Contributor Author

brchiu commented Oct 26, 2017

Perform make check under examples/sample.source.J2735 has the following error message :

Recoding sample-MessageFrame-1.der (MessageFrame) into XER and back (1)...
./.tmp.1.28180: Decode failed past byte 5673: Unexpected end of input
Makefile:26: recipe for target 'check-ber' failed
make: *** [check-ber] Error 3

The failure seems comes from one extra byte 0x0A added at the end of encoded XER file.
But I can't figure out why.

@mouse07410
Copy link

mouse07410 commented Oct 26, 2017

Hmm... 0x0A is an End-of-Line (<CR>) symbol, if memory serves. So the fact that ASCII (or UTF-8) file gets a appended, is not surprising. Try echo "test" > test.txt, and you'll find the resulting file test.txt contains 5 bytes. For example:

$ echo "test" > t.txt
$ od -t x1 -t c t.txt 
0000000    74  65  73  74  0a                                            
           t   e   s   t  \n                                            
0000005
$ 

I think appending that 0x0A to an XER encoding is OK. The bug here is being unable to parse it properly (aka - ignore) in the XER file, particularly at the end of this XML structure.

@brchiu
Copy link
Contributor Author

brchiu commented Oct 27, 2017

The reason I 'guess' this extra 0x0A is the culprit because, after truncating this character by dd utility, ./j2735-dump -ixer .tmp.1.xxxx -oxer can run without error.

I am not sure whether the XER encoder or the XER decoder should be blamed. But strangely, I did not directly modify xer_encoder.c nor xer_decoder.c in this merge. So there might be something wrong worth checking.

However, it is not my priority to do this. Still struggle with refinement for s1ap.

@mouse07410
Copy link

mouse07410 commented Oct 27, 2017

I am not sure whether the XER encoder or the XER decoder should be blamed.

I suspect - neither. Probably the trailing 0x0A gets introduced during copying. Or maybe during writing, in which case it's the encoder... Not during writing - as it appears to be writing normal C strings that are properly terminated.

Update
Or it's the issue with how the test is written. All of my XER files appear to have that trailing0x0A:

$ cat test2c_29_1.xer
<Test2c>
    <count>29</count>
    <weight>1.0</weight>
    <data>
        24 97 57 30 F7 C3 16 CF A3 37 03 C7 A0 38 1E 3F 
        CC 6B 48 D3 B3 68 51 B8 BD 85 EB E5 FF
    </data>
</Test2c>
$ od -t x1 -t c test2c_29_1.xer 
0000000    3c  54  65  73  74  32  63  3e  0a  20  20  20  20  3c  63  6f
           <   T   e   s   t   2   c   >  \n                   <   c   o
0000020    75  6e  74  3e  32  39  3c  2f  63  6f  75  6e  74  3e  0a  20
           u   n   t   >   2   9   <   /   c   o   u   n   t   >  \n    
0000040    20  20  20  3c  77  65  69  67  68  74  3e  31  2e  30  3c  2f
                       <   w   e   i   g   h   t   >   1   .   0   <   /
0000060    77  65  69  67  68  74  3e  0a  20  20  20  20  3c  64  61  74
           w   e   i   g   h   t   >  \n                   <   d   a   t
0000100    61  3e  0a  20  20  20  20  20  20  20  20  32  34  20  39  37
           a   >  \n                                   2   4       9   7
0000120    20  35  37  20  33  30  20  46  37  20  43  33  20  31  36  20
               5   7       3   0       F   7       C   3       1   6    
0000140    43  46  20  41  33  20  33  37  20  30  33  20  43  37  20  41
           C   F       A   3       3   7       0   3       C   7       A
0000160    30  20  33  38  20  31  45  20  33  46  20  0a  20  20  20  20
           0       3   8       1   E       3   F      \n                
0000200    20  20  20  20  43  43  20  36  42  20  34  38  20  44  33  20
                           C   C       6   B       4   8       D   3    
0000220    42  33  20  36  38  20  35  31  20  42  38  20  42  44  20  38
           B   3       6   8       5   1       B   8       B   D       8
0000240    35  20  45  42  20  45  35  20  46  46  0a  20  20  20  20  3c
           5       E   B       E   5       F   F  \n                   <
0000260    2f  64  61  74  61  3e  0a  3c  2f  54  65  73  74  32  63  3e
           /   d   a   t   a   >  \n   <   /   T   e   s   t   2   c   >
0000300    0a                                                            
          \n                                                            
0000301
$ 

@mouse07410
Copy link

mouse07410 commented Oct 29, 2017

@vlm Considering that Travis CI "approves", I'd say this PR can be merged.

And I'd really like a master that (finally :) supports APER. ;-)

@brchiu
Copy link
Contributor Author

brchiu commented Oct 30, 2017

I re-run with current master, there is also this 0x0A character. So XER decoder in this pull request might be more suspicious.

@mouse07410
Copy link

mouse07410 commented Oct 30, 2017

I re-run with current master, there is also this 0x0A character.

What I'm saying is that 0x0A seems to me a legitimate part of the output file. XER output is a set of printable strings, each ending with 0x0A. That's how my fork behaves, that's apparently how this master behaves.

If a test fails on it - my first suspicion is on the test. Do you have a manual XER decoder, something like progname from the ASN.1 compilation that you can try the XER file on?
Does your test pass on my fork? If not - then 100% the test is broken or the current asn1c code cannot handle J2735.

So XER decoder in this pull request might be more suspicious.

But this pull request does not touch XER at all? And it passes Travis CI?

I'd say this PR should be merged, because it's better to have some APER and give people the ability to translate and run at least some APER apps, rather than waiting until J2735 support gets fixed.

@mouse07410
Copy link

@vlm?

@brchiu brchiu force-pushed the merge_mouse07410_aper_implementation branch from d072a59 to ddbabf4 Compare November 2, 2017 04:09
@brchiu
Copy link
Contributor Author

brchiu commented Nov 2, 2017

I found that restartability_supported()'s logic needs be negated to produce correct behavior.
make check under sample.source.J2735 pass.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 71.068% when pulling ddbabf4 on brchiu:merge_mouse07410_aper_implementation into 3f52df0 on vlm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 71.068% when pulling ddbabf4 on brchiu:merge_mouse07410_aper_implementation into 3f52df0 on vlm:master.

@mouse07410
Copy link

@vlm?

@brchiu brchiu force-pushed the merge_mouse07410_aper_implementation branch from a642f63 to fb6080d Compare November 5, 2017 11:42
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 71.068% when pulling fb6080d on brchiu:merge_mouse07410_aper_implementation into 3f52df0 on vlm:master.

@mouse07410
Copy link

@vlm may I ask to accelerate review and merge of this PR? APER is a needed feature. Thanks!

@brchiu brchiu force-pushed the merge_mouse07410_aper_implementation branch 2 times, most recently from a2711fc to 1fcb51a Compare November 15, 2017 12:09
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 71.143% when pulling 5912147 on brchiu:merge_mouse07410_aper_implementation into 07f8c3e on vlm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 71.143% when pulling 5912147 on brchiu:merge_mouse07410_aper_implementation into 07f8c3e on vlm:master.

@brchiu
Copy link
Contributor Author

brchiu commented Nov 16, 2017

@rampradeep93 , in addition to this #226, have you also merge #234 ? Please use newest commits of these pull requests. As for the decoding result, please refer to : #185 (comment)

@rampradeep93
Copy link

@brchiu , Have used master branch with pull request 226 and 234.

Now getting the following error:

Decoding member "criticality" in InitialUEMessage-IEs (constr_SEQUENCE.c:1599)
Decoding Criticality as NativeEnumerated (NativeEnumerated.c:255)
[PER got 2<=664 bits => span 42 +5[2..664]:00 (662) => 0x0] (asn_bit_data.c:139)
Decoded Criticality = 0 (NativeEnumerated.c:293)
Decoding member "value" in InitialUEMessage-IEs (constr_SEQUENCE.c:1599)
Failed to decode element InitialUEMessage-IEs (OPEN_TYPE.c:425)
Failed decode value in InitialUEMessage-IEs (constr_SEQUENCE.c:1609)
ProtocolIE-Container SET OF InitialUEMessage-IEs decoded 2, 0x1a832b0 (constr_SET_OF.c:1164)
Failed decoding InitialUEMessage-IEs of ProtocolIE-Container (SET OF) (constr_SET_OF.c:1174)
Freeing InitialUEMessage-IEs as SEQUENCE (constr_SEQUENCE.c:994)
Freeing ProtocolIE-ID as INTEGER (1, 0x1a832b0, Native) (NativeInteger.c:429)
Freeing Criticality as INTEGER (1, 0x1a832b8, Native) (NativeInteger.c:429)
Freeing value as CHOICE (constr_CHOICE.c:1230)
Failed decode protocolIEs in InitialUEMessage (constr_SEQUENCE.c:1609)
Freeing InitialUEMessage as SEQUENCE (constr_SEQUENCE.c:994)
sample-S1AP-InitialUEMessage.aper: Decode failed past byte 0: Input processing error

Could you please help.

@brchiu
Copy link
Contributor Author

brchiu commented Nov 17, 2017

@rampradeep93 , the s1ap-dump I built can decode sample-S1AP-InitialUEMessage.aper under sample.source.S1AP directory. Decoding log is here.

By the way, please specify -iaper and add -per-nopad to your command line.

./s1ap-dump -iaper -per-nopad sample-S1AP-InitialUEMessage.aper

I push my merged branch at my own repository. You can do some file comparison to find out what's wrong with your merge.

@brchiu brchiu force-pushed the merge_mouse07410_aper_implementation branch from 5912147 to 2ecc970 Compare November 18, 2017 06:48
@coveralls
Copy link

coveralls commented Nov 18, 2017

Coverage Status

Coverage decreased (-0.3%) to 70.87% when pulling f4a1f58 on brchiu:merge_mouse07410_aper_implementation into 4cc779f on vlm:master.

@rampradeep93
Copy link

Hi @brchiu , When I decode S1 Setup Request and and then call aper_encode function for Setup Response, am getting the following error,

Failed to encode element ProtocolIE-Container (constr_SEQUENCE_OF.c:288)
Failed to encode element value (OPEN_TYPE.c:509)

But when I call decode function, after encode, the encoding is successful.
The list->count in constr_SEQUENCE_OF.c is 2, when I call encoding before decoding and it is successful.

But when I call decode before encode, the value of list->count is 3.

Could you please help.

@brchiu
Copy link
Contributor Author

brchiu commented Dec 22, 2017

Could you provide me the binary raw S1AP message ?

By the way, please also apply #234 and have a trial, #234 solve some problems found after this pr. If #234 works, please feedback here to encourage vlm accepting it.

@brchiu
Copy link
Contributor Author

brchiu commented Dec 22, 2017

@rampradeep93 , in addition to raw s1ap message, perhaps you need to provide your sample code used in your testing, because it might be a result of mis-use of asn1c's APIs.

@rampradeep93
Copy link

@brchiu , Please find the attached s1ap message.
s1_setup_request.zip

I am not able to share the code now.

But am using a similar kind of format:
//Am copying the file contents to a buffer and passing the buffer to the function
structure = data_decode_from_file(isyntax, pduType, asn_decode_buffer,
suggested_bufsize, first_pdu);

//Encode the structure contents
erv = asn_encode(NULL, osyntax, pduType, enc_structure, write_to_buffer, asn_encode_buffer);

Also when I call the decode - encode functions multiple times also, it fails.

@brchiu
Copy link
Contributor Author

brchiu commented Dec 23, 2017

@rampradeep93 , after merging #226 and #234, I compile this test program in https://gist.github.com/brchiu/54abb92f1035d881b0c13c2f2697422a with gcc test.c -g -o test -I. -L. -lasncodec, the result of executing it with ./test ./s1_setup.bin is :

$ ./test ./s1_setup.bin
decoding result 0
encoded 35 bytes, compare SAME

@brchiu brchiu force-pushed the merge_mouse07410_aper_implementation branch 3 times, most recently from 41b228d to b951ce6 Compare January 22, 2018 03:42
1. Copying and modifying ANY_decode_uper() and
ANY_encode_uper() to ANY_decode_aper() and
ANY_encode_aper().

2. Adding ANY_fromType_aper()/ANY_new_fromType_aper()/
ANY_to_type_aper().
@brchiu brchiu force-pushed the merge_mouse07410_aper_implementation branch from b951ce6 to 755538f Compare January 26, 2018 07:59
@mouse07410
Copy link

@vlm, are you planning to merge this PR? Is four months enough time for review?

mouse07410 added a commit to mouse07410/asn1c that referenced this pull request Feb 13, 2018
This commit addresses compatibility of PR vlm#234 with vlm#226 (addition of APER support)
@brchiu
Copy link
Contributor Author

brchiu commented Feb 13, 2018

@mouse07410, take it easy !

@vlm does not owe anybody for this project. I 'wish' he can make some decisions of this pull request, but he doesn't have to do it for anyone.

I learned this asn1c project in 2005 and found no UPER available at that time. Then I revisited it in 2013 and found no APER available. Now we at least have something better than 13 years ago. I am quite happy to see it. 😉

People who can not wait can either resort to commercial software, or write another asn1 converter on his/her own. Right ?

@mouse07410 mouse07410 mentioned this pull request Mar 26, 2018
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.

4 participants