update() returns true when update was not performed

[KentWalker]

Was there any particular reason for update() to return true when an actual update hadn't been performed (ie returning without a call to forceUpdate() )?

[bitsy]

I have the same exact question. I have been wondering that myself and have already edited my local copy of the library to only return true if a successful update actually occurs:

bool NTPClient::update() {
  if ((millis() - this->_lastUpdate >= this->_updateInterval)     // Update after _updateInterval
    || this->_lastUpdate == 0) {                                // Update if there was no update yet.
    if (!this->_udpSetup) this->begin();                         // setup the UDP client if needed
    return this->forceUpdate();
  }
  return false;  // I changed this from true to false in NTPClient.cpp
}

Making the above edit allows the following code to be valid (which I currently use to sync my RTC with an NTP server):

// Only when timeClient successfully updates, adjust (sync) RTC time.
    if (timeClient.update()) {
        RTC.adjust(DateTime(timeClient.getEpochTime()));
    }

(RTC Library is RTClib from Adafruit.)

[bitsy]

I submitted a pull request with the above change to NTPClient.cpp.

[jbrown123]

It returns false only when an actual update happens and that update fails. I believe this is the appropriate behavior since getEpochTime() uses millis() to adjust for the elapsed time since the reply was received.

If you want to force an update to occur, then call forceUpdate() which also returns true/false based on whether it actually received a time update or not.

[bperkins011]

I agree with bitsy above - update() should return false if it does nothing and true when it actually performs an update - which really just ends up in a call to forceUpdate(). Once the NTP time is reset - I can then set another time with it - but I don't want to do it continuously for no reason in loop()

[bitsy]

@jbrown123, I see your point: knowing when an update fails could be potentially useful information. However, knowing when an update occurs successfully is also useful (e.g. syncing an RTC only when an update occurs, as I show above).

If it is important to maintain an indication of failure, why not return an 8-bit integer value instead of a boolean? Such that, a failed update returns 0, a “non-update”* returns 1, and a successful update returns 2. It essentially maintains the current boolean behavior to avoid breaking other people’s implementations of the library yet also provides additional information to those who desire to know whether an update occurred at all.

*“Non-update”: when update() does not call forceUpdate() because the interval requirement has not been met

[jbrown123]

Sounds reasonable to me.

[aentinger]

Fixed in #56 - therefore closing this issue.

[jbrown123]

Note that 56 does something different than what bitsy suggested in his most recent post. The change in 56 absolutely does not distinguish between a failed update and no update.

[aentinger]

Agreed, however the initial question/topic of this answer has been resolved - there is still #75 open which would satisfy the desire for further error return value refinement.