From 19d90bd25e834698f369488e36b083698a6f21f2 Mon Sep 17 00:00:00 2001 From: Kimmo Vaisanen Date: Wed, 12 Sep 2018 10:26:44 +0300 Subject: [PATCH] Lora: Fix sticky MAC command retransmission This commit fixes the bug where sticky MAC commands were duplicated in send buffer everytime send() was called. --- features/lorawan/lorastack/mac/LoRaMac.cpp | 49 +++++++------------ .../lorawan/lorastack/mac/LoRaMacCommand.cpp | 34 ------------- .../lorawan/lorastack/mac/LoRaMacCommand.h | 24 --------- 3 files changed, 18 insertions(+), 89 deletions(-) diff --git a/features/lorawan/lorastack/mac/LoRaMac.cpp b/features/lorawan/lorastack/mac/LoRaMac.cpp index 18bd3ad56c..72e81eaa63 100644 --- a/features/lorawan/lorastack/mac/LoRaMac.cpp +++ b/features/lorawan/lorastack/mac/LoRaMac.cpp @@ -535,6 +535,7 @@ void LoRaMac::handle_data_frame(const uint8_t *const payload, _params.adr_ack_counter = 0; _mac_commands.clear_repeat_buffer(); + _mac_commands.clear_command_buffer(); if (is_multicast) { _mcps_indication.type = MCPS_MULTICAST; @@ -583,18 +584,9 @@ void LoRaMac::handle_data_frame(const uint8_t *const payload, _params.dl_frame_counter = downlink_counter; } - // This must be done before parsing the payload and the MAC commands. - // We need to reset the MacCommandsBufferIndex here, since we need - // to take retransmissions and repetitions into account. Error cases - // will be handled in function OnMacStateCheckTimerEvent. - if (_params.is_node_ack_requested) { - if (fctrl.bits.ack) { - _mac_commands.clear_command_buffer(); - _mcps_confirmation.ack_received = fctrl.bits.ack; - _mcps_indication.is_ack_recvd = fctrl.bits.ack; - } - } else { - _mac_commands.clear_command_buffer(); + if (_params.is_node_ack_requested && fctrl.bits.ack) { + _mcps_confirmation.ack_received = fctrl.bits.ack; + _mcps_indication.is_ack_recvd = fctrl.bits.ack; } uint8_t frame_len = (size - 4) - app_payload_start_index; @@ -663,6 +655,8 @@ void LoRaMac::on_radio_tx_done(lorawan_time_t timestamp) _lora_phy->set_last_tx_done(_params.channel, _is_nwk_joined, timestamp); _params.timers.aggregated_last_tx_time = timestamp; + + _mac_commands.clear_command_buffer(); } void LoRaMac::on_radio_rx_done(const uint8_t *const payload, uint16_t size, @@ -781,7 +775,6 @@ bool LoRaMac::continue_joining_process() bool LoRaMac::continue_sending_process() { if (_params.ack_timeout_retry_counter > _params.max_ack_timeout_retries) { - _mac_commands.clear_command_buffer(); _lora_time.stop(_params.timers.ack_timeout_timer); return false; } @@ -1193,7 +1186,6 @@ void LoRaMac::reset_mac_parameters(void) _mac_commands.clear_command_buffer(); _mac_commands.clear_repeat_buffer(); - _mac_commands.clear_mac_commands_in_next_tx(); _params.is_rx_window_enabled = true; @@ -1598,26 +1590,23 @@ lorawan_status_t LoRaMac::prepare_frame(loramac_mhdr_t *machdr, const uint8_t mac_commands_len = _mac_commands.get_mac_cmd_length(); if ((payload != NULL) && (_params.tx_buffer_len > 0)) { - if (_mac_commands.is_mac_command_in_next_tx() == true) { - if (mac_commands_len <= LORA_MAC_COMMAND_MAX_FOPTS_LENGTH) { - fctrl->bits.fopts_len += mac_commands_len; + if (mac_commands_len <= LORA_MAC_COMMAND_MAX_FOPTS_LENGTH) { + fctrl->bits.fopts_len += mac_commands_len; - // Update FCtrl field with new value of OptionsLength - _params.tx_buffer[0x05] = fctrl->value; + // Update FCtrl field with new value of OptionsLength + _params.tx_buffer[0x05] = fctrl->value; - const uint8_t *buffer = _mac_commands.get_mac_commands_buffer(); - for (i = 0; i < mac_commands_len; i++) { - _params.tx_buffer[pkt_header_len++] = buffer[i]; - } - } else { - _params.tx_buffer_len = mac_commands_len; - payload = _mac_commands.get_mac_commands_buffer(); - frame_port = 0; + const uint8_t *buffer = _mac_commands.get_mac_commands_buffer(); + for (i = 0; i < mac_commands_len; i++) { + _params.tx_buffer[pkt_header_len++] = buffer[i]; } + } else { + _params.tx_buffer_len = mac_commands_len; + payload = _mac_commands.get_mac_commands_buffer(); + frame_port = 0; } } else { - if ((mac_commands_len > 0) - && (_mac_commands.is_mac_command_in_next_tx() == true)) { + if (mac_commands_len > 0) { _params.tx_buffer_len = mac_commands_len; payload = _mac_commands.get_mac_commands_buffer(); frame_port = 0; @@ -1634,7 +1623,6 @@ lorawan_status_t LoRaMac::prepare_frame(loramac_mhdr_t *machdr, uint8_t *key = _params.keys.app_skey; uint32_t key_length = sizeof(_params.keys.app_skey) * 8; if (frame_port == 0) { - _mac_commands.clear_command_buffer(); key = _params.keys.nwk_skey; key_length = sizeof(_params.keys.nwk_skey) * 8; } @@ -1811,7 +1799,6 @@ void LoRaMac::disconnect() _mac_commands.clear_command_buffer(); _mac_commands.clear_repeat_buffer(); - _mac_commands.clear_mac_commands_in_next_tx(); reset_mcps_confirmation(); reset_mlme_confirmation(); diff --git a/features/lorawan/lorastack/mac/LoRaMacCommand.cpp b/features/lorawan/lorastack/mac/LoRaMacCommand.cpp index cde62355b3..7906289cc2 100644 --- a/features/lorawan/lorastack/mac/LoRaMacCommand.cpp +++ b/features/lorawan/lorastack/mac/LoRaMacCommand.cpp @@ -36,7 +36,6 @@ static const uint8_t max_eirp_table[] = { 8, 10, 12, 13, 14, 16, 18, 20, 21, 24, LoRaMacCommand::LoRaMacCommand() { - mac_cmd_in_next_tx = false; sticky_mac_cmd = false; mac_cmd_buf_idx = 0; mac_cmd_buf_idx_to_repeat = 0; @@ -99,15 +98,9 @@ void LoRaMacCommand::parse_mac_commands_to_repeat() } } - if (cmd_cnt > 0) { - mac_cmd_in_next_tx = true; - } else { - mac_cmd_in_next_tx = false; - } mac_cmd_buf_idx_to_repeat = cmd_cnt; } - void LoRaMacCommand::clear_repeat_buffer() { mac_cmd_buf_idx_to_repeat = 0; @@ -124,16 +117,6 @@ uint8_t LoRaMacCommand::get_repeat_commands_length() const return mac_cmd_buf_idx_to_repeat; } -void LoRaMacCommand::clear_mac_commands_in_next_tx() -{ - mac_cmd_in_next_tx = false; -} - -bool LoRaMacCommand::is_mac_command_in_next_tx() const -{ - return mac_cmd_in_next_tx; -} - void LoRaMacCommand::clear_sticky_mac_cmd() { sticky_mac_cmd = false; @@ -310,14 +293,6 @@ lorawan_status_t LoRaMacCommand::process_mac_commands(const uint8_t *payload, ui return ret_value; } -bool LoRaMacCommand::is_sticky_mac_command_pending() -{ - if (mac_cmd_buf_idx_to_repeat > 0) { - return true; - } - return false; -} - int32_t LoRaMacCommand::cmd_buffer_remaining() const { // The maximum buffer length must take MAC commands to re-send into account. @@ -336,7 +311,6 @@ lorawan_status_t LoRaMacCommand::add_link_check_req() mac_cmd_buffer[mac_cmd_buf_idx++] = MOTE_MAC_LINK_CHECK_REQ; // No payload for this command ret = LORAWAN_STATUS_OK; - mac_cmd_in_next_tx = true; } return ret; } @@ -348,7 +322,6 @@ lorawan_status_t LoRaMacCommand::add_link_adr_ans(uint8_t status) mac_cmd_buffer[mac_cmd_buf_idx++] = MOTE_MAC_LINK_ADR_ANS; mac_cmd_buffer[mac_cmd_buf_idx++] = status; ret = LORAWAN_STATUS_OK; - mac_cmd_in_next_tx = true; } return ret; } @@ -360,7 +333,6 @@ lorawan_status_t LoRaMacCommand::add_duty_cycle_ans() mac_cmd_buffer[mac_cmd_buf_idx++] = MOTE_MAC_DUTY_CYCLE_ANS; // No payload for this answer ret = LORAWAN_STATUS_OK; - mac_cmd_in_next_tx = true; } return ret; } @@ -375,7 +347,6 @@ lorawan_status_t LoRaMacCommand::add_rx_param_setup_ans(uint8_t status) // This is a sticky MAC command answer. Setup indication sticky_mac_cmd = true; ret = LORAWAN_STATUS_OK; - mac_cmd_in_next_tx = true; } return ret; } @@ -390,7 +361,6 @@ lorawan_status_t LoRaMacCommand::add_dev_status_ans(uint8_t battery, uint8_t mar mac_cmd_buffer[mac_cmd_buf_idx++] = battery; mac_cmd_buffer[mac_cmd_buf_idx++] = margin; ret = LORAWAN_STATUS_OK; - mac_cmd_in_next_tx = true; } return ret; } @@ -403,7 +373,6 @@ lorawan_status_t LoRaMacCommand::add_new_channel_ans(uint8_t status) // Status: Datarate range OK, Channel frequency OK mac_cmd_buffer[mac_cmd_buf_idx++] = status; ret = LORAWAN_STATUS_OK; - mac_cmd_in_next_tx = true; } return ret; } @@ -417,7 +386,6 @@ lorawan_status_t LoRaMacCommand::add_rx_timing_setup_ans() // This is a sticky MAC command answer. Setup indication sticky_mac_cmd = true; ret = LORAWAN_STATUS_OK; - mac_cmd_in_next_tx = true; } return ret; } @@ -429,7 +397,6 @@ lorawan_status_t LoRaMacCommand::add_tx_param_setup_ans() mac_cmd_buffer[mac_cmd_buf_idx++] = MOTE_MAC_TX_PARAM_SETUP_ANS; // No payload for this answer ret = LORAWAN_STATUS_OK; - mac_cmd_in_next_tx = true; } return ret; } @@ -444,7 +411,6 @@ lorawan_status_t LoRaMacCommand::add_dl_channel_ans(uint8_t status) // This is a sticky MAC command answer. Setup indication sticky_mac_cmd = true; ret = LORAWAN_STATUS_OK; - mac_cmd_in_next_tx = true; } return ret; } diff --git a/features/lorawan/lorastack/mac/LoRaMacCommand.h b/features/lorawan/lorastack/mac/LoRaMacCommand.h index dfbcd29608..aec31566c4 100644 --- a/features/lorawan/lorastack/mac/LoRaMacCommand.h +++ b/features/lorawan/lorastack/mac/LoRaMacCommand.h @@ -97,18 +97,6 @@ public: */ uint8_t get_repeat_commands_length() const; - /** - * @brief Clear MAC commands in next TX. - */ - void clear_mac_commands_in_next_tx(); - - /** - * @brief Check if MAC command buffer has commands to be sent in next TX - * - * @return status True: buffer has MAC commands to be sent, false: no commands in buffer - */ - bool is_mac_command_in_next_tx() const; - /** * @brief Clear sticky MAC commands. */ @@ -132,13 +120,6 @@ public: lora_mac_system_params_t& mac_params, LoRaPHY& lora_phy); - /** - * @brief Verifies if sticky MAC commands are pending. - * - * @return [true: sticky MAC commands pending, false: No MAC commands pending] - */ - bool is_sticky_mac_command_pending(); - /** * @brief Adds a new LinkCheckReq MAC command to be sent. * @@ -237,11 +218,6 @@ private: lorawan_status_t add_dl_channel_ans(uint8_t status); private: - /** - * Indicates if the MAC layer wants to send MAC commands - */ - bool mac_cmd_in_next_tx; - /** * Indicates if there are any pending sticky MAC commands */