sandeepmistry / arduino-LoRa

An Arduino library for sending and receiving data using LoRa radios.
MIT License
1.6k stars 621 forks source link

TTGO TxDone IRQ callback fails #645

Closed O-Factura closed 1 year ago

O-Factura commented 1 year ago

Very odd behavior with the TTGO ESP32/LoRa SX1276 board when transmitting a LoRa packet. The interrupt handler ISR_PREFIX void LoRaClass::onDio0Rise() is triggered upon receiving data, however, after transmitting, the handler is never executed. This issue occurs with the same three TTGO boards that I have. There is no problem to send data. No crashes either. Pinout values are good. I have two boards running a test, each one acting both as transmitter and receiver. One initiates, the other receives and sends back an ACK. A Rinse & Repeat test over days communicating every few seconds with all ACKs received.

I've had to modify int LoRaClass::endPacket(bool async) LoRa.cpp to get the tx callback to execute on this board when the TxDone flag is set in the register. This is the added code after the while loop completes;

// A workaround by checking if RegIrqFlags (0x12) TxDone bit 3 (8) has been set.
// See Semtech datasheet page 111.

if (_onTxDone && (readRegister(REG_IRQ_FLAGS) & IRQ_TX_DONE_MASK) == IRQ_TX_DONE_MASK)
{
    handleDio0Rise();
}
else
{
    // clear IRQ's
    writeRegister(REG_IRQ_FLAGS, IRQ_TX_DONE_MASK);
}

I shouldn't have to change this class and it should work as originally written.

Anybody else using this board and experiencing the same issue? https://github.com/LilyGO/TTGO-LORA32/tree/LilyGO-868-V1.0 It's strange that only the TxDone interrupt doesn't work and RxDone works.

Some of the things I've tried; Removing LoRa.onReceive() and only registering one callback: LoRa.onTxDone(onTxDone). No effect. Problem still exists. Connected 3.3V wire to pin 26: No problem with interrupt method executing. Also LoRa.onReceive(onReceive) executes, so proves attachInterrupt(digitalPinToInterrupt(_dio0), LoRaClass::onDio0Rise, RISING); works as intended.

Possibly a hardware issue in SX1276?

morganrallen commented 1 year ago

Yeah, I agree you should not have to do that. And looking at the code the TX IRQ probably should not be automatically cleared there either. If you can do another test... undo your changes and comment out the writeRegister(REG_IRQ_FLAGS, IRQ_TX_DONE_MASK); line, curious if this will also work.

O-Factura commented 1 year ago

What I suspect seems to be occurring is that DIO0 doesn't go high after transmit when it should and hence not triggering the IRQ. The internal TxDone flag of the SX1276 is set after the packet is transmitted and all is well in that respect. Because I've defined a TxDone callback, the expression if (_onTxDone && ...) evaluates to true and writeRegister(REG_IRQ_FLAGS, IRQ_TX_DONE_MASK); doesn't get called in this instance. If I don't define a tx callback, the code writeRegister(REG_IRQ_FLAGS, IRQ_TX_DONE_MASK); in the else block that I have executes automatically like it should as originally intended within int LoRaClass::endPacket(bool async).

In my workaround, the call to handleDio0Rise(); clears the TxDone flag, just like it clears the RxDone flag on the receive interrupt handling. This is why I've placed writeRegister(REG_IRQ_FLAGS, IRQ_TX_DONE_MASK); in the else block so not to clear the flags more than necessary. I previously tried commenting out that line you suggested, but that has no effect on the TxDone IRQ unfortunately.

When beginPacket() is called, writeRegister(REG_IRQ_FLAGS, IRQ_TX_DONE_MASK); is executed to reset the flags via the checks done in the isTransmitting() function. If this writeRegister is commented out as well, it just means that while ((readRegister(REG_IRQ_FLAGS) & IRQ_TX_DONE_MASK) == 0) { yield(); } will never run in int LoRaClass::endPacket(bool async).

So, the 3 boards I have behave the same way with the silent TX IRQ. A board design issue? Possibly a bad batch of Semtech chips? I'm really curious to know if there are other TTGO board owners using this library that have silent TX callbacks.

morganrallen commented 1 year ago

In my opinion the whole beginPacket/endPacket is kind of a mess and needs more scrutiny. My gut feeling is the TxDone IRQ Flag should ONLY be cleared from within handleDio0Rise().

Another thing to make sure, you are calling endPacket(true) yeah? Without true being passed the IRQ isn't even set up.

O-Factura commented 1 year ago

Thanks for the suggestion. It makes sense to pass true to set the DIO0 mapping for Tx. After that change was made and removing the workaround that fixed the non async mode issue, the TxDone IRQ still didn't work. So I started to comment out various writes made to registers. In particular this one got the result, writeRegister(REG_OP_MODE, MODE_LONG_RANGE_MODE | MODE_RX_CONTINUOUS); in receive().

So the reason that caused my issues was a combination of not passing true in endPacket(true) and a bad location for calling LoRa.receive(); which played havoc with the Tx mode setting. Its new location now sits in a statement block that checks a flag which the Tx callback sets. Now LoRa.receive(); changes the mode to Rx after Tx has had time to complete its tasks and flag resets.

if (txDone) { txDone = false; LoRa.receive(); digitalWrite(led_pin, LOW); Serial.println("\nTx DONE. Waiting for ACK...");
}

Thanks for your input. Sanity has been restored.

O-Factura commented 1 year ago

In my opinion the whole beginPacket/endPacket is kind of a mess and needs more scrutiny.

The way the LoRa transmit code is set up at the moment is a bit awkward I feel. I added template methods in my local build which replaces the process of making several function calls as it's currently done. Instead of doing;

byte data[<length>] = {<bytes>}; LoRa.beginPacket(<true|false>); LoRa.write(data, <length>); LoRa.endPacket(<true|false>);

I now use this,

byte data[<length>] = {<bytes>}; LoRa.operate(TX_EHEAD_ASYNC, data, <length>);

and this works as well,

LoRa.operate(TX_EHEAD_ASYNC, "A MESSAGE!");

There are four TX_... constants to choose from to cater for explicit/implicit header modes combined with async/block transmit modes. It's not necessary to call a write method from your program.

To keep it consistent, instead of LoRa.receive() I have this,

LoRa.operate(RX);

and instead of LoRa.receive(<size>),

LoRa.operate(RX, <size>);

The original code has been left as is and the changes are compatible. Let me know if anyone wants this in.