Add getters and setters to GPRS, GSM, GSMClient for custom inherited classes

[johncpang]

As mentioned in #84, when we need to create async version of these classes and keep the same logic as the original version, we need to access several internal variables. Therefore, we need getter and setter pairs for them.

I successfully create async version of these three classes (in my own library) with these modified sync version. New getters and setters has absolutely no impact to existing library/existing Arduino project, because they don't modify any existing functions.

[sandeepmistry]

Hi @johncpang,

I think there was a misunderstanding from my comment in https://github.com/arduino-libraries/MKRGSM/issues/84#issuecomment-478578459.

I'm ok with adding a getTimeout() API, but not keen on exposing the internal state machine, to me using ready() like you comment in https://github.com/arduino-libraries/MKRGSM/issues/84#issue-427375594 is fine. setState() is very dangerous to me as well.

[johncpang]

Hi @sandeepmistry I have been carefully on not to expose unnecessary APIs while building an asynchronous variants for GPRS, GSM and GSMClient. The variants, I named with prefix Async, usually do the same things as original synchronous version in asynchronous way. Thus, those are necessary.

Take GPRS::attach as example. Following is the code from line 71-82, which are skipped in asynchronous:

if (synchronous) {
  unsigned long start = millis();

  while (ready() == 0) {
    if (_timeout && !((millis() - start) < _timeout)) {
      _state = ERROR;
      break;
    }

    delay(100);
  }
}

Synchronous version will change _state when timeout, which will affect logic in ready() (line 112). Following is my extended AsyncGPRS class:

void AsyncGPRS::do_attach() {
  unsigned long start = millis();
  if (start > attach_delay) {
    if (ready() == 0) { // while (ready() == 0)
      unsigned long _timeout = timeout();
      if (_timeout && !((millis() - start) < _timeout)) {
        setState(ERROR);
        cb_attach(); // callback as break in while-loop
        return;
      }
      attach_delay = start + 100; // continue as in delay(100)
    } else {
      cb_attach(); // callback as didn't enter while-loop
    }
  }
}

There are a few things worth consider:

1. Change the access scope of setState() to protected. However, it seems inconsistent to other setters. And I believe users of the library know what they are doing.

2. Create a (set of) special function(s), like setStateError() or setTimeoutState() to avoid setter. However, we must also change the original logic. As you can see in the change logs, there is no change to original logic, but new getters/setters only. I would avoid touching the original code, yet allow the async version looks as close as original one.

[johncpang]

Hi @sandeepmistry, may I know what the status of this pull request now? What shall I change if need to be accepted?

Regarding not exposing the internal state machine, I'm on your side too. When I made the changes, I checked all related source codes to make sure (1) the change is minimum, (2) the change is necessary for extended classes to work just like the sync-version.

[sandeepmistry]

Hi @johncpang,

Please see my comment above in https://github.com/arduino-libraries/MKRGSM/pull/86#issuecomment-488336079.

If you add only getTimeout() that is fine with me.

[johncpang]

Hi @sandeepmistry, I totally agree with you that setting status is dangerous. However, if extended class cannot or does not set the status to ERROR when timed-out, ready() will be broken. Please check all the code related to ready() and I believe you'll see what I mean.

My update was to meant to minimize the changes so that it won't affect your sync-version. Probably would you consider one of the two suggestions I mentioned before?

[CLAassistant]

All committers have signed the CLA.

[johncpang]

I've accepted the CLA and updated my GitHub account's setting. Please check and accept my commit. Thanks.

[per1234]

The CLA check is now ✔️. Thanks so much @johncpang!