stp: rename stpEnabled and take the comments out of the header

The variable lives in rtlplayground.c, so it is declared in rtl837x_common.h
with the others there, and it follows the naming of the rest.

The header carried comments on the externs that the definitions in
rtl837x_stp.c repeat, sometimes differently, which is one place too many to
keep in step. What only the header had, the value ranges and what the
designated arrays hold, moved to the definitions; the rest is gone. Function
declarations lost their comments too.

The status printer only prints, so its running commentary went. A define
replaces the bare 33 in stp_in(), and the note on the loop check is down to
what applies at that line.

cmd_parser.c

@@ -26,7 +26,7 @@
 #pragma constseg BANK2
 
 extern __code struct machine machine;
-extern __xdata uint8_t stpEnabled;
+extern __xdata uint8_t stp_enabled;
 extern __code uint8_t log_to_phys_port[9];
 
 extern volatile __xdata uint32_t ticks;

httpd/page_impl.c

@@ -568,7 +568,7 @@ void send_stp(void)
 	slen = strtox(outbuf, HTTP_RESPONCE_JSON);
 
 	slen += strtox(outbuf + slen, "{\"on\":");
-	bool_to_html(stpEnabled);
+	bool_to_html(stp_enabled);
 	slen += strtox(outbuf + slen, ",\"rstp\":");
 	bool_to_html(stp_rstp);
 	slen += strtox(outbuf + slen, ",\"prio\":");

rtl837x_common.h

