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

SPI.cpp sets MOSI idle to low #367

Open
drmcnelson opened this issue Aug 19, 2024 · 4 comments
Open

SPI.cpp sets MOSI idle to low #367

drmcnelson opened this issue Aug 19, 2024 · 4 comments
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project

Comments

@drmcnelson
Copy link

drmcnelson commented Aug 19, 2024

SPI.cpp sets MOSI idle state to low.

This is an error and it makes it incompatible with accepted convention and incompatible with many peripheral chips.

Please comment out the following lines.

/* set MOSI idle value to low */
sppcr |= R_SPI0_SPPCR_MOIFE_Msk;
_spi_ctrl.p_regs->SPPCR = (uint8_t) sppcr;

If you insist on leaving it like that then at least provide an API so it can be corrected by users.

@per1234 per1234 added type: imperfection Perceived defect in any part of project topic: code Related to content of the project itself labels Aug 20, 2024
@facchinm
Copy link
Member

Hi @drmcnelson ,
that code is community contributed and we merged since our tests didn't find any device misbehaving. However, you are right about this not being the expected SPI behaviour, so if you could submit a patch we'll retest and eventually merge the change.

@drmcnelson
Copy link
Author

My version of the library is at https://github.com/drmcnelson/Arduino_UNO_R4_SPI_Speedup

There is another very important issue that is also addressed there. There are two interrelated parts to the issue; (a) transfer16() is far too slow, and (b) MOSI (after the patch) still drops momentarily before the SPI clock starts.

For my ADC, dropping MOSI even momentarily, clears the data from the ADC before the clock starts. This happens in the beginning of transfer16() when it sets up the 16 bit transfer. See the MCP33131D datasheet.

(Normally the procedure is set CNVST high for 700 nsecs, then set CNVST low, and then call transfer16(). But MOSI drops at the beginning of transfer16() and clears the data.)

So, to solve this, and also speed up reading the ADC inside of a loop, I added two calls to the library, read16_setup() and read16_transfer(). There is still a lot of overhead in the transfer, but at least it works.

Regarding submitting the patch:

I tried to submit a pull request, but as I recall, I was stopped by the CLA manager. It made an unacceptable demand for access to private data.

@dquadros
Copy link

I was looking at the MCP33131D datasheet and in one of the figures SDI is just connected to DVio (the supply voltage) instead of the MOSI pin, that should solve this part of your problems. IMHO, SDI does not look to me like a SPI input pin, as no data is accepted by the MCP33131D and it affects operation even when there is no clock pulse.

@drmcnelson
Copy link
Author

Yes, you are right, that is the way I did it on all of the designs until now. And will go back to it for the next fab.

(I think the idea was to experiment with using it as an enable, but on further thought, I am not sure what the benefit would be.)

Anyway, the 16bit setup does not need to be inside the loop, and that is where it happens. So, that solves it for the moment also.

Here is the board, in case you are interested. Custom InAmp with the ADC lower left, +/-5V supply upper middle, Controller with T4 lower right.

I am developing a set of instrumentation boards and firmware, and testing each with Teensy 4, UNO R4 and Nucleo-144.

IMG_20240628_143452904 p600

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

No branches or pull requests

4 participants