Page 1 of 2
LWIP: bug in sys_now
Posted: Thu Mar 07, 2024 5:18 pm
by drizzi
Hello,
regarding the LWIP bindings, I think there is a bug in sys_now (sys_arch.c), at least in the (OSAL_ST_FREQUENCY / 1000) >= 1 && OSAL_ST_FREQUENCY % 1000) == 0 case.
LWIP expects sys_now to wrap at 0xFFFFFFFF but, if e.g. OSAL_ST_FREQUENCY == 10kHz it wraps at 0x1999999A
Code: Select all
return ((u32_t)chVTGetSystemTimeX() - 1) / (OSAL_ST_FREQUENCY / 1000) + 1;
(0xFFFFFFFF-1)/10 + 1 = 0x1999999A
This result in the TCP thread to hang after ~5 days (see sys_timeouts_sleeptime and TIME_LESS_THAN in LWIP timeouts.c).
Re: LWIP: bug in sys_now
Posted: Thu Mar 07, 2024 9:24 pm
by Giovanni
Hi,
Moving in bug reports.
Giovanni
Re: LWIP: bug in sys_now
Posted: Tue Aug 20, 2024 5:44 pm
by steved
I concur. I've added a routine which extends the resolution of chVTGetSystemTimeX() for that specific case (it doesn't appear necessary to include the adjustments by 1):
Code: Select all
/**
* This routine creates a 32-bit incrementing timer tick which returns all possible
* values before wrapping round.
* Intended for situations where OSAL_ST_FREQUENCY is a multiple of =1kHz
* It's been tested with OSAL_ST_FREQUENCY=10000
* Must be called at least a few times for each complete cycling of the system tick
*/
static u32_t calcNext(u32_t ctr)
{
#define DIVISOR (OSAL_ST_FREQUENCY / 1000)
static u32_t topBit = 0;
static u32_t lastCtr = 0;
if (ctr < lastCtr)
{
/* wraparound */
if (++topBit >= DIVISOR)
topBit = 0;
}
lastCtr = ctr;
return (u32_t)((((uint64_t)(topBit) << 32) + ctr)/DIVISOR);
}
u32_t sys_now(void) {
#if OSAL_ST_FREQUENCY == 1000
return (u32_t)chVTGetSystemTimeX();
#elif (OSAL_ST_FREQUENCY / 1000) >= 1 && (OSAL_ST_FREQUENCY % 1000) == 0
return calcNext((u32_t)chVTGetSystemTimeX());
//return ((u32_t)chVTGetSystemTimeX() - 1) / (OSAL_ST_FREQUENCY / 1000) + 1;
#elif (1000 / OSAL_ST_FREQUENCY) >= 1 && (1000 % OSAL_ST_FREQUENCY) == 0
return ((u32_t)chVTGetSystemTimeX() - 1) * (1000 / OSAL_ST_FREQUENCY) + 1;
#else
return (u32_t)(((u64_t)(chVTGetSystemTimeX() - 1) * 1000) / OSAL_ST_FREQUENCY) + 1;
#endif
}
Re: LWIP: bug in sys_now
Posted: Tue Aug 20, 2024 9:13 pm
by twarge
I am thrilled that you identified this; hopefully it is the reason our ethernet dies. It has been a problem for *years*.
Re: LWIP: bug in sys_now
Posted: Wed Aug 21, 2024 12:49 pm
by Giovanni
This is actually very old, the following solution was also proposed:
https://sourceforge.net/p/chibios/bugs/973/Which one should I include?
Giovanni
Re: LWIP: bug in sys_now
Posted: Wed Aug 21, 2024 4:26 pm
by steved
Didn't find that proposal.
I'd tend towards the SourceForge suggestion, simply on the basis that it appears to have covered all the options (I'd really just covered the one that mattered to me).
However depends how well its been tested.
I'll try and run it through my test harness in the next few days.
Re: LWIP: bug in sys_now
Posted: Fri Aug 23, 2024 10:07 am
by steved
I've compared the two options for SYSTICK of 5kHz, 10kHz, 50kHz, 100kHz.
For the first two, both proposals agree.
For the second two, the results diverge.
Given this, I'd tend towards my solution because its easier to understand (extend the SYsTick counter beyond 32 bits, then divide), which is not to say the other solution is wrong.
I've attached my test harness (which runs on a PC) in case anyone wants to look further
Re: LWIP: bug in sys_now
Posted: Fri Aug 23, 2024 4:59 pm
by Giovanni
Sorry, I am confused, they cannot diverge and be both not wrong.
In addition, the proposal does not address the following cases which are probably also wrong:
Code: Select all
#elif (1000 / OSAL_ST_FREQUENCY) >= 1 && (1000 % OSAL_ST_FREQUENCY) == 0
return ((u32_t)chVTGetSystemTimeX() - 1) * (1000 / OSAL_ST_FREQUENCY) + 1;
#else
return (u32_t)(((u64_t)(chVTGetSystemTimeX() - 1) * 1000) / OSAL_ST_FREQUENCY) + 1;
#endif
Another problem is that the system time can be 16 bits wide, in this case everything fails, even the case "OSAL_ST_FREQUENCY == 1000". There are combinations of counter width and frequency which are not compatible at all, imagine 100MHz and 16 bits counter, which is actually supported, such a condition should be detected (system time total time should be greater of 1mS).
Sufficient headache for today, will try again tomorrow
Giovanni
Re: LWIP: bug in sys_now
Posted: Sat Aug 24, 2024 9:25 am
by Giovanni
Hi,
What about something like this?
Code: Select all
#if (OSAL_ST_FREQUENCY < 1000) || ((OSAL_ST_FREQUENCY % 1000) != 0)
#error "LWIP requires a systick frequency that is a multiple of 1000"
#endif
#if (OSAL_ST_RESOLUTION >= 32) && (OSAL_ST_FREQUENCY == 1000)
u32_t sys_now(void) {return (u32_t)osalOsGetSystemTimeX();}
#else
u32_t sys_now(void) {
static u32_t last_ms = (u32_t)0;
static u32_t last_unprocessed = (u32_t)0;
static systime_t last_system_time = (systime_t)0;
u32_t delta;
systime_t now_time;
/* Calculating delta, in ticks, from the last acquired system time.*/
now_time = osalOsGetSystemTimeX();
delta = (u32_t)osalTimeDiffX(last_system_time, now_time) + last_unprocessed;
last_system_time = now_time;
last_ms += delta / (OSAL_ST_FREQUENCY / 1000U);
last_unprocessed = delta % (OSAL_ST_FREQUENCY / 1000U);
return last_ms;
}
#endif
Trying to avoid 64 bits math and handle 16 bits counters.
Note, not tested yet, no HW until September.
Giovanni
Re: LWIP: bug in sys_now
Posted: Tue Aug 27, 2024 9:42 am
by Giovanni
Any feedback?
Giovanni