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

WIP: Custom baud rate refactor (linux) and testing (all platforms) #1464

Merged
merged 17 commits into from
Feb 5, 2018

Conversation

Fumon
Copy link
Contributor

@Fumon Fumon commented Feb 4, 2018

Forked from #1388, cleaned it up a bit and added more testing

The test environment didn't actually have a way to query the system to check the set baud rate so I added that to the bindings and rewrote those tests. Tested on linux with mock and arduino echo. Didn't have the build environment on windows to test but uses existing get interface on win32 so I can't imagine it doesn't work.

I added the GetBaudRate implementation for linux and windows but I have no mac hardware (or a paid dev licence) so I couldn't consult the Mac documentation for IOKit to add that in as well. This patch will break the test on darwin until it's implemented. If it can't be quickly added, I'll submit a new PR separate from the tests. I felt it was important to highlight that this functionality was broken on linux and the current tests didn't catch it.

@Fumon Fumon mentioned this pull request Feb 4, 2018
@codecov-io
Copy link

codecov-io commented Feb 4, 2018

Codecov Report

Merging #1464 into master will decrease coverage by 0.69%.
The diff coverage is 41.17%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1464     +/-   ##
=========================================
- Coverage   79.73%   79.04%   -0.7%     
=========================================
  Files          21       21             
  Lines         923      940     +17     
  Branches      169      170      +1     
=========================================
+ Hits          736      743      +7     
- Misses        187      197     +10
Impacted Files Coverage Δ
lib/bindings/win32.js 66.66% <0%> (-3.51%) ⬇️
lib/bindings/linux.js 66.1% <0%> (-3.55%) ⬇️
lib/bindings/darwin.js 65.51% <0%> (-3.58%) ⬇️
lib/bindings/mock.js 95.79% <100%> (+0.1%) ⬆️
lib/bindings/base.js 89.74% <80%> (-0.67%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1c5f75...0a8bf55. Read the comment docs.

@reconbot
Copy link
Member

reconbot commented Feb 5, 2018

This is awesome, this feature found it's champion. The getBaudRate() works for me, I wonder if we have a more generic settings object that we can get/set/open with begging to be created, but that's probably for another PR.

…json

The arbitrary baud interface has been working on darwin up to this point so it's
likely that it works properly. The GetBaudRate extension should still get
written to be thorough but for now, this will let the CI tests pass properly and
still throw a warning about the check being disabled.

That is, of course, if anyone's testing with the arduino hardware on darwin,
otherwise, these tests never run anyway.

Still not sure if these tests should be in integration?
@Fumon
Copy link
Contributor Author

Fumon commented Feb 5, 2018

All the tests are passing now. Sorry for the commit spam, needed the CI for windows and mac.

I separated the system level check in the testing harness on darwin so this should be good to go. Let me know if there are any style conventions or other issues.

I had the same thought about "GetBaudRate" being kinda clunky next to all the other single word get/set/open/list commands. I'd be willing to undertake rolling it into Get or possibly make a verbose "stat" call but that's, as you said, another PR.

@reconbot
Copy link
Member

reconbot commented Feb 5, 2018

No worries here, it's preferable to submit for feedback as people work. It also shows your thought process.

I don't read any issues with the code. I want to test it out on a few devices and then we can cut a beta and have people test on all the other crazy linux devices. When this goes to latest it will close #1388 #1362 and make the CNC crowd very very happy.

What did you run into when looking into OS X? Can you share any links or research?

@reconbot
Copy link
Member

reconbot commented Feb 5, 2018

Thank you for such a thorough PR, it's a nice to get one that hits all the marks (docs, tests, functionality, etc). If you don't mind I'll keep you in the loop on support requests around this code as people test it out.

Also happy to hear any feedback or feature requests. I have #1264 as a place to discuss the "roadmap".

Thanks!

@Fumon Fumon deleted the linuxBaudRateRefactor branch February 15, 2018 03:50
@Fumon
Copy link
Contributor Author

Fumon commented Feb 15, 2018

Sounds good.

WRT OS X: I found some more docs tonight while writing this reply. The main issue is that the custom baud rate setting on OSX that we're using (although I haven't found any alternative to using it yet since PySerial uses the same interface here's the line) does not update the termios struct with the new baudrate (cited here) and also does not have any additional ioctl's for retrieving baud rates (based on investigating this header and related files). However, based on the implementation of IOSSIOSPEED here it appears that the situation on Linux (where the previous hack was silently failing) is very unlikely to occur.

So in summary: There doesn't seem to be a way to retrieve the baud rate if it's set to something custom on Darwin, at least through the interface we're using. There could be other interfaces hiding in OS X but I haven't seen them.

IvanSanchez pushed a commit to NordicPlayground/node-serialport that referenced this pull request Feb 27, 2018
…rt#1464)

* 250k baudrate now works
* Generic linux non-standard baudrate setting using termios2
* Tested successfully with an FTDI usb chip with the tx and rx pins bridged. Previous solution using the B38400 hack returned gibberish in same setup.
* Test for linux and win32 baudrate setting. Darwin stubbed out.

The arbitrary baud interface has been working on darwin up to this point so it's
likely that it works properly. The GetBaudRate extension should still get
written to be thorough but for now, this will let the CI tests pass properly and
still throw a warning about the check being disabled.

That is, of course, if anyone's testing with the arduino hardware on darwin,
otherwise, these tests never run anyway.
oteku pushed a commit to Cutii/node-serialport that referenced this pull request Apr 9, 2018
…rt#1464)

* 250k baudrate now works
* Generic linux non-standard baudrate setting using termios2
* Tested successfully with an FTDI usb chip with the tx and rx pins bridged. Previous solution using the B38400 hack returned gibberish in same setup.
* Test for linux and win32 baudrate setting. Darwin stubbed out.

The arbitrary baud interface has been working on darwin up to this point so it's
likely that it works properly. The GetBaudRate extension should still get
written to be thorough but for now, this will let the CI tests pass properly and
still throw a warning about the check being disabled.

That is, of course, if anyone's testing with the arduino hardware on darwin,
otherwise, these tests never run anyway.
reconbot pushed a commit that referenced this pull request Jul 24, 2018
* 250k baudrate now works
* Generic linux non-standard baudrate setting using termios2
* Tested successfully with an FTDI usb chip with the tx and rx pins bridged. Previous solution using the B38400 hack returned gibberish in same setup.
* Test for linux and win32 baudrate setting. Darwin stubbed out.

The arbitrary baud interface has been working on darwin up to this point so it's
likely that it works properly. The GetBaudRate extension should still get
written to be thorough but for now, this will let the CI tests pass properly and
still throw a warning about the check being disabled.

That is, of course, if anyone's testing with the arduino hardware on darwin,
otherwise, these tests never run anyway.
@matthova
Copy link

@Fumon 🙌 awesome. I need to try this out this weeked :) Great PR!

@lock lock bot locked as resolved and limited conversation to collaborators Feb 6, 2019
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.

4 participants