question about changes to lwip2

[loph917]

Basic Infos

• This issue complies with the issue POLICY doc.
• I have read the documentation at readthedocs and the issue is not addressed there.
• I have tested that the issue is present in current master branch (aka latest git).
• I have searched the issue tracker for a similar issue.
• If there is a stack dump, I have decoded it.
• I have filled out all fields below.

Platform

• Hardware: [ESP-12]
• Core Version: [a3ea816]
• Development Env: [Platformio & other]
• Operating System: [Windows]

Settings in IDE

• Module: [Generic ESP8266 Module]
• Flash Mode: [qio]
• Flash Size: [4MB]
• lwip Variant: [Higher Bandwidth]
• Reset Method: [nodemcu]
• Flash Frequency: [40Mhz]
• CPU Frequency: [80Mhz]
• Upload Using: [SERIAL]
• Upload Speed: [other] (serial upload only)

Problem Description

This is not problem.

More of a, "I have made changes to lwip2, specifically as it relates to sntp and was wondering if there was interest in regards to including these as part of the official API. Or not."

Basically I've added the following feature and functions and exposed them:

features:

• extended the sntp_servers struct to include an update_delay parameter (per sntp server).

functions:

• sntp_get_current_server(): returns which of the defined servers is currently being used to solicit time.
• sntp_getreachability(): returns the reachability "score" of the NTP server. this function is already in the SDK, i have simply exposed it to sketches.
• sntp_setserver_update_delay(): allows for the setting of a per-server update delay in lieu of the default 3600000. by default, the value defined in SNTP_UPDATE_DELAY will be applied when sntp_setservername() is first called.
• sntp_getserver_update_delay(): returns the current update delay for a configured server

relevant code snippets below. what's not included (in this message) are the necessary header file changes that define the new functions. if there's a desire to include these as part of the official API then i'd be happy to get the changes into git or diff format.

struct sntp_server {
#if SNTP_SERVER_DNS
  const char *name;
#endif /* SNTP_SERVER_DNS */
  ip_addr_t addr;
#if SNTP_MONITOR_SERVER_REACHABILITY
  /** Reachability shift register as described in RFC 5905 */
  u8_t reachability;
#endif /* SNTP_MONITOR_SERVER_REACHABILITY */
  u32_t update_delay;
};

--- in function sntp_recv()
    if (sntp_opmode == SNTP_OPMODE_POLL) {
      u32_t sntp_update_delay;
      sys_untimeout(sntp_try_next_server, NULL);
      sys_untimeout(sntp_request, NULL);

      /* Correct response, reset retry timeout */
      SNTP_RESET_RETRY_TIMEOUT();

      sntp_update_delay = sntp_servers[sntp_current_server].update_delay;
      sys_timeout(sntp_update_delay, sntp_request, NULL);
      LWIP_DEBUGF(SNTP_DEBUG_STATE, ("sntp_recv: Scheduled next time request: %"U32_F" ms\n",
                                     sntp_update_delay));
    }
---

u8_t sntp_get_current_server()
{
    return sntp_current_server;
}

void
sntp_setserver_update_delay(u8_t idx, u32_t update_delay)
{
    LWIP_ASSERT_CORE_LOCKED();
    if (idx < SNTP_MAX_SERVERS) {
        if (update_delay < 15000) {
            update_delay = SNTP_UPDATE_DELAY;
        } else {
            sntp_servers[idx].update_delay = update_delay;
        }
    }
}

u32_t sntp_getserver_update_delay(u8_t idx)
{
  if (idx < SNTP_MAX_SERVERS) {
    return sntp_servers[idx].update_delay;
  }
  return (u32_t)NULL;
}

void
sntp_setservername(u8_t idx, const char *server)
{
  LWIP_ASSERT_CORE_LOCKED();
  if (idx < SNTP_MAX_SERVERS) {
    sntp_servers[idx].name = server;
    sntp_servers[idx].update_delay = SNTP_UPDATE_DELAY;
  }
}

[devyte]

