From ec069a5bcc817abad79c467abc089a33d666d77b Mon Sep 17 00:00:00 2001 From: d00f <8052722+DrDoof@users.noreply.github.com> Date: Tue, 18 Aug 2026 21:58:42 +0200 Subject: [PATCH] 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 | 2 +- httpd/page_impl.c | 2 +- rtl837x_common.h | 2 ++ rtl837x_stp.c | 74 ++++++++++++++--------------------------------- rtl837x_stp.h | 33 +++++++++------------ rtlplayground.c | 8 ++--- 6 files changed, 43 insertions(+), 78 deletions(-) diff --git a/cmd_parser.c b/cmd_parser.c index b724d15..5271dd4 100644 --- a/cmd_parser.c +++ b/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; diff --git a/httpd/page_impl.c b/httpd/page_impl.c index 75fcf08..36b3444 100644 --- a/httpd/page_impl.c +++ b/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\":"); diff --git a/rtl837x_common.h b/rtl837x_common.h index 4dcadce..0fd6f35 100644 --- a/rtl837x_common.h +++ b/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 ` 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. */ diff --git a/rtl837x_stp.c b/rtl837x_stp.c index f82afa3..2697cfc 100644 --- a/rtl837x_stp.c +++ b/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_maxage_s; -__xdata uint8_t stp_fwddelay_s; -__xdata uint8_t stp_rstp; -__xdata uint8_t stp_txhold; +__xdata uint8_t stp_hello_s; /* 1-10 s */ +__xdata uint8_t stp_maxage_s; /* 6-40 s */ +__xdata uint8_t stp_fwddelay_s; /* 4-30 s, also our listen period */ +__xdata uint8_t stp_rstp; /* 1 = RST BPDUs, 0 = legacy Config BPDUs */ +__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 - * checked further down before anything past it is read. */ - if (uip_len < 33) { + if (uip_len < BPDU_LEN_MIN_HEADER) { 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. - * Only the one with the worse Port ID has to stop forwarding - 802.1D - * calls it a backup port. Blocking both, as we used to, kills a segment - * 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. */ + /* Our own BPDU coming back: two of our ports sit on one segment. Only + * the one with the worse Port ID stops forwarding, and only the other + * one writes that state, so the two never race each other. + */ 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 diff --git a/rtl837x_stp.h b/rtl837x_stp.h index a8e68a8..97080ce 100644 --- a/rtl837x_stp.h +++ b/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_defaults(void) __banked; /* boot init: 802.1D/w default configuration */ +void stp_parse(void) __banked __reentrant; +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 stpEnabled; -extern __xdata uint8_t stp_prio; /* bridge priority, high byte: 0x80 = 32768; CLI takes 0-15 (steps of 4096) */ -extern __xdata uint8_t stp_hello_s; /* hello time, 1-10 s (default 2) */ -extern __xdata uint8_t stp_maxage_s; /* max age, 6-40 s (default 20) */ -extern __xdata uint8_t stp_fwddelay_s; /* forward delay, 4-30 s (default 15); our listen period */ -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) */ +extern __xdata uint8_t stp_prio; +extern __xdata uint8_t stp_hello_s; +extern __xdata uint8_t stp_maxage_s; +extern __xdata uint8_t stp_fwddelay_s; +extern __xdata uint8_t stp_rstp; +extern __xdata uint8_t stp_txhold; /* 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 uint8_t stp_root_port; /* logical port towards the root; 0xff = we are root */ -extern __xdata uint16_t stp_tc_count; /* topology change counter (diagnostics) */ +extern __xdata uint32_t root_bridge_cost; +extern __xdata uint8_t stp_root_port; +extern __xdata uint16_t stp_tc_count; #endif diff --git a/rtlplayground.c b/rtlplayground.c index 02fe55c..cf67699 100644 --- a/rtlplayground.c +++ b/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();