Skip to content

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 force-pushed the bugfix/1710_periodic branch from 9469332 to 5d18b4a Compare December 27, 2023 18:11
@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
@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