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

driver / RUDAT 13G 90 #1073

Merged
merged 21 commits into from
May 1, 2018
Merged

Conversation

sohailc
Copy link
Member

@sohailc sohailc commented Apr 25, 2018

Added a driver for the microcircuits RUDAT 13G 90 attenutator

@codecov
Copy link

codecov bot commented Apr 25, 2018

Codecov Report

Merging #1073 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1073   +/-   ##
=======================================
  Coverage   78.66%   78.66%           
=======================================
  Files          46       46           
  Lines        6478     6478           
=======================================
  Hits         5096     5096           
  Misses       1382     1382

@sohailc sohailc requested a review from Dominik-Vogel April 25, 2018 18:59
@WilliamHPNielsen WilliamHPNielsen changed the title added the RUDAT 13G 90 driver driver / RUDAT 13G 90 Apr 26, 2018
Copy link
Contributor

@Dominik-Vogel Dominik-Vogel left a comment

Choose a reason for hiding this comment

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

Great that you could implement the pywinusb library, like in PR #813. Yes, you are right, this version here is more condensed. The reason the original version in #813 is less condensed is, that it is more general in the sense that it provides a usb mixin class that should in principle be possible to use in all similar devices.
Your reimplementation also will run into problems once you have multiple devices of the same type.
I also noted that the IDN format is not conforming with the other devices.

@Dominik-Vogel
Copy link
Contributor

Looking at it again I think you should build on the old PR. You can simply push to that branch.

@sohailc
Copy link
Member Author

sohailc commented Apr 30, 2018

Actually, @Dominik-Vogel , I think I should keep the new PR. Your PR (#813) adds a driver for the rcdat_6000_60, which I do not have. I would either have to remove your driver or I'd have to push untested code, neither of which is desirable. I think we should merge this PR (once it is approved) and then in your branch you can use my mixin class.

@sohailc
Copy link
Member Author

sohailc commented May 1, 2018

Using a Mixin class did not work well with mypy. A similar situation is described here:
python/mypy#4001

I think the only solution is to make a subclass USBHIDInstrument and inherit from that.

@Dominik-Vogel
Copy link
Contributor

Dear @sohailc, I would prefer making a mixin and make an exception for mypy until the issue gets resolved. The reason for this is, that mypy is checking, a commodity feature, and the mixin is required to not repeat the same code: For minicircuits there exist quite a few devices that are ethernet and usb connectable. If you want to make a consistent driver you can implement all the same commands and then only add a mixin for the usb case.

@sohailc
Copy link
Member Author

sohailc commented May 1, 2018

Ok, I am using a Mixin, have another look, @Dominik-Vogel

Copy link
Contributor

@Dominik-Vogel Dominik-Vogel left a comment

Choose a reason for hiding this comment

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

I would suggest one last change:
the vendor_id and product_id should be set to some invalid value in the mixin and then get their current values in the RUDAT_13G_90_USB

@Dominik-Vogel Dominik-Vogel merged commit e2d224d into microsoft:master May 1, 2018
@sohailc sohailc deleted the RUDAT_13G_90_attenutator branch June 7, 2018 08:25
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.

2 participants