Skip to content

fix: add receive ts - #853

Closed
al3xa23 wants to merge 2 commits into
masterfrom
node-test_rtt
Closed

fix: add receive ts#853
al3xa23 wants to merge 2 commits into
masterfrom
node-test_rtt

Conversation

@al3xa23

Copy link
Copy Markdown
Contributor

No description provided.

Signed-off-by: al3xa23 <140614263+al3xa23@users.noreply.github.com>
@al3xa23
al3xa23 requested a review from stv0g as a code ownerJanuary 28, 2025 07:53
@stv0g

Copy link
Copy Markdown
Contributor

Hi @al3xa23,

thanks for the PR 🥳 Could you please add a brief description / motivation for the change?
Why is it required? Which config did you use for testing?

I am a bit surprised you need this change, as usually the ts_received field should be set by the receiving node (hence the name..). If it is not done by the receiving node (via its _read() function), we also have the fix hook which will set the ts_received field:

smp->ts.received = now;

The fix hook is an implicit hook, meaning it will be added to the receiving path by default.

@stv0gstv0g mentioned this pull request Jan 28, 2025
@stv0g

Copy link
Copy Markdown
Contributor

I also realized, that the fix is breaking the CI:

villas-full> /tmp/nix-build-villas-full.drv-0/1wfq2b8g15jqv7sh6caqyg78ll4jv0j1-source/lib/nodes/test_rtt.cpp: In member function 'virtual int villas::node::TestRTT::_write(villas::node::Sample**, unsigned int)':
villas-full> /tmp/nix-build-villas-full.drv-0/1wfq2b8g15jqv7sh6caqyg78ll4jv0j1-source/lib/nodes/test_rtt.cpp:474:29: error: request for member 'now' in 'time', which is of non-class type 'time_t(time_t*) noexcept' {aka 'long int(long int*) noexcept'}
villas-full> 474 | smp->ts.received = time.now();
villas-full> | ^~~

@al3xa23

Copy link
Copy Markdown
ContributorAuthor

Hi Steffen,

thanks for the feedback. I will discuss it with Laura again because we had some unclarities regarding how the test_rtt actual works. I try to come back to you before making a PR next time. If you have other suggestions, please let me know.

@stv0g

Copy link
Copy Markdown
Contributor

Hi @al3xa23,

no worries, I think the PRs here on GitHub are a great way to discuss such things :)
Maybe I missed something, so your feedback / explanations are very welcome :)

@stv0g

stv0g commented Feb 10, 2025

Copy link
Copy Markdown
Contributor

Hi @al3xa23, @fuentesgrau

are there some updates to this PR? Or something where I can unblock you?

@al3xa23

Copy link
Copy Markdown
ContributorAuthor

Hi Steffen,

Laura and I had a small discussion and she re-did some tests. The conlcusion, we don't need this PR as long as we set fake=true in udp. I will add it in the documentation as hint.
From our side, this PR is not necessary anymore. Hope that is fine for you.

Sign up for freeto 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

@al3xa23@stv0g