adafruit / Adafruit_CircuitPython_Wiznet5k

Pure-Python interface for WIZNET 5k Ethernet modules
Other
16 stars 36 forks source link

NTP Out of Sockets Error #118

Closed econeale closed 1 year ago

econeale commented 1 year ago

Hello! I've got an NTP client that receives an out of sockets error after running for some time. Can anyone tell if there's a leaking socket somewhere or am I doing something wrong? Here's the pared down function:

import asyncio
import rtc
import adafruit_logging as logging
from adafruit_wiznet5k.adafruit_wiznet5k import WIZNET5K
from adafruit_wiznet5k.adafruit_wiznet5k_ntp import NTP

async def update_time(eth_obj: WIZNET5K, logger: logging.Logger):
    ntp = None
    while True:
        try:
            if eth_obj is not None and eth_obj.link_status > 0:
                if ntp is None:
                    ntp = NTP(eth_obj, "pool.ntp.org", 0, False)
                rtc.RTC().datetime = ntp.get_time()
                logger.debug("NTP updated")

        except Exception as err:
            logger.error(f"Error while updating NTP: {err}")
            ntp = None
            interval = 60

        await asyncio.sleep(interval)
BiffoBear commented 1 year ago

Hi, please tell me what Wiznet chip are you using, w5100s (3 sockets available) or w5500 (7 sockets available)? Would the number of threads that you have open sockets on simultaneously exceed these limits?

Thx

econeale commented 1 year ago

We are using the w5500. There are two threads using sockets in my implementation; to make sure this is an issue with the NTP implementation, I rewrote the NTP thread using the adafruit_ntp library and that eliminated the problem.

That leads me to believe that there's an issue with this library...

anecdata commented 1 year ago

Are WIZnet sockets compatible enough that we can deprecate the NTP module here in favor of adafruit_ntp?

econeale commented 1 year ago

They appear to be. I migrated the function to the following and it hasn't thrown any errors / has kept the correct time for the last week. Beyond that I haven't done any testing.

import asyncio
import os
import rtc
import adafruit_logging as logging
import adafruit_ntp
import adafruit_wiznet5k.adafruit_wiznet5k_socket
from adafruit_wiznet5k.adafruit_wiznet5k import WIZNET5K

async def update_time(eth_obj: WIZNET5K, logger: logging.Logger):
    ntp = None
    while True:
        interval = 3600
        try:
            if eth_obj is not None and eth_obj.link_status > 0:
                if ntp is None:
                    ntp = adafruit_ntp.NTP(adafruit_wiznet5k.adafruit_wiznet5k_socket, tz_offset=0)
                rtc.RTC().datetime = ntp.datetime
                logger.debug("NTP updated")
            else:
                interval = 60

        except Exception as err:
            logger.error(f"Error while updating NTP: {err}")
            ntp = None
            interval = 60

        await asyncio.sleep(interval)
BiffoBear commented 1 year ago

Are WIZnet sockets compatible enough that we can deprecate the NTP module here in favor of adafruit_ntp?

I'll have a look at error handling, if there are incompatibilities, that is where they are likely to lie.

BiffoBear commented 1 year ago

@anecdata adafruit_ntp doesn't trap any exceptions so it will work with the WIZNET5K library. What is the best way to deprecate the Wiznet5k NTP module? Just delete it or have it raise an exception that explains the change?

anecdata commented 1 year ago

I'm really not sure. Any examples or learn guides that use it would change. I've asked kattni on Discord in #circuitpython-dev

anecdata commented 1 year ago

Conclusion is:

delete the code (or make it raise NotImplementedError), and do a major version bump

https://discord.com/channels/327254708534116352/327298996332658690/1121119809313189938 discord://discord.com/channels/327254708534116352/327298996332658690/1121119809313189938

Also Kattni and DanH should be tagged on PR

edit: and just to be sure, is there any reason not to deprecate?