Skip to content

fix(tx): correct _tx_loop to pop _TxItem and send frames contiguously - #13

Merged
kstaniek merged 6 commits into
mainfrom
feature/batch
Sep 3, 2025
Merged

fix(tx): correct _tx_loop to pop _TxItem and send frames contiguously#13
kstaniek merged 6 commits into
mainfrom
feature/batch

Conversation

@kstaniek

@kstaniek kstaniek commented Sep 3, 2025

Copy link
Copy Markdown
Owner

Summary

  • pop _TxItem (not raw bytes) and check item is None
  • avoid variable shadowing; use frame_bytes in send loop
  • on writer=None: requeue whole item, clear _connected, yield
  • on write error: requeue remaining frames as one atomic item
  • preserves non-interleaving batches and prevents tight re-loops

What’s changed

  • Feature
  • Bug fix
  • Documentation
  • Maintenance / Refactor
  • Tests

Checklist

  • Tests added/updated
  • Docs updated (README/Examples/Docstrings)
  • ruff check and ruff format pass
  • mypy caneth passes
  • CI green

- pop `_TxItem` (not raw bytes) and check `item is None`
- avoid variable shadowing; use `frame_bytes` in send loop
- on writer=None: requeue whole item, clear `_connected`, yield
- on write error: requeue remaining frames as one atomic item
- preserves non-interleaving batches and prevents tight re-loops
@kstaniek
kstaniek requested a review from Copilot September 3, 2025 15:05

This comment was marked as outdated.

@kstaniek
kstaniek requested a review from Copilot September 3, 2025 15:23

This comment was marked as outdated.

@kstaniek
kstaniek requested a review from Copilot September 3, 2025 15:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the transmission loop in WaveShareCANClient to properly handle atomic frame batches and fix several issues with frame ordering and error recovery. The changes introduce a new _TxItem data structure to group frames atomically and ensure they are sent contiguously without interleaving.

Key changes:

  • Introduces _TxItem structure to encapsulate atomic frame groups with metadata
  • Refactors _tx_loop to pop entire items and send all frames within an item back-to-back
  • Adds proper error handling that re-queues remaining frames atomically on write failures
  • Implements atomic context manager for creating non-interleaved frame sequences

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
caneth/client.py Core refactoring of transmission system with new _TxItem structure, updated TX loop logic, and atomic batching functionality
tests/test_atomic_context.py Comprehensive test coverage for atomic frame transmission and mid-batch disconnect scenarios

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread caneth/client.py Outdated
Comment thread caneth/client.py Outdated
Comment on lines +590 to +591
for idx, frame_bytes in enumerate(item.frames):
writer.write(frame_bytes)

Copilot AI Sep 3, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name frame_bytes shadows the outer scope variable mentioned in the PR description. Consider using a more descriptive name like frame_data or encoded_frame to avoid confusion and improve code clarity.

Suggested change
for idx, frame_bytes in enumerate(item.frames):
writer.write(frame_bytes)
for idx, frame_data in enumerate(item.frames):
writer.write(frame_data)

Copilot uses AI. Check for mistakes.
Comment thread caneth/client.py Outdated
Comment on lines +599 to +602
remaining = item.frames[sent:]
if remaining:
async with self._tx_cv:
self._tx_buf.appendleft(_TxItem(frames=remaining, atomic=True, can_id=item.can_id))

Copilot AI Sep 3, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When re-queuing remaining frames, the code always sets atomic=True regardless of the original item's atomic flag. This could change the behavior for non-atomic items. Consider preserving the original atomic value: _TxItem(frames=remaining, atomic=item.atomic, can_id=item.can_id).

Copilot uses AI. Check for mistakes.
kstaniek and others added 3 commits September 3, 2025 18:00
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@kstaniek
kstaniek merged commit 0b87426 into main Sep 3, 2025
15 checks passed
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