Skip to content

use uint8 instead of deprecated byte#13

Closed
mikaelarguedas wants to merge 1 commit intomasterfrom
deprecated_byte_char
Closed

use uint8 instead of deprecated byte#13
mikaelarguedas wants to merge 1 commit intomasterfrom
deprecated_byte_char

Conversation

@mikaelarguedas
Copy link
Copy Markdown
Member

don't use deprecated byte but uint8

connects to ros2/rosidl#190

@mikaelarguedas mikaelarguedas added the in progress Actively being worked on (Kanban column) label Dec 13, 2016
@codebot
Copy link
Copy Markdown
Member

codebot commented Dec 13, 2016

👍 lgtm

in an ideal world, we would be able to change the identifier as well, but I'm not sure that would be worth the problems it would cause going through the bridge, etc., so I think it's fine to leave the identifier as bytes_value.

@mikaelarguedas
Copy link
Copy Markdown
Member Author

I kept it as is so that people feel like they can pass hexadecimal values without worrying about sign.
This message being used only on the ROS2 side so we could change the field name without resulting in changes in the bridge but it seemed to be a less intrusive change (bytes are already treated as uint8 in the ROS2 code using parameters so it is transparent)

@codebot
Copy link
Copy Markdown
Member

codebot commented Dec 13, 2016

I agree, lgtm. 🐔

@mikaelarguedas mikaelarguedas added ready Work is about to start (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jul 17, 2017
@mikaelarguedas
Copy link
Copy Markdown
Member Author

this also relates to the upcoming C parameter implementation that may require this change and / or additional changes to the possible parameter types (e.g. arrays of parameters)

@clalancette
Copy link
Copy Markdown
Contributor

We haven't done this until now, so I'm going to venture to say we aren't going to do it. Because of that, I'm going to close this out, but feel free to reopen if you are going to pursue it in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready Work is about to start (Kanban column)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants