pycrate icon indicating copy to clipboard operation
pycrate copied to clipboard

Feature: added EFC ASN.1 modules

Open rmwesley opened this issue 1 year ago • 3 comments

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.

rmwesley avatar Dec 04 '24 01:12 rmwesley

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

mitshell avatar Dec 09 '24 20:12 mitshell

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: https://github.com/pycrate-org/pycrate/blob/8acbc8be52d18470d6283c0c40940e2cbc32ecc5/test/test_asn1rt.py#L2593.

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

mitshell avatar Dec 09 '24 20:12 mitshell

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?

rmwesley avatar Dec 10 '24 10:12 rmwesley