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 handling of multiple responses in one read #152

Merged
merged 10 commits into from
May 8, 2024

Conversation

at-wat
Copy link
Member

@at-wat at-wat commented Apr 18, 2024

boost::asio::async_read_until may return multiple blocks of data (separated by specified delimiter) when they are received shortly.
Current implementation dropped second and later blocks.


Found during #151 development

@at-wat at-wat self-assigned this Apr 18, 2024
@sqbot

This comment has been minimized.

Copy link

codecov bot commented Apr 18, 2024

Codecov Report

Attention: Patch coverage is 75.60976% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 80.31%. Comparing base (c27faaa) to head (d655009).

Files Patch % Lines
include/scip2/response/time_sync.h 20.00% 4 Missing ⚠️
test/src/test_scip2.cpp 83.33% 4 Missing ⚠️
include/scip2/response/reset.h 50.00% 1 Missing ⚠️
include/scip2/response/stream.h 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #152      +/-   ##
==========================================
+ Coverage   80.15%   80.31%   +0.16%     
==========================================
  Files          31       33       +2     
  Lines        1305     1326      +21     
==========================================
+ Hits         1046     1065      +19     
- Misses        259      261       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sqbot

This comment has been minimized.

@at-wat at-wat requested review from nhatao and removed request for nhatao April 19, 2024 06:45
@at-wat at-wat marked this pull request as draft April 19, 2024 07:02
@sqbot

This comment has been minimized.

@sqbot

This comment has been minimized.

@sqbot

This comment has been minimized.

@sqbot

This comment has been minimized.

Comment on lines -60 to -63
std::string line;
while (std::getline(stream, line))
{
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Continued lines must be processed on the next callback

@sqbot
Copy link
Member

sqbot commented Apr 24, 2024

[262] PASSED on noetic

All tests passed
build/test_results/urg_stamped/gtest-test_decode.xml: 4 tests
build/test_results/urg_stamped/gtest-test_device_time_origin.xml: 6 tests
build/test_results/urg_stamped/gtest-test_first_order_filter.xml: 6 tests
build/test_results/urg_stamped/gtest-test_param.xml: 12 tests
build/test_results/urg_stamped/gtest-test_scip2.xml: 2 tests
build/test_results/urg_stamped/gtest-test_timestamp_moving_average.xml: 6 tests
build/test_results/urg_stamped/gtest-test_timestamp_outlier_remover.xml: 4 tests
build/test_results/urg_stamped/gtest-test_walltime.xml: 4 tests
build/test_results/urg_stamped/roslint-urg_stamped.xml: 1 tests
build/test_results/urg_stamped/rostest-test_tests_e2e.xml: 1 tests
build/test_results/urg_stamped/rosunit-e2e.xml: 6 tests
Summary: 52 tests, 0 errors, 0 failures, 0 skipped

@at-wat at-wat marked this pull request as ready for review April 24, 2024 01:44
@at-wat at-wat requested a review from nhatao April 24, 2024 08:43
Copy link

@nhatao nhatao left a comment

Choose a reason for hiding this comment

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

Sorry for the late review.
I am concerned that the readUntilEnd() becomes a busy loop. Is it guaranteed that the readUntilEnd() will not be called while the lidar sends its measurement data continuously?

@at-wat
Copy link
Member Author

at-wat commented May 7, 2024

@nhatao
All LIDAR responses including continuous measurement data are terminated by \n\n and the callback is registered by async_read_until(..., "\n\n"), readUntilEnd must always read until \n\n and return.

Even if the input buffer doesn't contains \n\n (by bug), std::getline will stop when the input buffer is entirely consumed.
(Input buffer won't be fed during callback as the next async_read_until is called after the callback)

@nhatao nhatao self-requested a review May 8, 2024 01:48
@at-wat at-wat merged commit ce4fec5 into master May 8, 2024
5 checks passed
@at-wat at-wat deleted the fix-multi-responses-in-one-read branch May 8, 2024 05:55
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.

3 participants