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 timed read for smartcards + littles changes #160

Merged
merged 2 commits into from Feb 18, 2024

Conversation

0xDRRB
Copy link
Contributor

@0xDRRB 0xDRRB commented Jan 30, 2024

Hi,

Here's a patch adding a command to receive a smartcard message with a timeout.
With the existing code, a successful read can only be achieved by specifying the exact number of characters (or less). This works:

smartcard1> ]%%[read:18 
RST DOWN
DELAY: 1 ms
DELAY: 1 ms
RST UP
READ: 0x3B 0xF8 0x13 0x00 0x00 0x81 0x31 0xFE 0x45 0x4A 0x43 0x4F 0x50 0x76 0x32 0x34 0x31 0xB7 

But not this:

smartcard1> ]%%[read:33
RST DOWN
DELAY: 1 ms
DELAY: 1 ms
RST UP
READ error:3

Of course, ATR is just an example, we don't always know the size of the data returned by the card.. So I added a new tread (timed read) command and a timeout parameter. And now we can do this:

> smartcard 
Device: SMARTCARD1
Speed: 9408 bps
Parity: even
Stop bits: 1.5
Convention: normal
Prescaler: 12 / 3.50mhz
Timeout: 10000 msec

smartcard1> timeout 2000

smartcard1> ]%%[tread:33
RST DOWN
DELAY: 1 ms
DELAY: 1 ms
RST UP
READ: 0x3B 0xF8 0x13 0x00 0x00 0x81 0x31 0xFE 0x45 0x4A 0x43 0x4F 0x50 0x76 0x32 0x34 0x31 0xB7 
                                                                                                                     
smartcard1> 0x00 0x00 0x0f 0x00 0xa4 0x04 0x00 0x0a 0xf2 0x76 0xa2 0x88 0xbc 0xde 0xad 0xbe 0xef 0x01 0x94 % tread:18
WRITE: 0x00 0x00 0x0F 0x00 0xA4 0x04 0x00 0x0A 0xF2 0x76 0xA2 0x88 0xBC 0xDE 0xAD 0xBE 0xEF 0x01 0x94 
DELAY: 1 ms
READ: 0x00 0x00 0x02 0x90 0x00 0x92 
                                                                 
smartcard1> 0x00 0x40 0x05 0x80 0x50 0x00 0x00 0x01 0x94 tread:10
WRITE: 0x00 0x40 0x05 0x80 0x50 0x00 0x00 0x01 0x94 
READ: 0x00 0x40 0x03 0x0A 0x90 0x00 0xD9 
            
smartcard1>

Here, after the ATR, I selected the application on the smartcard (NXP J2A081, T=1 protocol) and used an APDU for a counter reading.

I have also configured a pull-up resistor on PA7 so that the normally closed contactor on the card reader opens and query displays CD=1 when the card is present.

Finally, the default communication speed should be 9408 bps because (1/((1/3500000)*372)). 9600 bps is a rounding-off that works but the ETU is normally 106µs with a 3.5 MHz clock.

Perhaps timeout read could be interesting for other buses too.

@bvernoux bvernoux changed the title Add timed read for smatcards + littles changes Add timed read for smartcards + littles changes Jan 30, 2024
@bvernoux
Copy link
Member

bvernoux commented Jan 30, 2024

Thanks for the contribution
@Baldanos could you do a review of those improvements / modifications ?

uint8_t nb_rdata, i;
mode_config_proto_t* proto = &con->mode->proto;

nb_rdata = bsp_smartcard_read_u8_timeout(proto->dev_num, rx_data, nb_data, proto->config.smartcard.dev_timeout * 10);
Copy link
Member

Choose a reason for hiding this comment

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

Avoid hard coded value "proto->config.smartcard.dev_timeout * 10" it shall be TIME_MS2I(proto->config.smartcard.dev_timeout) (to be checked as the unit is expected to be in ms)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@@ -63,6 +63,7 @@ static void init_proto_default(t_hydra_console *con)
proto->config.smartcard.dev_guardtime = 16;
proto->config.smartcard.dev_phase = 0;
proto->config.smartcard.dev_convention = DEV_CONVENTION_NORMAL;
proto->config.smartcard.dev_timeout = 10000;
Copy link
Member

Choose a reason for hiding this comment

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

Why default timeout is so long (10000ms => 10s) ? (It will be better to be less than 1s especially as default value and usually any HW shall return data in less than 1s)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also found it huge but I kept the value used by bsp_smartcard_read_u8() in src/drv/stm32cube/bsp_smartcard.c (SMARTCARDx_TIMEOUT_MAX).

@bvernoux bvernoux added this to the 0.12 milestone Feb 4, 2024
@Baldanos
Copy link
Collaborator

Baldanos commented Feb 13, 2024

Thanks for the contribution !

I really like the read with timeout idea and it could be applied to all modes by changing the read() function prototype in all modes.
In that case, the timeout would be part of the main mode_config_proto_t structure and other changes. This would require a dedicated PR IMO.

I would suggest to change this PR by only including 0d0862a and d9a7097 so it can be merged quickly, and I'll open an issue regarding the read with timeout feature for all modes.

Edit: #164 has been opened

@0xDRRB
Copy link
Contributor Author

0xDRRB commented Feb 13, 2024

0d0862a is linked to 56989e0. So you mean d9a7097 alone, right ?

@0xDRRB
Copy link
Contributor Author

0xDRRB commented Feb 13, 2024

ok. i removed 0d0862a and 56989e0 for PR and added 9826b18 (for SMARTCARD_DEFAULT_SPEED).

I've moved the changes about timeout to a SCtimeout branch.

@Baldanos Baldanos merged commit 8e44c7b into hydrabus:master Feb 18, 2024
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.

None yet

3 participants