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

Fix wait_session_status listen timeout #322

Merged
merged 3 commits into from
Jun 16, 2022
Merged

Fix wait_session_status listen timeout #322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
74f39b7
Fix wait_session_status listen timeout
Acuadros95 Jun 16, 2022
aa576ae
Update timeout calc and test
Acuadros95 Jun 16, 2022
de481c1
Increase time tolerance
Acuadros95 Jun 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/c/core/session/session.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -748,7 +748,15 @@ bool wait_session_status(
for (size_t i = 0; i < attempts && session->info.last_requested_status == UXR_STATUS_NONE; ++i)
{
send_message(session, buffer, length);
listen_message(session, UXR_CONFIG_MIN_SESSION_CONNECTION_INTERVAL);

int64_t start_timestamp = uxr_millis();
int remaining_time = UXR_CONFIG_MIN_SESSION_CONNECTION_INTERVAL;

do
{
listen_message(session, remaining_time);
remaining_time = UXR_CONFIG_MIN_SESSION_CONNECTION_INTERVAL - (int)(uxr_millis() - start_timestamp);
Copy link

@Ryanf55 Ryanf55 Mar 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This causes invalid underflow when the uxr_millis() returns something invalid, such as the embedded device does not have GPS time yet. I could use some info on how this is being used internally. Our embedded device has multiple time sources. The boot time is what we used to fix this bug, however that does not comply with using ROS2 time (UTC). If we switch time sources at runtime, it would be good to know what to do (from boot time to RTC time).

Copy link
Member

Choose a reason for hiding this comment

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

Which implementation of uxr_millis() are you using?

Copy link

Choose a reason for hiding this comment

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

Our software runs on multiple OS's. This one is ChibiOS, so we had to write our own implentation of uxr_millis(). It was not clear from the public API on the requirements for that function. I filed this bug for more info:
#350

} while (remaining_time > 0 && session->info.last_requested_status == UXR_STATUS_NONE);
}

return session->info.last_requested_status != UXR_STATUS_NONE;
Expand Down
8 changes: 7 additions & 1 deletion test/unitary/session/Session.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,9 +361,15 @@ TEST_F(SessionTest, WaitSessionStatusBad)
uint8_t buffer[MTU];
size_t length = 0;
size_t attempts = 10;
int64_t start_timestamp = uxr_millis();
bool found = wait_session_status(&session, buffer, length, attempts);
EXPECT_FALSE(found);
EXPECT_EQ(attempts, size_t(SessionTest::listening_counter));
EXPECT_GE(size_t(SessionTest::listening_counter), attempts);

// Check elapsed time with 100 ms tolerance
int64_t final_time = uxr_millis() - start_timestamp;
int64_t expected_time = (int64_t) (UXR_CONFIG_MIN_SESSION_CONNECTION_INTERVAL * attempts);
EXPECT_NEAR((double) final_time, (double) expected_time, 100);
}

TEST_F(SessionTest, SendMessageOk)
Expand Down