antfs.c sample code bug in transport_layer_cmd_decode()

[rschmitz]

It appears that there is a bug in the antfs.c sample code in version 15.3.0

nRF5_SDK_15.3.0_59ac345\components\ant\ant_fs\antfs.c

It the transport_layer_cmd_decode() function in the ANTFS_CMD_DOWNLOAD_ID: case there is the following If statement (the second one) that appears to be attempting to determine if this is the first burst packet.

        case ANTFS_CMD_DOWNLOAD_ID:
            if (m_current_state.sub_state.trans_sub_state != ANTFS_TRANS_SUBSTATE_NONE)
            {
                // Ignore the command if we are busy.
                break;
            }

            if ((control_byte & ~SEQUENCE_LAST_MESSAGE) == 0x00)
            {
                // First burst packet.
...

The control byte appears to have the 5 least significant bits representing the channel number that the message was received on and the 3 most significant bits representing the sequence number.  I'm basing this on the following defines

// ////////////////////////////////////////////
/** @name Rx Burst Message Sequencing Defines
 *  @{ */
// ////////////////////////////////////////////
#define CHANNEL_NUMBER_MASK                        ((uint8_t)0x1F) ///< Valid bitfields for channel number
#define SEQUENCE_NUMBER_MASK                       ((uint8_t)0xE0) ///< Valid bitfields for burst sequence
#define SEQUENCE_NUMBER_ROLLOVER                   ((uint8_t)0x60) ///< Sequence rollover
#define SEQUENCE_FIRST_MESSAGE                     ((uint8_t)0x00) ///< Sequence indicating first burst message
#define SEQUENCE_LAST_MESSAGE                      ((uint8_t)0x80) ///< Sequence indicating last burst message
#define SEQUENCE_NUMBER_INC                        ((uint8_t)0x20) ///< Incremental sequence value

ANDing the control byte with the complement of the SEQUENCE_LAST_MESSAGE seems to be a mistake.  The code seems to function properly when the channel number is 0, but it does not function properly (it never gets inside the IF statement) if the channel number is something other than 0.

I'm guessing that the proper code would probably be something like the following.

if ((control_byte & SEQUENCE_NUMBER_MASK) == SEQUENCE_FIRST_MESSAGE)

However, I don't understand the details of the code precisely so I'm not certain this is the correct fix.  Can someone comment on what the correct fix for this problem should be?

[rschmitz]

Hi Torbjørn,

Has there been any response on this issue yet?

Thanks!

Roger

[ovrebekk]

Hi Roger

There hasn't been any updates yet, no. I will ask the developers if they can share a schedule. 

Best regards
Torbjørn

[ovrebekk]

Hi again

The developers confirmed that this is a bug and has prepared a fix that will go into the next SDK release, which is expected out next month. 

I have asked them if they can provide a patch so that you can verify the fix without having to wait for the next release, and will try to update you on that next week. 

Best regards
Torbjørn

[rschmitz]

Hello Torbjørn,

I am revisiting this issue.  The fix that was implemented seems to work.  However, in reviewing the code, we have noticed that there are 16 other cases where the control_byte is ANDed with something other than a bit mask which seems a bit odd.

• control_byte & ~SEQUENCE_LAST_MESSAGE (3 times)
• control_byte & SEQUENCE_LAST_MESSAGE (11 times)
• control_byte & SEQUENCE_NUMBER_ROLLOVER (2 times)

We are not seeing any mal-behavior from this, but we may not be testing all of the cases.  I attempted to change all 16 cases to ANDing the control_byte with the SEQUENCE_NUMBER_MASK and then making the proper comparison to the SEQUENCE_.....  However, this caused the code to fail in the following case.

static void transport_layer_cmd_decode(uint8_t control_byte, const uint8_t * p_command_buffer)
......
    case ANTFS_CMD_DOWNLOAD_ID:
    .....
            //else if (control_byte & SEQUENCE_LAST_MESSAGE)  // original code
            else if ((control_byte & SEQUENCE_NUMBER_MASK) == SEQUENCE_LAST_MESSAGE)  //Seems like what was acutally intended
            {
                // Last burst packet (download command should be two packets long).

                // Get CRC seed from host.
                m_compared_crc  = (uint16_t)p_command_buffer[DATA_INDEX_OFFSET_LOW];
                m_compared_crc |= ((uint16_t)p_command_buffer[DATA_INDEX_OFFSET_HIGH] << 8u);

                // Maximum block size allowed by host.
                m_max_block_size.bytes.byte0 = p_command_buffer[ADDRESS_PARAMETER_OFFSET_0];
                m_max_block_size.bytes.byte1 = p_command_buffer[ADDRESS_PARAMETER_OFFSET_1];
                m_max_block_size.bytes.byte2 = p_command_buffer[ADDRESS_PARAMETER_OFFSET_2];
                m_max_block_size.bytes.byte3 = p_command_buffer[ADDRESS_PARAMETER_OFFSET_3];

                // Initialize number of remaining bytes for this block to the maximum block size.
                m_bytes_remaining.data = m_max_block_size.data;

                if (p_command_buffer[INITIAL_REQUEST_OFFSET])
                {
                    // This request is the start of a new transfer.

                    // Initialize data offset for CRC calculation to the requested data offset.
                    m_saved_crc_offset = m_link_burst_index.data;
                    m_saved_buffer_crc_offset = m_link_burst_index.data;

                    // Use CRC seed provided by host for CRC checking of the data.
                    m_transfer_crc                            = m_compared_crc;
                    m_saved_transfer_crc                      = m_compared_crc;
                    m_saved_buffer_crc                        = m_compared_crc;
                    m_current_state.sub_state.trans_sub_state = ANTFS_TRANS_SUBSTATE_VERIFY_CRC;
                }
                else
                {
                    // This is a request to resume a partially completed transfer.

                    if (m_saved_crc_offset > m_link_burst_index.data)
                    {
                        // We can not check the received CRC seed as the requested offset is before
                        // our last save point.

                        // Set CRC checking as invalid.
                        m_saved_crc_offset = ANTFS_MAX_FILE_SIZE;
                    }
                    else
                    {
                        m_current_state.sub_state.trans_sub_state = ANTFS_TRANS_SUBSTATE_VERIFY_CRC;
                    }
                }

                m_is_data_request_pending = false;

                // Send download request to the application for further handling.
                event_queue_write(ANTFS_EVENT_DOWNLOAD_REQUEST);

                timeout_start(ANTFS_CONFIG_LINK_COMMAND_TIMEOUT);
                m_link_command_in_progress = ANTFS_CMD_DOWNLOAD_ID;
            }
            break;

However, this code failed because a control_byte of 0b 1010 0000 was sent.  This made the original code TRUE but what I thought the intended code was to be FALSE.

It seems almost certain that at least the 3 times that "control_byte & ~SEQUENCE_LAST_MESSAGE" is used are incorrect.  This is the same as the original situation that was posted.

Can someone look through the 16 remaining cases where control_byte is ANDed and verify that the logic is correct in all cases?

Thanks!

[ovrebekk]

Hi Roger

Thanks for the update. I have forwarded your findings to the software team again. 

I agree that simply ANDing the control_byte variable with the expected result doesn't seem like the right way to do it. 

Best regards
Torbjørn