Lora-net / SWL2001

LoRa Basics Modem LoRaWAN stack
BSD 3-Clause Clear License
87 stars 50 forks source link

McGroupID wrong bit mask format #30

Closed MpmTwtg closed 5 months ago

MpmTwtg commented 6 months ago

According to ts005-2-0-0-remote-multicast-setup the McGroupID is 2 bit,

currently it looks like the stack is handling it as 4bit. https://github.com/Lora-net/SWL2001/blob/ffca22633d111ef2874fa7c2f35498a57d87f3f0/lbm_lib/smtc_modem_core/lorawan_packages/fragmented_data_block_transport/v1.0.0/lorawan_fragmentation_package_v1.0.0.c#L555C1-L556C98

Here is my suggested fix.

frag_session_data_tmp.frag_group_data.frag_session.mc_group_bit_mask = 1 <<
                (fragmentation_package_rx_buffer[fragmentation_package_rx_buffer_index + 1] & 0x3);

Same for version 2.

I also have a question about the purpose of the function after it on line 718:

if( is_received_on_multicast_window( fragmentation_package_rx_window ) == false )
{
    accept_data = true;
}

this is set to false, if it was set to true the previous code is redunent and why is it false?

lbm-team commented 6 months ago

McGroupID is 2bits but here it is a mc_group_bit_mask which is on 4 bits. the ts005-2-0-0-remote-multicast-setup page 8 explains it in detail.

MpmTwtg commented 6 months ago

you refere to the group status 4.2 this is for 4.3 Multicast Group Setup Commands, page 9

lbm-team commented 6 months ago

this piece of code implements the "LoRaWAN Fragmented Data Block Transport Specification"(TS004), for the specific cmd : FragSessionSetupReq (describe page 8 ;-)).

MpmTwtg commented 6 months ago

image

McGroupID in the current release is not translated to a mask mc_group_bit_mask .

If McGroupID is 0 with this code:

frag_session_data_tmp.frag_group_data.frag_session.mc_group_bit_mask = 
             fragmentation_package_rx_buffer[fragmentation_package_rx_buffer_index + 1] & 0xF;

Follow up state checks will fail, like this if statement in the same file on line 675:

if( ( frag_session_data[frag_index].frag_group_data.frag_session.mc_group_bit_mask & 0x1 ) == 0x1 )
            {
                if( ( fragmentation_package_rx_window == RECEIVE_ON_RXB_MC_GRP0 ) ||
                    ( fragmentation_package_rx_window == RECEIVE_ON_RXC_MC_GRP0 ) )
                {
                    accept_data = true;
                }
            }

That's why I suggest to change it to:

frag_session_data_tmp.frag_group_data.frag_session.mc_group_bit_mask = 1 <<
                (fragmentation_package_rx_buffer[fragmentation_package_rx_buffer_index + 1] & 0x3);
lbm-team commented 6 months ago

let me keep your example, if McGroupID is 0 then .mc_group_bit_mask = 1 and so accept_data= true in : if( ( frag_session_data[frag_index].frag_group_data.frag_session.mc_group_bit_mask & 0x1 ) == 0x1 ) { if( ( fragmentation_package_rx_window == RECEIVE_ON_RXB_MC_GRP0 ) || ( fragmentation_package_rx_window == RECEIVE_ON_RXC_MC_GRP0 ) ) { accept_data = true; } }