I think the changes makes sense, but I'm not sure where to put them. @d-a-v ?

[loph917]

i don't believe the builder dir is tracked via git in the repo which is a reason why i asked the original question. i know this isn't the preferred way to submit changes.

just to be clear on where the changes go:

.c file changes:

.c file --> esp8266/tools/sdk/lwip2/builder/lwip2-src/src/apps/sntp

function prototypes in these files:

.h file #1 --> esp8266\tools\sdk\lwip2\include\lwip\apps
.h file #2 --> esp8266\tools\sdk\lwip2\builder\lwip2-src\src\include\lwip\apps
.h file #3 --> esp8266\tools\sdk\lwip\include\lwip (not sure about this one as i think it's lwip 1)

git status tells me:

modified:   tools/sdk/lwip2/builder (modified content, untracked content)

[devyte]

So you modified the lwip sources. Our lwip is built from @d-a-v's repo, so one possibility is to add a patch there, but the best would be to propose the changes upstream to the lwip repo.

[d-a-v]

#2 is the good ref.

There is a patches/ directory for diffs with upstream lwIP in builder/. PRs must be directed to lwip2 repo (glue between lwIP and this arduino core).

[d-a-v]

#2 is the good ref.

(a previous message seems to have been modified)

@loph917,

You can propose your changes with the following:
First make a github fork of https://github.com/d-a-v/esp82xx-nonos-linklayer,
no need to pull it locally,
then,

cd tools/sdk/lwip2/builder
git remote add myrepo https://github.com/loph910/esp82xx-nonos-linklayer

cd lwip2-src
git diff master > ../patches/sntp-update-delay.patch
cd ../patches
git checkout -b sntp-update-api
git add sntp-update-delay.patch
git commit
git push myrepo

Then from your github-fork/clone you can make a pull request on lwip2 submodule repository, that will finally land here.

@bperrybap is interested in your changes.

[loph917]

pushed up a PR. hopefully someone can find it useful. i'm using it but have not tested extensively nor am a programmer by trade so be kind. to be clear, only the bits revolving around sntp are mine; if there's other things in the PR not related to sntp, they ain't mine and i disavow all knowledge and there's a chance something might self destruct. in 5 seconds. or less. depending upon frequency skew.

[bperrybap]

As a nice feature, it might be nice if sntp_setserver_update_delay(0) could be use to force an update/sync but not alter the currently set update delay value.

[d-a-v]

Update delay setting is now offered in lwip2, not per server though.
@loph917 do you intend to pursue the work on lwip2 or is the current feature enough (same update delay for all servers) ?

[loph917]

@d-a-v the current features are good enough for me. i haven't looked at this code in months.

@bperrybap i encourage you to go ahead and patch my original patch. here's a potential drop-in replacement for what you are asking for. completely untested. your mileage may vary. past performance is no guarantee of future results. etc. etc.

+void
+sntp_setserver_update_delay(u8_t idx, u32_t update_delay)
+{
+   LWIP_ASSERT_CORE_LOCKED();
+   if (idx < SNTP_MAX_SERVERS) {
+       if (update_delay == 0) {
+           ip_addr_t sntp_server_address;
+           sntp_server_address = sntp_getserver(idx);
+           err = (ip_addr_isany_val(sntp_server_address)) ? ERR_ARG : ERR_OK;
+           if (err == ERR_OK) {
+              LWIP_DEBUGF(SNTP_DEBUG_TRACE, ("sntp_request: current server address is %s\n",
+                                             ipaddr_ntoa(&sntp_server_address)));
+              sntp_send_request(&sntp_server_address);
+              return (u32_t)NULL;
+          } else {
+              return (u32_t)NULL;
+          }
+       }
+       if (update_delay < 15000) {
+          update_delay = SNTP_UPDATE_DELAY;
+       } else {
+          sntp_servers[idx].update_delay = update_delay;
+       }
+   }
+}

[d-a-v]

Closing as solved, NTP has received numerous fixes and improvements. The API is showed in the ntp-tz-dst example.
Feel free to open a new issue is something is missing.