From d9505bbbccd50767fd40a78c8bef873d0711e876 Mon Sep 17 00:00:00 2001 From: Romuald Conty Date: Wed, 6 Oct 2010 15:12:33 +0000 Subject: [PATCH] Better handling of SetParameters command, use a cache, prevent from double set, adjust comments, remove junk code. --- include/nfc/nfc-types.h | 4 ++- libnfc/chips/pn53x.c | 62 ++++++++++++++++++++++++++++--------- libnfc/chips/pn53x.h | 15 +++++---- libnfc/drivers/acr122.c | 4 +-- libnfc/drivers/arygon.c | 4 +-- libnfc/drivers/pn532_uart.c | 4 +-- libnfc/drivers/pn53x_usb.c | 6 ++-- libnfc/nfc.c | 4 +-- 8 files changed, 65 insertions(+), 38 deletions(-) diff --git a/include/nfc/nfc-types.h b/include/nfc/nfc-types.h index 8563552..783117f 100644 --- a/include/nfc/nfc-types.h +++ b/include/nfc/nfc-types.h @@ -69,8 +69,10 @@ typedef struct { bool bPar; /** Should the PN53x chip handle frames encapsulation and chaining */ bool bEasyFraming; -/** The last tx bits setting, we need to reset this if it does not apply anymore */ +/** Register cache for REG_CIU_BIT_FRAMING, SYMBOL_TX_LAST_BITS: The last TX bits setting, we need to reset this if it does not apply anymore */ uint8_t ui8TxBits; +/** Register cache for SetParameters function. */ + uint8_t ui8Parameters; /** Last error reported by the PCD / encountered by the PCD driver * MSB LSB * | 00 | 00 | diff --git a/libnfc/chips/pn53x.c b/libnfc/chips/pn53x.c index 2ff935d..4598a0c 100644 --- a/libnfc/chips/pn53x.c +++ b/libnfc/chips/pn53x.c @@ -81,6 +81,29 @@ static const byte_t pn53x_ack_frame[] = { 0x00, 0x00, 0xff, 0x00, 0xff, 0x00 }; static const byte_t pn53x_nack_frame[] = { 0x00, 0x00, 0xff, 0xff, 0x00, 0x00 }; static const byte_t pn53x_error_frame[] = { 0x00, 0x00, 0xff, 0x01, 0xff, 0x7f, 0x81, 0x00 }; +bool +pn53x_init(nfc_device_t * pnd) +{ + // CRC handling is enabled by default + pnd->bCrc = true; + // Parity handling is enabled by default + pnd->bPar = true; + + // Reset the ending transmission bits register, it is unknown what the last tranmission used there + pnd->ui8TxBits = 0; + if (!pn53x_set_reg (pnd, REG_CIU_BIT_FRAMING, SYMBOL_TX_LAST_BITS, 0x00)) { + return false; + } + + // We can't read these parameters, so we set a default config by using the SetParameters wrapper + // Note: pn53x_SetParameters() will save the sent value in pnd->ui8Parameters cache + if(!pn53x_SetParameters(pnd, PARAM_AUTO_ATR_RES | PARAM_AUTO_RATS)) { + return false; + } + + return true; +} + // XXX: Is this function correctly named ? bool pn53x_transceive_check_ack_frame_callback (nfc_device_t * pnd, const byte_t * pbtRxFrame, const size_t szRxFrameLen) @@ -203,17 +226,32 @@ pn53x_set_reg (nfc_device_t * pnd, uint16_t ui16Reg, uint8_t ui8SymbolMask, uint } bool -pn53x_set_parameters (nfc_device_t * pnd, uint8_t ui8Value) +pn53x_set_parameter (nfc_device_t * pnd, const uint8_t ui8Parameter, const bool bEnable) +{ + uint8_t ui8Value = (bEnable) ? (pnd->ui8Parameters | ui8Parameter) : (pnd->ui8Parameters & ~(ui8Parameter)); + if (ui8Value != pnd->ui8Parameters) { + return pn53x_SetParameters(pnd, ui8Value); + } + return true; +} + +bool +pn53x_SetParameters (nfc_device_t * pnd, const uint8_t ui8Value) { byte_t abtCmd[sizeof (pncmd_set_parameters)]; memcpy (abtCmd, pncmd_set_parameters, sizeof (pncmd_set_parameters)); abtCmd[2] = ui8Value; - return pn53x_transceive (pnd, abtCmd, sizeof (pncmd_set_parameters), NULL, NULL); + if(!pn53x_transceive (pnd, abtCmd, sizeof (pncmd_set_parameters), NULL, NULL)) { + return false; + } + // We save last parameters in register cache + pnd->ui8Parameters = ui8Value; + return true; } bool -pn53x_set_tx_bits (nfc_device_t * pnd, uint8_t ui8Bits) +pn53x_set_tx_bits (nfc_device_t * pnd, const uint8_t ui8Bits) { // Test if we need to update the transmission bits register setting if (pnd->ui8TxBits != ui8Bits) { @@ -228,8 +266,8 @@ pn53x_set_tx_bits (nfc_device_t * pnd, uint8_t ui8Bits) } bool -pn53x_wrap_frame (const byte_t * pbtTx, const size_t szTxBits, const byte_t * pbtTxPar, byte_t * pbtFrame, - size_t * pszFrameBits) +pn53x_wrap_frame (const byte_t * pbtTx, const size_t szTxBits, const byte_t * pbtTxPar, + byte_t * pbtFrame, size_t * pszFrameBits) { byte_t btFrame; byte_t btData; @@ -759,12 +797,7 @@ pn53x_configure (nfc_device_t * pnd, const nfc_device_option_t ndo, const bool b break; case NDO_AUTO_ISO14443_4: - // TODO: PN53x parameters could not be read, so we have to buffered current value in order to prevent from configuration overwrite - // ATM, buffered current value is not needed due to a single usage of these parameters - btValue = (bEnable) ? (SYMBOL_PARAM_fAutomaticRATS | SYMBOL_PARAM_fAutomaticATR_RES) : SYMBOL_PARAM_fAutomaticATR_RES; - if (!pn53x_set_parameters (pnd, btValue)) - return false; - return true; + return pn53x_set_parameter(pnd, PARAM_AUTO_RATS, bEnable); break; case NDO_FORCE_ISO14443_A: @@ -958,15 +991,16 @@ pn53x_target_init (nfc_device_t * pnd, const nfc_target_mode_t ntm, const nfc_ta bool bPar = pnd->bPar; // Configure the target corresponding to the requested mode + // XXX I (Romuald) don't think that a good thing to select NDO_EASY_FRAMING here, that's a user choice... switch(ntm) { case NTM_PASSIVE: - pn53x_set_parameters(pnd, 0); + pn53x_set_parameter(pnd, PARAM_AUTO_ATR_RES, false); pn53x_configure(pnd, NDO_EASY_FRAMING, false); break; case NTM_DEP: - pn53x_set_parameters(pnd, SYMBOL_PARAM_fAutomaticATR_RES); + pn53x_set_parameter(pnd, PARAM_AUTO_ATR_RES, true); pn53x_configure(pnd, NDO_EASY_FRAMING, true); break; @@ -976,7 +1010,7 @@ pn53x_target_init (nfc_device_t * pnd, const nfc_target_mode_t ntm, const nfc_ta pnd->iLastError = DENOTSUP; return false; } - pn53x_set_parameters(pnd, SYMBOL_PARAM_fISO14443_4_PICC); + pn53x_set_parameter(pnd, PARAM_14443_4_PICC, true); pn53x_configure(pnd, NDO_EASY_FRAMING, true); break; diff --git a/libnfc/chips/pn53x.h b/libnfc/chips/pn53x.h index 95567ef..0e2c9a1 100644 --- a/libnfc/chips/pn53x.h +++ b/libnfc/chips/pn53x.h @@ -64,18 +64,15 @@ # define REG_CIU_BIT_FRAMING 0x633D # define SYMBOL_TX_LAST_BITS 0x07 -# define SYMBOL_PARAM_fAutomaticRATS 0x10 -# define SYMBOL_PARAM_fAutomaticATR_RES 0x04 -# define SYMBOL_PARAM_fISO14443_4_PICC 0x20 - // Internal parameters flags # define PARAM_NONE 0x00 # define PARAM_NAD_USED 0x01 # define PARAM_DID_USED 0x02 # define PARAM_AUTO_ATR_RES 0x04 # define PARAM_AUTO_RATS 0x10 -# define PARAM_14443_4_PICC 0x20 -# define PARAM_NO_AMBLE 0x40 +# define PARAM_14443_4_PICC 0x20 /* Only for PN532 */ +# define PARAM_NFC_SECURE 0x20 /* Only for PN533 */ +# define PARAM_NO_AMBLE 0x40 /* Only for PN532 */ // Radio Field Configure Items // Configuration Data length # define RFCI_FIELD 0x01 // 1 @@ -93,6 +90,7 @@ # define DEISERRFRAME 0x0300/* Error frame */ # define DENOTSUP 0x0400/* Not supported */ +bool pn53x_init(nfc_device_t * pnd); bool pn53x_transceive_check_ack_frame_callback (nfc_device_t * pnd, const byte_t * pbtRxFrame, const size_t szRxFrameLen); bool pn53x_transceive_check_error_frame_callback (nfc_device_t * pnd, const byte_t * pbtRxFrame, @@ -101,8 +99,8 @@ bool pn53x_transceive (nfc_device_t * pnd, const byte_t * pbtTx, const size_t size_t * pszRxLen); bool pn53x_get_reg (nfc_device_t * pnd, uint16_t ui16Reg, uint8_t * ui8Value); bool pn53x_set_reg (nfc_device_t * pnd, uint16_t ui16Reg, uint8_t ui8SymbolMask, uint8_t ui8Value); -bool pn53x_set_parameters (nfc_device_t * pnd, uint8_t ui8Value); -bool pn53x_set_tx_bits (nfc_device_t * pnd, uint8_t ui8Bits); +bool pn53x_set_parameter (nfc_device_t * pnd, const uint8_t ui8Value, const bool bEnable); +bool pn53x_set_tx_bits (nfc_device_t * pnd, const uint8_t ui8Bits); bool pn53x_wrap_frame (const byte_t * pbtTx, const size_t szTxBits, const byte_t * pbtTxPar, byte_t * pbtFrame, size_t * pszFrameBits); bool pn53x_unwrap_frame (const byte_t * pbtFrame, const size_t szFrameBits, byte_t * pbtRx, size_t * pszRxBits, @@ -137,6 +135,7 @@ static const struct chip_callbacks pn53x_callbacks_list = { }; // C wrappers for PN53x commands +bool pn53x_SetParameters (nfc_device_t * pnd, const uint8_t ui8Value); bool pn53x_InListPassiveTarget (nfc_device_t * pnd, const nfc_modulation_t nmInitModulation, const byte_t szMaxTargets, const byte_t * pbtInitiatorData, const size_t szInitiatorDataLen, byte_t * pbtTargetsData, size_t * pszTargetsData); diff --git a/libnfc/drivers/acr122.c b/libnfc/drivers/acr122.c index b617eca..d21c8b6 100644 --- a/libnfc/drivers/acr122.c +++ b/libnfc/drivers/acr122.c @@ -240,9 +240,7 @@ acr122_connect (const nfc_device_desc_t * pndd) pnd->nc = NC_PN532; pnd->nds = (nfc_device_spec_t) pas; pnd->bActive = true; - pnd->bCrc = true; - pnd->bPar = true; - pnd->ui8TxBits = 0; + return pnd; } diff --git a/libnfc/drivers/arygon.c b/libnfc/drivers/arygon.c index d25c41d..f2688c3 100644 --- a/libnfc/drivers/arygon.c +++ b/libnfc/drivers/arygon.c @@ -183,9 +183,7 @@ arygon_connect (const nfc_device_desc_t * pndd) pnd->nc = NC_PN532; pnd->nds = (nfc_device_spec_t) sp; pnd->bActive = true; - pnd->bCrc = true; - pnd->bPar = true; - pnd->ui8TxBits = 0; + return pnd; } diff --git a/libnfc/drivers/pn532_uart.c b/libnfc/drivers/pn532_uart.c index 9b47c8c..a0acd4b 100644 --- a/libnfc/drivers/pn532_uart.c +++ b/libnfc/drivers/pn532_uart.c @@ -166,9 +166,7 @@ pn532_uart_connect (const nfc_device_desc_t * pndd) pnd->nc = NC_PN532; pnd->nds = (nfc_device_spec_t) sp; pnd->bActive = true; - pnd->bCrc = true; - pnd->bPar = true; - pnd->ui8TxBits = 0; + return pnd; } diff --git a/libnfc/drivers/pn53x_usb.c b/libnfc/drivers/pn53x_usb.c index 61751d2..46da46c 100644 --- a/libnfc/drivers/pn53x_usb.c +++ b/libnfc/drivers/pn53x_usb.c @@ -194,11 +194,8 @@ pn53x_usb_connect (const nfc_device_desc_t * pndd, const char *target_name, int pnd->nc = target_chip; pnd->nds = (nfc_device_spec_t) pus; pnd->bActive = true; - pnd->bCrc = true; - pnd->bPar = true; - pnd->ui8TxBits = 0; - // TODO Move this HACK1 into an upper level in rder to benefit to other devices that use PN53x + // TODO Move this HACK1 into an upper level in order to benefit to other devices that use PN53x // HACK1: Send first an ACK as Abort command, to reset chip before talking to it: uint8_t ack_frame[] = { 0x00, 0x00, 0xff, 0x00, 0xff, 0x00 }; #ifdef DEBUG @@ -249,6 +246,7 @@ pn53x_usb_connect (const nfc_device_desc_t * pndd, const char *target_name, int PRINT_HEX ("RX", abtRx, ret); #endif } + return pnd; } } diff --git a/libnfc/nfc.c b/libnfc/nfc.c index 678f3b1..f3957ce 100644 --- a/libnfc/nfc.c +++ b/libnfc/nfc.c @@ -99,11 +99,11 @@ nfc_connect (nfc_device_desc_t * pndd) // Great we have claimed a device pnd->pdc = &(drivers_callbacks_list[uiDriver]); + // FIXME Why do we do this ? if (!pn53x_get_firmware_version (pnd)) return NULL; - // Reset the ending transmission bits register, it is unknown what the last tranmission used there - if (!pn53x_set_reg (pnd, REG_CIU_BIT_FRAMING, SYMBOL_TX_LAST_BITS, 0x00)) + if (!pn53x_init (pnd)) return NULL; // Set default configuration options