Revision 64:28ef790e4ef7, committed 2017-07-03

Comitter:

Wolfgang Betz

Date:

Mon Jul 03 14:39:01 2017 +0200

Parent:

63:d7530f62ed93

Child:

65:a16f0064110a

Commit message:

Implement indications received by Kevin Bracey

Date: Mon, 3 Jul 2017 10:14:23 +0000
From: Kevin Bracey <notifications@github.com>

https://github.com/ARMmbed/mbed-os-example-client/issues/266#issuecomment-312606944

I've just spent a little while reviewing the Spirit driver code, just to see if I can see any obvious flaws. (Hard to say much without knowing the hardware, but I can look for general issues.)

I'm a bit wary about the software ack handling - can be tricky.

There is one specific problem that could be affecting performance now - it seems to me the acks are sent with a common send() routine that enables hardware CSMA-CD. An ack should be being sent 192us after transmission completion, without CSMA. Backing off the ack will greatly reduce the chance of packets being successfully acknowledged.

Other notes on ack reception - you're calling TX_DONE whenever tx_sequence == seq_number, whether you were expecting an ack or not. This could cause stack confusion in various ways (eg if you were backing off while someone else used the same sequence number). You should only process an ACK when you actually expect one (TX completed, and AR bit was set in it)

Also, while expecting an ack, it can be beneficial to report TX_FAIL and stop expecting when you receive anything other then an ack with the expected sequence number. The stack will eventually time out if it doesn't get TX_DONE, but receipt of anything else (including a wrongly-numbered ack) indicates a lack of acknowledgment, which can be reported immediately.

Changed in this revision

--- a/source/NanostackRfPhySpirit1.cpp	Mon Jun 26 18:04:11 2017 +0200
+++ b/source/NanostackRfPhySpirit1.cpp	Mon Jul 03 14:39:01 2017 +0200
@@ -37,6 +37,7 @@
 static Thread rf_ack_sender(osPriorityRealtime);
 static volatile uint8_t rf_rx_sequence;
 static volatile bool rf_ack_sent = false;
+static volatile bool expecting_ack = false;
 
 /* MAC frame helper macros */
 #define MAC_FCF_FRAME_TYPE_MASK         0x0007
@@ -103,16 +104,13 @@
         /*Return busy*/
         return -1;
     } else {
-#ifdef HEAVY_TRACING
         uint16_t fcf = rf_read_16_bit(data_ptr);
-        uint16_t need_ack;
 
         /*Check if transmitted data needs to be acked*/
         if((fcf & MAC_FCF_ACK_REQ_BIT_MASK) >> MAC_FCF_ACK_REQ_BIT_SHIFT)
-            need_ack = 1;
+            expecting_ack = true;
         else
-            need_ack = 0;
-#endif
+            expecting_ack = false;
 
         /*Store the sequence number for ACK handling*/
         tx_sequence = *(data_ptr + 2);
@@ -121,8 +119,8 @@
         mac_tx_handle = tx_handle;
 
 #ifdef HEAVY_TRACING
-        tr_info("%s (%d), len=%d, tx_handle=%x, tx_seq=%x, need_ack=%d (%x:%x, %x:%x, %x:%x, %x:%x)", __func__, __LINE__,
-                data_length, tx_handle, tx_sequence, need_ack,
+        tr_info("%s (%d), len=%d, tx_handle=%x, tx_seq=%x, expecting_ack=%d (%x:%x, %x:%x, %x:%x, %x:%x)", __func__, __LINE__,
+                data_length, tx_handle, tx_sequence, expecting_ack,
                 data_ptr[3], data_ptr[4], data_ptr[5], data_ptr[6],
                 data_ptr[7], data_ptr[8], data_ptr[9], data_ptr[10]);
 #endif
@@ -289,7 +287,7 @@
     rf_ack_sender.signal_set(signal);
 }
 
-static phy_link_tx_status_e phy_status;
+static volatile phy_link_tx_status_e phy_status;
 /* Note: we are in IRQ context */
 static void rf_handle_ack(uint8_t seq_number)
 {
@@ -309,6 +307,12 @@
 #ifdef HEAVY_TRACING
         tr_info("%s (%d)", __func__, __LINE__);
 #endif
+
+        /*Call PHY TX Done API*/
+        if(device_driver.phy_tx_done_cb){
+            phy_status = PHY_LINK_TX_FAIL;
+            rf_send_signal(RF_SIG_CB_TX_DONE);
+        }
     }
 }
 
@@ -493,14 +497,24 @@
     }
 
     /* If waiting for ACK, check here if the packet is an ACK to a message previously sent */
