espressif / esp-lwip

Fork of lwIP (https://savannah.nongnu.org/projects/lwip/) with ESP-IDF specific patches
Other
79 stars 126 forks source link

Fixed NTP server address as fallback for DHCP-provided NTP server (IDFGH-6835) #42

Closed ridgy-b closed 2 years ago

ridgy-b commented 2 years ago

For a (sort of) clock I need to synchronize time on a regular basis. As the clock may be used in different WLANs, and not all internet providers allow access to public NTP servers, having a fixed ntp server (like pool.ntp.org) does not work. In these cases the NTP service has to be provided by the internet router. On the other hand, not all internet routers provide dhcp option 42. So, a mixed configuration is needed.

The code thus far is (using esp-arduino):

   sntp_servermode_dhcp(1); 
   configTzTime(TIME_ZONE, ntpServer1, ntpServer2);
   WiFi.begin(ssid, pass);

But this does not set any fallback server. Digging deeper, in WiFi.begin() dhcp functionality is called, and from dhcp.c:

dhcp_set_ntp_servers(n, ntp_server_addrs);

In sntp.c I read:

/**
 * Initialize one of the NTP servers by IP address, required by DHCP
 *
 * @param num the index of the NTP server to set must be < SNTP_MAX_SERVERS
 * @param server IP address of the NTP server to set
 */
void
dhcp_set_ntp_servers(u8_t num, const ip4_addr_t *server)
{
  LWIP_DEBUGF(SNTP_DEBUG_TRACE, ("sntp: %s %u.%u.%u.%u as NTP server #%u via DHCP\n",
                                 (sntp_set_servers_from_dhcp ? "Got" : "Rejected"),
                                 ip4_addr1(server), ip4_addr2(server), ip4_addr3(server), ip4_addr4(server), num));
  if (sntp_set_servers_from_dhcp && num) {
    u8_t i;
    for (i = 0; (i < num) && (i < SNTP_MAX_SERVERS); i++) {
      ip_addr_t addr;
      ip_addr_copy_from_ip4(addr, server[i]);
      sntp_setserver(i, &addr);
    }
    for (i = num; i < SNTP_MAX_SERVERS; i++) {
      sntp_setserver(i, NULL);
    }
  }
}

But the comment is not correct. "Initialize one of the NTP servers" does not reflect the function. @param num is not the index, but the number of ntp servers to be set from DHCP. @param server is a pointer to an array of IP addresses. Now, the function works as follows:

The solution so far (at the moment) is to call

      sntp_setservername(1, ntpServer1);
      sntp_setservername(2, ntpServer2);

after the call to WiFi.begin().

Is there any possibility to force keeping already configured (static) ntp server addresses, e.g. remove the lines setting the servers to NULL in dhcp_set_ntp_servers, overwriting only address #0 ?

david-cermak commented 2 years ago

Hi @ridgy-b

yes, it seems that dhcp_set_ntp_servers() does not only initialize DHCP provided NTP servers, but also cleans out the rest. and yes, the recommended usage is to 1) enable NTP over DHCP before getting the IP 2) set the static NTP servers after receiving the DHCP lease

As outlined in the IDF example: Ad 1) https://github.com/espressif/esp-idf/blob/229ed084847455da5aeda1dfd17ad3b6ccadc9b0/examples/protocols/sntp/main/sntp_example_main.c#L135-L136

Ad2) https://github.com/espressif/esp-idf/blob/229ed084847455da5aeda1dfd17ad3b6ccadc9b0/examples/protocols/sntp/main/sntp_example_main.c#L176-L183

ridgy-b commented 2 years ago

Thank you, @david-cermak, so this seems to be intentionally. To prevent others from having issues, it would be nice to have some hint in the documentation.

david-cermak commented 2 years ago

would be nice to have some hint in the documentation.

sorry for the delay, documented directly in the IDF example

https://github.com/espressif/esp-idf/blob/56659cdf36790e182451cd892ea349b29a27d333/examples/protocols/sntp/README.md?plain=1#L33-L35

ridgy-b commented 2 years ago

Thank you.