@@ -118,6 +118,8 @@ struct flash_region_t {
 
 extern __xdata char port_names[9][PORT_NAME_SIZE];
 
+extern __xdata uint8_t stp_enabled;
+
 /* System hostname (device identity). Set via `hostname <text>` and the System
  * Settings page, reported in /information.json. Other modules (e.g. LLDP, which
  * advertises it as the System Name TLV) read it from here. */

rtl837x_stp.c

@@ -38,18 +38,21 @@ uint8_t atoi_byte(__xdata uint8_t *out, uint8_t idx);
 
 /* ---- Configuration ---- */
 __xdata uint8_t  stp_prio;	/* bridge priority high byte (0x80 = 32768) */
-__xdata uint8_t  stp_hello_s;
+__xdata uint8_t  stp_hello_s;		/* 1-10 s */
-__xdata uint8_t  stp_maxage_s;
+__xdata uint8_t  stp_maxage_s;		/* 6-40 s */
-__xdata uint8_t  stp_fwddelay_s;
+__xdata uint8_t  stp_fwddelay_s;	/* 4-30 s, also our listen period */
-__xdata uint8_t  stp_rstp;
+__xdata uint8_t  stp_rstp;		/* 1 = RST BPDUs, 0 = legacy Config BPDUs */
-__xdata uint8_t  stp_txhold;
+__xdata uint8_t  stp_txhold;		/* BPDUs per port per second */
 
 
 __xdata uint8_t  stp_pflags[10];
-__xdata uint32_t stp_pcost[10];
+__xdata uint32_t stp_pcost[10];		/* 0 = auto */
 __xdata uint8_t  stp_pprio[10];
-__xdata uint8_t  stp_pp2p[10];
+__xdata uint8_t  stp_pp2p[10];		/* admin point-to-point: 0 auto, 1 on, 2 off */
 
+/* Designated bridge, port and cost last heard on the port; stp_bpdu_age tells
+ * whether they are still current.
+ */
 __xdata struct bridge stp_dbridge[10];
 __xdata uint16_t stp_dpid[10];
 __xdata uint32_t stp_dcost[10];
@@ -145,6 +148,7 @@ struct stp_pkt_in {
 
 #define BPDU_LEN_CONFIG		0x26	// LLC and a 35 byte body
 #define BPDU_LEN_RST		0x27	// LLC and a 36 byte body
+#define BPDU_LEN_MIN_HEADER	33	// addresses through bpdu_type
 
 #define BPDU_FLAG_TC		0x01
 #define BPDU_FLAG_LEARNING	0x10
@@ -185,10 +189,9 @@ static void print_field(__code const char *txt, uint8_t idx, uint8_t width) __re
 }
 
 
-/* Where you look when the tree is not what you expected. */
 static void stp_status(void)
 {
-	if (!stpEnabled) {
+	if (!stp_enabled) {
 		print_string("STP off\n");
 		return;
 	}
@@ -217,23 +220,11 @@ static void stp_status(void)
 		print_string("  ");
 		print_field(stp_state_txt, (sfr_data[3 - (stp_i >> 2)] >> ((stp_i << 1) & 0x7)) & 0x3, 5);
 		write_char(' ');
-		/* Only the root port is named. Everything else reads as designated
-		 * because that is all the state machine tracks today; an alternate
-		 * port is a designated one that happens to sit in blocking. */
 		print_field(stp_role_txt, stp_i == stp_root_port ? 1 : 0, 4);
 		write_char(' ');
 		print_field(stp_edge_txt, stp_pflags[stp_i] & STP_PF_OPEREDGE ? 1 : 0, 4);
-		/* BPDUs we put on the wire here. A designated port must show this
-		 * climbing once per hello; the root port never does, because we do
-		 * not announce back towards the root. Without it the only way to
-		 * tell "we are silent" from "the neighbour is not listening" is a
-		 * capture on the far side. */
 		write_char(' ');
 		print_byte(stp_tx_count[stp_i]);
-		/* Seconds since the last BPDU on this port, capped at 255. Without
-		 * it nothing in the output separates "nobody is speaking (R)STP
-		 * out there" from "we are dropping what arrives", and stp_in()
-		 * leaves on eight different conditions without saying so. */
 		write_char(' ');
 		stp_scratch16 = stp_bpdu_age[stp_i] / STP_HZ;
 		itoa(stp_scratch16 > 255 ? 255 : (uint8_t)stp_scratch16);
@@ -242,8 +233,6 @@ static void stp_status(void)
 }
 
 
-/* __reentrant so the temporaries land on the stack: stp_in() is __banked and
- * its locals get exclusive internal RAM, which is what runs out first here. */
 static void stp_record_designated(uint8_t port) __reentrant
 {
 	stp_dbridge[port].prio = STP_I->bridge.prio;
@@ -418,9 +407,7 @@ void stp_in(void) __banked
 {
 	uint8_t port;
 
-	/* The header through bpdu_type is 33 bytes, the full Config/RST body is
+	if (uip_len < BPDU_LEN_MIN_HEADER) {
-	 * checked further down before anything past it is read. */
-	if (uip_len < 33) {
 		uip_len = 0;
 		return;
 	}
@@ -483,29 +470,10 @@ void stp_in(void) __banked
 	if (stp_rxlen < 64)
 		return;
 
-	/* Our own BPDU coming back at us: two of our ports sit on one segment.
+	/* Our own BPDU coming back: two of our ports sit on one segment. Only
-	 * Only the one with the worse Port ID has to stop forwarding - 802.1D
+	 * the one with the worse Port ID stops forwarding, and only the other
-	 * calls it a backup port. Blocking both, as we used to, kills a segment
+	 * one writes that state, so the two never race each other.
-	 * that can still carry traffic, and worse, leaves nobody forwarding to
+	 */
-	 * hear the loop: both then time out of blocking together and the pair
-	 * oscillates for as long as the cable is in (measured: a topology change
-	 * every ~4 s).
-	 *
-	 * The port with the better Port ID decides for both and is the only one
-	 * that touches state - the other just drops the frame. One writer is
-	 * what makes this safe: while both were still deciding for themselves,
-	 * the winner's re-arm landed in the loser's port_timers[] first, the
-	 * loser then read it as "already blocked" and skipped its own
-	 * stp_state_set(), and the loop stayed open. Whether that happened came
-	 * down to which frame the switch handed us first.
-	 *
-	 * The winner is forwarding by construction (nothing here ever blocks
-	 * it), so it goes on hearing the loop and re-arms the loser's timer on
-	 * every BPDU - that is what makes the block a latch rather than a
-	 * forward-delay pulse, and it needs no assumption about a blocked port
-	 * still receiving. Pull the cable and the re-arming stops, so the loser
-	 * comes back on its own after a forward delay - and the link
-	 * supervision above gets there first anyway. */
 	if (cmpBytes(STP_I->bridge.mac, uip_ethaddr.addr, 6) == 0) {
 		/* Equal means the frame came back on the port it left: a loop
 		 * further out, behind an unmanaged switch. There is no pair to
@@ -807,14 +775,14 @@ void stp_parse(void) __banked __reentrant
 
 	if (cmd_compare(1, "on")) {
 		print_string("STP enabled\n");
-		stpEnabled = 1;
+		stp_enabled = 1;
 		stp_setup();
 		return;
 	}
 	if (cmd_compare(1, "off")) {
 		print_string("STP disabled\n");
 		stp_off();
-		stpEnabled = 0;
+		stp_enabled = 0;
 		return;
 	}
 	if (cmd_compare(1, "status")) {
@@ -835,13 +803,13 @@ void stp_parse(void) __banked __reentrant
 		if (cmd_compare(3, "on")) {
 			stp_pflags[port] |= STP_PF_ENABLED;
 			stp_pflags[port] &= ~STP_PF_TRIPPED;
-			if (stpEnabled) {	/* (re)join: listen first */
+			if (stp_enabled) {	/* (re)join: listen first */
 				stp_state_set(port, 0b01);
 				port_timers[port] = (uint16_t)stp_fwddelay_s * STP_HZ;
 			}
 		} else if (cmd_compare(3, "off")) {
 			stp_pflags[port] &= ~STP_PF_ENABLED;
-			if (stpEnabled)
+			if (stp_enabled)
 				stp_state_set(port, 0b11);	/* plain forwarding */
 		} else if (cmd_compare(3, "edge")) {
 			/* Also drop the *operational* edge flag: it is what exempts the

rtl837x_stp.h

@@ -6,8 +6,8 @@ void stp_in(void) __banked;
 void stp_setup(void) __banked;
 void stp_timers(void) __banked;
 void stp_off(void) __banked;
-void stp_parse(void) __banked __reentrant;	/* "stp ..." CLI handler (cmd_parser delegates here) */
+void stp_parse(void) __banked __reentrant;
-void stp_defaults(void) __banked;	/* boot init: 802.1D/w default configuration */
+void stp_defaults(void) __banked;
 
 /* Tick rate of stp_timers(), also used by the web UI. */
 #define STP_HZ 50
@@ -19,14 +19,12 @@ struct bridge {
 	uint8_t mac[6];
 };
 
-/* ---- Configuration (defaults per 802.1D-2004/802.1w, set in stp_defaults) --- */
+extern __xdata uint8_t  stp_prio;
-extern __xdata uint8_t  stpEnabled;
+extern __xdata uint8_t  stp_hello_s;
-extern __xdata uint8_t  stp_prio;	/* bridge priority, high byte: 0x80 = 32768; CLI takes 0-15 (steps of 4096) */
+extern __xdata uint8_t  stp_maxage_s;
-extern __xdata uint8_t  stp_hello_s;	/* hello time, 1-10 s (default 2)   */
+extern __xdata uint8_t  stp_fwddelay_s;
-extern __xdata uint8_t  stp_maxage_s;	/* max age, 6-40 s (default 20)     */
+extern __xdata uint8_t  stp_rstp;
-extern __xdata uint8_t  stp_fwddelay_s;	/* forward delay, 4-30 s (default 15); our listen period */
+extern __xdata uint8_t  stp_txhold;
-extern __xdata uint8_t  stp_rstp;	/* 1 = RSTP BPDUs (v2), 0 = STP-compatible Config BPDUs (v0) */
-extern __xdata uint8_t  stp_txhold;	/* max BPDUs per port per second (default 6) */
 
 /* Per-port config/status flags (stp_pflags[]) */
 #define STP_PF_ENABLED	0x01	/* port participates in STP (default on)     */
@@ -39,21 +37,18 @@ extern __xdata uint8_t  stp_txhold;	/* max BPDUs per port per second (default 6)
 #define STP_PF_TRIPPED	0x80	/* runtime: disabled by BPDU guard           */
 
 extern __xdata uint8_t  stp_pflags[10];
-extern __xdata uint32_t stp_pcost[10];	/* path cost; 0 = auto (20000)      */
+extern __xdata uint32_t stp_pcost[10];
 extern __xdata uint8_t  stp_pprio[10];
-extern __xdata uint8_t  stp_pp2p[10];	/* admin point-to-point: 0 auto, 1 on, 2 off */
+extern __xdata uint8_t  stp_pp2p[10];
 
-/* Last-heard designated info per port (from received BPDUs); consult
- * stp_bpdu_age to decide whether it is still current. */
 extern __xdata struct bridge stp_dbridge[10];
 extern __xdata uint16_t stp_dpid[10];
 extern __xdata uint32_t stp_dcost[10];
-extern __xdata uint16_t stp_bpdu_age[10];	/* ticks since a BPDU was heard */	/* port priority (default 0x80)     */
+extern __xdata uint16_t stp_bpdu_age[10];
 
-/* ---- Status, exposed read-only for the web UI (send_stp) ---- */
 extern __xdata struct bridge root_bridge;
-extern __xdata uint32_t root_bridge_cost;	/* our path cost to the root (0 if we are root) */
+extern __xdata uint32_t root_bridge_cost;
-extern __xdata uint8_t  stp_root_port;		/* logical port towards the root; 0xff = we are root */
+extern __xdata uint8_t  stp_root_port;
-extern __xdata uint16_t stp_tc_count;		/* topology change counter (diagnostics) */
+extern __xdata uint16_t stp_tc_count;
 
 #endif

rtlplayground.c

@@ -120,7 +120,7 @@ __xdata uint16_t rx_packet_vlan;
 __xdata uint16_t management_vlan;
 __xdata uint8_t tx_seq;
 
-__xdata uint8_t stpEnabled;
+__xdata uint8_t stp_enabled;
 __xdata uint8_t igmpEnabled;
 __xdata char hostname[24];	/* device hostname, default set at boot, see rtl837x_common.h */
 
@@ -1167,7 +1167,7 @@ void handle_rx(void)
 		print_byte(uip_buf[3]); print_byte(uip_buf[4]); print_byte(uip_buf[5]); write_char('\n');
 		print_string(" MGMT-VLAN: "); print_short(management_vlan); write_char('\n');
 #endif
-		if (stpEnabled && uip_buf[0] == 0x01 && uip_buf[1] == 0x80 && uip_buf[2] == 0xc2 // STP packet?
+		if (stp_enabled && uip_buf[0] == 0x01 && uip_buf[1] == 0x80 && uip_buf[2] == 0xc2 // STP packet?
 			&& uip_buf[3] == 0x00 && uip_buf[4] == 0x00 && uip_buf[5] == 0x00) {
 			stp_in();
 			if (uip_len) {
@@ -1511,7 +1511,7 @@ void idle(void)
 	// Check UIP for packets to transmit
 	handle_tx();
 	// If STP protocol enabled, decrease STP timers to trigger actions
-	if (stpEnabled) {
+	if (stp_enabled) {
 		if (!stp_clock) {
 			stp_clock = STP_TICK_DIVIDER;
 			stp_timers();
@@ -2200,7 +2200,7 @@ void main(void)
 	REG_SET(RTL837X_REG_SEC_COUNTER, 0x3); write_char(' ');
 	print_reg(RTL837X_REG_SEC_COUNTER);
 #endif
-	stpEnabled = 0;
+	stp_enabled = 0;
 	stp_defaults();		/* 802.1D/w default config before any "stp ..." replay */
 	nic_setup();
 	vlan_setup();