-    uint16_t fcf = rf_read_16_bit(rf_rx_buf);
-    if(((fcf & MAC_FCF_FRAME_TYPE_MASK) >> MAC_FCF_FRAME_TYPE_SHIFT) == FC_ACK_FRAME) {
-        /*Send sequence number in ACK handler*/
+    if(expecting_ack) {
+        uint16_t fcf = rf_read_16_bit(rf_rx_buf);
+        expecting_ack = false;
+
+        if(((fcf & MAC_FCF_FRAME_TYPE_MASK) >> MAC_FCF_FRAME_TYPE_SHIFT) == FC_ACK_FRAME) {
+            /*Send sequence number in ACK handler*/
 #ifdef HEAVY_TRACING
-        tr_debug("%s (%d), len=%u", __func__, __LINE__, (unsigned int)rf_buffer_len);
+            tr_debug("%s (%d), len=%u", __func__, __LINE__, (unsigned int)rf_buffer_len);
 #endif
-        rf_handle_ack(rf_rx_buf[2]);
-        return;
+            rf_handle_ack(rf_rx_buf[2]);
+            return;
+        } else {
+            /*Call PHY TX Done API*/
+            if(device_driver.phy_tx_done_cb){
+                phy_status = PHY_LINK_TX_FAIL;
+                rf_send_signal(RF_SIG_CB_TX_DONE);
+            }
+        }
     }
 
     /* Kick off ACK sending */
@@ -542,7 +556,7 @@
 
     /*Call PHY TX Done API*/
     if(device_driver.phy_tx_done_cb){
-        phy_status = PHY_LINK_TX_SUCCESS;
+        phy_status = expecting_ack ? PHY_LINK_TX_DONE_PENDING : PHY_LINK_TX_SUCCESS;
         rf_send_signal(RF_SIG_CB_TX_DONE);
     }
 }
@@ -551,6 +565,7 @@
 static inline void rf_handle_tx_err(void) {
     /*Call PHY TX Done API*/
     if(device_driver.phy_tx_done_cb){
+        expecting_ack = false;
         phy_status = PHY_LINK_TX_FAIL;
         rf_send_signal(RF_SIG_CB_TX_DONE);
     }
@@ -627,7 +642,7 @@
             rf_ack_sent = true;
 
             /*Send the packet*/
-            rf_device->send((uint8_t*)buffer, 3);
+            rf_device->send((uint8_t*)buffer, 3, false);
 
 #ifdef HEAVY_TRACING
             tr_debug("%s (%d), hdr=%x, nr=%x", __func__, __LINE__, buffer[0], ptr[0]);

--- a/source/SimpleSpirit1.cpp	Mon Jun 26 18:04:11 2017 +0200
+++ b/source/SimpleSpirit1.cpp	Mon Jul 03 14:39:01 2017 +0200
@@ -161,7 +161,7 @@
 static volatile int tx_fifo_remaining = 0;     // to be used in irq handler
 static volatile int tx_buffer_pos = 0;         // to be used in irq handler
 const volatile uint8_t *tx_fifo_buffer = NULL; // to be used in irq handler
-int SimpleSpirit1::send(const void *payload, unsigned int payload_len) {
+int SimpleSpirit1::send(const void *payload, unsigned int payload_len, bool use_csma_ca) {
 	/* Checks if the payload length is supported */
 	if(payload_len > MAX_PACKET_LEN) {
 		return RADIO_TX_ERR;
@@ -187,7 +187,9 @@
 
 	pkt_basic_set_payload_length(payload_len); // set desired payload len
 
-	csma_ca_state(S_ENABLE); // enable CSMA/CA
+	if(use_csma_ca) {
+	    csma_ca_state(S_ENABLE); // enable CSMA/CA
+	}
 
 	/* Init buffer & number of bytes to be send */
 	tx_fifo_remaining = payload_len;
@@ -220,7 +222,9 @@
 
 	_spirit_tx_started = false; // in case of state timeout
 
-	csma_ca_state(S_DISABLE); // disable CSMA/CA
+    if(use_csma_ca) {
+        csma_ca_state(S_DISABLE); // disable CSMA/CA
+    }
 
 	cmd_strobe(SPIRIT1_STROBE_RX); // Return to RX state
 

--- a/stm-spirit1-rf-driver/SimpleSpirit1.h	Mon Jun 26 18:04:11 2017 +0200
+++ b/stm-spirit1-rf-driver/SimpleSpirit1.h	Mon Jul 03 14:39:01 2017 +0200
@@ -416,7 +416,7 @@
     }
 
     /** Send a Buffer **/
-    int send(const void *payload, unsigned int payload_len);
+    int send(const void *payload, unsigned int payload_len, bool use_csma_ca = true);
 
     /** Read into Buffer **/
     int read(void *buf, unsigned int bufsize);