Skip to content

Commit

Permalink
Workaround for hang when receiving data while waiting for transmission
Browse files Browse the repository at this point in the history
  • Loading branch information
tryone144 committed May 4, 2023
1 parent 15ce52f commit 176da0f
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 4 deletions.
15 changes: 14 additions & 1 deletion src/EthernetUdp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,22 @@ int EthernetUDP::beginPacket(IPAddress ip, uint16_t port)
return Ethernet.socketStartUDP(sockindex, rawIPAddress(ip), port);
}

// Variable defined and set in socket.cpp
extern uint8_t udp_send_error;

int EthernetUDP::endPacket()
{
return Ethernet.socketSendUDP(sockindex);
bool result = Ethernet.socketSendUDP(sockindex);
if (!result && udp_send_error) {
// XXX While waiting for SEND_OK or TIMEOUT, a RECV is returned because something
// arrived while trying to send — and SEND_OK or TIMEOUT is never received.
// After this happens a few times, the W5100 will lock up. Try to compensate by
// closing, reseting, and calling begin() again when this happens.
stop();
W5100.reset();
begin(_port);
}
return result;
}

size_t EthernetUDP::write(uint8_t byte)
Expand Down
18 changes: 18 additions & 0 deletions src/socket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -509,11 +509,18 @@ bool EthernetClass::socketStartUDP(uint8_t s, uint8_t* addr, uint16_t port)
return true;
}

// Keep track of UDP send failures when loosing SEND_OK response.
uint8_t udp_send_error = 0;

bool EthernetClass::socketSendUDP(uint8_t s)
{
udp_send_error = 0;
SPI.beginTransaction(SPI_ETHERNET_SETTINGS);
W5100.execCmdSn(s, Sock_SEND);

uint16_t timeoutMax = W5100.getRetransmissionTimeoutMs() + 1;
uint16_t start = millis();

/* +2008.01 bj */
while ( (W5100.readSnIR(s) & SnIR::SEND_OK) != SnIR::SEND_OK ) {
if (W5100.readSnIR(s) & SnIR::TIMEOUT) {
Expand All @@ -523,6 +530,17 @@ bool EthernetClass::socketSendUDP(uint8_t s)
//Serial.printf("sendUDP timeout\n");
return false;
}

// XXX While waiting for SEND_OK or TIMEOUT, a RECV is returned because something
// arrived while trying to send — and SEND_OK or TIMEOUT is never received.
// Forcefully break the loop after the expected timeout period has passed.
if (millis() - start > timeoutMax) {
udp_send_error = 1;
W5100.writeSnIR(s, SnIR::SEND_OK);
SPI.endTransaction();
return false;
}

SPI.endTransaction();
yield();
SPI.beginTransaction(SPI_ETHERNET_SETTINGS);
Expand Down
32 changes: 31 additions & 1 deletion src/utility/w5100.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
uint8_t W5100Class::chip = 0;
uint8_t W5100Class::CH_BASE_MSB;
uint8_t W5100Class::ss_pin = SS_PIN_DEFAULT;
bool W5100Class::initialized = false;
#ifdef ETHERNET_LARGE_BUFFERS
uint16_t W5100Class::SSIZE = 2048;
uint16_t W5100Class::SMASK = 0x07FF;
Expand Down Expand Up @@ -82,10 +83,39 @@ W5100Class W5100;
uint32_t W5100Class::ss_pin_mask;
#endif

uint8_t W5100Class::reset(void)
{
// Get current configuration
uint8_t gw[4], sn[4], mac[8], ip[4];
W5100.getGatewayIp(gw);
W5100.getSubnetMask(sn);
W5100.getMACAddress(mac);
W5100.getIPAddress(ip);

uint16_t rtr = W5100.getRetransmissionTime();
uint8_t rcr = W5100.getRetransmissionCount();

// Clear the init flag, perform a soft-reset, and init() again
initialized = false;
softReset();
bool success = init();

// Re-apply the previous configuration
if (success) {
W5100.setGatewayIp(gw);
W5100.setSubnetMask(sn);
W5100.setMACAddress(mac);
W5100.setIPAddress(ip);

W5100.setRetransmissionTime(rtr);
W5100.setRetransmissionCount(rcr);
}

return success;
}

uint8_t W5100Class::init(void)
{
static bool initialized = false;
uint8_t i;

if (initialized) return 1;
Expand Down
15 changes: 13 additions & 2 deletions src/utility/w5100.h
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ enum W5100Linkstatus {
class W5100Class {

public:
static uint8_t reset(void);
static uint8_t init(void);

inline void setGatewayIp(const uint8_t * addr) { writeGAR(addr); }
Expand All @@ -142,11 +143,20 @@ class W5100Class {
inline void setIPAddress(const uint8_t * addr) { writeSIPR(addr); }
inline void getIPAddress(uint8_t * addr) { readSIPR(addr); }

inline void setRetransmissionTime(uint16_t timeout) { if (chip == 55) writeRTR_W5500(timeout); else writeRTR(timeout); }
inline void setRetransmissionCount(uint8_t retry) { if (chip == 55) writeRCR_W5500(retry); else writeRCR(retry); }
inline void setRetransmissionTime(uint16_t timeout) { retransmissionTime = timeout; if (chip == 55) writeRTR_W5500(timeout); else writeRTR(timeout); }
inline uint16_t getRetransmissionTime(void) { if (chip == 55) return readRTR_W5500(); else return readRTR(); }

inline void setRetransmissionCount(uint8_t retry) { retransmissionCount = retry; if (chip == 55) writeRCR_W5500(retry); else writeRCR(retry); }
inline uint8_t getRetransmissionCount(void) { if (chip == 55) return readRCR_W5500(); else return readRCR(); }

static void execCmdSn(SOCKET s, SockCMD _cmd);

private:
uint16_t retransmissionTime = 2000;
uint8_t retransmissionCount = 8;

public:
inline uint16_t getRetransmissionTimeoutMs() { return retransmissionTime / 10 * (retransmissionCount + 1); }

// W5100 Registers
// ---------------
Expand Down Expand Up @@ -321,6 +331,7 @@ class W5100Class {
private:
static uint8_t chip;
static uint8_t ss_pin;
static bool initialized;
static uint8_t softReset(void);
static uint8_t isW5100(void);
static uint8_t isW5200(void);
Expand Down

3 comments on commit 176da0f

@stephanbrunner
Copy link

Choose a reason for hiding this comment

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

Thanks a lot @tryone144 This seems to fix arduino-libraries#166. Maybe you wanna try to initiate a pull request? Or let me know if I can help. I already failed issuing a pull-request due to an error ("Validation failed: must be a collaborator").

@tryone144
Copy link
Owner Author

Choose a reason for hiding this comment

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

This "fix" is mostly based on the work by fredilarsen in arduino-libraries#78. This is a more brute-force approach to the fix proposed in the linked issue, as it forcefully resets the ethernet module.

In its current state, I would not classify this as production ready — reset() is missing the SPI transfer setup and does not work in the current state. Furthermore, in local testing this has some adverse effect on the internal DHCP flow (at least in my network). More testing is needed here...

I created this fork mainly because I needed multiple different fixes from (multiple) pending PRs, that either have not been accepted yet, or their PRs have been closed in the meantime. Sadly, upstream's track record for accepting useful PRs is not that great. 😢

@stephanbrunner
Copy link

@stephanbrunner stephanbrunner commented on 176da0f Jul 5, 2023

Choose a reason for hiding this comment

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

Well, thanks for your insight! I will do further testing to see if we don't get other issues with e.g. dhcp. And hopefully I find time to understand what exactly you mean and maybe find a better solution. The issue itself is pretty disappointing for us, as we are usually looking for (as) bulletproof (as possible) solutions if we install controllers.

Still hoping this workaround is good enough for us! I was pretty exited about the resent tests.

Please sign in to comment.