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

TCP/IP server support #296

Merged

Conversation

bernhard-thiele
Copy link
Collaborator

@bernhard-thiele bernhard-thiele commented Sep 17, 2019

PR for #295.

@bernhard-thiele bernhard-thiele added this to the v1.8.0 milestone Sep 17, 2019
@bernhard-thiele bernhard-thiele changed the title TCP/IP server support for Windows (#295) TCP/IP server support (#295) Sep 23, 2019
@bernhard-thiele bernhard-thiele changed the title TCP/IP server support (#295) TCP/IP server support Sep 23, 2019
@bernhard-thiele
Copy link
Collaborator Author

@tbeu The PR should be fine now and is ready to be merged in. I tested with Dymola on Windows and with OpenModelica on Windows and Linux. If you like you can check if everything works for SimulationX, too.

@tbeu
Copy link
Collaborator

tbeu commented Sep 24, 2019

Will do. What is the timeline for v1.8.0?

@bernhard-thiele
Copy link
Collaborator Author

The end of the first October week seems realistic (without STM32 support). Should maybe have a milestone "undecided" for features where the timeline is very unclear.

Copy link
Collaborator

@tbeu tbeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rename

  • MDD_TCPIPServer_read -> MDD_TCPIPServer_Read
  • MDD_TCPIPServer_readP -> MDD_TCPIPServer_ReadP
  • MDD_TCPIPServer_send -> MDD_TCPIPServer_Send
  • MDD_TCPIPServer_sendP -> MDD_TCPIPServer_SendP

to be consistent with the other implementations.

Copy link
Collaborator

@tbeu tbeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend to make TCPIPServerConfig.tcpipserver protected, too. And I believe the output is not needed here.

Copy link
Collaborator

@tbeu tbeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also tested, that it still works with Cygwin.

@bernhard-thiele
Copy link
Collaborator Author

Thanks for the review and the fixes!

Unfortunately, I didn't manage to finalize the release last week, there were unplanned issues which reduced my free time. So hopefully I'll release something until the end of this week.

I'm not sure regarding the proposed renaming of the functions. The naming is not completely consistent in the library. We have a consistent prefix MDD_, after that there is usually a string denoting the "class", e.g., MDD_udpSend. But sometimes the "class" is lowercase, sometimes upper case, e.g., MDD_SharedMemoryRead, sometimes there is a _ as delimiter starting with lower case, e.g., MDD_avr_rt_init, or with uppercase MDD_TCPIPClient_Send. I prefer lower case for the "function" part (if using the _ delimeter), but have no strong opinion about it. Since the functions in MDD_TCPIPClient already use upper case, we can do the same for MDD_TCPIPServer.

@bernhard-thiele bernhard-thiele merged commit 18b8396 into modelica-3rdparty:master Dec 4, 2019
@bernhard-thiele bernhard-thiele deleted the feature/TCPIPServer3 branch December 4, 2019 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants