Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly close socked which is already in disconnection state #150

Merged
merged 1 commit into from
Apr 27, 2024

Conversation

pinkavaj
Copy link
Contributor

@pinkavaj pinkavaj commented Mar 3, 2024

The socker might be already disconnecting due to FIN from the other side, in this case still close the socket to finish the operation but do not reset the ISR, because we will not get another SNIR_DISCON and would end up in inifite loop.

The socker might be already disconnecting due to FIN from the other side,
in this case still close the socket to finish the operation but do not reset
the ISR, because we will not get another SNIR_DISCON and would end up in inifite loop.
Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The particulars of this change are beyond my knowledge.

I'm also not entirely sure how to correctly test it in a way that reproduces the issue that this change is resolving.

I did test the simpletest from this repo succesfully on this branch with a Feather RP2040 + Ethernet Featherwing

I also used this code to attempt to test the __exit__ context manager function:

import time

import board
import busio
import digitalio
import adafruit_connection_manager
import adafruit_requests
from adafruit_wiznet5k.adafruit_wiznet5k import WIZNET5K

print("Wiznet5k WebClient Test")

TEXT_URL = "http://wifitest.adafruit.com/testwifi/index.html"
JSON_URL = "http://api.coindesk.com/v1/bpi/currentprice/USD.json"

# For Adafruit Ethernet FeatherWing
cs = digitalio.DigitalInOut(board.D10)
# For Particle Ethernet FeatherWing
# cs = digitalio.DigitalInOut(board.D5)
spi_bus = busio.SPI(board.SCK, MOSI=board.MOSI, MISO=board.MISO)

# Initialize ethernet interface with DHCP
eth = WIZNET5K(spi_bus, cs)

pool = adafruit_connection_manager.get_radio_socketpool(eth)
conn_manager = adafruit_connection_manager.get_connection_manager(pool)

with conn_manager.get_socket("www.adafruit.com", 80, "http") as sock:
   print(f"type(sock): {type(sock)}")
   print(f"dir(sock): {dir(sock)}")
   
   print("sleeping 10...")
   time.sleep(10)

print("after")

Honestly I'm not entirely sure what outcomes are expected or not, but I can confirm it doesn't crash or seem to have any other adverse effects that I can tell.

@pinkavaj if you have one already, or know the proper way to write a short script that illustrates the issue and resolution if you can share that here it will be quite helpful.

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SNIR is the "socket n interrupt register", called Sn_IR in the datasheet. There are 5 interrupt bits defined in wiznet 5500, in bits 0-4. bits 5-7 are reserved. Writing a "1" bit clears the corresponding interrupt flag if it was set.

The change causes the call on that line to not clear the "disconnect interrupt", just every other kind of interrupt.

This change makes sense at least superficially, because the code is about to wait for the "disconnect interrupt" to occur before continuing.

Questions:

  • is similar treatment needed for the timeout interrupt?
  • the manufacturer recommended writing 0xff but does not wait for the socket to reach the closed state: https://docs.wiznet.io/Product/iEthernet/W5500/Application/tcp#socket-close -- is there something incorrect about the whole idea of waiting for the interrupt flag? It's also possible to poll the status register for the socket entering the closed state, separate from checking the interrupt flags.

@FoamyGuy FoamyGuy merged commit 4c35a79 into adafruit:main Apr 27, 2024
1 check passed
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Apr 28, 2024
Updating https://github.com/adafruit/Adafruit_CircuitPython_Wiznet5k to 6.0.0 from 5.2.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#150 from pinkavaj/pi-fix-sock-exit
  > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#160 from justmobilize/remove-wsgiserver

Updating https://github.com/adafruit/Adafruit_CircuitPython_ConnectionManager to 1.2.0 from 1.1.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_ConnectionManager#13 from justmobilize/close-all-and-counts

Updating https://github.com/adafruit/Adafruit_CircuitPython_Requests to 3.2.6 from 3.2.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_Requests#188 from justmobilize/with-updates

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
@pinkavaj pinkavaj deleted the pi-fix-sock-exit branch August 5, 2024 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants