Initial commit #1
Conversation
|
|
||
| trigger: | ||
| - develop | ||
| branches: |
There was a problem hiding this comment.
See this pipeline for example, it's up to date: https://github.com/nanoframework/nanoFramework.Azure.Devices/blob/develop/azure-pipelines.yml
The one you are using seems not the latest version.
| <Compile Include="Sockets\TcpListener.cs" /> | ||
| </ItemGroup> | ||
| <ItemGroup> | ||
| <Reference Include="mscorlib, Version=1.12.0.0, Culture=neutral, PublicKeyToken=c07d481e9758c731"> |
There was a problem hiding this comment.
You are missing something in the nuspec:
There was a problem hiding this comment.
Added the missing System.Net
| <tags>nanoFramework C# csharp netmf netnf nanoFramework.System.Net</tags> | ||
| <dependencies> | ||
| <dependency id="nanoFramework.CoreLibrary" version="1.12.0-preview.5" /> | ||
| <dependency id="nanoFramework.Runtime.Events" version="1.10.0-preview.6" /> |
There was a problem hiding this comment.
missing System.Net
Please use the latest yaml template, it will catch it automatically
| public Socket Client | ||
| { | ||
| get => _client; | ||
| set => _client = value; |
There was a problem hiding this comment.
why is there the possibility to bring your own Socket client? This should not be exposed publicly. Especially following the code and the path, client is always inialized while creation the TcpClient.
There was a problem hiding this comment.
OK, I checked the API in .NET and it's a get; set; so let it be a get; set; then :-)
There was a problem hiding this comment.
Changed to Auto property
| /// <returns>The underlying NetworkStream.</returns> | ||
| public NetworkStream GetStream() | ||
| { | ||
| //if (!_client.Connected) |
There was a problem hiding this comment.
Please remove dead code. If you keep dead code, please write why you keep it. In general, in classes like this one, there is no reason ;-) In samples, it can be handy but in all cases, a comment is needed.
There was a problem hiding this comment.
Will look at adding Connected property to Socket first
| /// <param name="port">The port number to which you intend to connect.</param> | ||
| public void Connect(IPAddress[] address, int port) | ||
| { | ||
| Connect(new IPEndPoint(address[0], port)); |
There was a problem hiding this comment.
Are you sure about the logic here?
I think it should be a foreach trying to connect up to the point you manage to.
There was a problem hiding this comment.
I did it that way as the Socket class normally does the connect using array
Added foreach loop
| } | ||
| else | ||
| { | ||
| Connect(new IPEndPoint(address, port)); |
There was a problem hiding this comment.
Is it the right logic here?
It seems to me that if the client us not null, you'll call again the same function again and again. I must miss something but something seems a bit weird in the logic all up.
There was a problem hiding this comment.
The break after Connect should take you out of For loop unless Connect fails
| { | ||
| private Socket _client; | ||
| private NetworkStream _stream; | ||
| private bool disposedValue; |
There was a problem hiding this comment.
_disposed
otherwise it does not follow the naming rules and the Value doesn't bring any value ;-)
| @@ -0,0 +1,21 @@ | |||
| MIT License | |||
There was a problem hiding this comment.
Please add a proper README.md as well with a usage.
Description
Initial commit of TcpClient & TCpListener
Motivation and Context
How Has This Been Tested?
Pending
Screenshots
Types of changes
Checklist: