Question about IsSticky handling in LoRaMacCommands.c

[catsup]

Hello,

I am a little confused about the purpose of the CIDsStickyAnsCmds[] array. It appears to be used to doublecheck that a command is really sticky before removing it from the linked list:

        if( curElement->IsSticky == true )
        {
            for( uint8_t i = 0; i < sizeof( CIDsStickyAnsCmds ); i++ )
            {
                if( curElement->CID == CIDsStickyAnsCmds[i] )
                {
                    LoRaMacCommandsRemoveCmd( curElement );
                    break;
                }
            }
        }
        curElement = nexElement;

Why not use IsSticky() instead ?

Better yet, why bother at all, since curElement->IsSticky already shows whether the command is sticky ?

IMV, the above could be reduced to

        if( curElement->IsSticky == true )
        {
            LoRaMacCommandsRemoveCmd( curElement );
        }
        curElement = nexElement;

and CIDsStickyAnsCmds[] could be removed from the code.

Did I miss something ?

[ConnyBusy]

Hi catsup,

see issue #529 which lead to that change from pullrequest #5b4ac08

[catsup]

Hi ConnyBusy,

yes, that explains why this change was made, removing all rather than only the first sticky mac element.

However, the way this was implemented is IMHO unnecessarily complicated, and could be simplified as shown above.

Regards,
Marc

[ConnyBusy]

Hi Marc,

I think you are right the for loop is unnecessary. Your proposed code is better
Why not use IsSticky() instead ?
each MAC Command is stored already with the IsSticky boolean member which is confusing with the function IsSticky. I think it would have been better to use isSticky(lowercase i) for the member instead

Best regards

[djaeckle]

Hi all,

thanks for the report. I agree that this can be simplified. We will provide a fix for it.

[msmits-definium]

This change breaks V1.1:
After joining, the MAC command RekeyInd is sent as DataUp (CID 11). The server responds with RekeyConf. This is now removed from the mac commands list because there is no check with CIDsStickyAnsCmds[]. It is however also removed a second time in ProcessMacCommands, case SRV_MAC_REKEY_CONF.

    case SRV_MAC_REKEY_CONF:
    {
        uint8_t serverMinorVersion = payload[macIndex++];

        // Compare own LoRaWAN Version with server's
        if( MacCtx.NvmCtx->Version.Fields.Minor == serverMinorVersion )
        {
            // If they equal remove the sticky RekeyInd MAC-Command.
            LoRaMacCommandsGetCmd( MOTE_MAC_REKEY_IND, &macCmd);
            LoRaMacCommandsRemoveCmd( macCmd );
        }
        break;
    }

Because there is no check on the return value of LoRaMacCommandsGetCmd, the remove proceeds and results in an invalid deletion on the underlying linked list, It causes a hardfault on my LPC 54628.

See trace below which gives an idea of th flow:

[lorawan] initialisation
[lorawan] join;attempt=0
[lorawan] join=executing
[lorawan] lwn_network_task RECV
DEBUG - ProcessRadioTxDone
$ [lorawan] lwn_network_task RECV
DEBUG - ProcessRadioRxDone
LoRaMacCommandsAddCmd - cid: 11 <---- mac command added
DEBUG - ProcessRadioRxDone - exit
[lorawan] JOIN OK
[lorawan] MIB_DEV_ADDR: FC0023C4
[lorawan] join=success
[lorawan] session=get
[lorawan] SEND START
[lorawan] lwn_network_task RECV
DEBUG - ProcessRadioTxDone
[lorawan] lwn_network_task RECV
DEBUG - ProcessRadioRxDone
LoRaMacCommandsRemoveCmd - cid: 11 <---- mac command removed first time, OK
ProcessMacCommands - payload[macIndex]: 11
LoRaMacCommandsRemoveCmd - cid: 181 <----- mac command remove attempted 2nd time but on empty list, FAIL

[djaeckle]

Hi msmits-definium,

i will double check it and will come back to you soon. Could you please open up a new issue for it?

EDIT: Could you also provide a short description about the fail state? What is the consequence ?

[msmits-definium]

Hi djaeckle thanks for your quick response. I've created issue #772 Regarding your question as to the fail state and the consequence: as mentioned in my original comment:
The fail state is a double deletion on the mac commands linked list.
The consequence is a crash / hardfault of my system (an LPC 54628 based board).