Skip to content

Commit 4fe6c13

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 be2802e commit 4fe6c13

2 files changed

Lines changed: 16 additions & 23 deletions

File tree

can/io/asc.py

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
import re
1111
from collections.abc import Generator
1212
from datetime import datetime
13-
from typing import Any, Final, TextIO
13+
from typing import Any, Final, Literal, TextIO
1414

1515
from ..message import Message
1616
from ..typechecking import StringPathLike
@@ -66,7 +66,6 @@ def __init__(
6666
self.relative_timestamp = relative_timestamp
6767
self.date: str | None = None
6868
self.start_time = 0.0
69-
# TODO - what is this used for? The ASC Writer only prints `absolute`
7069
self.timestamps_format: str | None = None
7170
self.internal_events_logged = False
7271

@@ -358,7 +357,7 @@ def __init__(
358357
self,
359358
file: StringPathLike | TextIO,
360359
channel: int = 1,
361-
timestamps_format: str = "absolute",
360+
timestamps_format: Literal["absolute", "relative"] = "absolute",
362361
**kwargs: Any,
363362
) -> None:
364363
"""
@@ -442,19 +441,14 @@ def log_event(self, message: str, timestamp: float | None = None) -> None:
442441
# Use last known timestamp if unknown
443442
if timestamp is None:
444443
timestamp = self.last_timestamp
444+
timestamp = max(timestamp, self.last_timestamp)
445445
# Compute written timestamp based on configured format
446446
if self.timestamps_format == "absolute":
447447
# offsets from the start of measurement
448-
written_timestamp = (
449-
timestamp - self.started if timestamp >= self.started else timestamp
450-
)
448+
written_timestamp = timestamp - self.started
451449
else:
452450
# deltas from the preceding event
453-
written_timestamp = (
454-
timestamp - self.last_timestamp
455-
if timestamp >= self.last_timestamp
456-
else 0.0
457-
)
451+
written_timestamp = timestamp - self.last_timestamp
458452
# Track last timestamp so the next event can compute its delta
459453
self.last_timestamp = timestamp
460454
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
@@ -704,7 +704,7 @@ def test_write_timestamps_format_invalid(self):
704704
can.ASCWriter(self.test_file_name, timestamps_format="unix")
705705

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

717-
with can.ASCReader(self.test_file_name, relative_timestamp=True) as reader:
717+
with can.ASCReader(self.test_file_name, relative_timestamp=False) as reader:
718718
result = list(reader)
719719

720720
self.assertEqual(len(result), len(msgs))
721-
# Timestamps in file are per-event deltas; reader reads them as-is
722-
self.assertAlmostEqual(result[0].timestamp, 0.0, places=5)
723-
self.assertAlmostEqual(result[1].timestamp, 0.5, places=5)
721+
self.assertAlmostEqual(result[0].timestamp, 100.0, places=3)
722+
self.assertAlmostEqual(result[1].timestamp, 100.5, places=3)
724723

725724
def test_write_relative_timestamps_are_per_event_deltas(self):
726725
"""With timestamps_format='relative', each written timestamp is a delta from the
@@ -747,8 +746,8 @@ def test_write_relative_timestamps_are_per_event_deltas(self):
747746
self.assertAlmostEqual(result[2].timestamp, 0.7, places=5)
748747

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

762-
with can.ASCReader(self.test_file_name, relative_timestamp=True) as reader:
761+
with can.ASCReader(self.test_file_name, relative_timestamp=False) as reader:
763762
result = list(reader)
764763

765764
self.assertEqual(len(result), len(msgs))
766-
# All timestamps are offsets from the measurement start (100.0):
767-
self.assertAlmostEqual(result[0].timestamp, 0.0, places=5)
768-
self.assertAlmostEqual(result[1].timestamp, 0.3, places=5)
769-
self.assertAlmostEqual(result[2].timestamp, 1.0, places=5)
765+
# Timestamps are recovered from the triggerblock start time + file offset:
766+
self.assertAlmostEqual(result[0].timestamp, 100.0, places=3)
767+
self.assertAlmostEqual(result[1].timestamp, 100.3, places=3)
768+
self.assertAlmostEqual(result[2].timestamp, 101.0, places=3)
770769

771770
@parameterized.expand(
772771
[

0 commit comments

Comments
 (0)