Merge pull request #498 from DrDoof/up/tick-block

rtlplayground: run the timed work once per tick

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'
 

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

rtlplayground.c

@@ -88,11 +88,14 @@ __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 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
+#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
@@ -134,10 +137,13 @@ __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;
 volatile __bit tx_buf_empty;
+__bit rx_seen;
 
 
 struct eth_in {
@@ -489,14 +495,14 @@ void print_cmd_prompt(void)
 }
 
 /*
- * External IRQ 0 Service Routine: Called on link change?
+ * External IRQ 0 Service Routine: the switch reports a link change. The
- * Note that all registers are being put on the STACK because of calling a subroutine
+ * 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
+	EX0 = 0;
-	IT0 = 1;	// Trigger on falling edge of external interrupt
+	link_irq = 1;
-	EX0 = 1;	// Re-enable interrupt
 }
 
 
@@ -991,10 +997,14 @@ void handle_rx(void)
 	__xdata uint8_t budget = RX_BUDGET;
 
 	while (budget--) {
-		// Check the amount of data available on the NIC/ASIC side
+		if (rx_seen) {
-		reg_read(RTL837X_REG_NIC_RX_BUFF_DATA);
+			rx_seen = 0;
-		if (!SFR_DATA_U16)
+		} else {
-			break;
+			// 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;
@@ -1169,57 +1179,8 @@ void handle_button(void)
 	}
 }
 
-//
+void check_links(void)
-// 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);
-	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];
 
@@ -1252,30 +1213,95 @@ void idle(void)
 			cpy_4(linkbits_last, sfr_data);
 		}
 	}
+}
+
+
+//
+// An idle function that sleeps for 1 tick and does all the house-keeping
+//
+static void handle_tick(void)
+{
+	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
+	}
 
 	health_phase(HEALTH_PH_LINK);
-
+	if (TICKS_DUE(sfp_tick_last, SFP_TICK_STEP)) {
-	// Check for changes with SFP modules
+		sfp_tick_last += SFP_TICK_STEP;
-	handle_sfp();
+		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, decrease STP timers to trigger actions
 	if (stp_enabled) {
-		if (!stp_clock) {
+		uint8_t n = STP_CATCH_UP;
-			stp_clock = STP_TICK_DIVIDER;
+		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);
-	// 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();
@@ -1309,12 +1335,13 @@ void reset_chip(void)
 
 void setup_external_irqs(void)
 {
-	REG_SET(0x5f84, 0x42);
+	REG_SET(RTL837X_ISR_SW_INT_MODE, 0x42);
-	REG_SET(0x5f34, 0x3ff);
+	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)
+	link_irq = 1;
-	EX0 = 0;
+	IT0 = 1;	// External IRQ 0 on falling edge (link change)
-	IT0 = 1;	// External IRQ on falling edge
+	EX0 = 1;
 
 	EX1 = 1;	// External IRQ 1 enable
 	EX2 = 1;	// External IRQ 2 enable: bit EIE.0
@@ -1597,7 +1624,9 @@ void check_and_flash_update_image(void)
 void main(void)
 {
 	ticks = 0;
-	stp_clock = STP_TICK_DIVIDER;
+	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;
 

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)
 

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 <stdint.h>
+#include <stdio.h>
+
+#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;
+}