From 1c8c6458788a5ed5c1d5224cce9177db166409f0 Mon Sep 17 00:00:00 2001 From: Ian Gulliver Date: Sat, 11 Apr 2026 15:01:37 +0900 Subject: [PATCH] Clean up callback_list, remove sentinel/active/tail, callbacks return bool for removal, register before send --- firmware/include/callback_list.h | 77 +++++++++++++++----------------- firmware/include/net.h | 6 +-- firmware/include/timer_queue.h | 8 ++-- firmware/lib/net.cpp | 8 ++-- firmware/lib/test_handlers.cpp | 39 ++++++++-------- 5 files changed, 66 insertions(+), 72 deletions(-) diff --git a/firmware/include/callback_list.h b/firmware/include/callback_list.h index e6758fa..acd6af6 100644 --- a/firmware/include/callback_list.h +++ b/firmware/include/callback_list.h @@ -1,87 +1,80 @@ #pragma once -#include -#include #include template 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(storage); } - const T& value() const { return *reinterpret_cast(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)); - link_before(&sentinel, n); + node* n = free_head; + 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 node* insert_sorted(T value, Less&& less) { if (!free_head) return nullptr; - node* n = alloc(std::move(value)); - node* pos = sentinel.next; - while (pos != &sentinel && !less(n->value(), pos->value())) - pos = pos->next; - link_before(pos, n); + node* n = free_head; + free_head = n->next; + n->value = std::move(value); + if (!head || less(n->value, head->value)) { + 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; - n->prev->next = n->next; - n->next->prev = n->prev; - n->active = false; - n->value().~T(); + if (!n) return; + if (n->prev) n->prev->next = n->next; + else head = n->next; + if (n->next) n->next->prev = n->prev; + 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 void for_each(Fn&& fn) { - node* cur = sentinel.next; - while (cur != &sentinel) { + node* cur = head; + 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; - } }; diff --git a/firmware/include/net.h b/firmware/include/net.h index ecc8caa..2716941 100644 --- a/firmware/include/net.h +++ b/firmware/include/net.h @@ -18,11 +18,7 @@ using net_handler = std::function payload, using net_frame_callback = std::function frame)>; -struct frame_callback_entry { - net_frame_callback fn; -}; - -using frame_cb_list = callback_list; +using frame_cb_list = callback_list; using frame_cb_handle = frame_cb_list::node*; bool net_init(); diff --git a/firmware/include/timer_queue.h b/firmware/include/timer_queue.h index 4733622..7c22699 100644 --- a/firmware/include/timer_queue.h +++ b/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; - auto fn = std::move(n->value().fn); + if (absolute_time_diff_us(get_absolute_time(), n->value.when) > 0) break; + 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); } }; diff --git a/firmware/lib/net.cpp b/firmware/lib/net.cpp index 5e16ce2..01d16d9 100644 --- a/firmware/lib/net.cpp +++ b/firmware/lib/net.cpp @@ -72,7 +72,7 @@ static void process_frame(std::span 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 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; diff --git a/firmware/lib/test_handlers.cpp b/firmware/lib/test_handlers.cpp index a14654a..f561b5f 100644 --- a/firmware/lib/test_handlers.cpp +++ b/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(nullptr); - auto cb_id = std::make_shared(nullptr); - *cb_id = net_add_frame_callback([our_ip, timer, cb_id, on_found = std::move(on_found)](std::span frame) -> bool { + auto cb = std::make_shared(nullptr); + *cb = net_add_frame_callback([our_ip, timer, on_found = std::move(on_found)](std::span frame) -> bool { parse_buffer pb(frame); auto* eth_hdr = pb.consume(); 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)]() { - net_remove_frame_callback(*cb_id); + *timer = dispatch_schedule_ms(5000, [cb, on_timeout = std::move(on_timeout)]() { + 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(nullptr); - auto cb_id = std::make_shared(nullptr); - *cb_id = net_add_frame_callback([resp, timer](std::span frame) -> bool { + auto cb = std::make_shared(nullptr); + *cb = net_add_frame_callback([resp, timer](std::span 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]() { - net_remove_frame_callback(*cb_id); + *timer = dispatch_schedule_ms(5000, [cb, resp]() { + 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(nullptr); - auto cb_id = std::make_shared(nullptr); - *cb_id = net_add_frame_callback([resp, ping_id, our_ip, timer](std::span frame) -> bool { + auto cb = std::make_shared(nullptr); + *cb = net_add_frame_callback([resp, ping_id, our_ip, timer](std::span 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]() { - net_remove_frame_callback(*cb_id); + *timer = dispatch_schedule_ms(5000, [cb, resp]() { + 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 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(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"}});