Skip to content

Commit 48578f5

Browse files
committed
fix(asc): address PR review feedback on timestamps_format
Reviewer feedback received (zariiii9003): 1. Use Literal type hint for timestamps_format parameter - Changed: timestamps_format: str = "absolute" - To: timestamps_format: Literal["absolute", "relative"] = "absolute" - Added Literal to typing imports 2. Simplify log_event timestamp computation - Moved monotonic clamp out of if/else blocks: timestamp = max(timestamp, self.last_timestamp) - Each branch now contains only simple arithmetic: absolute: written_timestamp = timestamp - self.started relative: written_timestamp = timestamp - self.last_timestamp 3. Use relative_timestamp=False in roundtrip tests - Updated test_write_relative_timestamp_roundtrip and test_write_absolute_timestamps_are_offsets_from_start to use relative_timestamp=False so assertions verify original message timestamps are recovered (100.0, 100.3, 101.0) rather than file-stored offsets (0.0, 0.3, 1.0) Additional issues found and fixed during review: 4. Removed outdated TODO comment in ASCReader - Removed: "TODO - what is this used for? The ASC Writer only prints absolute" — no longer accurate since ASCWriter now supports both "absolute" and "relative" formats 5. Lowered assertAlmostEqual precision from places=5 to places=3 - The datetime triggerblock roundtrip (fromtimestamp -> strftime -> strptime -> timestamp) only preserves millisecond precision due to the ".NNN" format. places=5 (5 microseconds) is stricter than what the format can guarantee; places=3 (0.5 ms) correctly reflects the actual precision limit. Verified empirically: sub-millisecond timestamps incur ~0.456 ms error which passes places=3 but fails places=5. 6. Updated docstrings for both modified roundtrip tests to accurately describe the new assertion semantics (original timestamp recovery)
1 parent a48e2fd commit 48578f5

2 files changed

Lines changed: 14 additions & 21 deletions

File tree

can/io/asc.py

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,6 @@ def __init__(
7575
self.relative_timestamp = relative_timestamp
7676
self.date: str | None = None
7777
self.start_time = 0.0
78-
# TODO - what is this used for? The ASC Writer only prints `absolute`
7978
self.timestamps_format: str | None = None
8079
self.internal_events_logged = False
8180

@@ -462,19 +461,14 @@ def log_event(self, message: str, timestamp: float | None = None) -> None:
462461
# Use last known timestamp if unknown
463462
if timestamp is None:
464463
timestamp = self.last_timestamp
464+
timestamp = max(timestamp, self.last_timestamp)
465465
# Compute written timestamp based on configured format
466466
if self.timestamps_format == "absolute":
467467
# offsets from the start of measurement
468-
written_timestamp = (
469-
timestamp - self.started if timestamp >= self.started else timestamp
470-
)
468+
written_timestamp = timestamp - self.started
471469
else:
472470
# deltas from the preceding event
473-
written_timestamp = (
474-
timestamp - self.last_timestamp
475-
if timestamp >= self.last_timestamp
476-
else 0.0
477-
)
471+
written_timestamp = timestamp - self.last_timestamp
478472
# Track last timestamp so the next event can compute its delta
479473
self.last_timestamp = timestamp
480474
line = self.FORMAT_EVENT.format(timestamp=written_timestamp, message=message)

test/logformats_test.py

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -711,7 +711,7 @@ def test_write_timestamps_format_invalid(self):
711711
can.ASCWriter(self.test_file_name, timestamps_format="unix")
712712

713713
def test_write_relative_timestamp_roundtrip(self):
714-
"""Messages written with relative format round-trip with relative timestamps."""
714+
"""Messages written with relative format round-trip to their original timestamps."""
715715
msgs = [
716716
can.Message(timestamp=100.0, arbitration_id=0x1, data=b"\x01"),
717717
can.Message(timestamp=100.5, arbitration_id=0x2, data=b"\x02"),
@@ -721,13 +721,12 @@ def test_write_relative_timestamp_roundtrip(self):
721721
for m in msgs:
722722
writer.on_message_received(m)
723723

724-
with can.ASCReader(self.test_file_name, relative_timestamp=True) as reader:
724+
with can.ASCReader(self.test_file_name, relative_timestamp=False) as reader:
725725
result = list(reader)
726726

727727
self.assertEqual(len(result), len(msgs))
728-
# Timestamps in file are per-event deltas; reader reads them as-is
729-
self.assertAlmostEqual(result[0].timestamp, 0.0, places=5)
730-
self.assertAlmostEqual(result[1].timestamp, 0.5, places=5)
728+
self.assertAlmostEqual(result[0].timestamp, 100.0, places=3)
729+
self.assertAlmostEqual(result[1].timestamp, 100.5, places=3)
731730

732731
def test_write_relative_timestamps_are_per_event_deltas(self):
733732
"""With timestamps_format='relative', each written timestamp is a delta from the
@@ -754,8 +753,8 @@ def test_write_relative_timestamps_are_per_event_deltas(self):
754753
self.assertAlmostEqual(result[2].timestamp, 0.7, places=5)
755754

756755
def test_write_absolute_timestamps_are_offsets_from_start(self):
757-
"""With timestamps_format='absolute' (default), each written timestamp is an
758-
offset from the measurement start, not a per-event delta."""
756+
"""With timestamps_format='absolute' (default), messages round-trip to their
757+
original timestamps when read back with relative_timestamp=False."""
759758
msgs = [
760759
can.Message(timestamp=100.0, arbitration_id=0x1, data=b"\x01"),
761760
can.Message(timestamp=100.3, arbitration_id=0x2, data=b"\x02"),
@@ -766,14 +765,14 @@ def test_write_absolute_timestamps_are_offsets_from_start(self):
766765
for m in msgs:
767766
writer.on_message_received(m)
768767

769-
with can.ASCReader(self.test_file_name, relative_timestamp=True) as reader:
768+
with can.ASCReader(self.test_file_name, relative_timestamp=False) as reader:
770769
result = list(reader)
771770

772771
self.assertEqual(len(result), len(msgs))
773-
# All timestamps are offsets from the measurement start (100.0):
774-
self.assertAlmostEqual(result[0].timestamp, 0.0, places=5)
775-
self.assertAlmostEqual(result[1].timestamp, 0.3, places=5)
776-
self.assertAlmostEqual(result[2].timestamp, 1.0, places=5)
772+
# Timestamps are recovered from the triggerblock start time + file offset:
773+
self.assertAlmostEqual(result[0].timestamp, 100.0, places=3)
774+
self.assertAlmostEqual(result[1].timestamp, 100.3, places=3)
775+
self.assertAlmostEqual(result[2].timestamp, 101.0, places=3)
777776

778777
@parameterized.expand(
779778
[

0 commit comments

Comments
 (0)