Strip over-commenting in test_handlers.cpp

firmware/lib/test_handlers.cpp

@@ -10,8 +10,6 @@
 #include "parse_buffer.h"
 #include "prepend_buffer.h"
 
-// Echo IDs are just tags used to match our ping replies; constants suffice
-// since tests are one-at-a-time.
 static constexpr uint16_t PING_ECHO_ID = 0x1234;
 static constexpr uint16_t PING_RATE_ECHO_ID = 0x5678;
 
@@ -20,16 +18,11 @@ struct peer_info {
     ipv4::ip4_addr ip;
 };
 
-// Shared sub-struct types. Used as fields inside per-test-command structs
-// when two tests happen to carry the same shape of data.
-
-// State for the discover_peer primitive.
 struct discovery_data {
     void (*on_found)(const peer_info&) = nullptr;
     void (*on_timeout)() = nullptr;
 };
 
-// Rolling state for a rate-testing ping run.
 struct ping_rate_data {
     peer_info peer;
     uint16_t target;
@@ -40,8 +33,6 @@ struct ping_rate_data {
     uint32_t start_us;
 };
 
-// One struct per test command. Empty structs (where a test command has no
-// extra state beyond the common fields) are kept for symmetry/documentation.
 struct discovery_igmp_test {};
 struct discovery_info_test {
     discovery_data discovery;
@@ -57,23 +48,17 @@ struct byte_rate_test {
     ping_rate_data rate;
 };
 
-// All test state lives in this single instance. The protocol is one-test-at-
+// One test runs at a time; in_flight gates that. active_* let shared
-// a-time: handle_test rejects a new request while in_flight is set. Only the
+// primitive callbacks find the running test's sub-state.
-// fields for the running test are populated; the rest are wasted bytes traded
-// for clarity.
 struct test_state {
-    // Common to every async test.
     bool in_flight = false;
     responder resp;
     timer_handle timer = nullptr;
     frame_cb_handle frame_cb = nullptr;
 
-    // Views into the running test's shared sub-state so the shared primitive
-    // callbacks (discover_peer, ping_rate) know where to find it.
     discovery_data* active_discovery = nullptr;
     ping_rate_data* active_rate = nullptr;
 
-    // Per-test-command storage.
     discovery_igmp_test discovery_igmp;
     discovery_info_test discovery_info;
     ping_subnet_test ping_subnet;
@@ -93,13 +78,9 @@ static void test_end(const ResponseTest& result) {
     ts.in_flight = false;
 }
 
-// ----- discover_peer (shared building block) -----
+// When a callback fires, its dispatcher (net or timer_queue) has already
-
+// removed the node; the matching ts.timer / ts.frame_cb is stale. Callbacks
-// Note on frame/timer handle lifetimes: when a callback fires, the dispatcher
+// that self-consume must null that handle before calling test_end.
-// (net or timer_queue) has already taken ownership of the node. Callbacks that
-// self-consume (frame returning true, or any timer fire) must null the
-// matching `ts.` handle before calling test_end, so test_end doesn't try to
-// cancel a stale handle.
 
 static bool discover_reply_cb(std::span<const uint8_t> frame) {
     parse_buffer pb(frame);
@@ -114,7 +95,7 @@ static bool discover_reply_cb(std::span<const uint8_t> frame) {
     if (ip->src == net_get_state().ip) return false;
     dispatch_cancel_timer(ts.timer);
     ts.timer = nullptr;
-    ts.frame_cb = nullptr;  // self-consumed via `return true` below
+    ts.frame_cb = nullptr;
     auto cont = ts.active_discovery ? ts.active_discovery->on_found : nullptr;
     ts.active_discovery = nullptr;
     peer_info peer{eth_hdr->src, ip->src};
@@ -161,19 +142,17 @@ static void discover_peer(discovery_data& d,
     net_send_raw(buf.span());
 }
 
-// ----- discovery_igmp -----
-
 static bool igmp_report_cb(std::span<const uint8_t> frame) {
     ipv4::ip4_addr group;
     if (!igmp::parse_report(frame, group)) return false;
     if (group != igmp::PICOMAP_DISCOVERY_GROUP) return false;
-    ts.frame_cb = nullptr;  // self-consumed via `return true`
+    ts.frame_cb = nullptr;
     test_end({true, {"got IGMP report for " + ipv4::to_string(group)}});
     return true;
 }
 
 static void igmp_timeout_cb() {
-    ts.timer = nullptr;  // already fired
+    ts.timer = nullptr;
     test_end({false, {"no IGMP report within 5s"}});
 }
 
@@ -188,8 +167,6 @@ static void test_discovery_igmp() {
     net_send_raw(buf.span());
 }
 
-// ----- discovery_info -----
-
 static void info_found(const peer_info& peer) {
     test_end({true, {"got info response from " + ipv4::to_string(peer.ip)}});
 }
@@ -202,12 +179,10 @@ static void test_discovery_info() {
     discover_peer(ts.discovery_info.discovery, info_found, info_timeout);
 }
 
-// ----- ping_subnet / ping_global -----
-
 static bool ping_reply_cb(std::span<const uint8_t> frame) {
     ipv4::ip4_addr src_ip;
     if (!icmp::parse_echo_reply(frame, src_ip, PING_ECHO_ID)) return false;
-    ts.frame_cb = nullptr;  // self-consumed via `return true`
+    ts.frame_cb = nullptr;
     if (src_ip == net_get_state().ip)
         test_end({false, {"got reply from self: " + ipv4::to_string(src_ip)}});
     else
@@ -216,7 +191,7 @@ static bool ping_reply_cb(std::span<const uint8_t> frame) {
 }
 
 static void ping_timeout_cb() {
-    ts.timer = nullptr;  // already fired
+    ts.timer = nullptr;
     test_end({false, {"no reply from non-self host within 5s"}});
 }
 
@@ -233,8 +208,6 @@ static void start_ping(ipv4::ip4_addr dst_ip) {
 static void test_ping_subnet() { start_ping({169, 254, 255, 255}); }
 static void test_ping_global() { start_ping({255, 255, 255, 255}); }
 
-// ----- packet_rate / byte_rate -----
-
 static size_t ping_rate_frame_size() {
     return sizeof(eth::header) + sizeof(ipv4::header) + sizeof(icmp::echo)
          + ts.active_rate->payload_len;
@@ -274,7 +247,7 @@ static bool ping_rate_reply_cb(std::span<const uint8_t> frame) {
                  static_cast<unsigned long>(pps),
                  static_cast<unsigned long>(total_bytes),
                  static_cast<unsigned long>(kbps));
-        ts.frame_cb = nullptr;  // self-consumed via `return true`
+        ts.frame_cb = nullptr;
         test_end({true, {msg}});
         return true;
     }
@@ -285,7 +258,7 @@ static bool ping_rate_reply_cb(std::span<const uint8_t> frame) {
 }
 
 static void ping_rate_timeout_cb() {
-    ts.timer = nullptr;  // already fired
+    ts.timer = nullptr;
     auto& r = *ts.active_rate;
     uint32_t elapsed_us = time_us_32() - r.start_us;
     char msg[64];
@@ -329,8 +302,6 @@ static void test_byte_rate() {
     start_ping_rate(ts.byte_rate.discovery, ts.byte_rate.rate, 2048, 1400, 8);
 }
 
-// ----- registry -----
-
 using sync_test_fn = ResponseTest (*)();
 using async_test_fn = void (*)();