TheThingsNetwork / arduino-device-lib

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

Improve handling of needsHardReset flag and add method to check valid module connected to serial port #282

Closed danalvarez closed 2 years ago

danalvarez commented 2 years ago

This PR is related to #280 . I believe the handling of the needsHardReset flag could be improved, by not setting it to false with each call to reset().

Additionally, I added a function called checkValidModuleConnected() to address https://github.com/TheThingsNetwork/arduino-device-lib/issues/216#issuecomment-310430834 . In this manner, the user application can call checkValidModuleConnected() in the setup() function of the Arduino code, and make sure a valid module is connected before any join attempt. This is especially important, because we need a function that returns as fast as possible when a module is not connected or incorrectly wired. For example, a call to reset() takes several minutes to return if a module is not connected, because of the many instructions carried out within it and the innate timeout given in readLine(). The checkValidModuleConnected() function returns in ~30s if autobaud_first is set to false, and ~60s if set to true.

I also added an example CheckModule.ino to showcase the usage of the checkValidModuleConnected() function.

Additionally, I added a bool reset_first to personalize() and provision(), so the user application can tell the library if they want a soft reset to be performed when calling these functions. For example, if a user performs:

  1. Hard reset RN module
  2. call checkValidModuleConnected()

Before calling personalize(), it would make sense to go directly into the ABP join, without a soft reset first, as the hard reset was already performed. Anyways, I set these params true by default so that prior behaviour is not broken.

Finally, I also updated the documentation.

Help wanted:

Broken: The only caveat with this PR is that the module validation in checkValidModuleConnected() is not working as expected. This section in particular:

  // buffer contains "RN2xx3[xx] x.x.x ...", getting only model (RN2xx3[xx])
  char *model = strtok(buffer, " ");
  debugPrintIndex(SHOW_MODEL, model);
  // check if module is valid (must be RN2483, RN2483A, RN2903 or RN2903AS)
  if(pgmstrcmp(model, CMP_RN2483) == 0 || pgmstrcmp(model, CMP_RN2483A) == 0 || pgmstrcmp(model, CMP_RN2903) == 0 || pgmstrcmp(model, CMP_RN2903AS) == 0)
  {
    debugPrintMessage(SUCCESS_MESSAGE, SCS_VALID_MODULE);
    return true;                                                // module responded and is valid (recognized/supported)
  }

For example, if model = "RN2903AX" (impossible, I know, but for the sake of the example), the function will think that this is a valid module, because pgmstrcmp(model, CMP_RN2903) == 0 returns true. I am not so familiar with the c string functions. Could anyone help me by suggesting how to make the verification work correctly?

I went ahead and submitted the PR with this caveat, so that it may be reviewed and a fix to this small issue could be suggested.

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

danalvarez commented 2 years ago

Hi @jpmeijers @johanstokking any chance you could review this?

jpmeijers commented 2 years ago

I agree that the change looks OK, but I would want to test this change on a physical device. Unfortunately I do not have one at hand at the moment, so it will only be in a couple of days.

danalvarez commented 2 years ago

Regarding naming; we use lowerCamelCase, so it would be resetFirst and not reset_first.

@johanstokking I've updated the variable names to use camelCase.

@jpmeijers meanwhile, maybe you could assist with the "Help wanted" section of my original post in this PR?