Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/signal real timestamp #969

Merged
merged 4 commits into from
Nov 16, 2023
Merged

Feature/signal real timestamp #969

merged 4 commits into from
Nov 16, 2023

Conversation

jpacov
Copy link
Contributor

@jpacov jpacov commented Mar 15, 2022 •
edited
Loading

Adding timestamps to Recorded Signals and ProtocolAnalyzer messages
Adding timestamps to messages generated in ProtocolSniffer

Before this PR, signals and messages interpreted by protocol analyzer, had no real timestamps; just some relative time measurements between each messages in signal. Now timestamps are absolute and are saved into proto.xml files correctly for future reference.

@CLAassistant
Copy link

CLAassistant commented Mar 15, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@jpacov
Copy link
Contributor Author

jpacov commented Mar 15, 2022

Physical test carried with HackRf One (Native)

@jpacov jpacov force-pushed the feature/signal-real-timestamp branch from 6af7cf8 to 15618c1 Compare March 17, 2022 22:11
@jpacov
Copy link
Contributor Author

jpacov commented Mar 22, 2022

@jopohl Let me know if you need further information.

@jpacov jpacov force-pushed the feature/signal-real-timestamp branch from 15618c1 to f95cd7e Compare March 28, 2022 21:55
@jpacov
Copy link
Contributor Author

jpacov commented May 2, 2022

@jopohl let me know if further changes are required ;)

@jpacov jpacov force-pushed the feature/signal-real-timestamp branch from f95cd7e to a0eea05 Compare October 14, 2022 12:36
@jopohl
Copy link
Owner

jopohl commented Oct 17, 2022

@jpacov could you please sign the CLA? Probably it wants that again due to the force-push or something.

@jpacov jpacov force-pushed the feature/signal-real-timestamp branch from e6de77a to e563910 Compare October 18, 2022 01:43
@jpacov
Copy link
Contributor Author

jpacov commented Oct 18, 2022

Dne @jopohl . It was a problem with 2 accounts mixed. Signed with both now.

@andynoack
Copy link
Collaborator

@jopohl can we merge now?

jopohl
jopohl reviewed Jan 26, 2023
View reviewed changes
Copy link
Owner

@jopohl jopohl left a comment

Choose a reason for hiding this comment

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

It would be nice to add some unit-tests or update existing ones to verify the new timestamps work as expected. I do not have an SDR here to test recording/Protocol Sniffer so if someone from the community could test this would be appreciated.

src/urh/signalprocessing/ProtocolSniffer.py Outdated
Comment on lines 214 to 215
self.signal.bits_per_symbol, write_bit_sample_pos=False)
self.signal.bits_per_symbol, write_bit_sample_pos=True)
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that _ppseq_to_bits populates bit_sample_pos array and we are able to calculate more accurate timestamps using bit sample position.

@jpacov jpacov force-pushed the feature/signal-real-timestamp branch from e563910 to 714d36b Compare February 24, 2023 23:32
@jpacov
Copy link
Contributor Author

jpacov commented Feb 24, 2023

@jopohl I tested more extensively the timestamps, and made some changes and small fixes. I also added some small unit tests for the ProtocolSniffer as you requested. Let me know if that is enough.

@jpacov jpacov force-pushed the feature/signal-real-timestamp branch from 714d36b to d5494ea Compare October 24, 2023 03:44
@jopohl
Copy link
Owner

jopohl commented Oct 28, 2023

@jpacov sorry for late reply, could you please pull in latest changes from master? I just updated the CI to support more recent Python versions and like to ensure your changes are compatible.

@jpacov jpacov force-pushed the feature/signal-real-timestamp branch from d5494ea to c78a850 Compare November 14, 2023 00:34
@jpacov jpacov force-pushed the feature/signal-real-timestamp branch from c78a850 to bb740c5 Compare November 15, 2023 04:26
@jpacov
Copy link
Contributor Author

jpacov commented Nov 15, 2023

With this MR, timestamps for decoded messages are calculated correctly (useful when exporting decoded messages).
Working for signals recorded on Record signal window, Sniff protocol window and for signals derivated from other signals (e.g. after filtering a signal).

Here an example:
image
image
image

@jpacov
Copy link
Contributor Author

jpacov commented Nov 15, 2023 •
edited
Loading

@jpacov sorry for late reply, could you please pull in latest changes from master? I just updated the CI to support more recent Python versions and like to ensure your changes are compatible.

@jopohl Done. Let me know if further actions are needed.

@jopohl
Copy link
Owner

jopohl commented Nov 16, 2023

Thank you for your contribution.

@jopohl jopohl merged commit a390350 into jopohl:master Nov 16, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jopohl jopohl jopohl left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jpacov @CLAassistant @jopohl @andynoack @PepsConti