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

Feature: added EFC ASN.1 modules #24

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

Conversation

rmwesley
Copy link

@rmwesley rmwesley commented Dec 4, 2024

Hello! This is my first PR, I hope I followed the proper etiquette.

Changes

In the first 3 out of the 5 commits, I:

  • Added 2 complete EFC bundles with release years set to 2015 and 2023, containing ASN.1 modules for:
    DSRC, EFC, CCC, LAC, AutonomousCharging, InfoExchange + dependencies (X509, RFC5035)
  • Added 2 partial EFC bundles (DSRC, EFC only) with release years set to 2014 and 2018
  • Added 2 partial EFC bundles (DSRC, EFC, CCC only) with release years set to 2009 and 2023

I then also did 2 other commits with some tests.
I added them to the test/ dir. Please tell me if this impacts the CI/CD pipeline. Should I have added my custom tests to res/ instead?

Description

For each of the 3 first commits, I:

  • Added the EFC ASN.1 files to pycrate_asn1dir/
  • Updated specdir.py
  • Manually ran python -m pycrate_asn1c.asnproc to recompile all the specs
  • Committed all the changes.

These bundles include DSRC, EFC, CCC, LAC, AutonomousCharging and
InfoExchange specs with dependencies, such as X509 and RFC5035.
These specs were obtained from the standards.iso.org/iso/ website.

The ISO 14906 DSRC ASN.1 specs sometimes use Windows code pages to
encode characters in comments. I replaced them for their ASCII
counterparts where I could to avoid issues.
Again, the ASN.1 specs were modified to only contain ASCII characters
(even in comments).
CCC imports from DSRC specs, so they are added as dependencies
@mitshell
Copy link
Member

mitshell commented Dec 9, 2024

Thanks for the PR : this is much appreciated. I'll check the proper test integration before merging, but overall this is looking nice.

@mitshell mitshell self-assigned this Dec 9, 2024
@mitshell mitshell added the enhancement New feature or request label Dec 9, 2024
@mitshell
Copy link
Member

mitshell commented Dec 9, 2024

Looking at the test_efc.py file, this would have been preferable to have something that integrates the automatic testing which is run during CI. Something that would be like this x509 testing, for instance:

pkts_X509 = tuple(map(unhexlify, (
.

Do you think you could have your EFC tests implemented similar as this?

@rmwesley
Copy link
Author

rmwesley commented Dec 10, 2024

Yes, I will look into the test automation !

Also, there is a patch I applied to the ISO12813(2023)EfcCccV4.1.asn specs (a new commit).
I did not commit it because I was testing it.

I thoroughly tested CCC by switching back-and-forth T-APDUs from RSE (beacon) to OBE (device) and decoding/encoding them with CccTApdus. So far no errors! But I don't know if the patch I did was a correction or a workaround.

Here go the details of my latest CCC patch...
In the https://standards.iso.org/iso/12813/ed-3/en/ISO12813(2023)EfcCccV4.1.asn spec, we have this line :
CccAuthDataRetrievalResponse ::= Action-Response {GetStampedRs{CccContainer}} (WITH COMPONENTS {..., iid ABSENT, responseParameter PRESENT})
A parameter that contains a parameter itself.

I modified it to:
CccAuthDataRetrievalResponse ::= Action-Response {CccContainer} (WITH COMPONENTS {..., iid ABSENT, responseParameter PRESENT})

Most of the alterations/manipulations I did to the ASN.1 files were minimal and I believe in good faith, but it always feels weird to edit standards' documents like this...

Finally, I confess I like to do rebases to edit git story and then do push -f.
I plan on renaming my commit messages a bit. I forgot to say in each message I committed 2 bundles in each commit.
Is that ok for you? I mean, I am on a fork, I think there will be no conflicts since the pull request has not been accepted and merged yet...

Since I will be editing the commit messages anyway, should I fix the ISO12813(2023)EfcCccV4.1.asn too? Or should I do a new commit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants