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

Matlab support, unicast udp, debug functionality for cpp #38

Closed
wants to merge 5 commits into from

Conversation

cd127
Copy link

@cd127 cd127 commented Oct 17, 2015

It includes mostly:

  • small bug fixes / code improvement
  • support for unicast udp
  • lots of added debug functionality for cpp
  • matlab support

It has a significant amount of changes, so please review the code carefully. I made sure it all compiles smoothly but could not test any further.

The commits are explained and well split into stand-alone features, so you could choose to reject any commit without any problem.

and the corresponding tests
There is a very-widely-used embedded platform (at least one)
which does not support multicast.
Having the choice to use unicast is very useful.
These changes have been extremely useful for us to test our
programs and eventually debug.  Having the possibility to print
any lcm object to string saved us a lot of time.
@ashuang
Copy link
Member

ashuang commented Oct 25, 2015

Hi Cedric,

Thanks for the contribution. It would be good to have a discussion about each of the feature additions on the lcm-dev mailing list before we can accept the pull request. In general, we strive very hard to keep the LCM API small to aid both its maintainability and its simplicity of usage.

Here are some general thoughts.

  • Supporting a new protocol (i.e., unicast UDP) in master is a very big change that I would only want to do if we can have consist support across all languages and platforms currently in the distribution. This is generally easy for the languages that wrap to the C implementation, but is harder for Java and .NET, as those use completely different backends. We've avoided supporting other protocols for this reason.
  • I support adding an equality comparison operator to the C++ type bindings, but it should be a true equality comparison operator, not one that uses an epsilon threshold for floats and doubles. That choice is too application specific to be embedded in the core bindings.
  • The debug string idea is a good one, but I'd prefer to do it in a way that's more generically useful. Specifically, it would be good to add type introspection to the C++ bindings so that users can easily convert to a string representation of their choosing. For example, with C++ introspection support it would be easy for someone to add their own function to output a message as valid JSON.
  • The C++ test object stuff doesn't belong in a core type. It's too specific to testing, and not generically useful enough to be in every single type definition.
  • Perhaps you could separate out the MATLAB commit and ask the folks on the lcm-dev thread for a review?

@ashuang ashuang closed this Jan 2, 2017
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.

2 participants