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 defragmentation capability and rosbridge_server_tcp #48

Closed

Conversation

dabertram
Copy link
Contributor

Feel free to give feedback to this pull request!

@rctoris
Copy link
Contributor

rctoris commented Jun 5, 2013

Not sure why the travis build failed. Looks like it couldn't find the travis.yml file even though its there? Not a problem though, we can ignore the failure since the tests were removed until we fix it.

Nice work!

@baalexander I'll leave the final merge up to you.

@baalexander
Copy link
Contributor

Thanks for the pull request and thanks for adding TCP support @ipa-fxm-db!

I'm going to pull in the changes and tweak a few minor things (comments, etc.) and test it all out tomorrow.

You have some TODO items in the code, are you planning on finishing those soon? If not, I'm going to move those tomorrow from code to GitHub issues.

@dabertram
Copy link
Contributor Author

Hi Brandon, 

please feel free to move the TODO items to github issues.
I added them because I think these are things that should be taken care of before defragmentation or tcp support can be seen as being 'done'.. 
If and/or when I start working on any of those I will let you know by answering to github issues.

@baalexander baalexander mentioned this pull request Jun 10, 2013
6 tasks
@baalexander
Copy link
Contributor

I pulled in your changes locally @ipa-fxm-db and tweaked a few things. The changes can be seen on my defragmentation branch. I mainly reorganized the comments, moved the todo items to a ticket, added a rosbridge_tcp launch file, and moved rosbridge_tcp to scripts.

Your code was targeted for fuerte-devel. Was this intentional? For my defragmentation branch, I cherry-picked your changes on top of our groovy-devel branch.

If merging this into groovy-devel is okay by you, do you mind adding unit tests to defragmentation.py? The code is complex enough I would feel safer with some testing in place.

If you make any changes, you can pull from my branch first to get my edits. You can do this by either cloning my repository and performing a git checkout defragmentation or adding my repository as a remote.

@dabertram
Copy link
Contributor Author

@baalexander: yes, the code was targeted for fuerte. I think it should work with groovy too.

I will think about adding some unit tests.. you're right, it has enought complexity for testing that way. especially the timeouts.. and also reconstruction depending on correct JSON-parsing etc..

I will update on this when I decide to write some tests.

@dabertram
Copy link
Contributor Author

I noticed an error in the line before the last one in defragmentation.py:
"self.received_fragments_for_msg_ID = None"

should be:
self.received_fragments = None

@baalexander
Copy link
Contributor

Thanks @ipa-fxm-db! If you want to keep targeting Fuerte-devel, you can cherry-pick my changes in the demonstrated branch. Or if you would like to target groovy-devel, you can use my branch as a starting point and we can work on back porting it to Fuerte when the tests are complete.

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.

None yet

3 participants