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

Jump to bottom

Topic content filter fallback #1

Open
wants to merge 51 commits into
base: main
Choose a base branch
from
Open

Topic content filter fallback #1

wants to merge 51 commits into from

Conversation

iuhilnehc-ynos
Copy link
Owner

Internal review

Chen Lihui added 25 commits July 5, 2022 16:09
init
69ba1ff 
Signed-off-by: Chen Lihui <[email protected]>
move files from FastDDS, make building and calling some functions suc…
af121d1 
…cessfully

Signed-off-by: Chen Lihui <[email protected]>
add ros2 message_type_support, not finished
54ee96e 
Signed-off-by: Chen Lihui <[email protected]>
add the test access_path for content_filtering_subscriber
8a85a6a 
Signed-off-by: Chen Lihui <[email protected]>
test
ab7a0dd 
Signed-off-by: Chen Lihui <[email protected]>
hard code to make content_filter_subscriber run successfully
ace4980 
Signed-off-by: Chen Lihui <[email protected]>
parameter event subscriber with 'new_parameters[0].value.type = 4'
f9612d5 
Signed-off-by: Chen Lihui <[email protected]>
refactor
49e6748 
Signed-off-by: Chen Lihui <[email protected]>
re 2
58da6ef 
Signed-off-by: Chen Lihui <[email protected]>
refactor 3
32be4b7 
Signed-off-by: Chen Lihui <[email protected]>
use strncpy for string
0051c2b 
Signed-off-by: Chen Lihui <[email protected]>
r4
887d577 
Signed-off-by: Chen Lihui <[email protected]>
r5
8e38611 
Signed-off-by: Chen Lihui <[email protected]>
update define macro
b868a03 
Signed-off-by: Chen Lihui <[email protected]>
update package description
088d9dc 
Signed-off-by: Chen Lihui <[email protected]>
r6
5ac257a 
Signed-off-by: Chen Lihui <[email protected]>
add set/get_content_filter
c246a18 
Signed-off-by: Chen Lihui <[email protected]>
r7
97512a4 
Signed-off-by: Chen Lihui <[email protected]>
update
9664133 
Signed-off-by: Chen Lihui <[email protected]>
uncrustify and lint
f727ad7 
Signed-off-by: Chen Lihui <[email protected]>
update
3b95aa3 
Signed-off-by: Chen Lihui <[email protected]>
use rosidl_runtime_c__String for string in c type_support
dffb8df 
Signed-off-by: Chen Lihui <[email protected]>
update
dff09f0 
Signed-off-by: Chen Lihui <[email protected]>
use rcutils log
5c8c970 
Signed-off-by: Chen Lihui <[email protected]>
add test cases for the exported functions
4695902 
Signed-off-by: Chen Lihui <[email protected]>
@iuhilnehc-ynos
Copy link
Owner Author

@fujitatomoya @Barry-Xu-2018

Could you help to do an internal review for these two PRs (this PR and iuhilnehc-ynos/rcl#4)?

@iuhilnehc-ynos
Copy link
Owner Author

About the file name(such as DDSFilter[*].h/cpp), I wondered if we should keep them or update them? Do you have any suggestions?
For the first time internal review, I think we can focus on the basic logic.

@fujitatomoya
Copy link
Collaborator

@iuhilnehc-ynos wow, this is huge 👍 cannot promise when but i will try to do my best.

@Barry-Xu-2018
Copy link
Collaborator

Partial comments. I will continue to review.

r8
e7957a0 
Signed-off-by: Chen Lihui <[email protected]>
Barry-Xu-2018
Barry-Xu-2018 reviewed Jul 8, 2022
View reviewed changes
common_content_filter/src/api.cpp Outdated Show resolved Hide resolved
common_content_filter/src/DDSFilterFactory.cpp Outdated
Comment on lines 390 to 391
const char * filter_class_name,
const char * type_name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If unused, better to simplify the parameter.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Updated

common_content_filter/test/test_api.cpp Show resolved Hide resolved
common_content_filter/test/test_api.cpp Outdated Show resolved Hide resolved
Chen Lihui added 11 commits July 8, 2022 21:14
address partial review
d237fc9 
Signed-off-by: Chen Lihui <[email protected]>
add complex message and test
7aa2b9c 
Signed-off-by: Chen Lihui <[email protected]>
remove unuseful code
547265d 
Signed-off-by: Chen Lihui <[email protected]>
add error message for test case
42a28a3 
Signed-off-by: Chen Lihui <[email protected]>
add unbounded and bounded array type
345e88e 
Signed-off-by: Chen Lihui <[email protected]>
update test and add the missing header file for windows platform
870d207 
Signed-off-by: Chen Lihui <[email protected]>
use pegtl as a vendor package
4e7dea6 
Signed-off-by: Chen Lihui <[email protected]>
add/update readme with simple description
d75a22d 
Signed-off-by: Chen Lihui <[email protected]>
update the year of copyright
0f26f5d 
Signed-off-by: Chen Lihui <[email protected]>
update/add new test cases
4389596 
Signed-off-by: Chen Lihui <[email protected]>
update
a8f48e2 
Signed-off-by: Chen Lihui <[email protected]>
@Barry-Xu-2018
Copy link
Collaborator

Whether tao_pegtl_vendor directory need to be moved to another repository finally?

@iuhilnehc-ynos
Copy link
Owner Author

iuhilnehc-ynos commented Jul 12, 2022 •
edited
Loading

Whether tao_pegtl_vendor directory need to be moved to another repository finally?

I'm not sure. It's better to let the community make the final decision.
I thought that before, but I also thought that the tao_pegtl_vendor might only use for common_content_filter in ros2 repositories. (e.g. sqlite3_vendor in the rosbag2)

use the fixed version
5579db4 
Signed-off-by: Chen Lihui <[email protected]>
@Barry-Xu-2018
Copy link
Collaborator

I'm not sure. It's better to let the community make the final decision.
I thought that before, but I also thought that the tao_pegtl_vendor might only use for common_content_filter in ros2 repositories. (e.g. sqlite3_vendor in the rosbag2)

Yes. Let's hear the community's suggestions.

Chen Lihui added 3 commits July 12, 2022 10:06
@Barry-Xu-2018
Copy link
Collaborator

Thanks. I have no more comments.

Chen Lihui added 8 commits July 14, 2022 10:10
tiny fix
04f9464 
Signed-off-by: Chen Lihui <[email protected]>
rename files
3982265 
remove DDS and use common_content_filter instead of eprosima*::fastdds::dds

Signed-off-by: Chen Lihui <[email protected]>
remove include Log.hpp that is not used
dd053d7 
Signed-off-by: Chen Lihui <[email protected]>
fix issues detected by cpplint
f53b044 
Signed-off-by: Chen Lihui <[email protected]>
update copyright
80cdd97 
Signed-off-by: Chen Lihui <[email protected]>
use Sony Group Corporation instead
27a263e 
Signed-off-by: Chen Lihui <[email protected]>
fix build issue on ubuntu22
daae088 
Signed-off-by: Chen Lihui <[email protected]>
add missing files
dbd5a02 
Signed-off-by: Chen Lihui <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Barry-Xu-2018 Barry-Xu-2018 Barry-Xu-2018 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@iuhilnehc-ynos @fujitatomoya @Barry-Xu-2018