TheThingsNetwork / arduino-device-lib

Arduino Library for TTN Devices
MIT License
206 stars 96 forks source link

Handling of needsHardReset flag #280

Closed danalvarez closed 2 years ago

danalvarez commented 2 years ago

In my application, I am interfacing with an RN2903 module at 9600 baud. When calling setup(), I am checking comm with the RN module via a call to reset(). However, looking into the code for this function, it returns by setting this->needsHardReset = false at the very end: https://github.com/TheThingsNetwork/arduino-device-lib/blob/f0a4ddd721557625273fa83e92f423970eee80a0/src/TheThingsNetwork.cpp#L650 This means that, even if comm with the module is unsuccessful, by the time the function returns, needsHardReset is false again and my application cannot know that the module is unresponsive.

My code looks like this:

  do {
    DEBUG_PORT.println(F("Checking comm..."));
    myLora.resetHard(RFRESET);                            // hard-reset lora module
    delay(100);                                           // wait for module startup
    // when myLora.reset(false) returns, needsHardReset
    // will be false again, even if the module was
    // unresponsive, so the do-while will exit thinking
    // comm with the module is OK
    myLora.reset(false);                                  // soft reset and adr set to false
    retries--;
  } while(myLora.needsHardReset && retries > 0);

I would propose removing the this->needsHardReset = false in the reset() function, and instead setting the flag in readLine(), as follows:

size_t TheThingsNetwork::readLine(char *buffer, size_t size, uint8_t attempts)
{
  size_t read = 0;
  while (!read && attempts--)
  {
    read = modemStream->readBytesUntil('\n', buffer, size);
  }
  if (!read)
  { // If attempts is activated return 0 and set RN state marker
    this->needsHardReset = true; // Inform the application about the radio module is not responsive.
    debugPrintLn("No response from RN module.");
    return 0;
  }
  this->needsHardReset = false; // ADDED LINE: Inform the application that the radio module is responsive.
  buffer[read - 1] = '\0'; // set \r to \0
  return read;
}

In this manner, the same function that sets needsHardReset to true when the module is unresponsive, also sets the flag to false when it is responsive.

What do you think?

danalvarez commented 2 years ago

Closing this because #282 was merged