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

allow byte array to use int data #184

Open
wants to merge 7 commits into
base: rolling
Choose a base branch
from

Conversation

iuhilnehc-ynos
Copy link

to fix ros2/ros2cli#760

Signed-off-by: Chen Lihui [email protected]

Chen Lihui added 2 commits September 14, 2022 15:43
Signed-off-by: Chen Lihui <[email protected]>
@iuhilnehc-ynos
Copy link
Author

and use ros2 topic to publish ByteMultiArray message as follows:

# console A
$ ros2 topic pub -1 /whatever std_msgs/msg/ByteMultiArray "{data: [1, 2, 3]}"

# console B
$ ros2 topic echo /whatever std_msgs/msg/ByteMultiArray

Signed-off-by: Chen Lihui <[email protected]>
Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

a couple of comments, but overall lgtm.

@@ -0,0 +1,73 @@
# Copyright 2022 Open Source Robotics Foundation, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Copyright 2022 Open Source Robotics Foundation, Inc.
# Copyright 2022 Sony Group Corporation.

Copy link
Author

Choose a reason for hiding this comment

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

Updated in 2a61cfc

@{assert_msg_suffixes.append("and each value of type '%s'" % get_python_type(type_))}@
@[ if get_python_type(type_) == 'bytes']@
@{byte_array_detected = True}@
@{assert_msg_suffixes.append("or type 'int' in range(0, 256)")}@
Copy link
Contributor

Choose a reason for hiding this comment

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

1 byte range should be 0 to 255?

Suggested change
@{assert_msg_suffixes.append("or type 'int' in range(0, 256)")}@
@{assert_msg_suffixes.append("or type 'int' in range(0, 255)")}@

Copy link
Author

Choose a reason for hiding this comment

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

Thank you.

Chen Lihui and others added 2 commits January 17, 2023 17:28
@fujitatomoya
Copy link
Contributor

@sloretz can you take a look?

@clalancette clalancette self-assigned this Feb 2, 2023
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I don't love adding more work to the assert checking; this is already a very slow portion of Python, and this will make it worse. But it does allow this to proceed, so I'm reluctantly going to approve this.

@sloretz A final review by you would be appreciated here.

@fujitatomoya
Copy link
Contributor

@iuhilnehc-ynos can you rebase and start CI for this?

@iuhilnehc-ynos
Copy link
Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Contributor

@sloretz friendly ping.

@andrew-chapman-smc
Copy link

@sloretz any chance this can go in?

I just hit this issue

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.

Cannot/don't know how to publish message with byte[] field
4 participants