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

Adding missing functions #58

Merged

Conversation

anders-jakobsson
Copy link
Contributor

This PR adds all the cross-platform functionality that was missing from Wrapper, enabling MPSSE functionality for supported hardware. High level functions have also been added to LibFTD2XX. Tests are included for all added functions where it seemed meaningful, however, the FT_SetFlowControl hardware test could probably be expanded upon. Tests have been run against an FT4232H device on Windows 10.

I also threw in some extra blank lines between functions for enhanced readability, I hope you don't mind.

Disclaimer: I'm not a programmer by trade, and fairly new to Julia. This is also my first ever pull request. All in all you might want to check my additions extra carefully.

// Anders Jakobsson

@samuelpowell
Copy link
Contributor

@anders-jakobsson thank you for this PR, these additions are certainly very useful for hardware interfacing. I will review when I'm back in front of hardware.

@ReubenHill would you also be able to have a look over this?

@samuelpowell
Copy link
Contributor

@anders-jakobsson this isn't passing CI (nor local tests) because of a missing definition of USHORT. Was this working for you?

I don't mind a bit more whitespace but would prefer we don't go beyond two newlines as this is a reasonable balance between readability and navigability.

Otherwise looks good - will push changes to this branch shortly.

@codecov
Copy link

codecov bot commented Mar 7, 2020

Codecov Report

Merging #58 into master will decrease coverage by 8.27%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
- Coverage   68.53%   60.26%   -8.28%     
==========================================
  Files           3        3              
  Lines         232      307      +75     
==========================================
+ Hits          159      185      +26     
- Misses         73      122      +49
Impacted Files Coverage Δ
src/LibFTD2XX.jl 39.84% <0%> (-11.68%) ⬇️
src/wrapper.jl 72.83% <42.3%> (-6.48%) ⬇️

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 44c0d5f...e01f3e7. Read the comment docs.

src/wrapper.jl Outdated Show resolved Hide resolved
src/wrapper.jl Outdated Show resolved Hide resolved
@anders-jakobsson
Copy link
Contributor Author

@anders-jakobsson this isn't passing CI (nor local tests) because of a missing definition of USHORT. Was this working for you?

I don't mind a bit more whitespace but would prefer we don't go beyond two newlines as this is a reasonable balance between readability and navigability.

Otherwise looks good - will push changes to this branch shortly.

Interestingly, it did. That's strange. Only test that fails for me is the "locid==0 on windows" assertion, but that's a separate issue.

Anders Jakobsson added 2 commits March 9, 2020 11:19
@anders-jakobsson
Copy link
Contributor Author

Changes have been implemented. Also noticed that under Linux, at least for my device, no serial number is returned. This caused test failures so I conditioned the open-by tests in both wrapper and libftd2xx. This assumes that the error lies in the driver/device, not the julia wrapper, which seems reasonable.

Please have a new look.

@samuelpowell
Copy link
Contributor

Thanks @anders-jakobsson, any comments @ReubenHill ?

@anders-jakobsson
Copy link
Contributor Author

I will do something about the coverage. I'm just hoping for everything else to get squared away first.

@samuelpowell
Copy link
Contributor

@anders-jakobsson improved test coverage is of course nice, but it's really tricky to make it meaningful for a package like whit which involves interfacing with hardware. I'd like @ReubenHill to have a look over this before merge, but it otherwise looks good to me.

@samuelpowell samuelpowell merged commit 2901eea into Gowerlabs:master Mar 23, 2020
@samuelpowell
Copy link
Contributor

Thanks @anders-jakobsson will push a version to the registry shortly

@ReubenHill
Copy link
Collaborator

Sorry for not having responded sooner. I appear to have inadvertently switched off github's email alerts for this repo so only just saw all this. I see this has already been merged but will add some comments anyway:

  1. @anders-jakobsson It looks like the documentation for the wrapper functions have been taken verbatim from the programmers manual. I purposefully didn't do this since I believe that text is copyright to FTDI - @samuelpowell you're better on copyright than I am though!

  2. I'd have liked to see tests added for all the new functions - it looks like they are not all covered?

Otherwise thanks very much for your contribution @anders-jakobsson !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants