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

Add option to disable RTS on Windows #1277

Merged
merged 2 commits into from
Aug 6, 2017

Conversation

kealist
Copy link
Contributor

@kealist kealist commented Aug 3, 2017

Regarding #1276 I was trying to add this option. I am not familiar with getting grunt working, but I'll go ahead and push this and test it once I figure out how. Thanks

@serialport serialport deleted a comment from codecov-io Aug 4, 2017
@reconbot
Copy link
Member

reconbot commented Aug 4, 2017

(We don't use grunt?)

This looks good, will give it a test and then probably a merge! Thank you so much!

@kealist
Copy link
Contributor Author

kealist commented Aug 4, 2017

Regarding Grunt from Contributing.MD

Submitting Pull Requests

To contribute code to SerialPort, fork the project onto your github account and do your work in a branch. Before you submit the PR, make sure to rebase master into your branch so that you have the most recent changes and nothing breaks or conflicts. Lint and test your code using grunt. Also squash your commits to a reasonable size before submitting.

I can try to add the other options if I get a minute tonight.

@reconbot
Copy link
Member

reconbot commented Aug 4, 2017 via email

…omplete in terms of missing the XonChar, XoffChar, XonLim fields of DCB, but I am not as familiar with this aspect of serial comms.
@kealist
Copy link
Contributor Author

kealist commented Aug 5, 2017

I gave up on xany because there isn't anything that seems to directly map from termios to the Windows-side.

@serialport serialport deleted a comment from codecov-io Aug 6, 2017
@reconbot
Copy link
Member

reconbot commented Aug 6, 2017

@kealist we can have binding specific options that we can pass when you want to use them. It sounds like it would make sense to put xany there. I'm not sure how to elegantly handle the fact that option would exist two places.

This PR is good! I have a question about releasing it. This looks like it will change the default behavior.

In our open settings rtscts defaults to false, but we had hard coded to RTS_CONTROL_ENABLE which is true. I'm surprised this hasn't come up on my radar before as these are two very different modes. Is RTS not really important in most USB serial devices or something?

@reconbot reconbot merged commit 5b8d163 into serialport:master Aug 6, 2017
@kealist
Copy link
Contributor Author

kealist commented Aug 7, 2017

It is my understanding that USB-based traditional serial devices generally are less likely to use it, so I think disabled by default is probably better, but I'm not 100% sure.

@kealist
Copy link
Contributor Author

kealist commented Aug 7, 2017

In terms of node modules updating, will I see this patch if I update the serialport node module (i.e. does it pull code straight from github), or would I have to wait for an official release? I tried to find the answer but don't see anything clearly.

@reconbot
Copy link
Member

reconbot commented Aug 7, 2017

It's up to you. I'll have a beta release before the next official release where you can test this and a few other changes out. This change is major enough we'll have to do a 6x release.

You can install the latest stable via npm install serialport the latest beta via npm install serialport@beta and from github with npm install EmergingTechnologyAdvisors/node-serialport. You can find more info at npm's docs. https://docs.npmjs.com/cli/install#synopsis

@kealist
Copy link
Contributor Author

kealist commented Aug 7, 2017

Seems getting the build system up on this old netbook isn't working smoothly, so I'll have to wait for beta

reconbot added a commit that referenced this pull request Aug 7, 2017
### Bug Fixes

* **linux:** The productID should be a number not a description string ([#1279](#1279)) ([bf46f68](bf46f68))
* **tests:** fixup for [#1279](#1279) ([#1285](#1285)) ([56074f6](56074f6))
* **windows:** Add option to disable RTS ([#1277](#1277)) ([5b8d163](5b8d163))
* **windows:** Parse more types of pnpIds ([#1288](#1288)) ([0b554d7](0b554d7)), closes [#1220](#1220)


### Chores

* **binaries:** Lets switch to prebuild! ([#1282](#1282)) ([8c36e99](8c36e99))


### Features

* **test:** tone down codecov comments ([#1289](#1289)) ([749ffac](749ffac))


### BREAKING CHANGES

* **binaries:** I'm considering the switch to `prebuild` a breaking change because it's substantially changes our install processes. It's also possible the install flags to ensure downloading or building from source has changed slightly. That's not our api per say, but it's enough.
* **windows:** We previously hard coded to have RTS on for windows at all times it now default to off.
@reconbot
Copy link
Member

reconbot commented Aug 8, 2017

[email protected] released

@kealist
Copy link
Contributor Author

kealist commented Aug 8, 2017

Thank you for the release. I'm currently getting bogus data (write 0x8184 out the serial port, but it is spitting out 0xFEFE) Trying to make sure it's not something on my side (but it used to be correct with previous release)

Incorrect Data

@lock lock bot locked and limited conversation to collaborators Feb 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants