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

Allow user to set default transfer byte for block read #4744

Merged
merged 2 commits into from Jul 24, 2017

Conversation

Projects
None yet
8 participants
@deepikabhavnani
Contributor

deepikabhavnani commented Jul 11, 2017

Description

Default 0x00 byte was transmitted for SPI read operations. SD card require default data to be 0xFF.
Added API for user to set the required default data byte for SPI read.

Status

Ready

Migrations

Default data was 0x00 for few targets, and default behavior has changed with this code change. Default now is 0xFF and user can set required value with API '_spi.set_default_write_value(data);'

Issue #4743

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 11, 2017

Looks like doxygen is not happy:

/home/travis/build/ARMmbed/mbed-os/drivers/SPI.h:147: warning: argument 'dummy' of command @param is not found in the argument list of SPI::dummy(char data)

/home/travis/build/ARMmbed/mbed-os/drivers/SPI.h:147: warning: The following parameters of mbed::SPI::dummy(char data) are not documented:

  parameter 'data'

/home/travis/build/ARMmbed/mbed-os/drivers/SPI.h:147: warning: argument 'dummy' of command @param is not found in the argument list of SPI::dummy(char data)

/home/travis/build/ARMmbed/mbed-os/drivers/SPI.h:147: warning: The following parameters of mbed::SPI::dummy(char data) are not documented:

  parameter 'data'
@theotherjimmy

Some grammar edits for the comments.

@@ -143,6 +143,14 @@ class SPI : private NonCopyable<SPI> {
*/
virtual void unlock(void);

/** SPI block read dummy data
* SPI requires master to send dummy data, in order to perform read operation.

This comment has been minimized.

@theotherjimmy

theotherjimmy Jul 11, 2017

Contributor

Edits in bold:
SPI requires the master to send dummy data during a read operation.
 

@@ -143,6 +143,14 @@ class SPI : private NonCopyable<SPI> {
*/
virtual void unlock(void);

/** SPI block read dummy data
* SPI requires master to send dummy data, in order to perform read operation.
* Dummy bytes can be different for devices. Example SD Card require 0xFF.

This comment has been minimized.

@theotherjimmy

theotherjimmy Jul 11, 2017

Contributor

Edits in bold:
Different devices may require different dummy byte values. For example: A SD Card requires dummy bytes to be 0xFF.

* SPI requires master to send dummy data, in order to perform read operation.
* Dummy bytes can be different for devices. Example SD Card require 0xFF.
*
* @param dummy Dummy character to be transmitted while read operation

This comment has been minimized.

@theotherjimmy

theotherjimmy Jul 11, 2017

Contributor

Edits in bold:
Dummy character byte to be transmitted while during a read operation

This should probably be

* @param data
@@ -127,11 +127,12 @@ int spi_master_write(spi_t *obj, int value);
* @param[in] tx_length Number of bytes to write, may be zero
* @param[in] rx_buffer Pointer to the byte-array of data to read from the device
* @param[in] rx_length Number of bytes to read, may be zero
* @param[in] dummy Dummy data transmitted while performing read

This comment has been minimized.

@theotherjimmy

theotherjimmy Jul 11, 2017

Contributor

Edits in bold:
Dummy data transmitted while performing a read

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 11, 2017

Travis is still unhappy:

/home/travis/build/ARMmbed/mbed-os/targets/TARGET_NXP/TARGET_LPC176X/spi_api.c:193:5: error: conflicting types for 'spi_master_block_write'

 int spi_master_block_write(spi_t *obj, const char *tx_buffer, int tx_length, char *rx_buffer, int rx_length) {

     ^

In file included from /home/travis/build/ARMmbed/mbed-os/targets/TARGET_NXP/TARGET_LPC176X/spi_api.c:19:0:

/home/travis/build/ARMmbed/mbed-os/BUILD/mbed/hal/spi_api.h:135:5: note: previous declaration of 'spi_master_block_write' was here

 int spi_master_block_write(spi_t *obj, const char *tx_buffer, int tx_length, char *rx_buffer, int rx_length, char dummy);
@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Jul 11, 2017

@theotherjimmy - Missed change in HAL behavior will break things for other devices.
@c1728p9 - Shall I add another API "spi_master_block_write" or update the code for all TARGETS?

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Jul 11, 2017

I would add another parameter to spi_master_block_write rather than adding a new function

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 12, 2017

I would add another parameter to spi_master_block_write rather than adding a new function

+1

@theotherjimmy - Missed change in HAL behavior will break things for other devices.

Always any extension to HAL needs to be carefully considered. The default value I would say should be 0xFF rather than 0x00, but that is detail. This needs to be extended to all targets.

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Jul 12, 2017

NXP devices have default dummy bytes 0x00, all other targets set it to 0xFFFF (SPI_FILL_WORD)
Changing default to 0xFF for NXP might break things for other SPI users, but since we are providing an API to set the value, 0xFF as default will be fine.

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Jul 13, 2017

@c1728p9 @0xc0170 Please review

drivers/SPI.h Outdated
*
* @param data Dummy character to be transmitted while read operation
*/
void dummy(char data);

This comment has been minimized.

@c1728p9

c1728p9 Jul 13, 2017

Contributor

Would this make more sense as default_write, write_fill or something like that? What do you think?

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Jul 13, 2017

Contributor

write_fill / tx_fill +1

This comment has been minimized.

@0xc0170

0xc0170 Jul 14, 2017

Member

set_default_write_value() or set_spi_dummy_value(), anything more verbose than dummy

This comment has been minimized.

@sg-

sg- Jul 16, 2017

Member

+1 for set_default_write_value()

@deepikabhavnani deepikabhavnani requested a review from geky Jul 13, 2017

@geky

geky approved these changes Jul 13, 2017

Assuming this passes CI (all implementations are updated) this looks good to me 👍

@sg-

In general, the spi block write was added 1 or 2 releases ago and DSPI_MasterTransferBlocking wasn't previously used. Is it not better to point fix this? dummy everwhere needs to be changed and probably should match async API which is SPI_FILL_WORD in the hal.

targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/drivers/fsl_dspi.c Outdated
{
assert(transfer);

uint16_t wordToSend = 0;
uint16_t wordReceived = 0;
uint8_t dummyData = DSPI_DUMMY_DATA;

This comment has been minimized.

@sg-

sg- Jul 13, 2017

Member

I would suggest not to modify an external driver like this. Chances are next update to this fill will revert the addition and this change will be deleted unless they take it into their SDK

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Jul 14, 2017

Contributor

@sg- the bug is in external driver, simplest solution was to change define value from 0x00 to 0xFF. This was done to have consistency. Either ways external driver needs modification

This comment has been minimized.

@sg-

sg- Jul 14, 2017

Member

I understand that but this addition to the code is less than a month old. Maybe its best to not use these SDK functions if they require internal modifications.

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Jul 14, 2017

Contributor

@sg- Addition to code was done by @geky to improve the SPI read/write performance. But I believe no one is using it still. In case of SD, issue is only during SPI read.
Options:

  1. Revert SPI read to byte reads in SD driver. Read performance will be affected.
  2. Report this bug to NXP, and change the define DSPI_DUMMY_DATA (Hoping they will change it in their SDK as well).
  3. Please suggest :-)

This comment has been minimized.

@sg-

sg- Jul 16, 2017

Member

2/3) revert k64f to use generic write loop like every other target. Get clarification from NXP on how this default write value is intended to be used. Its not very usable in current form.

