[bug report] Shockburst sends wrong acknowledge payload

[Yannick_Alflex]

Hello,

I have found a bug within the current (and earlier) release of the NRF SDK.

I have a project where I am using ESB to communicate. There is a primary transmitter sending data, and a primary receiver receiving this data. The primary receiver has one packet preloaded to send as acknowledge to the primary transmitter. Due to how the project is set up, I want to ensure there is only one packet in the receiver's TX fifo. To ensure this, I call nrf_esb_flush_tx().

Instead of properly emptying the fifo, the flush function simply sets the fifo count to 0, and the entry and exit point to 0 as well. This is fine, but highlights a bug elsewhere in the code.

Within the function on_radio_disabled_rx(), a check is performed to see if the preloaded packet is on the same pipe as the just received packet. After this check, the exit point is incremented, and the packet on the exit point location is sent as acknowledge.

if (m_tx_fifo.count > 0 &&
            (m_tx_fifo.p_payload[m_tx_fifo.exit_point]->pipe == NRF_RADIO->RXMATCH)
           )
        {
            // Pipe stays in ACK with payload until TX FIFO is empty
            // Do not report TX success on first ack payload or retransmit
            if (p_pipe_info->ack_payload == true && !retransmit_payload)
            {
                if (++m_tx_fifo.exit_point >= NRF_ESB_TX_FIFO_SIZE)
                {
                    m_tx_fifo.exit_point = 0;
                }

                m_tx_fifo.count--;

                // ACK payloads also require TX_DS
                // (page 40 of the 'nRF24LE1_Product_Specification_rev1_6.pdf').
                m_interrupt_flags |= NRF_ESB_INT_TX_SUCCESS_MSK;
            }

            p_pipe_info->ack_payload = true;

            mp_current_payload = m_tx_fifo.p_payload[m_tx_fifo.exit_point];

Let's assume the preloaded packet is on pipe 1, and the received packet was also on pipe 1. As we only ever have one packet preloaded, this will always be in the fifo on position 0.

The check if there is a matching pipe will succeed, as they are the same. After this the exit point is incremented by one, setting it from 0 to 1. The exit point is now pointing to the wrong location, as there is no packet stored on location 1. Therefore, a packet without data will be sent as acknowledge.

For my implementation, this can be easily fixed by putting my acknowledge packets into the TX fifo twice, both on position 0 and 1. A more permanent fix, (one the doesn't increment the exit point prematurely) would be a lot better.

[Jørgen Holmefjord]

Hi,

This looks similar to the issue described in this post. Can you check if the proposed changes fix the issue you are seeing?

Best regards,
Jørgen

[Yannick_Alflex]

Dear Jørgen,

This is indeed the correct issue. The proposed fix does however not solve my issue. The proposed fix is in fact flawed as well. In the proposed fix, the fifo exit point is incremented before checking pipe and payload, causing the exit point to be 1, and will still cause an empty acknowledge being sent.

In the meantime I have implemented my own fix. This fix works for my use case. The fix is, as mentioned earlier, to not prematurely increment the exit point.

The following code is part of the on_radio_disabled_rx() function, in nrf_esb.c

case NRF_ESB_PROTOCOL_ESB_DPL:
    {
        if (m_tx_fifo.count > 0 &&
            (m_tx_fifo.p_payload[m_tx_fifo.exit_point]->pipe == NRF_RADIO->RXMATCH)
           )
        {

            mp_current_payload = m_tx_fifo.p_payload[m_tx_fifo.exit_point];

            update_rf_payload_format(mp_current_payload->length);
            m_tx_payload_buffer[0] = mp_current_payload->length;
            memcpy(&m_tx_payload_buffer[2],
                   mp_current_payload->data,
                   mp_current_payload->length);

            p_pipe_info->ack_payload = true;
            // Pipe stays in ACK with payload until TX FIFO is empty
            // Do not report TX success on first ack payload or retransmit
            if (p_pipe_info->ack_payload == true && !retransmit_payload)
            {
                if (++m_tx_fifo.exit_point >= NRF_ESB_TX_FIFO_SIZE)
                {
                    m_tx_fifo.exit_point = 0;
                }

                m_tx_fifo.count--;

                // ACK payloads also require TX_DS
                // (page 40 of the 'nRF24LE1_Product_Specification_rev1_6.pdf').
                m_interrupt_flags |= NRF_ESB_INT_TX_SUCCESS_MSK;
            }
        }
        else
        {
            p_pipe_info->ack_payload = false;
            update_rf_payload_format(0);
            m_tx_payload_buffer[0] = 0;
        }

        m_tx_payload_buffer[1] = m_rx_payload_buffer[1];
    }
    break;

Why was this not fixed in the latest release of the SDK? This issue is nearly 3.5 years old, and the previous "release of the nRF5 SDK was limited in scope" does not apply, as the latest release was a major update.

[Jørgen Holmefjord]

Thanks for your feedback. I have bumped the internal report and added a link to your proposed fix.

I will check with the developers if this can be fixed for a future release.