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

Update hspi_slave.c #5489

Closed
wants to merge 5 commits into from
Closed

Update hspi_slave.c #5489

wants to merge 5 commits into from

Conversation

GuaAck
Copy link

@GuaAck GuaAck commented Dec 12, 2018

I found that SPI communication did not work due te a wrong timing of the MISO signal with respect to SCK. With slower processores like Arduino UNO (and a small delay of sck due to th level converter) it works, but with a faster Arduino DUE (without converter) it did not. Together with Jiri Bilek I found that MISODM_S has to be set. In the documentation for the ESP32 (should have a very similar SPI interface) I found that also MOSIDN_S should be set in SPI1C2. The MISO timing is correct, I tested it for a clock of 4 MHZ and 320 kHz. With experiments I found that also the timing of MOSI is robust. Neither a delay of 70 ns in SCK nor in MOSI did disturb the communication. Some oscillograms are here: http://homepage.o2mail.de/g.ackermann/ESP8266.htm

Jiri Bilek added the ICACHE_RAM_ATTR to the function hspi_slave_setStatus() because this function may be indirect called from an interrupt handler.

I found that SPI communication did not work due te a wrong timing of the MISO signal with respect to SCK. With slower processores like Arduino UNO (and a small delay of sck due to th level converter) it works, but with a faster Arduino DUE (without converter) it did not. Together with Jiri Bilek I found that MISODM_S has to be set. In the documentation for the ESP32 (should have a very similar SPI interface) I found that also MOSIDN_S should be set in SPI1C2.  The MISO timing is correct, I tested it for a clock of 4 MHZ and 320 kHz. With experiments I found that also the timing of MOSI is robust. Neither a delay of 70 ns in SCK nor in MOSI did disturb the communication. Some oscillograms are here: http://homepage.o2mail.de/g.ackermann/hspi_slave.htm

Jiri Bilek added the ICACHE_RAM_ATTR to the function hspi_slave_setStatus() because this function may be indirect called from an interrupt handler.
@JiriBilek
Copy link
Contributor

JiriBilek commented Dec 12, 2018

Adding a picture from a logic analyzer. In the first picture you can see the MISO signal changes on rising edges. This is bad because sampling is done in the very moment.

image

@devyte
Copy link
Collaborator

devyte commented Dec 14, 2018

@GuaAck @JiriBilek thank you for this, timing issues are difficult to find and resolve.
I think none of the currently active developers have the hw to test/verify this, so confirmation from others is needed. In the meantime, I have a few minor remarks, I'll explain inline with the code.

libraries/SPISlave/src/hspi_slave.c Outdated Show resolved Hide resolved
@@ -114,7 +114,7 @@ void hspi_slave_end()
SPI1P = B110;
}

void hspi_slave_setStatus(uint32_t status)
void ICACHE_RAM_ATTR hspi_slave_setStatus(uint32_t status) // GuaAck. 30.10.2018
Copy link
Collaborator

Choose a reason for hiding this comment

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

You provided an explanation for why this function should have the decorator. I suggest replacing the comment with that.
Also, I suggest inline.

Copy link
Collaborator

@devyte devyte Dec 18, 2018

Choose a reason for hiding this comment

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

Same here, please change the comment to add the explanation.

@devyte devyte added help wanted Help needed from the community good first issue If you want to help, this is is a good place to start and removed good first issue If you want to help, this is is a good place to start labels Dec 19, 2018
@JiriBilek
Copy link
Contributor

Hi, there is a problem with this PR.
The updated file hspi_slave.c is now put in wrong directory: libraries instead of libraries/SPISlave/src


void hspi_slave_end()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you delete hspi_slave_end function?

@@ -0,0 +1 @@
Dummy, should be deleted.
Copy link
Contributor

Choose a reason for hiding this comment

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

I hope you can delete the file in your repository and the deletion is then propagated to master.

@devyte
Copy link
Collaborator

devyte commented Jan 30, 2019

@Makuna is this PR relevant to your SPI testing?

@devyte
Copy link
Collaborator

devyte commented Feb 2, 2019

@JiriBilek @GuaAck what is the status here? I see a deleted function and an unneeded dummy file.

@Makuna
Copy link
Collaborator

Makuna commented Feb 2, 2019

This timing relationship of the data pulse to clock pulse is defined by the SPI mode. See WikiPedia SPI.
I could not find what the mode was supposed to be for SPI slave in this code. It is also not exposed to change like the standard SPI master
.

@GuaAck
Copy link
Author

GuaAck commented Feb 5, 2019

@JiriBilek @GuaAck what is the status here? I see a deleted function and an unneeded dummy file.

I have nothing to do with deleted functions and dummy files, Perhaps I did siomething by accident, Ia am very beginner in github.

With my changes, the SPI communication in my project works reliable for different clock rates. For me the matter is closed. Me and Jiri provided in github a I lot of notes and diagrams with measured wave forms . The owner of hspi_slave has to decide what to do.

I am open for any technical questions,
GuaAck

@Makuna
Copy link
Collaborator

Makuna commented Feb 5, 2019

@GuaAck @JiriBilek What SPI mode are your changes setting the SPI Slave to? When you tested with a SPI Master, did you test all the modes?

@GuaAck
Copy link
Author

GuaAck commented Feb 6, 2019

Yes, I tested the original hspi_slave with all modes (selected in the master), all failed.

The changed hspi_slave is only valid for mode 0. I found no switch to select the mode and mode 0 seems to be most common.

To remember: The problem came up, when I changed over from my UNO to the very much faster DUE. Then I found the incorrect timing.

Best regards
GuaAck

@devyte
Copy link
Collaborator

devyte commented Feb 6, 2019

@GuaAck with commit 01cf0e6 you deleted the end function, and the commit after that deleted the content of hspi_slave.c, leaving the empty file. I am therefore not sure if the PR has all changes necessary, or more changes than necessary (i.e. unneeded deletions).

@JiriBilek
Copy link
Contributor

@devyte, sorry haven't seen the PR for a while. I am afraid, @GuaAck made a mistake and moved some files to a bad directory. I'm gonna fix it in another PR soon as I have no more info from the OP.

@devyte
Copy link
Collaborator

devyte commented Apr 29, 2019

Closing in favor of #6022 .

@devyte devyte closed this Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants