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

Fix dropped bytes on spi write #11660

Merged
merged 2 commits into from Oct 15, 2019

Conversation

@shuopeng-deng
Copy link

shuopeng-deng commented Oct 9, 2019

Description

The cyhal_spi_send api was changed to read and discard a byte on every
send operation (at the protocol level all SPI transfers are bidirectional).
This means that to achieve a truly bidirectional transfer, the
cyhal_spi_transfer API must be called (as opposed to a write followed
by a read).

GreenTea_CY8CKIT_062S2_43012_GCC_ARM.txt
GreenTea_CY8CPROTO_062_4343W_GCC_ARM.txt

Pull request type

[ X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@ARMmbed/team-cypress

Release Notes

The cyhal_spi_send api was changed to read and discard a byte on every
send operation (at the protocol level all SPI transfers are bidirectional).
This means that to achieve a truly bidirectional transfer, the
cyhal_spi_transfer API must be called (as opposed to a write followed
by a read).
@ciarmcom ciarmcom requested review from maclobdell and ARMmbed/mbed-os-maintainers Oct 9, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Oct 9, 2019

@shuopeng-deng, thank you for your changes.
@maclobdell @ARMmbed/mbed-os-maintainers please review.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Oct 14, 2019
Copy link
Contributor

kjbracey-arm left a comment

This wouldn't be quite right if configured for > 8-bit transfers, but not sure this HAL (or even the Mbed OS core) correctly supports that case anyway. (There's a seperate SPI feature branch which I believe is looking at that, among other things).

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 14, 2019

This wouldn't be quite right if configured for > 8-bit transfers, but not sure this HAL (or even the Mbed OS core) correctly supports that case anyway.

Correct, master_write is for 8bit transfer only.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

kjbracey-arm commented Oct 14, 2019

Correct, master_write is for 8bit transfer only.

None of that is specified - some of the code suggests that the intent is that it work, and there's no reason it shouldn't. (SPI::format says width could be up to 16 bit, and passes SPI::write straight to spi_master_write).

The SPI feature branch drops that HAL API anyway, and sends it all through spi_transfer, which is expected to use 8/16/32-bit units in its buffer depending on format.

And tests it. I don't think any of this is tested on master.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 14, 2019

I was just checking the docs : Write a byte out in master mode and receive a value for spi_master_write() .
This is misleading as it really depends on format or init (from HAL perspective).

@shuopeng-deng Is this fix correct - cypress driver states it supports 8 or 16 bits (format function docs).
Reading the code, cyhal_spi_send() should be sufficient here - removing cyhal_spi_recv().

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

kjbracey-arm commented Oct 14, 2019

Reading the code, cyhal_spi_send() should be sufficient here

Don't think so - despite the name spi_master_write is bidirectional, and needs to return the value read, which cyhal_spi_send doesn't do.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 14, 2019

The proper fix would be then as it is, but also support 2 bytes transfer (based on bits set via format()). I recall there was already similar implementation for this in one of our target.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

kjbracey-arm commented Oct 14, 2019

You could basically cheat and rely on the little-endianness - just make both received and value_byte be uint16_t or uint32_t, so there's room for more bits. If the width is <= 8-bit, then cyhal_spi_transfer just accesses the bottom byte, which has the correct value.

(This trick working is why little-endianness is the one true endianness).

That's how SPI::write on the SPI feature branch works too.

remove an unnecessary cast
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 14, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 15, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit a806237 into ARMmbed:master Oct 15, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8648 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
}
if (CY_RSLT_SUCCESS != cyhal_spi_recv(&(spi->hal_spi), &received)) {
MBED_ERROR(MBED_MAKE_ERROR(MBED_MODULE_DRIVER_SPI, MBED_ERROR_CODE_FAILED_OPERATION), "cyhal_spi_recv");
int received;

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 15, 2019

Contributor

It occurred to me after writing my previous little-endian hack suggestion that you do need to initialise received to 0, as the transfer call will only fill in the bottom byte in the 8-bit case (or 2 bytes in 9-16 bit).

Can you please make that tweak?

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 21, 2019

@shuopeng-deng Please review above suggestion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.