-
Notifications
You must be signed in to change notification settings - Fork 177
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
Feature/csharp folder structure 2 #1616
Conversation
All C# related code is not in the subfolder `lang/csharp`. The folders are then separated based on the functionality. All related samples share the same CMakeLists.txt file, which makes it easier to add new samples.
…f.Person.Samples.Datatypes - Calculate C# protobuf version from available protoc.
set(minimal_rec_cs_src | ||
minimal_rec.cs | ||
) | ||
macro(ecal_add_csharp_core_sample sample_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe move this macro to /cmake/helper_functions/ecal_add_functions.cmake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to have any dependencies to the "outside", e.g. lang/csharp should be standalone...
{ | ||
string message = "HELLO WORLD FROM C#"; | ||
publisher.Send(message, -1); | ||
var received_message = subscriber.Receive(100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are publisher/subscriber are registered fast enough to establish a connection and exchange the test payload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the "sleep" is done beforehand.
|
||
set_target_properties(${PROJECT_NAME} PROPERTIES | ||
VS_GLOBAL_ROOTNAMESPACE ${PROJECT_NAME} | ||
macro(ecal_add_csharp_protobuf_sample sample_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe move this macro to /cmake/helper_functions/ecal_add_functions.cmake
|
||
set_target_properties(${PROJECT_NAME} PROPERTIES | ||
VS_GLOBAL_ROOTNAMESPACE ${PROJECT_NAME} | ||
VS_DOTNET_TARGET_FRAMEWORK_VERSION "v4.6.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can the installed dot net framework version be dectected programmatically instead of hard-coded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we can think about making it a CMake variable. But i would do it in a separate PR, in needs to be modified in a few places.
|
||
{ | ||
publisher.Send(person0); | ||
var person_rec_0 = subscriber.Receive(100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here with the connection timespan between pub and sub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No major findings - just a few cosmetic things. ;-)
Description
This changes the folder layout of the C# eCAL bindings.
They can be build 100% standalone against an eCAL installation.