Decawave / mynewt-dw1000-core

[DEPRECATED] Use https://github.com/Decawave/uwb-core for new designs.
Apache License 2.0
57 stars 34 forks source link

TWR complete_cb called before transmission #32

Closed Beetix closed 3 years ago

Beetix commented 4 years ago

Hi,

I have noticed in the TWR libraries that the complete_cb callbacks are called before the transmission of the final message. As an example here is an extract of the code from the SS TWR library: https://github.com/Decawave/mynewt-dw1000-core/blob/d6b1414f1b4527abda7521a304baa1c648244108/lib/twr_ss/src/twr_ss.c#L322-L339 This can lead to ranging issues when operating the transceiver (putting it to sleep for example) straight after the completion callback.

For cases where the ranging ends with a transmission, I suggest to call the complete_cb callbacks on tx_complete_cb. Going back to the code above I replaced the code inside the else with a line that sets a global variable: g_wait_for_tx = true;. Also I have implemented the tx_complete_cb callback:

static bool
tx_complete_cb(struct uwb_dev * inst, struct uwb_mac_interface * cbs)
{
    if (!g_wait_for_tx) return false;
    g_wait_for_tx = false;

    struct uwb_rng_instance * rng = (struct uwb_rng_instance *)cbs->inst_ptr;
    STATS_INC(g_stat, complete);
    dpl_sem_release(&rng->sem);
    {
        struct uwb_mac_interface * cbs = NULL;
        if(!(SLIST_EMPTY(&inst->interface_cbs))){
            SLIST_FOREACH(cbs, &inst->interface_cbs, next){
            if (cbs!=NULL && cbs->complete_cb)
                if(cbs->complete_cb(inst, cbs)) continue;
            }
        }
    }
    return true;
}

The issue is that the tx_complete_cb in uwb_rng.c gets called first and prevents any following callback to be called: https://github.com/Decawave/mynewt-dw1000-core/blob/d6b1414f1b4527abda7521a304baa1c648244108/lib/uwb_rng/src/uwb_rng.c#L838-L858 A workaround in this case is to change line 853 and to return false in order to allow following tx_complete_cb callbacks to be called.

The solution now works but I have only handled SS TWR. I am sure a more generic solution can be developed.

Does anyone have any thoughts on this?

Thanks

Beetix commented 4 years ago

Just found an interesting generic solution.

I have added a new complete_after_tx bit in the uwb_rng_control_t struct. I changed the case code to:

case DWT_SS_TWR ... DWT_DS_TWR_EXT_END:
    RNG_STATS_INC(tx_complete);
    if (!rng->control.complete_after_tx)
        return true;

    dpl_sem_release(&rng->sem);
    struct uwb_mac_interface * cbs = NULL;
    if(!(SLIST_EMPTY(&inst->interface_cbs))){
        SLIST_FOREACH(cbs, &inst->interface_cbs, next){
        if (cbs!=NULL && cbs->complete_cb)
            if(cbs->complete_cb(inst, cbs)) continue;
        }
    }
    rng->control.complete_after_tx = 0;
    return true;
    break;

Now in twr_ss.c instead of setting a global variable limited to the scope of the file as proposed in my previous comment (g_wait_for_tx = true;) I set the new control bit: rng->control.complete_after_tx = 1;. I have removed the tx_complete_cb callback since it is now useless.

ncasaril commented 4 years ago

Hi @Beetix,

I've created a PR for this in uwb-core. Have a look and let me know if this solves what you saw too. Thanks, Niklas

Beetix commented 3 years ago

Hi @ncasaril,

Just looked at the code. Haven't tested it but it seems right.

Thanks!