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

Support for CS active high #258

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Support for CS active high #258

wants to merge 5 commits into from

Conversation

tfnil
Copy link

@tfnil tfnil commented Jul 10, 2021

CS lines can now be active high. Ideas taken from
https://github.com/eblot/pyftdi/pull/86/files

Main difference is that we can individually select which CS lines that
shall be active high. This is done using a bitfield.

tfnil added 3 commits July 10, 2021 12:13
CS lines can now be active high. Ideas taken from
https://github.com/eblot/pyftdi/pull/86/files

Main difference is that we can individually select which CS lines that
shall be active high. This is done using a bitfield.
Property not needed - static bit mask better/faster.
@Yinameah
Copy link

Yinameah commented Aug 19, 2021

I have a proprietary device with active high CS line, and you PR gave me great service.

Thank you very much for your job. It would be nice to see this feature merged.

@tfnil
Copy link
Author

tfnil commented Aug 19, 2021

Glad to hear that someone found it useful.

I am in the same situation, I also need this for a proprietary design. I would also like to see it merged into the main project.

@eblot
Copy link
Owner

eblot commented Aug 19, 2021

Will do but I think a couple of changes are required. I lack time at the moment :-(

@tfnil
Copy link
Author

tfnil commented Aug 19, 2021

No worries. And thanks for the time you have put into the project.

Copy link
Owner

@eblot eblot left a comment

Choose a reason for hiding this comment

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

The main issue here is the CS initial bitmap meaning.
I think an iterable of CS lines to set high would be easier to use and less error prone, e.g.
cs_act_hi = [0, 2, 3] would tell to use CS0, CS2 and CS3 active high.
It should be cross-checked with the number of CS line (so an early error could be reported), and the cs_bits property would also return an iterable (list or set).

A minimal unit test would be nice as well, as least to check it does not break pre-existing implementation.

Thanks.

pyftdi/spi.py Outdated Show resolved Hide resolved
pyftdi/spi.py Outdated Show resolved Hide resolved
pyftdi/spi.py Outdated Show resolved Hide resolved
pyftdi/spi.py Outdated Show resolved Hide resolved
pyftdi/spi.py Outdated Show resolved Hide resolved
@tfnil
Copy link
Author

tfnil commented Aug 23, 2021

I agree that an iterable is better, good idea.
Not sure what you mean by cross-checking, do you mean check that we are not using non-CS-configured pins? This is done already.

I looked at writing a small unit test, but it looked like the existing ones were designed to run against a particular hardware configuration (expects to find chips at different CS locations). Maybe I misunderstood? I wasn't sure how to proceed. I will take another look.

tfnil added 2 commits August 24, 2021 21:51
More user friendly API for configuring active high CS lines. Can now
check the configuration, both on the controller and on each port.

Also add a minimal unit test that verifies we can set cs_act_hi and that
the settings seem to take effect.
We are using unittest, not pytest...
@tfnil
Copy link
Author

tfnil commented Aug 28, 2021

I pushed the fixes I made a few days ago. I am not familiar with the work flow on github, and I think it is not exactly intuitive... Am I expected to do something else?

I also just 'resolved' the comments you made, which seemed the right thing to do. Please bear with me, I am a noob. :)

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