Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TimerInit called multiple times on the same TimerEvent_t struct #1587

Open
nfroggy opened this issue Nov 28, 2023 · 1 comment
Open

TimerInit called multiple times on the same TimerEvent_t struct #1587

nfroggy opened this issue Nov 28, 2023 · 1 comment

Comments

@nfroggy
Copy link

nfroggy commented Nov 28, 2023

Hi,

On a Class B device, calling LmHandlerJoin will call LmHandlerJoinRequest, which calls LoRaMacMlmeRequest, which calls ResetMacParameters, which calls LoRaMacClassBInit.

The issue is that the 3 timers in that function will get TimerInit called on them multiple times if there's multiple join requests. TimerInit will set a TimerEvent_t's Next pointer to NULL, which can mess up the timer linked list. Our internal fix was to check if the timers were initialized prior to calling TimerInit:

void LoRaMacClassBInit( LoRaMacClassBParams_t *classBParams, LoRaMacClassBCallback_t *callbacks, LoRaMacClassBNvmData_t* nvm )
{
#ifdef LORAMAC_CLASSB_ENABLED
    static bool timersInitialized = false;

    // Assign non-volatile context
    if( nvm == NULL )
    {
        return;
    }
    ClassBNvm = nvm;

    // Store callbacks
    Ctx.LoRaMacClassBCallbacks = *callbacks;

    // Store parameter pointers
    Ctx.LoRaMacClassBParams = *classBParams;

    // Initialize timers
    if ( !timersInitialized )
    {
        timersInitialized = true;
        TimerInit( &Ctx.BeaconTimer, LoRaMacClassBBeaconTimerEvent );
        TimerInit( &Ctx.PingSlotTimer, LoRaMacClassBPingSlotTimerEvent );
        TimerInit( &Ctx.MulticastSlotTimer, LoRaMacClassBMulticastSlotTimerEvent );
    }

    InitClassB( );
#endif // LORAMAC_CLASSB_ENABLED
}

I can submit a PR if you think that's an acceptable fix.

@nfroggy
Copy link
Author

nfroggy commented Dec 1, 2023

This might be a better fix (not sure if stopping the timers is necessary but it makes me feel better)

void LoRaMacClassBInit( LoRaMacClassBParams_t *classBParams, LoRaMacClassBCallback_t *callbacks, LoRaMacClassBNvmData_t* nvm )
{
#ifdef LORAMAC_CLASSB_ENABLED
    static bool timersInitialized = false;

    // Assign non-volatile context
    if( nvm == NULL )
    {
        return;
    }
    ClassBNvm = nvm;

    // Store callbacks
    Ctx.LoRaMacClassBCallbacks = *callbacks;

    // Store parameter pointers
    Ctx.LoRaMacClassBParams = *classBParams;

    // Initialize timers
    if ( !timersInitialized )
    {
        timersInitialized = true;
        TimerInit( &Ctx.BeaconTimer, LoRaMacClassBBeaconTimerEvent );
        TimerInit( &Ctx.PingSlotTimer, LoRaMacClassBPingSlotTimerEvent );
        TimerInit( &Ctx.MulticastSlotTimer, LoRaMacClassBMulticastSlotTimerEvent );
    }
    else
    {
        if (Ctx.BeaconTimer.IsStarted)        { TimerStop(&Ctx.BeaconTimer); }
        if (Ctx.PingSlotTimer.IsStarted)      { TimerStop(&Ctx.PingSlotTimer); }
        if (Ctx.MulticastSlotTimer.IsStarted) { TimerStop(&Ctx.MulticastSlotTimer); }
    }

    InitClassB( );
#endif // LORAMAC_CLASSB_ENABLED
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant