Clean up callback_list, remove sentinel/active/tail, callbacks return bool for removal, register before send

firmware/include/callback_list.h

@@ -1,87 +1,80 @@
 #pragma once
-#include <cstdint>
-#include <new>
 #include <utility>
 
 template <typename T, int N>
 struct callback_list {
     struct node {
-        alignas(T) uint8_t storage[sizeof(T)];
+        T value;
         node* prev = nullptr;
         node* next = nullptr;
-        bool active = false;
-
-        T& value() { return *reinterpret_cast<T*>(storage); }
-        const T& value() const { return *reinterpret_cast<const T*>(storage); }
     };
 
     node nodes[N];
     node* free_head = &nodes[0];
-    node sentinel;
+    node* head = nullptr;
 
     callback_list() {
         for (int i = 0; i < N - 1; i++) nodes[i].next = &nodes[i + 1];
         nodes[N - 1].next = nullptr;
-        sentinel.prev = &sentinel;
-        sentinel.next = &sentinel;
     }
 
-    bool empty() const { return sentinel.next == &sentinel; }
+    bool empty() const { return head == nullptr; }
 
     node* insert(T value) {
         if (!free_head) return nullptr;
-        node* n = alloc(std::move(value));
+        node* n = free_head;
-        link_before(&sentinel, n);
+        free_head = n->next;
+        n->value = std::move(value);
+        n->prev = nullptr;
+        n->next = head;
+        if (head) head->prev = n;
+        head = n;
         return n;
     }
 
     template <typename Less>
     node* insert_sorted(T value, Less&& less) {
         if (!free_head) return nullptr;
-        node* n = alloc(std::move(value));
+        node* n = free_head;
-        node* pos = sentinel.next;
+        free_head = n->next;
-        while (pos != &sentinel && !less(n->value(), pos->value()))
+        n->value = std::move(value);
-            pos = pos->next;
+        if (!head || less(n->value, head->value)) {
-        link_before(pos, n);
+            n->prev = nullptr;
+            n->next = head;
+            if (head) head->prev = n;
+            head = n;
+            return n;
+        }
+        node* cur = head;
+        while (cur->next && !less(n->value, cur->next->value))
+            cur = cur->next;
+        n->prev = cur;
+        n->next = cur->next;
+        if (cur->next) cur->next->prev = n;
+        cur->next = n;
         return n;
     }
 
     void remove(node* n) {
-        if (!n || !n->active) return;
+        if (!n) return;
-        n->prev->next = n->next;
+        if (n->prev) n->prev->next = n->next;
-        n->next->prev = n->prev;
+        else head = n->next;
-        n->active = false;
+        if (n->next) n->next->prev = n->prev;
-        n->value().~T();
+        n->value = T{};
         n->next = free_head;
         n->prev = nullptr;
         free_head = n;
     }
 
-    node* front() { return sentinel.next != &sentinel ? sentinel.next : nullptr; }
+    node* front() { return head; }
 
     template <typename Fn>
     void for_each(Fn&& fn) {
-        node* cur = sentinel.next;
+        node* cur = head;
-        while (cur != &sentinel) {
+        while (cur) {
             node* next = cur->next;
             fn(cur);
             cur = next;
         }
     }
-
-private:
-    node* alloc(T value) {
-        node* n = free_head;
-        free_head = n->next;
-        new (n->storage) T(std::move(value));
-        n->active = true;
-        return n;
-    }
-
-    void link_before(node* pos, node* n) {
-        n->prev = pos->prev;
-        n->next = pos;
-        pos->prev->next = n;
-        pos->prev = n;
-    }
 };

firmware/include/net.h

@@ -18,11 +18,7 @@ using net_handler = std::function<void(std::span<const uint8_t> payload,
 
 using net_frame_callback = std::function<bool(std::span<const uint8_t> frame)>;
 
-struct frame_callback_entry {
+using frame_cb_list = callback_list<net_frame_callback, 16>;
-    net_frame_callback fn;
-};
-
-using frame_cb_list = callback_list<frame_callback_entry, 16>;
 using frame_cb_handle = frame_cb_list::node*;
 
 bool net_init();

firmware/include/timer_queue.h

@@ -29,7 +29,7 @@ struct timer_queue {
     }
 
     bool cancel(timer_handle h) {
-        if (!h || !h->active) return false;
+        if (!h) return false;
         list.remove(h);
         arm();
         return true;
@@ -39,8 +39,8 @@ struct timer_queue {
         if (!irq_pending) return;
         irq_pending = false;
         while (auto* n = list.front()) {
-            if (absolute_time_diff_us(get_absolute_time(), n->value().when) > 0) break;
+            if (absolute_time_diff_us(get_absolute_time(), n->value.when) > 0) break;
-            auto fn = std::move(n->value().fn);
+            auto fn = std::move(n->value.fn);
             list.remove(n);
             fn();
         }
@@ -59,6 +59,6 @@ private:
         if (alarm >= 0) cancel_alarm(alarm);
         alarm = -1;
         if (auto* n = list.front())
-            alarm = add_alarm_at(n->value().when, alarm_cb, this, false);
+            alarm = add_alarm_at(n->value.when, alarm_cb, this, false);
     }
 };

firmware/lib/net.cpp

@@ -72,7 +72,7 @@ static void process_frame(std::span<const uint8_t> frame, span_writer& tx) {
     if (!mac_match(eth_hdr.dst)) return;
 
     frame_callbacks.for_each([&](frame_cb_list::node* n) {
-        if (n->value().fn(frame))
+        if (n->value(frame))
             frame_callbacks.remove(n);
     });
 
@@ -123,7 +123,9 @@ void net_set_handler(net_handler handler) {
 }
 
 frame_cb_handle net_add_frame_callback(net_frame_callback cb) {
-    return frame_callbacks.insert({std::move(cb)});
+    auto h = frame_callbacks.insert(std::move(cb));
+    if (!h) dlog("frame callback alloc failed");
+    return h;
 }
 
 void net_remove_frame_callback(frame_cb_handle h) {
@@ -135,7 +137,7 @@ void net_poll(std::span<uint8_t> tx) {
     w6300::irq_pending = false;
     w6300::clear_interrupt(w6300::ik_int_all);
     static uint8_t rx_buf[1518];
-    int budget = 2;
+    int budget = 10;
     while (budget-- > 0 && w6300::get_socket_recv_buf(raw_socket) > 0) {
         auto result = w6300::recv(raw_socket, std::span{rx_buf});
         if (!result) break;

firmware/lib/test_handlers.cpp

@@ -38,13 +38,12 @@ static void discover_peer(peer_callback on_found, fail_callback on_timeout) {
 
     udp::prepend(buf, mcast_mac, ns.mac, ns.ip, igmp::PICOMAP_DISCOVERY_GROUP,
                  PICOMAP_PORT, PICOMAP_PORT, *encoded, 1);
-    net_send_raw(buf.span());
 
     ipv4::ip4_addr our_ip = ns.ip;
 
     auto timer = std::make_shared<timer_handle>(nullptr);
-    auto cb_id = std::make_shared<frame_cb_handle>(nullptr);
+    auto cb = std::make_shared<frame_cb_handle>(nullptr);
-    *cb_id = net_add_frame_callback([our_ip, timer, cb_id, on_found = std::move(on_found)](std::span<const uint8_t> frame) -> bool {
+    *cb = net_add_frame_callback([our_ip, timer, on_found = std::move(on_found)](std::span<const uint8_t> frame) -> bool {
         parse_buffer pb(frame);
         auto* eth_hdr = pb.consume<eth::header>();
         if (!eth_hdr || eth_hdr->ethertype != eth::ETH_IPV4) return false;
@@ -60,10 +59,12 @@ static void discover_peer(peer_callback on_found, fail_callback on_timeout) {
         return true;
     });
 
-    *timer = dispatch_schedule_ms(5000, [cb_id, on_timeout = std::move(on_timeout)]() {
+    *timer = dispatch_schedule_ms(5000, [cb, on_timeout = std::move(on_timeout)]() {
-        net_remove_frame_callback(*cb_id);
+        net_remove_frame_callback(*cb);
         on_timeout();
     });
+
+    net_send_raw(buf.span());
 }
 
 static void test_discovery_igmp(const responder& resp) {
@@ -71,11 +72,10 @@ static void test_discovery_igmp(const responder& resp) {
 
     prepend_buffer<4096> buf;
     igmp::prepend_query(buf, ns.mac, ns.ip, igmp::PICOMAP_DISCOVERY_GROUP);
-    net_send_raw(buf.span());
 
     auto timer = std::make_shared<timer_handle>(nullptr);
-    auto cb_id = std::make_shared<frame_cb_handle>(nullptr);
+    auto cb = std::make_shared<frame_cb_handle>(nullptr);
-    *cb_id = net_add_frame_callback([resp, timer](std::span<const uint8_t> frame) -> bool {
+    *cb = net_add_frame_callback([resp, timer](std::span<const uint8_t> frame) -> bool {
         ipv4::ip4_addr group;
         if (!igmp::parse_report(frame, group)) return false;
         if (group != igmp::PICOMAP_DISCOVERY_GROUP) return false;
@@ -84,10 +84,12 @@ static void test_discovery_igmp(const responder& resp) {
         return true;
     });
 
-    *timer = dispatch_schedule_ms(5000, [cb_id, resp]() {
+    *timer = dispatch_schedule_ms(5000, [cb, resp]() {
-        net_remove_frame_callback(*cb_id);
+        net_remove_frame_callback(*cb);
         resp.respond(ResponseTest{false, {"no IGMP report within 5s"}});
     });
+
+    net_send_raw(buf.span());
 }
 
 static void test_discovery_info(const responder& resp) {
@@ -107,13 +109,12 @@ static void test_ping(const responder& resp, ipv4::ip4_addr dst_ip) {
     prepend_buffer<4096> buf;
     icmp::prepend_echo_request(buf, ns.mac, ns.ip,
                                eth::MAC_BROADCAST, dst_ip, ping_id, 1);
-    net_send_raw(buf.span());
 
     ipv4::ip4_addr our_ip = ns.ip;
 
     auto timer = std::make_shared<timer_handle>(nullptr);
-    auto cb_id = std::make_shared<frame_cb_handle>(nullptr);
+    auto cb = std::make_shared<frame_cb_handle>(nullptr);
-    *cb_id = net_add_frame_callback([resp, ping_id, our_ip, timer](std::span<const uint8_t> frame) -> bool {
+    *cb = net_add_frame_callback([resp, ping_id, our_ip, timer](std::span<const uint8_t> frame) -> bool {
         ipv4::ip4_addr src_ip;
         if (!icmp::parse_echo_reply(frame, src_ip, ping_id)) return false;
         dispatch_cancel_timer(*timer);
@@ -125,10 +126,12 @@ static void test_ping(const responder& resp, ipv4::ip4_addr dst_ip) {
         return true;
     });
 
-    *timer = dispatch_schedule_ms(5000, [cb_id, resp]() {
+    *timer = dispatch_schedule_ms(5000, [cb, resp]() {
-        net_remove_frame_callback(*cb_id);
+        net_remove_frame_callback(*cb);
         resp.respond(ResponseTest{false, {"no reply from non-self host within 5s"}});
     });
+
+    net_send_raw(buf.span());
 }
 
 static void test_ping_subnet(const responder& resp) {
@@ -187,9 +190,6 @@ static void start_ping_rate(const responder& resp, uint16_t target,
             st->peer = peer;
             st->start_us = time_us_32();
 
-            for (uint16_t i = 0; i < st->pipeline && st->sent < st->target; i++)
-                ping_rate_send_one(st);
-
             st->cb_handle = net_add_frame_callback([st](std::span<const uint8_t> frame) -> bool {
                 ipv4::ip4_addr src_ip;
                 if (!icmp::parse_echo_reply(frame, src_ip, st->ping_id)) return false;
@@ -229,6 +229,9 @@ static void start_ping_rate(const responder& resp, uint16_t target,
                          static_cast<unsigned long>(elapsed_us / 1000));
                 st->resp.respond(ResponseTest{false, {msg}});
             });
+
+            for (uint16_t i = 0; i < st->pipeline && st->sent < st->target; i++)
+                ping_rate_send_one(st);
         },
         [resp]() {
             resp.respond(ResponseTest{false, {"no peer found"}});