Skip to content

fix: PCAP traces have wrong timestamps - #430

Open
KonradBreitsprecherBkd wants to merge 2 commits into
mainfrom
dev_fix_pcap_timestamps
Open

KonradBreitsprecherBkd wants to merge 2 commits into
mainfrom
dev_fix_pcap_timestamps

Conversation

@KonradBreitsprecherBkd

@KonradBreitsprecherBkd KonradBreitsprecherBkd commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

The PCAP sink writes the nanosecond magic number (0xa1b23c4d) into the global header, but it wrote the packet timestamps in microseconds. Wireshark, tcpdump and every other PCAP consumer trust the magic number, so they showed the decimal places of all timestamps 1000 times too small (The seconds are treated seperately, so this was correct and likely the cause why this was overlooked). SIL Kit's own PcapReader hid the problem because it made the same mistake the other way round and always read the field as microseconds.

I confirmed this with a PCAP export from SIL Kit: A frame sent at T=6782 ms has ts_sec=6, ts_usec=782000, and Wireshark shows it as 6.000782 s instead of 6.782 s. With the fix here, the timestamps in the wireshark trace match the SIL Kit logs.
To reproduce use this config for the EthernetWriterDemo, look at the last timestamp in the log and compare it to the timestamp in wireshark :

Description: PCAP tracing configuration for the EthernetWriter demo
EthernetControllers:
  - Name: EthernetController1
    UseTraceSinks: [ PcapSink ]
Tracing:
  TraceSinks:
    - Name: PcapSink
      Type: PcapFile
      OutputPath: EthernetWriter_Trace.pcap
Logging:
  Sinks:
    - Type: Stdout
      Level: Info

This PR makes both sides follow the magic number:

  • Sink: packet timestamps are now written in nanoseconds, which matches the announced resolution.
  • Reader: it reads the resolution from the magic number. Besides nanosecond files it now also accepts microsecond files (0xa1b2c3d4), so captures made with Wireshark or tcpdump can be replayed.
  • PacketHeader::ts_usec is renamed to ts_fraction, because its unit depends on the magic number.

Compatibility: .pcap files written by earlier SIL Kit versions contain microseconds under the nanosecond magic. From now on, PcapReader reads their timestamps 1000 times too small, which is how every other PCAP tool already read them. Replaying such old traces will show compressed timing.

Instructions for review / testing

  • Main change: PcapSink::Trace (SilKit/source/tracing/PcapSink.cpp) and PcapReader::ReadGlobalHeader / Seek (SilKit/source/tracing/PcapReader.cpp).
  • Please check the compatibility trade-off above. The alternative would be to keep writing microseconds and switch the magic number to 0xa1b2c3d4 instead. That keeps old files readable, but traces lose sub-microsecond precision.
  • Manual check: record an Ethernet trace with a PcapFile sink, open it in Wireshark, and confirm the timestamps match the simulation time.

Developer checklist (address before review)

  • Changelog.md updated
  • Prepared update for depending repositories
  • Documentation updated (public API changes only)
  • API docstrings updated (public API changes only)
  • Rebase → commit history clean
  • Squash and merge → proper PR title

fixed: the PCAP sink announced nanosecond resolution (magic 0xa1b23c4d) but wrote microseconds, so Wireshark and other tools showed timestamps 1000 times too small; it now writes nanoseconds
changed: the PCAP reader honors the resolution of the magic number and also accepts microsecond files (0xa1b2c3d4), e.g., captured by Wireshark or tcpdump
note: files written by earlier SIL Kit versions contain microseconds under the nanosecond magic; the reader now reads their timestamps 1000 times too small, like every other PCAP tool already did
added: Test_Pcap cases for both resolutions

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Konrad Breitsprecher <Konrad.Breitsprecher@vector.com>
@KonradBreitsprecherBkd KonradBreitsprecherBkd changed the title Dev fix pcap timestamps PCAP traces have wrong timestamps Oct 1, 2026
@KonradBreitsprecherBkd KonradBreitsprecherBkd added needs reviewer This issue is looking for a reviewer. and removed needs reviewer This issue is looking for a reviewer. labels Oct 1, 2026
@KonradBreitsprecherBkd
KonradBreitsprecherBkd marked this pull request as ready for review October 2, 2026 09:55
@KonradBreitsprecherBkd KonradBreitsprecherBkd added the needs reviewer This issue is looking for a reviewer. label Oct 2, 2026
@VDanielEdwards
VDanielEdwards self-requested a review October 2, 2026 12:04
uint32_t ts_sec; /* timestamp seconds */
uint32_t ts_fraction; /* timestamp nanoseconds or microseconds, depending on the magic number */
uint32_t incl_len; /* number of octets of packet saved in file */
uint32_t orig_len; /* actual length of packet */

@VDanielEdwards VDanielEdwards Oct 2, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could add

  • a enum struct TimestampMode with two enumerators Nanoseconds and Microseconds
  • and a method GetTimestampMode() const -> TimestampMode that compares the magic number

That makes it simpler for code using the PacketHeader structure to get that info without having to know about the magic numbers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The magicNumber lives in GlobalHeader, I added GetTimestampMode there.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i think we should keep this code as simple as possible, and just support microseconds, and clamp the timestamps appropriately

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd say the error handling to check for the MicrosecondMagic only in the header and return an error is just as complicated as supporting both cases.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a downside to using nanoseconds? Since SIL Kit timestamps are nanoseconds that seems like the more natural choice for us.

Also since we can also read PCAP files for replay, shouldn't we support both, since we could get a file that was recorded, e.g., using tcpdump.

Comment thread SilKit/source/tracing/PcapReader.cpp Outdated
Add Pcap::TimestampMode and GlobalHeader::GetTimestampMode(), so code
reading PCAP headers does not need to know the magic numbers. An
unknown magic number throws a SilKitError.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Konrad Breitsprecher <Konrad.Breitsprecher@vector.com>
@KonradBreitsprecherBkd KonradBreitsprecherBkd removed the needs reviewer This issue is looking for a reviewer. label Oct 2, 2026
@KonradBreitsprecherBkd KonradBreitsprecherBkd changed the title PCAP traces have wrong timestamps fix: PCAP traces have wrong timestamps Oct 5, 2026

This branch has not been deployed

No deployments
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.

3 participants