From 214cc6b737cd3feab152ac1a1b6e0f6b61bfe321 Mon Sep 17 00:00:00 2001 From: DrDoof <8052722+DrDoof@users.noreply.github.com> Date: Wed, 23 Sep 2026 16:00:59 +0200 Subject: [PATCH 1/3] rtlplayground: keep the STP and TCP timers on the system tick stp_timers() ran every fourth pass of the main loop and stp_clock counted passes, which only measures time while a pass is one tick long. It never quite was: a pass is as long as the packets it handles, and #482 now takes up to four of them, so under an IPv4 stream a pass is about 16 ms and STP time runs at a third of its rate, with hellos 6 s apart and a root aged out after a minute. Since #483 the loop also stops sleeping while packets wait, so under a flood of cheap frames passes come faster than ticks and STP time runs fast instead, shortening the forward delay in exactly the situation a loop produces. uip_periodic() had the same problem from the other side: it ran once per pass, so the retransmit timer sped up with the loop. Both now go by the tick counter the timer interrupt keeps. The STP block catches up on ticks a long pass missed, at most eight steps at a time, and handle_tx() runs its periodics once per tick at most. One byte of xdata more, 48 bytes of code. A host test drives the gate with one pass every 5 ms, every 16 ms and every 0.6 ms for ten seconds each; the STP clock lands on 500 steps in all three, where counting passes gave 156 and 4166. A pass of 1.3 s loses time rather than firing 65 steps at once, and the arithmetic survives the tick counter wrapping. --- rtl837x_common.h | 8 +++++ rtlplayground.c | 20 ++++++----- test/Makefile | 6 +++- test/test_tick_gate.c | 83 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 9 deletions(-) create mode 100644 test/test_tick_gate.c diff --git a/rtl837x_common.h b/rtl837x_common.h index 737a6829..9b601730 100644 --- a/rtl837x_common.h +++ b/rtl837x_common.h @@ -7,6 +7,14 @@ #define SYS_TICK_HZ 200 +/* Timers of the main loop count system ticks, not loop passes: a pass is as + * long as the packets it handles, and since #483 the loop no longer sleeps + * while packets wait, so passes can outrun the tick as well as lag it. `last` + * is the low byte of ticks when the timer last ran; the byte arithmetic wraps + * every 256 ticks, so a pass longer than 1.28 s loses time rather than + * running the timer 64 times to catch up. */ +#define TICKS_DUE(last, period) ((uint8_t)((uint8_t)ticks - (last)) >= (period)) + #define CPU_PORT 9 #define NUL '\0' diff --git a/rtlplayground.c b/rtlplayground.c index 80f599cb..cd1281f7 100644 --- a/rtlplayground.c +++ b/rtlplayground.c @@ -88,11 +88,13 @@ __xdata struct uip_eth_addr uip_ethaddr; volatile __xdata uint32_t ticks; volatile __xdata uint8_t sec_counter; volatile __xdata uint16_t sleep_ticks; -__xdata uint8_t stp_clock; +__xdata uint8_t stp_tick_last; +__xdata uint8_t tx_tick_last; __xdata uint8_t arp_age_secs; extern __xdata struct dhcp_state dhcp_state; -#define STP_TICK_DIVIDER 3 +#define STP_TICK_STEP (SYS_TICK_HZ / STP_HZ) +#define STP_CATCH_UP 8 /* Buffer for serial input, SBUF_SIZE must be power of 2 < 256 * Writing to this buffer is under the sole control of the serial ISR @@ -1075,6 +1077,9 @@ void handle_rx(void) void handle_tx(void) { + if (!TICKS_DUE(tx_tick_last, 1)) + return; + tx_tick_last = (uint8_t)ticks; for(uint8_t i = 0; i < UIP_CONNS; i++) { uip_periodic(i); if(uip_len > 0) { @@ -1265,13 +1270,12 @@ void idle(void) // Check UIP for packets to transmit handle_tx(); health_phase(HEALTH_PH_TX); - // If STP protocol enabled, decrease STP timers to trigger actions + // If STP protocol enabled, run its timers once per STP tick that has passed if (stp_enabled) { - if (!stp_clock) { - stp_clock = STP_TICK_DIVIDER; + uint8_t n = STP_CATCH_UP; + while (n-- && TICKS_DUE(stp_tick_last, STP_TICK_STEP)) { + stp_tick_last += STP_TICK_STEP; stp_timers(); - } else { - stp_clock--; } } health_phase(HEALTH_PH_STP); @@ -1597,7 +1601,7 @@ void check_and_flash_update_image(void) void main(void) { ticks = 0; - stp_clock = STP_TICK_DIVIDER; + stp_tick_last = (uint8_t)ticks; dhcp_state.state = DHCP_OFF; sbuf_ptr = 0; diff --git a/test/Makefile b/test/Makefile index e394aec4..ae3884ac 100644 --- a/test/Makefile +++ b/test/Makefile @@ -27,7 +27,8 @@ BUILDDIR = build TESTS = $(BUILDDIR)/test_cmd_editor \ $(BUILDDIR)/test_port_tables \ $(BUILDDIR)/test_page_json \ - $(BUILDDIR)/test_httpd_tx + $(BUILDDIR)/test_httpd_tx \ + $(BUILDDIR)/test_tick_gate # The table tests link the register mock and one environment file; stub/ # stands in for the two headers a firmware build generates. @@ -47,6 +48,9 @@ $(BUILDDIR)/test_cmd_editor: test_cmd_editor.c support.c $(SRCROOT)/cmd_editor.c $(BUILDDIR)/test_port_tables: test_port_tables.c $(TABLE_ENV) hw_mock.h | $(BUILDDIR) $(CC) $(CFLAGS) $(filter %.c,$^) -o $@ $(LDFLAGS) +$(BUILDDIR)/test_tick_gate: test_tick_gate.c support.c $(SRCROOT)/cmd_editor.c | $(BUILDDIR) + $(CC) $(CFLAGS) $^ -o $@ $(LDFLAGS) + $(BUILDDIR)/test_page_json: test_page_json.c $(TABLE_ENV) hw_mock.h | $(BUILDDIR) $(CC) $(CFLAGS) $(filter %.c,$^) -o $@ $(LDFLAGS) diff --git a/test/test_tick_gate.c b/test/test_tick_gate.c new file mode 100644 index 00000000..3a9a62b9 --- /dev/null +++ b/test/test_tick_gate.c @@ -0,0 +1,83 @@ +/* + * test_tick_gate.c - the timers of the main loop keep time by the system + * tick, however often the loop passes. The gate is the TICKS_DUE macro from + * rtl837x_common.h; the loop bodies of idle() and handle_tx() are restated + * here, so this checks the arithmetic, not those functions. + */ +#include +#include + +#include "rtl837x_common.h" +#include "rtl837x_stp.h" +#include "support.h" + +volatile uint32_t ticks; + +#define STP_TICK_STEP (SYS_TICK_HZ / STP_HZ) +#define STP_CATCH_UP 8 + +static uint8_t stp_tick_last, tx_tick_last; +static unsigned stp_calls, tx_calls; + +/* One pass of the loop: the STP block of idle() and the gate of handle_tx() */ +static void pass(void) +{ + uint8_t n = STP_CATCH_UP; + while (n-- && TICKS_DUE(stp_tick_last, STP_TICK_STEP)) { + stp_tick_last += STP_TICK_STEP; + stp_calls++; + } + if (TICKS_DUE(tx_tick_last, 1)) { + tx_tick_last = (uint8_t)ticks; + tx_calls++; + } +} + +/* Run `seconds` of wall clock with one pass every `pass_us` microseconds. */ +static void run(double pass_us, unsigned seconds) +{ + double t = 0, next_tick = 0, tick_us = 1e6 / SYS_TICK_HZ, end = seconds * 1e6; + ticks = 0; + stp_tick_last = tx_tick_last = 0; + stp_calls = tx_calls = 0; + while (t < end) { + while (next_tick <= t) { ticks++; next_tick += tick_us; } + pass(); + t += pass_us; + } +} + +static void t_rates(void) +{ + printf("[test] STP time follows the tick whatever the loop does\n"); + run(5000, 10); + CHECK(stp_calls == 10 * STP_HZ, "one pass per tick: 500 STP steps in 10 s"); + CHECK(tx_calls == 10 * SYS_TICK_HZ, "and 2000 TX periodics"); + run(16000, 10); + CHECK(stp_calls >= 10 * STP_HZ - 1 && stp_calls <= 10 * STP_HZ, "a 16 ms pass, as under an IPv4 stream: still 500 (was 156 when the clock counted passes)"); + CHECK(tx_calls == 625, "TX periodic once per pass when passes are slower than the tick"); + run(600, 10); + CHECK(stp_calls == 10 * STP_HZ, "a 0.6 ms pass, the loop no longer sleeping: still 500 (was 4166)"); + CHECK(tx_calls == 10 * SYS_TICK_HZ, "TX periodic capped at the tick rate"); + run(1300000, 13); + CHECK(stp_calls <= 10 * STP_CATCH_UP, "a 1.3 s pass loses time instead of firing 65 steps at once"); +} + +static void t_wrap(void) +{ + printf("[test] the byte arithmetic survives the tick counter wrapping\n"); + ticks = 0xfffffffeUL; + stp_tick_last = (uint8_t)ticks; + stp_calls = 0; + for (int i = 0; i < 8; i++) { ticks++; pass(); } + CHECK(stp_calls == 2, "two STP steps across 0xffffffff -> 0x00000006"); +} + +int main(void) +{ + printf("== tick gate tests ==\n"); + t_rates(); + t_wrap(); + printf("\n%d checks, %d failed\n", tests_run, tests_failed); + return tests_failed ? 1 : 0; +} From f87eb70c6a881e6656693ab7edd9984afaf98b42 Mon Sep 17 00:00:00 2001 From: DrDoof <8052722+DrDoof@users.noreply.github.com> Date: Wed, 23 Sep 2026 16:04:23 +0200 Subject: [PATCH 2/3] rtlplayground: read the link registers when the switch reports a change The link-change interrupt was set up at boot (mode 0x5f84, mask 0x5f34) but never enabled: the handler did nothing and did not clear the event in the switch, so EX0 stayed commented out and idle() read the two link registers on every pass instead, 400 reads a second on a quiet switch. The handler now only latches a flag and masks itself; the register access macros build a write in the shared SFR data bytes, so an interrupt that touched the switch in the middle of a write from the main loop would corrupt it. idle() clears the event register (write-one-to-clear, 0x3ff for the ten ports), re-enables EX0 and then reads the link state, in that order, so a change that lands during the read raises a new edge. The flag is also set once a second from the seconds block, so a board whose INT0 line turns out not to be wired keeps detecting link changes, a second late at worst. Boot drops the events pending from bringing the links up before the mask is set. Measured on an RTL8373 switch before this change: 0x5f88 held 0x3fd, nine ports had reported a change nobody read. With the change a counter in the loop went 2 -> 3 -> 4 on "port 5 off" / "port 5 on" and stayed at 4 over three reads without events, so the event is delivered, cleared and re-armed, and there is no flood. SWTGW218AS: code +57 B, xdata 0, stack 151 -> 150 B; the flag is a bit variable. The handler compiles to clr EX0 / setb link_irq / reti. idle() also keeps in a bit what it saw in NIC_RX_BUFF_DATA when deciding whether to sleep, and the first iteration of the receive loop uses it instead of reading the register again a moment later. Between the two only the link check and handle_sfp() run, and neither can make a queued packet disappear. Every later iteration reads the register as before, so the budget loop behaves as it did. This part was not measured on hardware: one register read per pass needs a throughput run to show up. SWTGW218AS: BANK1 and BANK2 +6 bytes each, BANK3 unchanged. --- rtl837x_regs.h | 3 + rtlplayground.c | 148 +++++++++++++++++++++++++++--------------------- 2 files changed, 87 insertions(+), 64 deletions(-) diff --git a/rtl837x_regs.h b/rtl837x_regs.h index 8d85accd..6409043d 100644 --- a/rtl837x_regs.h +++ b/rtl837x_regs.h @@ -274,6 +274,9 @@ */ #define RTL8373_RLDP_TIMER 0x1074 #define RTL837X_RMA0_CONF 0x4ecc +#define RTL837X_IMR_INT_PORT_LINK_STS_CHG 0x5f34 +#define RTL837X_ISR_SW_INT_MODE 0x5f84 +#define RTL837X_ISR_INT_PORT_LINK_CHG 0x5f88 #define RTL837X_RMA_CONF 0x4f1c #define RTL837X_MSTP_STATES 0x5310 #define RTL837X_REG_LED_RLDP_1 0x65F8 diff --git a/rtlplayground.c b/rtlplayground.c index cd1281f7..0208f844 100644 --- a/rtlplayground.c +++ b/rtlplayground.c @@ -136,10 +136,12 @@ __code const uint16_t bit_mask[16] = { __xdata uint8_t linkbits_last[4]; __xdata uint8_t linkbits_last_p89; +volatile __bit link_irq; // Last known state of the SFP detection/Loss of Signal pins __xdata bool button_last; __xdata uint8_t button_sec_counter_last; volatile __bit tx_buf_empty; +__bit rx_seen; struct eth_in { @@ -491,14 +493,14 @@ void print_cmd_prompt(void) } /* - * External IRQ 0 Service Routine: Called on link change? - * Note that all registers are being put on the STACK because of calling a subroutine + * External IRQ 0 Service Routine: the switch reports a link change. The + * register access macros share the SFR data bytes with the main loop, so + * the handler only latches the event; idle() reads and clears it. */ void isr_ext0(void) __interrupt(0) { - EX0 = 0; // Disable interrupt for the moment - IT0 = 1; // Trigger on falling edge of external interrupt - EX0 = 1; // Re-enable interrupt + EX0 = 0; + link_irq = 1; } @@ -993,10 +995,14 @@ void handle_rx(void) __xdata uint8_t budget = RX_BUDGET; while (budget--) { - // Check the amount of data available on the NIC/ASIC side - reg_read(RTL837X_REG_NIC_RX_BUFF_DATA); - if (!SFR_DATA_U16) - break; + if (rx_seen) { + rx_seen = 0; + } else { + // Check the amount of data available on the NIC/ASIC side + reg_read(RTL837X_REG_NIC_RX_BUFF_DATA); + if (!SFR_DATA_U16) + break; + } reg_read(RTL837X_REG_CPU_RX_CURR_PKT); uint16_t ring_ptr = SFR_DATA_U16; ring_ptr <<= 3; @@ -1174,57 +1180,8 @@ void handle_button(void) } } -// -// An idle function that sleeps for 1 tick and does all the house-keeping -// -void idle(void) +void check_links(void) { - reg_read(RTL837X_REG_NIC_RX_BUFF_DATA); - if (!SFR_DATA_U16) - PCON |= 1; - health_loop_start(); - if (sec_counter >= SYS_TICK_HZ) { - sec_counter -= SYS_TICK_HZ; - reg_read_m(RTL837X_REG_SEC_COUNTER); - uint8_t v = sfr_data[3]; -#ifdef DEBUG - print_string(" Tick counter: "); print_long(ticks); write_char('\n'); -#endif - v++; - sfr_data[3] = v; - if (!v) { - v = sfr_data[2]; - v++; - sfr_data[2] = v; - if (!v) { - v = sfr_data[1]; - v++; - sfr_data[1] = v; - if (!v) { - v = sfr_data[0]; - v++; - sfr_data[0] = v; - } - } - } - reg_write_m(RTL837X_REG_SEC_COUNTER); - reg_read_m(RTL837X_REG_SEC_COUNTER); - - // Check for button presses once a second - handle_button(); - // Age the ARP cache: uip_arp_timer() expects a 10 s cadence - if (++arp_age_secs >= 10) { - arp_age_secs = 0; - uip_arp_timer(); - } - -#ifdef DEBUG - print_sfr_data(); - write_char('\n'); -#endif - } - - // Check for Link changes reg_read_m(RTL837X_REG_LINKS_89); __xdata uint8_t linkbits_p89 = sfr_data[3]; @@ -1257,6 +1214,68 @@ void idle(void) cpy_4(linkbits_last, sfr_data); } } +} + + +// +// An idle function that sleeps for 1 tick and does all the house-keeping +// +void idle(void) +{ + reg_read(RTL837X_REG_NIC_RX_BUFF_DATA); + rx_seen = SFR_DATA_U16 != 0; + if (!rx_seen) + PCON |= 1; + health_loop_start(); + if (sec_counter >= SYS_TICK_HZ) { + sec_counter -= SYS_TICK_HZ; + reg_read_m(RTL837X_REG_SEC_COUNTER); + uint8_t v = sfr_data[3]; +#ifdef DEBUG + print_string(" Tick counter: "); print_long(ticks); write_char('\n'); +#endif + v++; + sfr_data[3] = v; + if (!v) { + v = sfr_data[2]; + v++; + sfr_data[2] = v; + if (!v) { + v = sfr_data[1]; + v++; + sfr_data[1] = v; + if (!v) { + v = sfr_data[0]; + v++; + sfr_data[0] = v; + } + } + } + reg_write_m(RTL837X_REG_SEC_COUNTER); + reg_read_m(RTL837X_REG_SEC_COUNTER); + + // Check for button presses once a second + handle_button(); + link_irq = 1; + // Age the ARP cache: uip_arp_timer() expects a 10 s cadence + if (++arp_age_secs >= 10) { + arp_age_secs = 0; + uip_arp_timer(); + } + +#ifdef DEBUG + print_sfr_data(); + write_char('\n'); +#endif + } + + // Check for Link changes when the switch has reported one + if (link_irq) { + link_irq = 0; + REG_SET(RTL837X_ISR_INT_PORT_LINK_CHG, 0x3ff); + EX0 = 1; + check_links(); + } health_phase(HEALTH_PH_LINK); @@ -1313,12 +1332,13 @@ void reset_chip(void) void setup_external_irqs(void) { - REG_SET(0x5f84, 0x42); - REG_SET(0x5f34, 0x3ff); + REG_SET(RTL837X_ISR_SW_INT_MODE, 0x42); + REG_SET(RTL837X_ISR_INT_PORT_LINK_CHG, 0x3ff); + REG_SET(RTL837X_IMR_INT_PORT_LINK_STS_CHG, 0x3ff); -// EX0 = 1; // Enable external IRQ 0 (Link-change) - EX0 = 0; - IT0 = 1; // External IRQ on falling edge + link_irq = 1; + IT0 = 1; // External IRQ 0 on falling edge (link change) + EX0 = 1; EX1 = 1; // External IRQ 1 enable EX2 = 1; // External IRQ 2 enable: bit EIE.0 From ac3b4b17ae2bab0ce8d48cd46a7e0cb860358718 Mon Sep 17 00:00:00 2001 From: DrDoof <8052722+DrDoof@users.noreply.github.com> Date: Mon, 21 Sep 2026 16:36:47 +0200 Subject: [PATCH 3/3] rtlplayground: run the timed work once per tick idle() ran everything on every pass: the seconds block, the SFP pin test, the TCP periodic timer behind a gate of its own, the STP timers behind theirs. The timed work moves into handle_tick(), which idle() calls once per system tick from a single gate; handle_tx() loses its private one. handle_sfp() ran on every pass as well, two GPIO reads per pass to learn that nothing changed; it now runs every ten ticks. A module inserted is still seen within 50 ms, and the read itself has been off the loop since the wake-up wait moved to the tick. No change in behaviour otherwise: the link, receive and console paths stay where they were. SWTGW218AS: code +45 B, xdata +1 B, stack unchanged. The health checkpoints follow the work they measure: the seconds block, SFP, TX and STP phases are taken inside handle_tick(), the link, RX and command phases in idle(). --- rtlplayground.c | 61 ++++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 28 deletions(-) diff --git a/rtlplayground.c b/rtlplayground.c index 0208f844..0ef86eed 100644 --- a/rtlplayground.c +++ b/rtlplayground.c @@ -89,12 +89,13 @@ volatile __xdata uint32_t ticks; volatile __xdata uint8_t sec_counter; volatile __xdata uint16_t sleep_ticks; __xdata uint8_t stp_tick_last; -__xdata uint8_t tx_tick_last; __xdata uint8_t arp_age_secs; extern __xdata struct dhcp_state dhcp_state; #define STP_TICK_STEP (SYS_TICK_HZ / STP_HZ) #define STP_CATCH_UP 8 +#define SFP_TICK_STEP 10 +__xdata uint8_t sfp_tick_last; /* Buffer for serial input, SBUF_SIZE must be power of 2 < 256 * Writing to this buffer is under the sole control of the serial ISR @@ -137,6 +138,7 @@ __code const uint16_t bit_mask[16] = { __xdata uint8_t linkbits_last[4]; __xdata uint8_t linkbits_last_p89; volatile __bit link_irq; +__xdata uint8_t idle_tick_last; // Last known state of the SFP detection/Loss of Signal pins __xdata bool button_last; __xdata uint8_t button_sec_counter_last; @@ -1083,9 +1085,6 @@ void handle_rx(void) void handle_tx(void) { - if (!TICKS_DUE(tx_tick_last, 1)) - return; - tx_tick_last = (uint8_t)ticks; for(uint8_t i = 0; i < UIP_CONNS; i++) { uip_periodic(i); if(uip_len > 0) { @@ -1220,13 +1219,8 @@ void check_links(void) // // An idle function that sleeps for 1 tick and does all the house-keeping // -void idle(void) +static void handle_tick(void) { - reg_read(RTL837X_REG_NIC_RX_BUFF_DATA); - rx_seen = SFR_DATA_U16 != 0; - if (!rx_seen) - PCON |= 1; - health_loop_start(); if (sec_counter >= SYS_TICK_HZ) { sec_counter -= SYS_TICK_HZ; reg_read_m(RTL837X_REG_SEC_COUNTER); @@ -1269,27 +1263,14 @@ void idle(void) #endif } - // Check for Link changes when the switch has reported one - if (link_irq) { - link_irq = 0; - REG_SET(RTL837X_ISR_INT_PORT_LINK_CHG, 0x3ff); - EX0 = 1; - check_links(); - } - health_phase(HEALTH_PH_LINK); - - // Check for changes with SFP modules - handle_sfp(); + if (TICKS_DUE(sfp_tick_last, SFP_TICK_STEP)) { + sfp_tick_last += SFP_TICK_STEP; + handle_sfp(); + } health_phase(HEALTH_PH_SFP); - - // Check new Packets RX - handle_rx(); - health_phase(HEALTH_PH_RX); - // Check UIP for packets to transmit handle_tx(); health_phase(HEALTH_PH_TX); - // If STP protocol enabled, run its timers once per STP tick that has passed if (stp_enabled) { uint8_t n = STP_CATCH_UP; while (n-- && TICKS_DUE(stp_tick_last, STP_TICK_STEP)) { @@ -1298,7 +1279,29 @@ void idle(void) } } health_phase(HEALTH_PH_STP); - // Check whether a command is waiting in the cmd_buffer and execute +} + + +void idle(void) +{ + reg_read(RTL837X_REG_NIC_RX_BUFF_DATA); + rx_seen = SFR_DATA_U16 != 0; + if (!rx_seen) + PCON |= 1; + health_loop_start(); + if (TICKS_DUE(idle_tick_last, 1)) { + idle_tick_last = (uint8_t)ticks; + handle_tick(); + } + if (link_irq) { + link_irq = 0; + REG_SET(RTL837X_ISR_INT_PORT_LINK_CHG, 0x3ff); + EX0 = 1; + check_links(); + } + health_phase(HEALTH_PH_LINK); + handle_rx(); + health_phase(HEALTH_PH_RX); if (cmd_available) { cmd_available = 0; cmd_tokenize(); @@ -1622,6 +1625,8 @@ void main(void) { ticks = 0; stp_tick_last = (uint8_t)ticks; + idle_tick_last = (uint8_t)ticks; + sfp_tick_last = (uint8_t)ticks; dhcp_state.state = DHCP_OFF; sbuf_ptr = 0;