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.

Sign up for GitHub

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

Jump to bottom

Implement asynchronous SPI interface #15

Merged
merged 7 commits into from
Oct 12, 2015
Merged

Implement asynchronous SPI interface #15

merged 7 commits into from
Oct 12, 2015

Conversation

salkinium
Copy link

Adds the functions as defined by the asynchronous interface in mbed-hal.
Successfully tested using the asynch-spi unittest in mbed-hal.

It uses interrupts for now (3 interrupts per transferred byte).

These changes rely on ARMmbed/mbed-hal-st-stm32f429zi#13.

@bremoran @bogdanm @0xc0170

@niklarm
Refactor SPI driver to use the asynchronous spi_s struct.
405c085 
Each SPI driver now has its own static Handle which allows for
resource control for multiple accesses as required by the current
asynchronous API.
@niklarm
Add implementation of asynchronous SPI API.
60ef131
@niklarm
SPI: Add correct handling of rx_size != tx_size.
b10b972 
Also add debug output.
@miklis
Copy link

miklis commented Oct 8, 2015

Build finished. No test results found.

1 similar comment
@miklis
Copy link

miklis commented Oct 8, 2015

Build finished. No test results found.

@salkinium
Copy link
Author

Also please note that I only added the asynch APIs, but did not fundamentally fix the problem inherited by the synchronous API, which are mostly related to providing safe mutual exclusion for multi-access.

Specifically, the current implementation of SPI (well, really any API of the entire HAL) is not safe for true asynchronous operation.
For example, calling spi_init() after a transfer has been started with spi_master_transfer() will reset the spi master and invalidate the transfer.
Similarly, interoperability between the synchronous and asynchronous API is not possible.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 9, 2015

Also please note that I only added the asynch APIs, but did not fundamentally fix the problem inherited by the synchronous API, which are mostly related to providing safe mutual exclusion for multi-access.

No worries, we won't hunt you for that one. As noted, we are not there yet.

Can you please fix formating with these new changes, reason - consistency

0xc0170
0xc0170 reviewed Oct 9, 2015
View reviewed changes
source/spi_api.c
else {
// everything is ok, nothing else needs to be transferred
event = SPI_EVENT_COMPLETE | SPI_EVENT_INTERNAL_TRANSFER_COMPLETE;
// somehow this is needed, dunno why.
Copy link
Contributor

Choose a reason for hiding this comment

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

deprecated comment?

@bogdanm
Copy link
Contributor

bogdanm commented Oct 9, 2015

It uses interrupts for now (3 interrupts per transferred byte).

Could you please ellaborate a bit on this? Why 3 interrupts?

@salkinium
Copy link
Author

I'm still a little confused by there are 3 interrupts per byte.

I can reason for 2 interrupts per byte for TXRX transfers, because of the TxEmpty and RxNotEmpty interrupts.
For TX only, I measured 1 interrupt, but for RX its also 2 interrupts, because:

@0xc0170 just now found this (undocumented) gem in the Cube library in the HAL_SPI_Receive_IT function: The Cube library actually sends the receive buffer during reception (!).

In the unittest I memset the rx buffer and therefore I saw zeros in the tx line, instead of 0xff, which is the SPI_FILL_WORD.
This lead to the workaround here, where I tx one 8/16bit word 0f 0xff(ff) per received byte.
I will try to memset the rxbuffer to 0xff before starting the "reception".

For the tx only part, I now believe that I prematurely disabled the interrupt here, and therefore disallowed the tx-only configuration in Cube to properly unconfigure its internal states, and therefore it just continued the previous transfer on the next transfer.
This lead to this workaround, which costs 2 interrupts per byte.

@niklarm
SPI: Fix SPI transmit line fill words when receiving.
8f72c9c 
Fix length calculation for 16bit transfers.
Do not disable interrupt prematurely.
@miklis
Copy link

miklis commented Oct 9, 2015

Build finished. No test results found.

1 similar comment
@miklis
Copy link

miklis commented Oct 9, 2015

Build finished. No test results found.

@niklarm
SPI: Individualism stops at the coding style level.
96cbb8b 
"Conform" they whispered.
"Conform" they chanted.
"Conform" they wanted.
@miklis
Copy link

miklis commented Oct 9, 2015

Build finished. No test results found.

1 similar comment
@miklis
Copy link

miklis commented Oct 9, 2015

Build finished. No test results found.

@salkinium
Copy link
Author

Ok, so I've fixed the reception, it is now much more efficient.

Interrupts per byte:

  • Tx & Rx: 1
  • Rx only: 1
  • Tx only: 2

I still didn't find a fix for using the HAL_SPI_Transmit_IT function, so the workaround for that costs 2 interrupts per byte.

I now also "conform" to coding style.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 9, 2015

Looks good to me.

Unknown state for HAL_SPI_Transmit_IT - Shall we leave TODO there to remind us to look at this (or even a issue), https://github.com/ARMmbed/mbed-hal-st-stm32f4/pull/15/files#diff-bc7df4243a454d3629b0f7efffec53f7R461

@miklis
Copy link

miklis commented Oct 9, 2015

Build finished. No test results found.

1 similar comment
@miklis
Copy link

miklis commented Oct 9, 2015

Build finished. No test results found.

@miklis
Copy link

miklis commented Oct 9, 2015

Build finished. No test results found.

1 similar comment
@miklis
Copy link

miklis commented Oct 9, 2015

Build finished. No test results found.

0xc0170 added a commit that referenced this pull request Oct 12, 2015
@0xc0170
Merge pull request #15 from salkinium/feature/async_spi
06838a1 
Implement asynchronous SPI interface
@0xc0170 0xc0170 merged commit 06838a1 into ARMmbed:master Oct 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@salkinium @miklis @0xc0170 @bogdanm @niklarm