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

Adding ros2_socketcan_msgs #26

Merged
merged 2 commits into from
Nov 21, 2022
Merged

Adding ros2_socketcan_msgs #26

merged 2 commits into from
Nov 21, 2022

Conversation

JWhitleyWork
Copy link
Collaborator

Per #21, this adds a new package, ros2_socketcan_msgs. This is per option 3 in that issue.

Copy link
Contributor

@kenji-miyake kenji-miyake left a comment

Choose a reason for hiding this comment

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

Please let me confirm several items.

ros2_socketcan_msgs/msg/Frame.msg Outdated Show resolved Hide resolved
ros2_socketcan_msgs/msg/Frame.msg Show resolved Hide resolved
ros2_socketcan_msgs/package.xml Show resolved Hide resolved
ros2_socketcan_msgs/msg/Frame.msg Show resolved Hide resolved
@JWhitleyWork
Copy link
Collaborator Author

@kenji-miyake or @mitsudome-r friendly ping.

Copy link
Contributor

@kenji-miyake kenji-miyake left a comment

Choose a reason for hiding this comment

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

@JWhitleyWork I'm so sorry. I was extremely busy recently. 🥺
Seeing the discussions between other developers, it looks good to me.

Could you consider whether to use uint8[<=64] or not based on this comment?
#26 (comment)

@kenji-miyake
Copy link
Contributor

@xmfcx @mitsudome-r Could you also review this PR? 🙏

@JWhitleyWork
Copy link
Collaborator Author

Could you consider whether to use uint8[<=64] or not based on this comment? #26 (comment)

Done.

Signed-off-by: Joshua Whitley <[email protected]>
Copy link
Contributor

@xmfcx xmfcx left a comment

Choose a reason for hiding this comment

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

I've also read the related issue and the ros_canopen PR and I think this solution is good.

@xmfcx xmfcx merged commit 3962b98 into autowarefoundation:main Nov 21, 2022
@JWhitleyWork JWhitleyWork deleted the add-canfd-msgs branch November 21, 2022 20:52
@wep21
Copy link
Contributor

wep21 commented Nov 26, 2022

@JWhitleyWork Is it better to create new release?

@JWhitleyWork
Copy link
Collaborator Author

@JWhitleyWork Is it better to create new release?

Not yet. I need to actually add CAN-FD support to ros2_socketcan.

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.

7 participants