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 support for half-duplex SPI to CPy #5990

Merged
merged 6 commits into from Feb 9, 2022
Merged

Conversation

sgauche
Copy link

@sgauche sgauche commented Feb 8, 2022

I have a need for half-duplex SPI on a custom board. I thought it would be worth adding to CircuitPython too. I have tested this on the hardware and it is working.

tannewt
tannewt previously requested changes Feb 8, 2022
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

I think it'd be good to add an explicit way to setup half duplex and document it in the matching portion of shared-bindings. I think it's best to provide an explicit half duplex bool. I'm worried that the majority of folks do want read to mean read from MISO and removing the exception will make it less clear what their coding error is.

It crossed my mind to make mosi == miso mean half duplex but I suspect that'd be a common copy and paste bug.

@sgauche
Copy link
Author

sgauche commented Feb 8, 2022

I did think about that... instead of using if MISO and MOSI are defined, just let the user specify SPI_DIRECTION_2LINES_RXONLY, SPI_DIRECTION_2LINES, or SPI_DIRECTION_1LINE via a parameter when calling spi configure like:
spi.configure(baudrate=5000000, com_mode=[full, half, simplex], phase=0, polarity=0)

Would something like that be possible?

There are 3 modes, full-duplex (normal 4-wire SPI), half-duplex (3-wire SPI with bidirectional data), and simplex (3-wire SPI with unidirectional data, RX only). Cpy was already set up to auto set SPI_DIRECTION_2LINES/full-duplex and SPI_DIRECTION_2LINES_RXONLY/simplex based on MISO/MOSO being defined. I don't think we can use a boolean, since there are 3 modes, not 2.

@sgauche
Copy link
Author

sgauche commented Feb 8, 2022

I looked at that but only saw ways to pass bools and uints through the configure parameters, didn't see a way to pass strings.

@sgauche
Copy link
Author

sgauche commented Feb 8, 2022

Now I'm realizing if we change the spi configure parameters, that touches all of the different ports, not just stm... hmm.

@dhalbert
Copy link
Collaborator

dhalbert commented Feb 8, 2022

Now I'm realizing if we change the spi configure parameters, that touches all of the different ports, not just stm... hmm.

Right, we need to change the common_hal routines even if it's not implemented.

I think a half_duplex boolean would be sufficient. You can complain if both pins are specified and half_duplex is true.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thanks! Minor style request.

ports/atmel-samd/common-hal/busio/SPI.c Outdated Show resolved Hide resolved
@sgauche
Copy link
Author

sgauche commented Feb 9, 2022

I'm guessing the too little flash failures are because I added mp_raise_NotImplementedError(translate("Half duplex SPI is not implemented"));. I could just do a mp_raise_NotImplementedError(Null);?

@dhalbert
Copy link
Collaborator

dhalbert commented Feb 9, 2022

Don't worry about these "too little flash" errors. I will push some commits that squeeze the builds that are overflowing. An unlabeled "NotImplementedError" is confusing. This happens all the time when we add functionality to tiny builds.

@sgauche sgauche changed the title Add support for half-duplex SPI on STM parts Add support for half-duplex SPI to CPy Feb 9, 2022
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thanks for your perseverance on this!

@dhalbert dhalbert merged commit da035fe into adafruit:main Feb 9, 2022
@sgauche sgauche deleted the stm_spi_3wire branch February 26, 2022 17:34
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.

None yet

3 participants