Skip to content

Commit

Permalink
Clarify protocol. Disruptive changes: see below
Browse files Browse the repository at this point in the history
This huge commit tries to enhance several things related to the fw/cli protocol.
Generally, the idea is to be verbose, explicit and reuse conventions, in order to enhance code maintainability and understandability for the other contributors.

docs/protocol.md got heavily updated

Many commands have been renamed for consistency. you are invited to adapt your client for easier maintenance

Guidelines, also written in docs/protocol.md "New data payloads: guidelines for developers":
- Now protocol data exchanged over USB or BLE are defined in netdata.h as packed structs and values are stored in Network byte order (=Big Endian)
- Command-specific payloads are defined in their respective cmd_processor handler in app_cmd.c and chameleon_cmd.py
- Define C `struct` for cmd/resp data greater than a single byte, use and abuse of `struct.pack`/`struct.unpack` in Python. So one can understand the payload format at a simple glimpse.
- If single byte of data to return, still use a 1-byte `data`, not `status`.
- Use unambiguous types such as `uint16_t`, not `int` or `enum`. Cast explicitly `int` and `enum` to `uint_t` of proper size
- Use Network byte order for 16b and 32b integers
  - Macros `U16NTOHS`, `U32NTOHL` must be used on reception of a command payload.
  - Macros `U16HTONS`, `U32HTONL` must be used on creation of a response payload.
  - In Python, use the modifier `!` with all `struct.pack`/`struct.unpack`
- Concentrate payload parsing in the handlers, avoid further parsing in their callers. This is true for the firmware and the client.
- In cmd_processor handlers: don't reuse input `length`/`data` parameters for creating the response content
- Avoid hardcoding offsets, use `sizeof()`, `offsetof(struct, field)` in C and `struct.calcsize()` in Python
- Use the exact same command and fields names in firmware and in client, use function names matching the command names for their handlers unless there is a very good reason not to do so. This helps grepping around. Names must start with a letter, not a number, because some languages require it (e.g. `14a_scan` not possible in Python)
- Respect commands order in `m_data_cmd_map`, `data_cmd.h` and `chameleon_cmd.py` definitions
- Even if a command is not yet implemented in firmware or in client but a command number is allocated, add it to `data_cmd.h` and `chameleon_cmd.py` with some `FIXME: to be implemented` comment
- Validate data before using it, both when receiving command data in the firmware and when receiving response data in the client.
- Validate response status in client.

Disruptive changes:
- GET_DEVICE_CAPABILITIES: list of cmds in data are now really Big Endian
  Note: the initial attempt to use macros PP_HTONS were actually considering wrongly that the platform was Big Endian (BYTE_ORDER was actually undefined) while it is actually Little Endian.
- GET_APP_VERSION: response is now a tuple of bytes: major|minor (previously it was in reversed order as a single uint16_t in Little Endian)
- SET_SLOT_TAG_TYPE: tag_type now on 2 bytes, to prepare remapping of its enum
- SET_SLOT_DATA_DEFAULT: tag_type now on 2 bytes, to prepare remapping of its enum
- GET_SLOT_INFO: tag_type now on 2 bytes, to prepare remapping of its enum
- GET_DEVICE_CHIP_ID: now returns its 64b ID following Network byte order (previously, bytes were in the reverse order)
- GET_DEVICE_ADDRESS: now returns its 56b address following Network byte order (previously, bytes were in the reverse order). CLI does not reverse the response anymore so it displays the same value as before.
- MF1_GET_DETECTION_COUNT: now returns its 32b value following Network byte order (previously Little Endian)
- GET_GIT_VERSION response status is now STATUS_DEVICE_SUCCESS
- GET_DEVICE_MODEL response status is now STATUS_DEVICE_SUCCESS
- MF1_READ_EMU_BLOCK_DATA response status is now STATUS_DEVICE_SUCCESS
- GET_DEVICE_CAPABILITIES response status is now STATUS_DEVICE_SUCCESS
- HF14A_SCAN: entirely new response format, room for ATS and multiple tags
- MF1_DETECT_SUPPORT response status is now HF_TAG_OK and support is indicated as bool in 1 byte of data
- MF1_DETECT_PRNG response status is now HF_TAG_OK and prng_type is returned in 1 byte of data with a new enum mf1_prng_type_t == MifareClassicPrngType
- MF1_DETECT_DARKSIDE response status is now HF_TAG_OK and darkside_status is returned in 1 byte of data with a new enum mf1_darkside_status_t == MifareClassicDarksideStatus
- MF1_DARKSIDE_ACQUIRE response status is now HF_TAG_OK and darkside_status is returned in 1 byte of data. If OK, followed by 24 bytes as previously
- MF1_GET_ANTI_COLL_DATA: in case slot does not contain anticoll data, instead of STATUS_PAR_ERR, now it returns STATUS_DEVICE_SUCCESS with empty data
- MF1_SET_ANTI_COLL_DATA and MF1_GET_ANTI_COLL_DATA now use the same data format as HF14A_SCAN

For clients to detect Ultra/Lite with older firmwares, one can issue the GET_APP_VERSION and urge the user to flash his device if needed.
On older firmwares, it will return a status=b'\x00' and data=b'\x00\x01' while up-to-date firmwares will return status=STATUS_DEVICE_SUCCESS and data greater or equal to b'\x01\x00' (v1.0).

Other changes: cf CHANGELOG, and probably a few small changes I forgot about..

TODO:
- remap `tag_specific_type_t` enum to allow future tags (e.g. LF tags) without reshuffling enum and affecting users stored cards
- TEST!
  • Loading branch information
doegox committed Sep 17, 2023
1 parent 98605be commit 8499535
Show file tree
Hide file tree
Showing 30 changed files with 2,097 additions and 1,651 deletions.
13 changes: 12 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,18 @@ All notable changes to this project will be documented in this file.
This project uses the changelog in accordance with [keepchangelog](http://keepachangelog.com/). Please use this to write notable changes, which is not the same as git commit log...

## [unreleased][unreleased]
- Added `hf settings blepair` command to get and set ble pairing enable state, and default disable ble pair. (@xianglin1998)
- Changed `hw detection decrypt` show progression and remove duplicate keys (@doegox)
- Changed dynamic cmd_map_init() by static cmd_map initialization (@doegox)
- Changed `hf slot list` to add clarity and colors (@doegox)
- Changed `hf mf sim` and `hf mf info` to support ATS (still to be used in actual emulation) (@doegox)
- Changed `hf mf eload` and `hf mf eread`: uploads/downloads are now 30x faster (@doegox)
- Changed CLI HF14AInfo logic merged inside HF14AScan for more consistent display of the results (@doegox)
- Added guessed type information for NXP tags, and reorganization of HF information part. (@FlUxIuS)
- Changed `hw raw` to detail status message (@doegox)
- Changed CLI to query capabilities on connect, not on every single command if device does not support get_device_capabilities (@doegox)
- Changed CLI to not instanciate ChameleonCMD on every single command (@doegox)
- Changed massively the protocol and its handlers for more consistency and easier maintenance and future dev (@doegox)
- Added `hf settings blepair` command to get and set ble pairing enable state, and default disable ble pair (@xianglin1998)
- Added `hf mf info` command to get UID/SAK/ATQA from slot (@Foxushka)
- Added `hw raw` to send raw command to Chameleon (@Foxushka)
- Added command to fetch all available commands from Chameleon and test if Chameleon supports it (@Foxushka)
Expand Down
4 changes: 4 additions & 0 deletions docs/development.md
Original file line number Diff line number Diff line change
Expand Up @@ -345,3 +345,7 @@ Limitations:
* SWO pin is shared with... SWO so when e.g. reflashing the device, garbage may appear on the monitoring terminal.
* SWO pin is also shared with the blue channel of the RGB slot LEDs, so faint blue may appear briefly when logs are sent and LED might not work properly when supposed to be blue.
# Resources
* [nRF52840 Objective Product Specification v0.5.1](https://infocenter.nordicsemi.com/pdf/nRF52840_OPS_v0.5.1.pdf)
5 changes: 5 additions & 0 deletions docs/images/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
all:
pdflatex --shell-escape protocol-packet.tex

clean:
rm *.aux *.log *.pdf
Binary file modified docs/images/protocol-packet.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
24 changes: 24 additions & 0 deletions docs/images/protocol-packet.tex
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
\documentclass[border=10pt,png]{standalone}
\usepackage{bytefield}
\usepackage{xcolor}

\begin{document}

\definecolor{lightcyan}{rgb}{0.85,1,1}
\definecolor{lightgreen}{rgb}{0.85,1,0.85}
\definecolor{lightred}{rgb}{1,0.85,0.85}
\begin{bytefield}[bitwidth=1.1em]{32}
\bitbox{8}[bgcolor=lightcyan]{SOF} &
\bitbox{8}[bgcolor=lightcyan]{LRC1} &
\bitbox{16}[bgcolor=lightgreen]{CMD} \\
\bitbox{16}[bgcolor=lightgreen]{STATUS} &
\bitbox{16}[bgcolor=lightgreen]{LEN} \\
\bitbox{8}[bgcolor=lightgreen]{LRC2} &
\bitbox[tlr]{24}[bgcolor=lightred]{} \\
\wordbox[lr]{1}[bgcolor=lightred]{DATA} \\
\wordbox[lr]{1}[bgcolor=lightred]{$\cdots$} \\
\bitbox[blr]{24}[bgcolor=lightred]{} &
\bitbox{8}[bgcolor=lightred]{LRC3}
\end{bytefield}

\end{document}
381 changes: 366 additions & 15 deletions docs/protocol.md

Large diffs are not rendered by default.

1,216 changes: 598 additions & 618 deletions firmware/application/src/app_cmd.c

Large diffs are not rendered by default.

1 change: 0 additions & 1 deletion firmware/application/src/app_cmd.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,5 @@ typedef struct {
} cmd_data_map_t;

void on_data_frame_received(uint16_t cmd, uint16_t status, uint16_t length, uint8_t *data);
void cmd_map_init();

#endif
1 change: 0 additions & 1 deletion firmware/application/src/app_main.c
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,6 @@ static void ble_passkey_init(void) {
*/
int main(void) {
hw_connect_init(); // Remember to initialize the pins first
cmd_map_init(); // Set function in CMD map for DATA_CMD_GET_DEVICE_CAPABILITIES

fds_util_init(); // Initialize fds tool
settings_load_config(); // Load settings from flash
Expand Down
11 changes: 0 additions & 11 deletions firmware/application/src/app_status.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,6 @@
#define HF_ERR_PARITY (0x07) // IC card parity error


/////////////////////////////////////////////////////////////////////
// MIFARE status
/////////////////////////////////////////////////////////////////////
#define DARKSIDE_CANT_FIXED_NT (0x20) // Darkside, the random number cannot be fixed, this situation may appear on the UID card
#define DARKSIDE_LUCK_AUTH_OK (0x21) // Darkside, the direct verification is successful, maybe the key is just empty
#define DARKSIDE_NACK_NO_SEND (0x22) // Darkside, the card does not respond to NACK, it may be a card that fixes Nack logic vulnerabilities
#define DARKSIDE_TAG_CHANGED (0x23) // Darkside, card switching in the process of running DARKSIDE, May is the two cards quickly switched
#define NESTED_TAG_IS_STATIC (0x24) // Nested, the random number of the card response is fixed
#define NESTED_TAG_IS_HARD (0x25) // Nested, the random number of the card response is unpredictable


/////////////////////////////////////////////////////////////////////
// lf status
/////////////////////////////////////////////////////////////////////
Expand Down
6 changes: 3 additions & 3 deletions firmware/application/src/ble_main.c
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@ static ble_opt_t m_static_pin_option;
* @details This function will set up the ble connect passkey.
*/
void set_ble_connect_key(uint8_t *key) {
static uint8_t passkey[BLE_CONNECT_KEY_LEN_MAX];
memcpy(passkey, key, BLE_CONNECT_KEY_LEN_MAX);
static uint8_t passkey[BLE_PAIRING_KEY_LEN];
memcpy(passkey, key, BLE_PAIRING_KEY_LEN);
m_static_pin_option.gap_opt.passkey.p_passkey = passkey;
// NRF_LOG_RAW_HEXDUMP_INFO(passkey, 6);
APP_ERROR_CHECK(sd_ble_opt_set(BLE_GAP_OPT_PASSKEY, &m_static_pin_option));
Expand Down Expand Up @@ -331,7 +331,7 @@ static void services_init(void) {
bas_init_obj.bl_cccd_wr_sec = SEC_OPEN;
bas_init_obj.bl_report_rd_sec = SEC_OPEN;
}

err_code = ble_bas_init(&m_bas, &bas_init_obj);
APP_ERROR_CHECK(err_code);
}
Expand Down
67 changes: 33 additions & 34 deletions firmware/application/src/data_cmd.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#define DATA_CMD_GET_APP_VERSION (1000)
#define DATA_CMD_CHANGE_DEVICE_MODE (1001)
#define DATA_CMD_GET_DEVICE_MODE (1002)
#define DATA_CMD_SET_SLOT_ACTIVATED (1003)
#define DATA_CMD_SET_ACTIVE_SLOT (1003)
#define DATA_CMD_SET_SLOT_TAG_TYPE (1004)
#define DATA_CMD_SET_SLOT_DATA_DEFAULT (1005)
#define DATA_CMD_SET_SLOT_ENABLE (1006)
Expand All @@ -28,18 +28,19 @@
#define DATA_CMD_GET_ACTIVE_SLOT (1018)
#define DATA_CMD_GET_SLOT_INFO (1019)
#define DATA_CMD_WIPE_FDS (1020)

#define DATA_CMD_GET_ENABLED_SLOTS (1023)
#define DATA_CMD_DELETE_SLOT_SENSE_TYPE (1024)
#define DATA_CMD_GET_BATTERY_INFO (1025)
#define DATA_CMD_GET_BUTTON_PRESS_CONFIG (1026)
#define DATA_CMD_SET_BUTTON_PRESS_CONFIG (1027)
#define DATA_CMD_GET_LONG_BUTTON_PRESS_CONFIG (1028)
#define DATA_CMD_SET_LONG_BUTTON_PRESS_CONFIG (1029)
#define DATA_CMD_SET_BLE_CONNECT_KEY_CONFIG (1030)
#define DATA_CMD_GET_BLE_CONNECT_KEY_CONFIG (1031)
#define DATA_CMD_SET_BLE_PAIRING_KEY (1030)
#define DATA_CMD_GET_BLE_PAIRING_KEY (1031)
#define DATA_CMD_DELETE_ALL_BLE_BONDS (1032)
#define DATA_CMD_GET_DEVICE (1033)
#define DATA_CMD_GET_SETTINGS (1034)
#define DATA_CMD_GET_DEVICE_MODEL (1033)
#define DATA_CMD_GET_DEVICE_SETTINGS (1034)
#define DATA_CMD_GET_DEVICE_CAPABILITIES (1035)
#define DATA_CMD_GET_BLE_PAIRING_ENABLE (1036)
#define DATA_CMD_SET_BLE_PAIRING_ENABLE (1037)
Expand All @@ -53,14 +54,14 @@
// Range from 2000 -> 2999
// ******************************************************************
//
#define DATA_CMD_SCAN_14A_TAG (2000)
#define DATA_CMD_MF1_SUPPORT_DETECT (2001)
#define DATA_CMD_MF1_NT_LEVEL_DETECT (2002)
#define DATA_CMD_MF1_DARKSIDE_DETECT (2003)
#define DATA_CMD_HF14A_SCAN (2000)
#define DATA_CMD_MF1_DETECT_SUPPORT (2001)
#define DATA_CMD_MF1_DETECT_PRNG (2002)
#define DATA_CMD_MF1_DETECT_DARKSIDE (2003)
#define DATA_CMD_MF1_DARKSIDE_ACQUIRE (2004)
#define DATA_CMD_MF1_NT_DIST_DETECT (2005)
#define DATA_CMD_MF1_DETECT_NT_DIST (2005)
#define DATA_CMD_MF1_NESTED_ACQUIRE (2006)
#define DATA_CMD_MF1_CHECK_ONE_KEY_BLOCK (2007)
#define DATA_CMD_MF1_AUTH_ONE_KEY_BLOCK (2007)
#define DATA_CMD_MF1_READ_ONE_BLOCK (2008)
#define DATA_CMD_MF1_WRITE_ONE_BLOCK (2009)
//
Expand All @@ -72,8 +73,8 @@
// Range from 3000 -> 3999
// ******************************************************************
//
#define DATA_CMD_SCAN_EM410X_TAG (3000)
#define DATA_CMD_WRITE_EM410X_TO_T5577 (3001)
#define DATA_CMD_EM410X_SCAN (3000)
#define DATA_CMD_EM410X_WRITE_TO_T55XX (3001)
//
// ******************************************************************

Expand All @@ -83,25 +84,23 @@
// Range from 4000 -> 4999
// ******************************************************************
//
#define DATA_CMD_LOAD_MF1_EMU_BLOCK_DATA (4000)
#define DATA_CMD_SET_MF1_ANTI_COLLISION_RES (4001)
#define DATA_CMD_SET_MF1_ANTI_COLLISION_INFO (4002)
#define DATA_CMD_SET_MF1_ATS_RESOURCE (4003)
#define DATA_CMD_SET_MF1_DETECTION_ENABLE (4004)
#define DATA_CMD_GET_MF1_DETECTION_COUNT (4005)
#define DATA_CMD_GET_MF1_DETECTION_RESULT (4006)
#define DATA_CMD_GET_MF1_DETECTION_STATUS (4007)
#define DATA_CMD_READ_MF1_EMU_BLOCK_DATA (4008)
#define DATA_CMD_GET_MF1_EMULATOR_CONFIG (4009)
#define DATA_CMD_GET_MF1_GEN1A_MODE (4010)
#define DATA_CMD_SET_MF1_GEN1A_MODE (4011)
#define DATA_CMD_GET_MF1_GEN2_MODE (4012)
#define DATA_CMD_SET_MF1_GEN2_MODE (4013)
#define DATA_CMD_GET_MF1_USE_FIRST_BLOCK_COLL (4014)
#define DATA_CMD_SET_MF1_USE_FIRST_BLOCK_COLL (4015)
#define DATA_CMD_GET_MF1_WRITE_MODE (4016)
#define DATA_CMD_SET_MF1_WRITE_MODE (4017)
#define DATA_CMD_GET_MF1_ANTI_COLL_DATA (4018)
#define DATA_CMD_MF1_WRITE_EMU_BLOCK_DATA (4000)
#define DATA_CMD_HF14A_SET_ANTI_COLL_DATA (4001)
#define DATA_CMD_MF1_SET_DETECTION_ENABLE (4004)
#define DATA_CMD_MF1_GET_DETECTION_COUNT (4005)
#define DATA_CMD_MF1_GET_DETECTION_LOG (4006)
#define DATA_CMD_MF1_GET_DETECTION_ENABLE (4007)
#define DATA_CMD_MF1_READ_EMU_BLOCK_DATA (4008)
#define DATA_CMD_MF1_GET_EMULATOR_CONFIG (4009)
#define DATA_CMD_MF1_GET_GEN1A_MODE (4010)
#define DATA_CMD_MF1_SET_GEN1A_MODE (4011)
#define DATA_CMD_MF1_GET_GEN2_MODE (4012)
#define DATA_CMD_MF1_SET_GEN2_MODE (4013)
#define DATA_CMD_MF1_GET_BLOCK_ANTI_COLL_MODE (4014)
#define DATA_CMD_MF1_SET_BLOCK_ANTI_COLL_MODE (4015)
#define DATA_CMD_MF1_GET_WRITE_MODE (4016)
#define DATA_CMD_MF1_SET_WRITE_MODE (4017)
#define DATA_CMD_HF14A_GET_ANTI_COLL_DATA (4018)
//
// ******************************************************************

Expand All @@ -114,7 +113,7 @@

//
// ******************************************************************
#define DATA_CMD_SET_EM410X_EMU_ID (5000)
#define DATA_CMD_GET_EM410X_EMU_ID (5001)
#define DATA_CMD_EM410X_SET_EMU_ID (5000)
#define DATA_CMD_EM410X_GET_EMU_ID (5001)

#endif
11 changes: 7 additions & 4 deletions firmware/application/src/rfid/nfctag/hf/nfc_mf1.c
Original file line number Diff line number Diff line change
Expand Up @@ -344,10 +344,11 @@ void append_mf1_auth_log_step1(bool isKeyB, bool isNested, uint8_t block, uint8_
}
// Determine whether this card slot enables the detection log record
if (m_tag_information->config.detection_enable) {
m_auth_log.logs[m_auth_log.count].cmd.is_key_b = isKeyB;
m_auth_log.logs[m_auth_log.count].cmd.block = block;
m_auth_log.logs[m_auth_log.count].cmd.is_nested = isNested;
m_auth_log.logs[m_auth_log.count].is_key_b = isKeyB;
m_auth_log.logs[m_auth_log.count].block = block;
m_auth_log.logs[m_auth_log.count].is_nested = isNested;
memcpy(m_auth_log.logs[m_auth_log.count].uid, UID_BY_CASCADE_LEVEL, 4);
// m_auth_log.logs[m_auth_log.count].nt = U32HTONL(*(uint32_t *)nonce);
memcpy(m_auth_log.logs[m_auth_log.count].nt, nonce, 4);
}
}
Expand All @@ -363,6 +364,8 @@ void append_mf1_auth_log_step2(uint8_t *nr, uint8_t *ar) {
}
if (m_tag_information->config.detection_enable) {
// Cache encryption information
// m_auth_log.logs[m_auth_log.count].nr = U32HTONL(*(uint32_t *)nr);
// m_auth_log.logs[m_auth_log.count].ar = U32HTONL(*(uint32_t *)ar);
memcpy(m_auth_log.logs[m_auth_log.count].nr, nr, 4);
memcpy(m_auth_log.logs[m_auth_log.count].ar, ar, 4);
}
Expand All @@ -388,7 +391,7 @@ void append_mf1_auth_log_step3(bool is_auth_success) {
/** @brief MF1 obtain verification log
* @param count: The statistics of the verification log
*/
nfc_tag_mf1_auth_log_t *get_mf1_auth_log(uint32_t *count) {
nfc_tag_mf1_auth_log_t *mf1_get_auth_log(uint32_t *count) {
// First pass the total number of logs verified by verified
*count = m_auth_log.count;
// Just return to the head pointer of the log number array
Expand Down
20 changes: 11 additions & 9 deletions firmware/application/src/rfid/nfctag/hf/nfc_mf1.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#define NFC_MF1_H

#include "nfc_14a.h"
#include "netdata.h"

// Exchange space for time.
// Fast simulate enable(Implement By ChameleonMini Repo)
Expand Down Expand Up @@ -121,22 +122,23 @@ typedef struct {
// MF1 label verification history
typedef struct {
// Basic information of verification
struct {
uint8_t block;
uint8_t is_key_b: 1;
uint8_t is_nested: 1;
// Airspace, occupying positions
uint8_t : 6;
} cmd;
uint8_t block;
uint8_t is_key_b: 1;
uint8_t is_nested: 1;
// padding to full byte
uint8_t : 6;
// MFKEY32 necessary parameters
uint8_t uid[4];
uint8_t nt[4];
uint8_t nr[4];
uint8_t ar[4];
} nfc_tag_mf1_auth_log_t;
// uint32_t nt;
// uint32_t nr;
// uint32_t ar;
} PACKED nfc_tag_mf1_auth_log_t;


nfc_tag_mf1_auth_log_t *get_mf1_auth_log(uint32_t *count);
nfc_tag_mf1_auth_log_t *mf1_get_auth_log(uint32_t *count);
int nfc_tag_mf1_data_loadcb(tag_specific_type_t type, tag_data_buffer_t *buffer);
int nfc_tag_mf1_data_savecb(tag_specific_type_t type, tag_data_buffer_t *buffer);
bool nfc_tag_mf1_data_factory(uint8_t slot, tag_specific_type_t tag_type);
Expand Down
Loading

0 comments on commit 8499535

Please sign in to comment.