Skip to content

Addresses PR #46 review comments:#50

Merged
marklynch merged 1 commit into
marklynch:mainfrom
lawther:fix/address_46_comments
Jun 25, 2026
Merged

Addresses PR #46 review comments:#50
marklynch merged 1 commit into
marklynch:mainfrom
lawther:fix/address_46_comments

Conversation

@lawther

@lawther lawther commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Add default case for resync counter and document decode_message invariant

The resync type switch now has a default branch (resyncs_unexpected) so total stays consistent with the breakdown if a new resync type is added, and decode_message documents that it relies on the framing layer for validation since it is also called directly from the TX-echo path and test harness.

  1. unknown_msgs_json_handler holds the unknown-buffer mutex across the whole JSON build (medium)

unknown_buffer_lock_for_read() keeps the mutex held while serializing up to UNKNOWN_BUFFER_CAPACITY entries (cJSON allocations, snprintf, get_device_name). Meanwhile unknown_buffer_record() runs on the hot RX/resync path with a 100 ms timeout and drops the record on contention. Loading the Unknown Messages page while the bus is noisy can therefore both lose captures and add latency to the bridge task. Consider snapshotting the entries under the lock, releasing, then building the JSON from the copy.

This code is from an earlier patch. I originally wrote it as 'snapshot and build from copy' to lock for as little as possible, but the review comments for that patch suggested changing it to 'stream under lock', so I changed it to the way it is now. Either will work, first way costs a 9KB malloc (or I could do it statically), second holds the lock a bit longer.

Let's pick one way.

  1. resync_wrapper counter switch has no default (low)

The counter-incrementing switch (type) lacks a default, while the reason-mapping switch just below it has one. If a new tcp_bridge_resync_type_t is added later, resyncs_total still increments but no per-type counter does, so total would no longer equal the sum of the breakdown (and there's no compile-time nudge to catch it).

Done. The resync type switch now has a default branch to capture unexpected reasons (resyncs_unexpected).

  1. decode_message no longer validates header/length/data checksums (low)

Validation now lives solely in the framing layer, which is correct for RX since framing guarantees validity before dispatch. But decode_message is also called directly on the TX-echo path (tcp_bridge.c:330, user-typed hex) and from the test harness, so malformed input there is now decoded with no checksum warning where it previously was flagged. Low impact — mostly worth noting the new "assumes pre-validated input" invariant for decode_message.

Done. Comment added.

…iant

Addresses PR marklynch#46 review comments: the resync type switch now has a
default branch (resyncs_unexpected) so total stays consistent with the
breakdown if a new resync type is added, and decode_message documents
that it relies on the framing layer for validation since it is also
called directly from the TX-echo path and test harness.
@marklynch

Copy link
Copy Markdown
Owner

Ah yep sorry about the conflicting suggestions on the locking. Let's leave it for now. We may end up going back to original implementation if there is no memory pressure. But let's leave for now.

Thanks for looking at the others.

@marklynch marklynch merged commit 89f6015 into marklynch:main Jun 25, 2026
2 checks passed
@lawther

lawther commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

All good. In terms of memory pressure I had added it to status so we can keep an eye on it. Mine at the moment - memory: {
free_heap: "229.3 KB"
min_free_heap: "140.1 KB"
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants