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.

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

Fix send periodic #1713

Merged
merged 4 commits into from
Jan 3, 2024
Merged

Fix send periodic #1713

merged 4 commits into from
Jan 3, 2024

Conversation

Tbruno25
Copy link
Contributor

Closes #1710

The proposed fix is to check whether duration is exceeded before the transmit of the next message.

@grant-allan-ctct
Copy link
Contributor

I'm delighted to see that you've added a new test case. 🏆

@Tbruno25 Tbruno25 marked this pull request as ready for review December 27, 2023 18:17
@zariiii9003
Copy link
Collaborator

msg_due_time_ns and msg_index should only be incremented after the message was sent.

Copy link
Owner

@hardbyte hardbyte left a comment

Choose a reason for hiding this comment

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

Thanks both @grant-allan-ctct and @Tbruno25 for the bug report, fix and test.

@zariiii9003 zariiii9003 merged commit c619813 into hardbyte:main Jan 3, 2024
30 checks passed
@zariiii9003
Copy link
Collaborator

Now the CI is failing 😅
@Tbruno25 Could you try to improve the robustness of test_send_periodic_duration?

@Tbruno25
Copy link
Contributor Author

Tbruno25 commented Jan 3, 2024

@zariiii9003 absolutely! Although it appears to only be failing on the very specific windows/pypy combinations. Would it be wrong if I simply skipped this test for this environment?

@grant-allan-ctct
Copy link
Contributor

It's a interesting thing. I wonder what it's telling us.

Perhaps it has found a bug for which there is no other test coverage yet existing. For example, perhaps the message timestamp values are coming back as integers on this platform. Or perhaps one the calls to self.bus1.recv is choking on one of the arguments that the testcase is passing it.

Disclaimer: I just had to Google what pypy is, so please disregard if you think this comment could well be nonsense.

@zariiii9003
Copy link
Collaborator

@zariiii9003 absolutely! Although it appears to only be failing on the very specific windows/pypy combinations. Would it be wrong if I simply skipped this test for this environment?

Yes that's okay. I think we disable most timing sensitive tests in CI.

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.

send_periodic with non-None duration sends one extra message before stopping
4 participants