Skip to content

chore: resolved code findings by claude #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mad201802 merged 1 commit into from
Mar 7, 2026
Merged

chore: resolved code findings by claude #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions include/sero/core/config.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,29 @@ struct DefaultConfig {
static constexpr std::size_t MaxDtcs = 32;
};

/// Embedded-friendly configuration for resource-constrained MCUs (ESP32, STM32, etc.).
/// sizeof(Runtime) with this config is approximately 3–4 KB.
struct EmbeddedConfig {
static constexpr std::size_t MaxPayloadSize = 256;
static constexpr std::size_t MaxServices = 4;
static constexpr std::size_t MaxMethods = 8;
static constexpr std::size_t MaxEvents = 4;
static constexpr std::size_t MaxSubscribers = 4;
static constexpr std::size_t MaxPendingRequests = 4;
static constexpr std::size_t MaxKnownServices = 8;
static constexpr uint32_t RequestTimeoutMs = 500;
static constexpr uint16_t OfferTtlSeconds = 5;
static constexpr uint16_t SubscriptionTtlSeconds = 10;
static constexpr uint8_t SdFindRetryCount = 3;
static constexpr uint32_t SdFindInitialDelayMs = 100;
static constexpr uint8_t SdFindBackoffMultiplier = 2;
static constexpr uint32_t SdFindJitterMs = 50;
static constexpr uint8_t SeqCounterAcceptWindow = 15;
static constexpr std::size_t TransportAddressSize = 8;
static constexpr std::size_t MaxReceiveQueueSize = 8;
static constexpr std::size_t MaxTrackedPeers = 8;
static constexpr std::size_t HmacKeySize = 32;
static constexpr std::size_t MaxDtcs = 8;
};

} // namespace sero
2 changes: 1 addition & 1 deletion include/sero/core/diagnostic_counters.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ struct DiagnosticCounters {

void increment(DiagnosticCounter c, const uint8_t* header_or_null = nullptr) {
auto idx = static_cast<std::size_t>(c);
++counters[idx]; // wrapping uint32_t
if (counters[idx] < UINT32_MAX) ++counters[idx]; // saturating
if (callback) {
callback(c, header_or_null, callback_ctx);
}
Expand Down
4 changes: 2 additions & 2 deletions include/sero/core/dtc_store.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class DtcStore {
for (std::size_t i = 0; i < count_; ++i) {
if (dtcs_[i].active && dtcs_[i].code == code) {
dtcs_[i].severity = static_cast<uint8_t>(severity);
dtcs_[i].occurrence_count++;
if (dtcs_[i].occurrence_count < UINT32_MAX) dtcs_[i].occurrence_count++;
dtcs_[i].last_seen_ms = now_ms;
return true;
}
Expand All @@ -57,7 +57,7 @@ class DtcStore {
bool clear(uint16_t code) {
for (std::size_t i = 0; i < count_; ++i) {
if (dtcs_[i].active && dtcs_[i].code == code) {
dtcs_[i] = dtcs_[count_ - 1];
if (i != count_ - 1) dtcs_[i] = dtcs_[count_ - 1];
dtcs_[count_ - 1] = Dtc{};
--count_;
return true;
Expand Down
39 changes: 26 additions & 13 deletions include/sero/runtime.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,14 @@ class Runtime {
static constexpr std::size_t MAX_MSG_SIZE =
MessageHeader::SIZE + Config::MaxPayloadSize + HMAC_SIZE + CRC_SIZE;

// Compile-time sanity checks for config products that determine RAM usage.
static_assert(Config::MaxServices * Config::MaxEvents <= 1024,
"MaxServices * MaxEvents is very large — review RAM budget");
static_assert(Config::MaxKnownServices * Config::MaxEvents <= 1024,
"MaxKnownServices * MaxEvents is very large — review RAM budget");
static_assert(Config::MaxPayloadSize <= 8192,
"MaxPayloadSize exceeds 8 KB — review RAM budget");

// ── Construction ────────────────────────────────────────────

Runtime(TransportImpl& transport, uint16_t client_id)
Expand All @@ -58,11 +66,13 @@ class Runtime {
}
uptime_ms_ = now_ms - uptime_epoch_ms_;

// 1. Drain transport
// 1. Drain transport (bounded to prevent starvation)
Addr source{};
const uint8_t* data = nullptr;
std::size_t length = 0;
while (transport_.poll(source, data, length)) {
for (std::size_t drain = 0;
drain < Config::MaxReceiveQueueSize && transport_.poll(source, data, length);
++drain) {
validate_and_dispatch(source, data, length, now_ms);
}

Expand Down Expand Up @@ -99,7 +109,7 @@ class Runtime {
// ── Service registration (provider side) ────────────────────

template <typename Impl>
bool register_service(uint16_t service_id, IService<Impl>& svc,
[[nodiscard]] bool register_service(uint16_t service_id, IService<Impl>& svc,
uint8_t major, uint8_t minor,
bool auth_required = false)
{
Expand All @@ -112,13 +122,13 @@ class Runtime {
}

/// Register an event that this device can produce notifications for.
bool register_event(uint16_t service_id, uint16_t event_id) {
[[nodiscard]] bool register_event(uint16_t service_id, uint16_t event_id) {
return event_manager_.register_event(service_id, event_id);
}

// ── Service offering (provider SD) ──────────────────────────

bool offer_service(uint16_t service_id, uint16_t ttl_seconds, uint32_t now_ms) {
[[nodiscard]] bool offer_service(uint16_t service_id, uint16_t ttl_seconds, uint32_t now_ms) {
const ServiceEntry* se = dispatcher_.find(service_id);
if (!se) return false;
return sd_.offer(service_id, se->major_version, se->minor_version,
Expand All @@ -131,7 +141,7 @@ class Runtime {

// ── Service discovery (consumer SD) ─────────────────────────

bool find_service(uint16_t service_id, uint8_t major_version, uint32_t now_ms) {
[[nodiscard]] bool find_service(uint16_t service_id, uint8_t major_version, uint32_t now_ms) {
return sd_.find(service_id, major_version, now_ms);
}

Expand All @@ -141,7 +151,7 @@ class Runtime {

/// Send a REQUEST and track the pending response.
/// Returns the request_id or std::nullopt if failed.
std::optional<uint32_t> request(uint16_t service_id,
[[nodiscard]] std::optional<uint32_t> request(uint16_t service_id,
uint16_t method_id,
const uint8_t* payload,
std::size_t payload_length,
Expand Down Expand Up @@ -176,7 +186,7 @@ class Runtime {
}

/// Fire-and-forget REQUEST_NO_RETURN (§5.2).
bool fire_and_forget(uint16_t service_id, uint16_t method_id,
[[nodiscard]] bool fire_and_forget(uint16_t service_id, uint16_t method_id,
const uint8_t* payload, std::size_t payload_length)
{
Addr provider{};
Expand Down Expand Up @@ -207,7 +217,7 @@ class Runtime {

/// Subscribe to a remote event. Handler receives notifications.
template <typename Impl>
bool subscribe_event(uint16_t service_id, uint16_t event_id,
[[nodiscard]] bool subscribe_event(uint16_t service_id, uint16_t event_id,
IEventHandler<Impl>& handler,
uint16_t ttl_seconds, uint32_t now_ms)
{
Expand All @@ -227,7 +237,7 @@ class Runtime {
// ── Event notification (provider side, §6.2) ────────────────

/// Push a notification to all subscribers of an event.
bool notify_event(uint16_t service_id, uint16_t event_id,
[[nodiscard]] bool notify_event(uint16_t service_id, uint16_t event_id,
const uint8_t* payload, std::size_t payload_length,
uint32_t /*now_ms*/ = 0)
{
Expand Down Expand Up @@ -278,7 +288,7 @@ class Runtime {
/// @param auth_required When true, requests to the diagnostics service
/// must carry a valid HMAC (recommended for
/// state-changing methods like DIAG_CLEAR_DTCS).
bool enable_diagnostics(uint32_t now_ms, bool auth_required = false) {
[[nodiscard]] bool enable_diagnostics(uint32_t now_ms, bool auth_required = false) {
if (diag_enabled_) return true; // idempotent
diag_service_.init(&dtc_store_, &diag_, &dispatcher_,
client_id_, &uptime_ms_);
Expand All @@ -295,7 +305,7 @@ class Runtime {
}

/// Report a DTC (application-level error code).
bool report_dtc(uint16_t code, DtcSeverity severity, uint32_t now_ms) {
[[nodiscard]] bool report_dtc(uint16_t code, DtcSeverity severity, uint32_t now_ms) {
if constexpr (Config::MaxDtcs == 0) {
return false;
} else {
Expand Down Expand Up @@ -742,6 +752,9 @@ class Runtime {
bool auth,
const Addr& peer)
{
// Bounds check — prevent buffer overflow
if (payload_length > Config::MaxPayloadSize) return 0;

// Header
MessageHeader mutable_hdr = hdr;
mutable_hdr.serialize(tx_buffer_);
Expand Down Expand Up @@ -847,7 +860,7 @@ class Runtime {
if (event_handlers_[i].active &&
event_handlers_[i].service_id == service_id &&
event_handlers_[i].event_id == event_id) {
event_handlers_[i] = event_handlers_[event_handler_count_ - 1];
if (i != event_handler_count_ - 1) event_handlers_[i] = event_handlers_[event_handler_count_ - 1];
event_handlers_[event_handler_count_ - 1] = EventHandlerEntry{};
--event_handler_count_;
return;
Expand Down
2 changes: 1 addition & 1 deletion include/sero/security/e2e_protection.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class E2EProtection {
for (std::size_t i = 0; i < peer_count_; ++i) {
if (peers_[i].address == source) {
// Remove by compacting
peers_[i] = peers_[peer_count_ - 1];
if (i != peer_count_ - 1) peers_[i] = peers_[peer_count_ - 1];
peers_[peer_count_ - 1] = PeerEntry{};
--peer_count_;
return;
Expand Down
4 changes: 2 additions & 2 deletions include/sero/service/event_manager.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class EventManager {
for (std::size_t i = 0; i < ev->sub_count; ++i) {
if (ev->subscribers[i].client_id == client_id) {
// Compact: move last into this slot
ev->subscribers[i] = ev->subscribers[ev->sub_count - 1];
if (i != ev->sub_count - 1) ev->subscribers[i] = ev->subscribers[ev->sub_count - 1];
ev->subscribers[ev->sub_count - 1] = Subscriber{};
--ev->sub_count;
return;
Expand All @@ -95,7 +95,7 @@ class EventManager {
while (i < ev.sub_count) {
if (time_after(now_ms, ev.subscribers[i].expiry_ms)) {
// Expired — compact
ev.subscribers[i] = ev.subscribers[ev.sub_count - 1];
if (i != ev.sub_count - 1) ev.subscribers[i] = ev.subscribers[ev.sub_count - 1];
ev.subscribers[ev.sub_count - 1] = Subscriber{};
--ev.sub_count;
} else {
Expand Down
2 changes: 1 addition & 1 deletion include/sero/service/method_dispatcher.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class MethodDispatcher {
for (std::size_t i = 0; i < count_; ++i) {
if (services_[i].active && services_[i].service_id == service_id) {
// Move last into this slot to keep array compact
services_[i] = services_[count_ - 1];
if (i != count_ - 1) services_[i] = services_[count_ - 1];
services_[count_ - 1] = ServiceEntry{};
--count_;
return true;
Expand Down
16 changes: 15 additions & 1 deletion include/sero/service/request_tracker.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,21 @@ class RequestTracker {
{
for (std::size_t i = 0; i < Config::MaxPendingRequests; ++i) {
if (!entries_[i].active) {
uint32_t rid = next_request_id_++;
// Find a request_id that isn't already active
uint32_t rid = next_request_id_;
for (std::size_t attempt = 0; attempt < Config::MaxPendingRequests; ++attempt) {
bool collision = false;
for (std::size_t j = 0; j < Config::MaxPendingRequests; ++j) {
if (entries_[j].active && entries_[j].request_id == rid) {
collision = true;
break;
}
}
if (!collision) break;
++rid;
}
next_request_id_ = rid + 1;

auto& e = entries_[i];
e.request_id = rid;
e.service_id = service_id;
Expand Down
14 changes: 14 additions & 0 deletions include/sero/service/service_discovery.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,19 @@ class ServiceDiscovery {
send_subscribe(st, ce->provider_addr, send_fn);
st.last_sent_ms = now_ms;
}
} else {
// No ACK received yet — retry after SubscriptionTtlSeconds
uint32_t retry_interval = static_cast<uint32_t>(Config::SubscriptionTtlSeconds) * 1000u;
if (time_after_or_eq(now_ms, st.last_sent_ms + retry_interval)) {
if (st.ack_retry_count < 2) {
send_subscribe(st, ce->provider_addr, send_fn);
st.last_sent_ms = now_ms;
++st.ack_retry_count;
} else {
// Retries exhausted — give up
st.active = false;
}
}
}
}
}
Expand Down Expand Up @@ -542,6 +555,7 @@ class ServiceDiscovery {
bool needs_send = false;
bool needs_unsubscribe = false;
uint32_t last_sent_ms = 0;
uint8_t ack_retry_count = 0; ///< Retries when no ACK received.
};

std::array<ProviderEntry, Config::MaxServices> providers_{};
Expand Down
12 changes: 6 additions & 6 deletions src/verify_build.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,14 @@ int main() {
rt.set_local_address(local_addr);

EchoService echo;
rt.register_service(0x0100, echo, 1, 0);
rt.register_event(0x0100, 0x8001);
rt.offer_service(0x0100, 5, 0);
(void)rt.register_service(0x0100, echo, 1, 0);
(void)rt.register_event(0x0100, 0x8001);
(void)rt.offer_service(0x0100, 5, 0);

// Diagnostics service
rt.enable_diagnostics(0);
rt.report_dtc(0x0010, sero::DtcSeverity::Error, 100);
rt.report_dtc(0x0020, sero::DtcSeverity::Warning, 200);
(void)rt.enable_diagnostics(0);
(void)rt.report_dtc(0x0010, sero::DtcSeverity::Error, 100);
(void)rt.report_dtc(0x0020, sero::DtcSeverity::Warning, 200);
rt.clear_dtc(0x0010);
rt.clear_all_dtcs();

Expand Down
6 changes: 3 additions & 3 deletions test/test_diagnostics_service.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,8 +266,8 @@ TEST(DiagnosticsService_Runtime, ClearDtcs) {
test::MockTransport transport;
Runtime<test::MockTransport, test::SmallConfig> rt(transport, 0x0042);

rt.report_dtc(0x0001, DtcSeverity::Info, 100);
rt.report_dtc(0x0002, DtcSeverity::Warning, 200);
(void)rt.report_dtc(0x0001, DtcSeverity::Info, 100);
(void)rt.report_dtc(0x0002, DtcSeverity::Warning, 200);

EXPECT_TRUE(rt.clear_dtc(0x0001));
EXPECT_EQ(rt.dtc_store().count(), 1u);
Expand All @@ -285,7 +285,7 @@ TEST(DiagnosticsService_Runtime, EndToEndGetDeviceInfo) {
rt.set_local_address(local);

uint32_t now = 5000;
rt.enable_diagnostics(now);
(void)rt.enable_diagnostics(now);
rt.process(now); // establish uptime

// Build a request for DIAG_GET_DEVICE_INFO from a remote peer
Expand Down
Loading