@sg-

A few names and documentation updated needed. Also the PR title should be changed.

drivers/SPI.h Outdated
@@ -143,6 +143,15 @@ class SPI : private NonCopyable<SPI> {
*/
virtual void unlock(void);

/** SPI block read dummy data

This comment has been minimized.

@sg-

sg- Jul 19, 2017

Member

Need to update documentation (dummy not allowed)

hal/spi_api.h Outdated
@@ -127,11 +128,12 @@ int spi_master_write(spi_t *obj, int value);
* @param[in] tx_length Number of bytes to write, may be zero
* @param[in] rx_buffer Pointer to the byte-array of data to read from the device
* @param[in] rx_length Number of bytes to read, may be zero
* @param[in] dummy Dummy data transmitted while performing a read

This comment has been minimized.

@sg-

sg- Jul 19, 2017

Member

Documentation update (dummy not allowed)

targets/TARGET_Freescale/TARGET_KLXX/TARGET_KL46Z/spi_api.c Outdated
@@ -199,11 +199,12 @@ int spi_master_write(spi_t *obj, int value) {
return ret;
}

int spi_master_block_write(spi_t *obj, const char *tx_buffer, int tx_length, char *rx_buffer, int rx_length) {
int spi_master_block_write(spi_t *obj, const char *tx_buffer, int tx_length,
char *rx_buffer, int rx_length, char dummy) {

This comment has been minimized.

@sg-

sg- Jul 19, 2017

Member

Parameter name need to be updated (dummy not allowed)

targets/TARGET_Freescale/TARGET_KLXX/TARGET_KL46Z/spi_api.c Outdated
int total = (tx_length > rx_length) ? tx_length : rx_length;

for (int i = 0; i < total; i++) {
char out = (i < tx_length) ? tx_buffer[i] : 0xff;
char out = (i < tx_length) ? tx_buffer[i] : dummy;

This comment has been minimized.

@sg-

sg- Jul 19, 2017

Member

value name need to be updated (dummy not allowed)

targets/TARGET_Maxim/TARGET_MAX32620/spi_api.c Outdated
@@ -231,11 +231,12 @@ int spi_master_write(spi_t *obj, int value)
return spi_master_transaction(obj, value, MXC_S_SPI_FIFO_DIR_BOTH);
}

int spi_master_block_write(spi_t *obj, const char *tx_buffer, int tx_length, char *rx_buffer, int rx_length) {
int spi_master_block_write(spi_t *obj, const char *tx_buffer, int tx_length,
char *rx_buffer, int rx_length, char dummy) {

This comment has been minimized.

@sg-

sg- Jul 19, 2017

Member

Parameter name need to be updated (dummy not allowed)

targets/TARGET_Maxim/TARGET_MAX32620/spi_api.c Outdated
int total = (tx_length > rx_length) ? tx_length : rx_length;

for (int i = 0; i < total; i++) {
char out = (i < tx_length) ? tx_buffer[i] : 0xff;
char out = (i < tx_length) ? tx_buffer[i] : dummy;

This comment has been minimized.

@sg-

sg- Jul 19, 2017

Member

Value name need to be updated (dummy not allowed)

targets/TARGET_STM/stm_spi_api.c Outdated
@@ -387,12 +387,13 @@ int spi_master_write(spi_t *obj, int value)
}
}

int spi_master_block_write(spi_t *obj, const char *tx_buffer, int tx_length, char *rx_buffer, int rx_length)
int spi_master_block_write(spi_t *obj, const char *tx_buffer, int tx_length,
char *rx_buffer, int rx_length, char dummy)

This comment has been minimized.

@sg-

sg- Jul 19, 2017

Member

Parameter name need to be updated (dummy not allowed)

targets/TARGET_STM/stm_spi_api.c Outdated
{
int total = (tx_length > rx_length) ? tx_length : rx_length;

for (int i = 0; i < total; i++) {
char out = (i < tx_length) ? tx_buffer[i] : 0xff;
char out = (i < tx_length) ? tx_buffer[i] : dummy;

This comment has been minimized.

@sg-

sg- Jul 19, 2017

Member

Value name need to be updated (dummy not allowed)

@@ -33,6 +33,7 @@
#define SPI_EVENT_INTERNAL_TRANSFER_COMPLETE (1 << 30) // Internal flag to report that an event occurred

#define SPI_FILL_WORD (0xFFFF)
#define SPI_FILL_CHAR (0xFF)

This comment has been minimized.

@0xc0170

0xc0170 Jul 19, 2017

Member

Do we need this char? SPI supports 4-16 bit often, therefore we can use word, have word fill member to be 16-bit wide. Why did you decide to add this new value and use narrow type?

Looking at SPI class, all write support up to 16 bit, only the latest addition does not (buffers are type of char*) ?

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Jul 19, 2017

Contributor

'SPI::write' buffers are of char* type, and SPI SD driver support only 8-bit SPI (at present).
I was getting build warnings for both above cases.

SPI applications using block_writes will have build warning with "SPI_FILL_WORD" and to avoid having multiple defines / hardcoded values added generic define.

@deepikabhavnani deepikabhavnani changed the title from Allow user to set dummy transfer byte for block read to Allow user to set default transfer byte for block read Jul 19, 2017

@sg-

sg- approved these changes Jul 20, 2017

LGTM but I'd suggest we squash the commit history on merge.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 20, 2017

Once you rebase (clean the history), let us know please

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:spi_issue_4743 branch Jul 20, 2017

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Jul 20, 2017

@0xc0170 @sg- : Rebased and squashed commits.

@sg-

This comment has been minimized.

Member

sg- commented Jul 20, 2017

Can we rewrite the commit message? Allow user to set dummy tranfer byte for block read

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Jul 20, 2017

@sg- I left it intentionally that way as first commit, so that all other review comments and history make sense.

@sg-

This comment has been minimized.

Member

sg- commented Jul 20, 2017

works for me

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 21, 2017

/morph test

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Jul 21, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 21, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 860

Build failed!

Closed review comments
1. Doxygen and Grammar related
2. Change dummy to spi_fill
3. Remove NXP driver and add default loop in spi block read (same as all
other drivers)

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:spi_issue_4743 branch to 1b797e9 Jul 21, 2017

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Jul 21, 2017

/morph test

1 similar comment
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 24, 2017

/morph test

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Jul 24, 2017

@sg- @adbridge - can we get this patched to mbed OS 5.5, please? 5.5.4 preferrably.

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 24, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 868

All builds and test passed!

@theotherjimmy theotherjimmy merged commit 1f94ede into ARMmbed:master Jul 24, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@deepikabhavnani deepikabhavnani deleted the deepikabhavnani:spi_issue_4743 branch Jul 24, 2017

@JanneKiiskila JanneKiiskila referenced this pull request Jul 25, 2017

Merged

SPI block read fix #43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment