From 9a77b5d05f89db5c07b8a6a4d0cee65253a5febe Mon Sep 17 00:00:00 2001 From: Hasnain Virk Date: Tue, 10 Jul 2018 13:44:09 +0300 Subject: [PATCH 1/2] 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. --- features/lorawan/lorastack/mac/LoRaMac.cpp | 96 +++++++++++++--------- features/lorawan/lorastack/mac/LoRaMac.h | 29 ++++--- 2 files changed, 75 insertions(+), 50 deletions(-) diff --git a/features/lorawan/lorastack/mac/LoRaMac.cpp b/features/lorawan/lorastack/mac/LoRaMac.cpp index 1e32fc02a8..dfbd2344ff 100644 --- a/features/lorawan/lorastack/mac/LoRaMac.cpp +++ b/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,12 +1784,10 @@ 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, @@ -1772,22 +1795,19 @@ uint8_t LoRaMac::get_max_possible_tx_size(uint8_t size) _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() diff --git a/features/lorawan/lorastack/mac/LoRaMac.h b/features/lorawan/lorastack/mac/LoRaMac.h index cb3ebbd738..81d560624f 100644 --- a/features/lorawan/lorastack/mac/LoRaMac.h +++ b/features/lorawan/lorastack/mac/LoRaMac.h @@ -93,18 +93,18 @@ public: 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 @@ public: /** * @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, From ed9048f79c7f19fc4c7434a3a434afc34bf985fe Mon Sep 17 00:00:00 2001 From: Hasnain Virk Date: Thu, 12 Jul 2018 12:14:25 +0300 Subject: [PATCH 2/2] Correcting unit for timeout timeout unit should be ms not micro second. --- features/lorawan/LoRaRadio.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/features/lorawan/LoRaRadio.h b/features/lorawan/LoRaRadio.h index feaceb0bc3..e09560d7d9 100644 --- a/features/lorawan/LoRaRadio.h +++ b/features/lorawan/LoRaRadio.h @@ -281,7 +281,7 @@ public: * @param iq_inverted Inverts IQ signals (LoRa only) * FSK : N/A (set to 0). * LoRa: [0: not inverted, 1: inverted] - * @param timeout The transmission timeout [us]. + * @param timeout The transmission timeout [ms]. */ virtual void set_tx_config(radio_modems_t modem, int8_t power, uint32_t fdev, uint32_t bandwidth, uint32_t datarate,