From 774c7e1cc7c4517ce6de18a8cc93ea32c7c81043 Mon Sep 17 00:00:00 2001 From: mad201802 Date: Tue, 3 Mar 2026 15:22:09 +0100 Subject: [PATCH 1/2] feat: enhance diagnostics discovery with direct device querying and dashboard updates --- examples/desktop_peer.cpp | 1 + examples/diag_scanner.cpp | 90 ++++++++++++++++++++++----------------- include/sero/runtime.hpp | 31 ++++++++++++++ 3 files changed, 84 insertions(+), 38 deletions(-) diff --git a/examples/desktop_peer.cpp b/examples/desktop_peer.cpp index 80abe7c..f48c2a9 100644 --- a/examples/desktop_peer.cpp +++ b/examples/desktop_peer.cpp @@ -264,6 +264,7 @@ int main(int argc, char* argv[]) { app.last_tick_ms = now; app.last_diag_ms = now; + rt.enable_diagnostics(now); std::printf("\n[main] Entering main loop (Ctrl+C to quit)\n\n"); // ════════════════════════════════════════════════════════════ diff --git a/examples/diag_scanner.cpp b/examples/diag_scanner.cpp index 58528fe..756ea9d 100644 --- a/examples/diag_scanner.cpp +++ b/examples/diag_scanner.cpp @@ -78,6 +78,8 @@ struct DiscoveredDevice { static std::vector devices; static Runtime* g_rt = nullptr; static volatile std::sig_atomic_t running = 1; +static uint32_t g_last_scan_ms = 0; // last time queries were issued +static bool g_dashboard_printed = false; // ─── Signal Handler ───────────────────────────────────────────── @@ -202,7 +204,33 @@ static void on_clear_response(sero::ReturnCode rc, } } -// ─── SD Callback ──────────────────────────────────────────────── +// ─── Query a device ───────────────────────────────────────────── + +static void query_device(DiscoveredDevice& dev, uint32_t now_ms) { + using DM = sero::DiagMethod; + + // Use the explicit-address overload so each device gets its own requests, + // regardless of which provider the SD layer currently tracks. + g_rt->request(dev.addr, sero::DIAG_SERVICE_ID, + static_cast(DM::DIAG_GET_DEVICE_INFO), + nullptr, 0, on_device_info, &dev, REQUEST_TIMEOUT_MS, now_ms); + + g_rt->request(dev.addr, sero::DIAG_SERVICE_ID, + static_cast(DM::DIAG_GET_SERVICE_LIST), + nullptr, 0, on_service_list, &dev, REQUEST_TIMEOUT_MS, now_ms); + + g_rt->request(dev.addr, sero::DIAG_SERVICE_ID, + static_cast(DM::DIAG_GET_DTCS), + nullptr, 0, on_dtcs, &dev, REQUEST_TIMEOUT_MS, now_ms); + + g_rt->request(dev.addr, sero::DIAG_SERVICE_ID, + static_cast(DM::DIAG_GET_COUNTERS), + nullptr, 0, on_counters, &dev, REQUEST_TIMEOUT_MS, now_ms); + + dev.last_query_ms = now_ms; +} + +// ─── SD Callbacks ──────────────────────────────────────────────── static void on_diag_service_found(uint16_t sid, const Addr& addr, void* /*ctx*/) { if (sid != sero::DIAG_SERVICE_ID) return; @@ -218,39 +246,23 @@ static void on_diag_service_found(uint16_t sid, const Addr& addr, void* /*ctx*/) std::printf("[scanner] Discovered diagnostics device at %u.%u.%u.%u:%u\n", addr[0], addr[1], addr[2], addr[3], unsigned((addr[4] << 8) | addr[5])); + + // Query immediately so data is available within ~500 ms of discovery + // instead of waiting up to SCAN_INTERVAL_MS for the next periodic cycle. + // Also arm the dashboard window so it prints after responses arrive. + uint32_t now = example::now_ms(); + query_device(devices.back(), now); + g_last_scan_ms = now; + g_dashboard_printed = false; } static void on_diag_service_lost(uint16_t sid, void* /*ctx*/) { if (sid != sero::DIAG_SERVICE_ID) return; - std::printf("[scanner] Diagnostics service 0x%04X lost\n", sid); - // Mark devices as not found (they'll be rediscovered if they come back) - for (auto& dev : devices) { - dev.found = false; - } -} - -// ─── Query a device ───────────────────────────────────────────── - -static void query_device(DiscoveredDevice& dev, uint32_t now_ms) { - using DM = sero::DiagMethod; - - g_rt->request(sero::DIAG_SERVICE_ID, - static_cast(DM::DIAG_GET_DEVICE_INFO), - nullptr, 0, on_device_info, &dev, REQUEST_TIMEOUT_MS, now_ms); - - g_rt->request(sero::DIAG_SERVICE_ID, - static_cast(DM::DIAG_GET_SERVICE_LIST), - nullptr, 0, on_service_list, &dev, REQUEST_TIMEOUT_MS, now_ms); - - g_rt->request(sero::DIAG_SERVICE_ID, - static_cast(DM::DIAG_GET_DTCS), - nullptr, 0, on_dtcs, &dev, REQUEST_TIMEOUT_MS, now_ms); - - g_rt->request(sero::DIAG_SERVICE_ID, - static_cast(DM::DIAG_GET_COUNTERS), - nullptr, 0, on_counters, &dev, REQUEST_TIMEOUT_MS, now_ms); - - dev.last_query_ms = now_ms; + // The SD layer tracks only one provider slot per service_id — this fires + // when that slot's TTL expires, not when an individual device goes offline. + // Silently re-issue find_service so the slot is refreshed on the next + // offer from any device, keeping discovery live for newly appearing devices. + g_rt->find_service(sero::DIAG_SERVICE_ID, 1, example::now_ms()); } // ─── Print dashboard ──────────────────────────────────────────── @@ -389,16 +401,15 @@ int main(int argc, char* argv[]) { std::printf("[scanner] Searching for diagnostics services (0x%04X)...\n\n", sero::DIAG_SERVICE_ID); - uint32_t last_scan_ms = 0; - // ── Main loop ─────────────────────────────────────────────── while (running) { now = example::now_ms(); rt.process(now); // Query discovered devices periodically - if (now - last_scan_ms >= SCAN_INTERVAL_MS) { - last_scan_ms = now; + if (now - g_last_scan_ms >= SCAN_INTERVAL_MS) { + g_last_scan_ms = now; + g_dashboard_printed = false; for (auto& dev : devices) { if (dev.found) { @@ -414,7 +425,7 @@ int main(int argc, char* argv[]) { dev.info.client_id == clear_target_client) { uint8_t payload[2]; sero::MessageHeader::write_u16(payload, clear_dtc_code); - rt.request(sero::DIAG_SERVICE_ID, + rt.request(dev.addr, sero::DIAG_SERVICE_ID, static_cast(sero::DiagMethod::DIAG_CLEAR_DTCS), payload, 2, on_clear_response, nullptr, REQUEST_TIMEOUT_MS, now); @@ -429,10 +440,13 @@ int main(int argc, char* argv[]) { // Print after a brief delay to let responses arrive } - // Print dashboard shortly after query responses should have arrived - if (last_scan_ms > 0 && now - last_scan_ms > 500 && - now - last_scan_ms < 600) { + // Print dashboard once, ~500 ms after queries were sent, to allow + // responses to arrive before rendering. The flag prevents re-printing + // on every loop iteration during that window. + if (!g_dashboard_printed && g_last_scan_ms > 0 && + now - g_last_scan_ms >= 500) { print_dashboard(); + g_dashboard_printed = true; } std::this_thread::sleep_for(std::chrono::milliseconds(1)); diff --git a/include/sero/runtime.hpp b/include/sero/runtime.hpp index 12b66cc..69662cc 100644 --- a/include/sero/runtime.hpp +++ b/include/sero/runtime.hpp @@ -174,6 +174,37 @@ class Runtime { return rid; } + /// Send a REQUEST to an explicit target address, bypassing SD lookup. + /// Use this when you manage multiple providers of the same service_id + /// (e.g. a diagnostic scanner querying each discovered device directly). + std::optional request(const Addr& target, + uint16_t service_id, + uint16_t method_id, + const uint8_t* payload, + std::size_t payload_length, + RequestCallback callback, + void* user_ctx, + uint32_t timeout_ms, + uint32_t now_ms) + { + auto rid = request_tracker_.allocate(service_id, method_id, + timeout_ms, now_ms, + callback, user_ctx); + if (!rid) return std::nullopt; // table full + + bool auth = should_auth_outgoing(target); + MessageHeader hdr = make_request_header(service_id, method_id, + payload_length, *rid, auth); + hdr.sequence_counter = e2e_.next_sequence(); + std::size_t total = build_message(hdr, payload, payload_length, auth, target); + if (!transport_.send(target, tx_buffer_, total)) { + request_tracker_.complete(*rid, ReturnCode::E_NOT_OK, nullptr, 0); + diag_.increment(DiagnosticCounter::DroppedMessages); + return std::nullopt; + } + return rid; + } + /// Fire-and-forget REQUEST_NO_RETURN (§5.2). bool fire_and_forget(uint16_t service_id, uint16_t method_id, const uint8_t* payload, std::size_t payload_length) From 21af96a5ea069b5f6e398e3af1ee6ede42c437ef Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Wed, 4 Mar 2026 00:22:09 +0100 Subject: [PATCH 2/2] fix: address review feedback on diagnostics multi-device discovery (#4) * Initial plan * fix: address review feedback - check enable_diagnostics return, stable storage, request_impl refactor, new tests Co-authored-by: mad201802 <45396189+mad201802@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mad201802 <45396189+mad201802@users.noreply.github.com> --- examples/desktop_peer.cpp | 4 ++- examples/diag_scanner.cpp | 3 +- include/sero/runtime.hpp | 67 +++++++++++++++++++-------------------- test/test_runtime.cpp | 49 ++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 36 deletions(-) diff --git a/examples/desktop_peer.cpp b/examples/desktop_peer.cpp index f48c2a9..937a711 100644 --- a/examples/desktop_peer.cpp +++ b/examples/desktop_peer.cpp @@ -264,7 +264,9 @@ int main(int argc, char* argv[]) { app.last_tick_ms = now; app.last_diag_ms = now; - rt.enable_diagnostics(now); + if (!rt.enable_diagnostics(now)) { + std::fprintf(stderr, "[main] Failed to enable diagnostics; diagnostics will be unavailable.\n"); + } std::printf("\n[main] Entering main loop (Ctrl+C to quit)\n\n"); // ════════════════════════════════════════════════════════════ diff --git a/examples/diag_scanner.cpp b/examples/diag_scanner.cpp index 756ea9d..3a427cb 100644 --- a/examples/diag_scanner.cpp +++ b/examples/diag_scanner.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include // ─── Configuration ────────────────────────────────────────────── @@ -75,7 +76,7 @@ struct DiscoveredDevice { DeviceCounters counters; }; -static std::vector devices; +static std::deque devices; static Runtime* g_rt = nullptr; static volatile std::sig_atomic_t running = 1; static uint32_t g_last_scan_ms = 0; // last time queries were issued diff --git a/include/sero/runtime.hpp b/include/sero/runtime.hpp index 69662cc..708bd45 100644 --- a/include/sero/runtime.hpp +++ b/include/sero/runtime.hpp @@ -154,24 +154,8 @@ class Runtime { if (!sd_.get_provider_address(service_id, provider)) { return std::nullopt; // service not found } - - auto rid = request_tracker_.allocate(service_id, method_id, - timeout_ms, now_ms, - callback, user_ctx); - if (!rid) return std::nullopt; // table full - - bool auth = should_auth_outgoing(provider); - MessageHeader hdr = make_request_header(service_id, method_id, - payload_length, *rid, auth); - hdr.sequence_counter = e2e_.next_sequence(); - std::size_t total = build_message(hdr, payload, payload_length, auth, provider); - if (!transport_.send(provider, tx_buffer_, total)) { - // Send failed — cancel the pending request - request_tracker_.complete(*rid, ReturnCode::E_NOT_OK, nullptr, 0); - diag_.increment(DiagnosticCounter::DroppedMessages); - return std::nullopt; - } - return rid; + return request_impl(provider, service_id, method_id, payload, payload_length, + callback, user_ctx, timeout_ms, now_ms); } /// Send a REQUEST to an explicit target address, bypassing SD lookup. @@ -187,22 +171,8 @@ class Runtime { uint32_t timeout_ms, uint32_t now_ms) { - auto rid = request_tracker_.allocate(service_id, method_id, - timeout_ms, now_ms, - callback, user_ctx); - if (!rid) return std::nullopt; // table full - - bool auth = should_auth_outgoing(target); - MessageHeader hdr = make_request_header(service_id, method_id, - payload_length, *rid, auth); - hdr.sequence_counter = e2e_.next_sequence(); - std::size_t total = build_message(hdr, payload, payload_length, auth, target); - if (!transport_.send(target, tx_buffer_, total)) { - request_tracker_.complete(*rid, ReturnCode::E_NOT_OK, nullptr, 0); - diag_.increment(DiagnosticCounter::DroppedMessages); - return std::nullopt; - } - return rid; + return request_impl(target, service_id, method_id, payload, payload_length, + callback, user_ctx, timeout_ms, now_ms); } /// Fire-and-forget REQUEST_NO_RETURN (§5.2). @@ -727,6 +697,35 @@ class Runtime { // ── Message building ──────────────────────────────────────── + /// Internal: allocate a request ID, build, and send to a known target. + std::optional request_impl(const Addr& target, + uint16_t service_id, + uint16_t method_id, + const uint8_t* payload, + std::size_t payload_length, + RequestCallback callback, + void* user_ctx, + uint32_t timeout_ms, + uint32_t now_ms) + { + auto rid = request_tracker_.allocate(service_id, method_id, + timeout_ms, now_ms, + callback, user_ctx); + if (!rid) return std::nullopt; // table full + + bool auth = should_auth_outgoing(target); + MessageHeader hdr = make_request_header(service_id, method_id, + payload_length, *rid, auth); + hdr.sequence_counter = e2e_.next_sequence(); + std::size_t total = build_message(hdr, payload, payload_length, auth, target); + if (!transport_.send(target, tx_buffer_, total)) { + request_tracker_.complete(*rid, ReturnCode::E_NOT_OK, nullptr, 0); + diag_.increment(DiagnosticCounter::DroppedMessages); + return std::nullopt; + } + return rid; + } + /// Build a complete message in tx_buffer_. Returns total byte count. std::size_t build_message(const MessageHeader& hdr, const uint8_t* payload, diff --git a/test/test_runtime.cpp b/test/test_runtime.cpp index 188d3c4..273f936 100644 --- a/test/test_runtime.cpp +++ b/test/test_runtime.cpp @@ -349,6 +349,55 @@ TEST_F(RuntimeTest, ClientRequest_ServiceFound_SendsAndTracksRequest) { EXPECT_TRUE(callback_called); } +// ── Client-side Explicit-Target Request ───────────────────────── + +TEST_F(RuntimeTest, ClientRequest_ExplicitTarget_SendsWithoutSD) { + // SD has no provider registered — explicit-target overload must still send + Addr target = make_addr(42); + + bool callback_called = false; + auto cb = [](ReturnCode, const uint8_t*, std::size_t, void* ctx) { + *static_cast(ctx) = true; + }; + + auto rid = rt->request(target, 0x1000, 0x0001, nullptr, 0, + cb, &callback_called, 500, 0); + ASSERT_TRUE(rid.has_value()); + ASSERT_EQ(transport.sent.size(), 1u); + + // Verify the message was sent to the explicit target + EXPECT_EQ(transport.sent[0].destination, target); + + MessageHeader sent_hdr = MessageHeader::deserialize(transport.sent[0].data.data()); + EXPECT_EQ(sent_hdr.message_type, static_cast(MessageType::REQUEST)); + EXPECT_EQ(sent_hdr.service_id, 0x1000); + EXPECT_EQ(sent_hdr.method_event_id, 0x0001); +} + +TEST_F(RuntimeTest, ClientRequest_ExplicitTarget_ResponseCompletesCallback) { + Addr target = make_addr(42); + + bool callback_called = false; + ReturnCode received_rc = ReturnCode::E_NOT_OK; + auto cb = [](ReturnCode rc, const uint8_t*, std::size_t, void* ctx) { + auto* pair = static_cast*>(ctx); + *pair->first = true; + *pair->second = rc; + }; + + std::pair ctx{&callback_called, &received_rc}; + auto rid = rt->request(target, 0x1000, 0x0001, nullptr, 0, + cb, &ctx, 500, 100); + ASSERT_TRUE(rid.has_value()); + + // Inject a RESPONSE for this request_id from the explicit target + MessageHeader resp_hdr = make_response(0x1000, 0x0001, 0, *rid); + inject_and_process(resp_hdr, nullptr, 0, target, 200); + + EXPECT_TRUE(callback_called); + EXPECT_EQ(received_rc, ReturnCode::E_OK); +} + // ── Client-side Fire-and-Forget ───────────────────────────────── TEST_F(RuntimeTest, ClientFireAndForget_ServiceFound_Sends) {