search

tarm / serial

BSD 3-Clause "New" or "Revised" License
1.6k stars 453 forks source link

Added support for bytesize, stopbits, parity #34

Closed paocalvi closed 8 years ago

paocalvi commented 8 years ago

I try to add support for bytesize 5,6,7,8, parity, stopbits. I tested on windows and it seems to work. I don't know how to deal with the 1.5 stop bits, so I turn it on-the-fly to 2 I cannot compile the "posix" version so I can't guaranteed about that. Each new parameter is optional, so the library is retro-compatible and defaulted to 8N1, as requested by the author.

tarm commented 8 years ago

Hi Calvi,

Thanks for this PR! It's on my radar and I would like to merge it, but also want to do some testing on different platforms.

I hope to be able to do that in the next 2 weeks. I know that is a long time.

paocalvi commented 8 years ago

Don't worry. I tested that on Windows on a 4800, 8E1 serial porte and I discovered the bug which I corrected in my last commit. I hope I can test on Linux soon., also. Thanks for the great job.

Paolo

On Thu, Dec 10, 2015 at 7:09 AM, tarm notifications@github.com wrote:

Hi Calvi,

Thanks for this PR! It's on my radar and I would like to merge it, but also want to do some testing on different platforms.

I hope to be able to do that in the next 2 weeks. I know that is a long time.

— Reply to this email directly or view it on GitHub https://github.com/tarm/serial/pull/34#issuecomment-163508815.

tajtiattila commented 8 years ago

Thank you for this PR, I just realized I need these settings for my project.

Without this the serial package will reset the correct settings even if they were properly set by the user by other means (eg. MODE command on Windows).

I think DataBits should simply be a byte, there is no need for a new type. Just document the fact a zero value means 8 bits. The package-internal platform specific openPort function could then just fail with an error when an invalid value was present. Perhaps it should do just that for StopBits15 on linux until someone figures out how to do it right.

I would drop the "Type" bit from the new type names, and change the values to use CamelCase according to those in standard libs (eg. md5.BlockSize, os.PathSeparator). Furthermore they could be simply added to serial.go instead of having them in a separate file.

const DefaultSize = 8 // default value for Config.Size if it is 0

type StopBits byte
const (
    StopBits1 StopBits = iota
    StopBits15
    StopBits2
)

type Parity byte
const (
    ParityNone Parity = iota
    ParityOdd
    ParityEven
    ParityMark
    ParitySpace
)
tajtiattila commented 8 years ago

I added my own version based on the work from Paolo in commit 2a4c27de42eab481317d9621f389d26f41443649 to https://github.com/tajtiattila/serial.

tajtiattila commented 8 years ago

I added some changes to my fork, please feel free to comment.

Notable changes:

paocalvi commented 8 years ago

Great job. During next week I will try to convert my code (which is now based on my commit) to use yours, and I will test it on windows 115200 8N1 and 4800 8E1. Testing on linux will be somehow trick, for me.

On Sat, Dec 12, 2015 at 12:44 PM, Tajti Attila notifications@github.com wrote:

I added some changes to my fork, please feel free to comment.

Notable changes:

  • Parity and StopBits are not based on to the values in Windows anymore
  • Parity is still a byte, and is the first char of the parity name (of "NOEMS")
  • StopBits is 1 or 2 or 15, the constant Stop15 was renamed to Stop1Half
  • replaced some naked returns with arguments (naked returns being bad practice according to Brad/Andrew)
  • added Config.String() and ParseConfig() along with tests

— Reply to this email directly or view it on GitHub https://github.com/tarm/serial/pull/34#issuecomment-164140648.

tajtiattila commented 8 years ago

Thank you! I only have only an Arduino Uno for testing, 9600 Baud 8N1 worked with it.

One thing I'm not statisfied with in my version is the stop bit handling, and the actual constant names and values used. Basically I removed the 1.5 -> 2 stop bits conversion form linux/posix. This seems the correct thing to do, but is too restrictive. Perhaps one should be able to specify more than one stop bit (that is, 1.5 or more). Unfortunately all my knowledge of the serial port comes from Wikipedia.

paocalvi commented 8 years ago

Well, 1.5 stop bits is actually 2 stop bits when transmitting and 1 when receving. Apparently it is not supported in the linux/posix structure, since the port opening is done only once and you have to choose between 1 and 2.... Fortunately I guess nobody is using that anymore.... it is not a great loss. As the "mark"/"space" parities, think....

On Sat, Dec 12, 2015 at 9:00 PM, Tajti Attila notifications@github.com wrote:

Thank you! I only have only an Arduino Uno for testing, 9600 Baud 8N1 worked with it.

One thing I'm not statisfied with in my version is the stop bit handling, and the actual constant names and values used. Basically I removed the 1.5 -> 2 stop bits conversion form linux/posix. This seems the correct thing to do, but is too restrictive. Perhaps one should be able to specify more than one stop bit (that is, 1.5 or more). Unfortunately all my knowledge of the serial port comes from Wikipedia.

— Reply to this email directly or view it on GitHub https://github.com/tarm/serial/pull/34#issuecomment-164185665.

tarm commented 8 years ago

I agree with a most of the comments from @tajtiattila

Can you coordinate a single updated PR with him (not sure if you want to update this PR or use his branch)?

Also could you please squash the 3 commits into a single commit?

tarm commented 8 years ago

I merged the first 3 commits from https://github.com/tarm/serial/pull/38 which provide this same functionality. Thanks Calvi!