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

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

Conversation

danalvarez
Copy link
Contributor

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 #216 (comment) . 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.

…ValidModuleConnected() to check for correct wiring and supported module. Added getVersion to obtain response of sys get ver. Updated documentation. Added CheckModule example to showcase usage of checkValidModuleConnected().
@CLAassistant
Copy link

CLAassistant commented Jan 17, 2022

CLA assistant check
All committers have signed the CLA.

@danalvarez
Copy link
Contributor Author

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

Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general it looks OK to me.

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

@jpmeijers any comments?

@jpmeijers
Copy link
Collaborator

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
Copy link
Contributor Author

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?

Copy link
Collaborator

@jpmeijers jpmeijers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done the tests that I wanted, and they were successful.

The issue with only the prefix of the module number that is checked is not critical, and it can be fixed at a later stage in the pgmstrcmp function. The RN2xx3 modules are being deprecated, so I think we should anyway start limiting changes to this library.

@johanstokking johanstokking merged commit 4eb8762 into TheThingsNetwork:master Feb 4, 2022
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

Successfully merging this pull request may close these issues.

4 participants