FRMPayload size validity

It was pointed out in #7432 and #7232 that the stack was comparing frame payload size
with the allowed payload size in a wrong manner in shcedule_tx().
We must strip the overhead from the frame before comparison.

We did have a similar check in prepare_ongoing_tx() API which would correctly analyse
the situation but a check was needed in schedule_tx() as well. The reason is that the
schedule_tx() API can be called automatically by the stack if the user intiated requested
was not promptly entertained because of duty cycle restriction. Now, the datarate can change
here (for CONFIRMED messages if the ack was not received after retries max out). That's why
a test for validity was needed.

We now perform a comparison using _ongoing_tx_message structure which contains the actual
FRMPayload size.

For proprietary type of messages only MHDR and Port field is used so we shouldn't add MAC commands
and other overhead into them.

In order to have consistent frame overhead, we have opted to always include Port field in the
frame.

Author: Hasnain Virk

features/lorawan/lorastack/mac/LoRaMac.cpp

@@ -1033,6 +1033,7 @@ lorawan_status_t LoRaMac::schedule_tx()
 {
     channel_selection_params_t next_channel;
     lorawan_time_t backoff_time = 0;
+    uint8_t fopts_len = 0;
 
     if (_params.sys_params.max_duty_cycle == 255) {
         return LORAWAN_STATUS_DEVICE_OFF;
@@ -1090,9 +1091,25 @@ lorawan_status_t LoRaMac::schedule_tx()
         _params.rx_window2_delay = _params.sys_params.join_accept_delay2
                                    + _params.rx_window2_config.window_offset;
     } else {
-        if (validate_payload_length(_params.tx_buffer_len,
+
+        // if the outgoing message is a proprietary message, it doesn't include any
+        // standard message formatting except port and MHDR.
+        if (_ongoing_tx_msg.type == MCPS_PROPRIETARY) {
+            fopts_len = 0;
+        } else {
+            fopts_len = _mac_commands.get_mac_cmd_length() + _mac_commands.get_repeat_commands_length();
+        }
+
+        // A check was performed for validity of FRMPayload in ::prepare_ongoing_tx() API.
+        // However, owing to the asynch nature of the send() API, we should check the
+        // validity again, as datarate may have changed since we last attempted to transmit.
+        if (validate_payload_length(_ongoing_tx_msg.f_buffer_size,
                                     _params.sys_params.channel_data_rate,
-                                    _mac_commands.get_mac_cmd_length()) == false) {
+                                    fopts_len) == false) {
+            tr_error("Allowed FRMPayload = %d, FRMPayload = %d, MAC commands pending = %d",
+                     _lora_phy->get_max_payload(_params.sys_params.channel_data_rate,
+                                               _params.is_repeater_supported),
+                                               _ongoing_tx_msg.f_buffer_size, fopts_len);
             return LORAWAN_STATUS_LENGTH_ERROR;
         }
         _params.rx_window1_delay = _params.sys_params.recv_delay1
@@ -1215,27 +1232,9 @@ int16_t LoRaMac::prepare_ongoing_tx(const uint8_t port,
                                     uint8_t num_retries)
 {
     _ongoing_tx_msg.port = port;
-
-    uint8_t max_possible_size = get_max_possible_tx_size(length);
-
-    if (max_possible_size > MBED_CONF_LORA_TX_MAX_SIZE) {
-        max_possible_size = MBED_CONF_LORA_TX_MAX_SIZE;
-    }
-
-    if (max_possible_size < length) {
-        tr_info("Cannot transmit %d bytes. Possible TX Size is %d bytes",
-                length, max_possible_size);
-
-        _ongoing_tx_msg.pending_size = length - max_possible_size;
-        _ongoing_tx_msg.f_buffer_size = max_possible_size;
-        memcpy(_ongoing_tx_msg.f_buffer, data, _ongoing_tx_msg.f_buffer_size);
-    } else {
-        _ongoing_tx_msg.f_buffer_size = length;
-        _ongoing_tx_msg.pending_size = 0;
-        if (length > 0) {
-            memcpy(_ongoing_tx_msg.f_buffer, data, length);
-        }
-    }
+    uint8_t max_possible_size = 0;
+    uint8_t fopts_len = _mac_commands.get_mac_cmd_length()
+            + _mac_commands.get_repeat_commands_length();
 
     // Handles unconfirmed messages
     if (flags & MSG_UNCONFIRMED_FLAG) {
@@ -1256,6 +1255,30 @@ int16_t LoRaMac::prepare_ongoing_tx(const uint8_t port,
         _ongoing_tx_msg.type = MCPS_PROPRIETARY;
         _ongoing_tx_msg.fport = port;
         _ongoing_tx_msg.nb_trials = 1;
+        // a proprietary frame only includes an MHDR field which contains MTYPE field.
+        // Everything else is at the discretion of the implementer
+        fopts_len = 0;
+    }
+
+    max_possible_size = get_max_possible_tx_size(fopts_len);
+
+    if (max_possible_size > MBED_CONF_LORA_TX_MAX_SIZE) {
+        max_possible_size = MBED_CONF_LORA_TX_MAX_SIZE;
+    }
+
+    if (max_possible_size < length) {
+        tr_info("Cannot transmit %d bytes. Possible TX Size is %d bytes",
+                length, max_possible_size);
+
+        _ongoing_tx_msg.pending_size = length - max_possible_size;
+        _ongoing_tx_msg.f_buffer_size = max_possible_size;
+        memcpy(_ongoing_tx_msg.f_buffer, data, _ongoing_tx_msg.f_buffer_size);
+    } else {
+        _ongoing_tx_msg.f_buffer_size = length;
+        _ongoing_tx_msg.pending_size = 0;
+        if (length > 0) {
+            memcpy(_ongoing_tx_msg.f_buffer, data, length);
+        }
     }
 
     tr_info("RTS = %u bytes, PEND = %u, Port: %u",
@@ -1566,8 +1589,10 @@ lorawan_status_t LoRaMac::prepare_frame(loramac_mhdr_t *machdr,
 
             _mac_commands.parse_mac_commands_to_repeat();
 
+            // We always add Port Field. Spec leaves it optional.
+            _params.tx_buffer[pkt_header_len++] = frame_port;
+
             if ((payload != NULL) && (_params.tx_buffer_len > 0)) {
-                _params.tx_buffer[pkt_header_len++] = frame_port;
 
                 uint8_t *key = _params.keys.app_skey;
                 uint32_t key_length = sizeof(_params.keys.app_skey) * 8;
@@ -1759,35 +1784,30 @@ void LoRaMac::disconnect()
     reset_mcps_indication();
 }
 
-uint8_t LoRaMac::get_max_possible_tx_size(uint8_t size)
+uint8_t LoRaMac::get_max_possible_tx_size(uint8_t fopts_len)
 {
     uint8_t max_possible_payload_size = 0;
-    uint8_t current_payload_size = 0;
-    uint8_t fopt_len = _mac_commands.get_mac_cmd_length()
-                       + _mac_commands.get_repeat_commands_length();
+    uint8_t allowed_frm_payload_size = 0;
 
     if (_params.sys_params.adr_on) {
         _lora_phy->get_next_ADR(false, _params.sys_params.channel_data_rate,
                                _params.sys_params.channel_tx_power,
                                _params.adr_ack_counter);
     }
 
-    current_payload_size = _lora_phy->get_max_payload(_params.sys_params.channel_data_rate, _params.is_repeater_supported);
+    allowed_frm_payload_size = _lora_phy->get_max_payload(_params.sys_params.channel_data_rate,
+                                                          _params.is_repeater_supported);
 
-    if (current_payload_size >= fopt_len) {
-        max_possible_payload_size = current_payload_size - fopt_len;
+    if (allowed_frm_payload_size >= fopts_len) {
+        max_possible_payload_size = allowed_frm_payload_size - fopts_len;
     } else {
-        max_possible_payload_size = current_payload_size;
-        fopt_len = 0;
+        max_possible_payload_size = allowed_frm_payload_size;
+        fopts_len = 0;
         _mac_commands.clear_command_buffer();
         _mac_commands.clear_repeat_buffer();
     }
 
-    if (validate_payload_length(size, _params.sys_params.channel_data_rate,
-                                fopt_len) == false) {
-        return max_possible_payload_size;
-    }
-    return current_payload_size;
+    return max_possible_payload_size;
 }
 
 bool LoRaMac::nwk_joined()

features/lorawan/lorastack/mac/LoRaMac.h

@@ -93,18 +93,18 @@ class LoRaMac {
     void disconnect(void);
 
     /**
-     * @brief   Queries the LoRaMAC whether it is possible to send the next frame with
-     *          a given payload size. The LoRaMAC takes the scheduled MAC commands into
-     *          account and returns corresponding value.
+     * @brief   Queries the LoRaMAC the maximum possible FRMPayload size to send.
+     *          The LoRaMAC takes the scheduled MAC commands into account and returns
+     *          corresponding value.
      *
-     * @param   size     [in]    The size of the applicable payload to be sent next.
+     * @param   fopts_len     [in]    Number of mac commands in the queue pending.
      *
      * @return  Size of the biggest packet that can be sent.
      *          Please note that if the size of the MAC commands in the queue do
      *          not fit into the payload size on the related datarate, the LoRaMAC will
      *          omit the MAC commands.
      */
-    uint8_t get_max_possible_tx_size(uint8_t size);
+    uint8_t get_max_possible_tx_size(uint8_t fopts_len);
 
     /**
      * @brief nwk_joined Checks if device has joined to network
@@ -316,13 +316,18 @@ class LoRaMac {
 
     /**
      * @brief prepare_ongoing_tx This will prepare (and override) ongoing_tx_msg.
-     * @param port The application port number.
-     * @param data A pointer to the data being sent. The ownership of the
-     *             buffer is not transferred.
-     * @param length The size of data in bytes.
-     * @param flags A flag used to determine what type of
-     *              message is being sent.
-     * @param num_retries Number of retries for a confirmed type message
+     * @param port                          The application port number.
+     *
+     * @param data                          A pointer to the data being sent. The ownership of the
+     *                                      buffer is not transferred.
+     *
+     * @param length                        The size of data in bytes.
+     *
+     * @param flags                         A flag used to determine what type of
+     *                                      message is being sent.
+     *
+     * @param num_retries                   Number of retries for a confirmed type message
+     *
      * @return The number of bytes prepared for sending.
      */
     int16_t prepare_ongoing_tx(const uint8_t port, const uint8_t *data,