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

Feature serial port #1834

Merged
merged 10 commits into from
May 12, 2022
Merged

Conversation

raffaeler
Copy link
Contributor

@raffaeler raffaeler commented Mar 17, 2022

This is the initial work to create a new SerialPort class as for #1832.
The initial phase is the following:

  • Project structure
  • Basic definitions (enums, classes)
  • Messages definition
  • Interop (Platform Invoke)

More to come.

Microsoft Reviewers: Open in CodeFlow

@raffaeler raffaeler added enhancement New feature or request area-infrastructure labels Mar 17, 2022
/// In the Windows implementation, this is EV_RXFLAG.
/// </summary>
Eof = 0x02
}
Copy link
Member

Choose a reason for hiding this comment

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

Something to think about is a nothing of WatchChar we have introduced in nanoFramework: https://github.com/nanoframework/System.IO.Ports/blob/develop/System.IO.Ports/SerialData.cs.

See usage here: https://github.com/nanoframework/System.IO.Ports#case-of-watchchar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, this is substantially the same principle used in Win32 for watching a specific character.
Why did you rename the Eof=0x02 to WatchChar=0x02 in SerialData give they have the same meaning?
If I understand well this enum can be left as-is for compat reasons. Then we can add the WatchChar property to specify what is the character that will trigger the event.
Did I miss something?

@joperezr joperezr self-assigned this Mar 17, 2022
/// Both the Request-to-Send (RTS) hardware control and
/// the XON/XOFF software controls are used
/// </summary>
RequestToSendXOnXOff
Copy link
Contributor

@pgrawehr pgrawehr Mar 17, 2022

Choose a reason for hiding this comment

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

This enum should be reviewed. RequestToSendXOnXOff is probably never used (I can't think of a reason why you would use hw flow control and sw flow control at the same time), but instead RtsCts handshake (bidirectional full hardware flow control) should be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree, I also have never seen that mix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will have to futher discuss what handshakes we want to support because I am reading an old book on the serial port and there are plenty variations.

public enum Parity
{
/// <summary>
/// No parity check occurs.
Copy link
Contributor

Choose a reason for hiding this comment

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

This description is wrong. Right would be: No parity bit is sent or expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, as I said many descriptions from the official docs are inaccurate.

using System.Net.Sockets;
using System.Runtime.InteropServices;

using Microsoft.Win32.SafeHandles;
Copy link
Member

Choose a reason for hiding this comment

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

NIT: extra space.

using System.Device.Ports.SerialPort.Resources;
using System.IO;

using Marshal = System.Runtime.InteropServices.Marshal;
Copy link
Member

Choose a reason for hiding this comment

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

NIT: why do we need to use aliases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just got the code from the original SerialPort class, no particular reason.

@joperezr
Copy link
Member

Thanks for starting to work on this @raffaeler and sorry for the delayed review. I added some comments but feel free to only address what you think you should for this first iteration. I'm fine if you decide to address some of those changes in a follow up PR.

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

This looks good to me as a starting point. We should probably consider naming the library System.Device.Ports instead of System.Device.Ports.SerialPort, but no need to block the PR for that as we can continue to make progress and discuss naming and structure later in API Review.

@raffaeler raffaeler marked this pull request as ready for review May 10, 2022 19:27
@raffaeler
Copy link
Contributor Author

After a brief discussion with @joperezr , I implemented the factory pattern to avoid separate builds for each platform.
I pushed almost all the shared code while the platform specific is just a NotImplementedException.
Also, I added the CsWin32 code generator to automatically add the interop declarations for Windows.
Of course there is still a lot of work, but given this is a separate branch I will merge it to provide the opportunity for anyone to give feedback.

I will first start implementing the Windows communication to see if there is the need to change something.

Next "experimental step":

  • remove any read/write method from the serial port
  • define an interface for pluggable readers/writers (streams)
  • plug in a basic implementation for that interface exposing the read/write methods of the System.IO implementation.

The goal of the experimental step is to provide the following communication patterns without having to deal with the serial port specifics:

  • single stream reading/writing
  • separate read and write streams (as for @Ellerbach request)
  • packet-oriented communication
  • other kind of fixed/dynamic communication

{
/// <summary>
/// A character was received and placed in the input buffer.
/// In the Windows implementation, this is EV_RXCHAR.
Copy link
Member

Choose a reason for hiding this comment

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

I'd leave platform implementation details out of here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I just ported the class in the project to resolve other issues in the first place.

Comment on lines +204 to +208
if (value == _breakState)
{
return;
}
*/
Copy link
Member

Choose a reason for hiding this comment

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

can you write a comment why this is commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to add this comment:

In the old implementation SetBreakState is set every time using platform specific code for this reason, this code is commented but I left it here because the old behavior is still to be confirmed.

{
get
{
if (!_isOpen)
Copy link
Member

Choose a reason for hiding this comment

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

should we have similar check everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to see as soon as something starts working.
I am trying to port the same behavior of the SerialPort even if the code is different.

/// <summary>
/// Gets or sets the byte encoding for pre- and post-transmission conversion of text.
/// </summary>
public Encoding Encoding
Copy link
Member

@krwq krwq May 11, 2022

Choose a reason for hiding this comment

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

I'd prefer we didn't depend on encoding (i.e. remove all string related helper methods and encourage people to use string writer)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I wrote in my comment after the last push, I am willing to remove any read/write method from the SerialPort class.
Instead, I plan to add a sort of "plugin" (based on an interface) that allows to customize the typology of reads/writes.
For example Laurent asked to have separate streams for the read and write channels.

@joperezr joperezr merged commit 2dac8be into dotnet:feature-SerialPort May 12, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants