Skip to content
This repository was archived by the owner on Jan 14, 2023. It is now read-only.

Using ChannelBuffer for int8[] types #72

Merged
merged 2 commits into from
Oct 1, 2018

Conversation

jubeira
Copy link

@jubeira jubeira commented Sep 20, 2018

I noticed that the changes in #65, messages with int8[] fields are converted to byte[] datatypes in Java messages, but all the message artifacts in rosjava_mvn_repo and the code in android_core and so on is still using ChannelBuffer for these cases.

Steps to reproduce: build rosjava ws from source, source the environment, and build android_core from scratch using catkin. catkin will use the generated message artifacts in the first step instead of using the artifacts in rosjava_mvn_repo, and the build will break when trying to use getData from messages containing byte[]. Error copied below:

src/org/ros/android/view/visualization/layer/CompressedOccupancyGridLayer.java:94: error: incompatible types: byte[] cannot be converted to ChannelBuffer
    ChannelBuffer buffer = message.getData();

I'm not saying that using byte[] is wrong, but it breaks the API for messages using this type of objects and therefore all the code using these messages.

This PR reverts the changes in #65 and attempts to fix the test by using ChannelBuffer instead of byte[] in RawMessage interface. I'm not 100% sure whether we should support byte[] in the set method.

Comments on this are welcome.

@drigz
Copy link
Member

drigz commented Sep 30, 2018

Thanks and sorry for the breakage! How much work do you think it would be to run that build with catkin in Travis to catch breakages like this in future?

@jubeira jubeira merged commit c5e124f into rosjava:kinetic Oct 1, 2018
@jubeira jubeira deleted the fix/channel_buffer branch October 1, 2018 12:24
@jubeira
Copy link
Author

jubeira commented Oct 1, 2018

Thanks for the review @drigz!

I think it is doable, but first it would be good to restore the failing tests. I'm a bit packed right now and I'll be OOO for most of October, so it'll be a bit complicated for me to do it in the near future.

Besides that, I don't have admin permissions for rosjava organization, so I can't setup a CI platform to work with these repos. Maybe I can do it in my fork and then port it here eventually?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants