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

Increase stm32 timeout for spi transfers #4305

Merged
merged 1 commit into from May 15, 2017

Conversation

Projects
None yet
4 participants
@LMESTM
Contributor

LMESTM commented May 11, 2017

Description

Default timeout of 10ms was reported in #4300

There seems to be conditions or use cases where the system is loaded with
higher priority tasks so that SPI transfer would be delayed more than 10ms.

Status

This PR is sent because of request for a quick fix and this should unblock the situation, nevertheless I am not sure that the problem is entirely understood.

Steps to test or reproduce

How to reproduce the issue was not explained in the ticket.
The patch was not tested and needs to be tested by reporters of the issue #4300

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 11, 2017

Why not to use HAL_MAX_DELAY ? I would say it should be better than anything less than that. As we could see 10 might work for some, might not for others. As this is a limit of API that we use there (timeout is required, or is it? cant be set to 0?), then we shall use the max value possible? (I havent looked at internals of that function how this timeout is used, I just noticed that max delay defined and used in some places).

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 11, 2017

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented May 12, 2017

@0xc0170 if using HAL_MAX_DELAY then there will be no timeout at all.
One application or driver may want to check if the SPI slave device is up and running, and in case it's not (communication timeout), it might trigger a power cycle of the device and start again. You can't do this without a timeout ...

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 12, 2017

I would agree with @0xc0170. With the current mbed HAL API there isn't a good way to convey timeout errors or to request one specifically.

@LMESTM LMESTM force-pushed the LMESTM:fix_increase_stm32_spi_timeout branch May 12, 2017

STM32 SPI do not use a timeout for spi transfers
Default timeout of 10ms was reported as an issue in #4300

There seems to be conditions or use cases where the system is loaded with
higher priority tasks so that SPI transfer would be delayed more than 10ms.
Recommendation from MBED team is to not implement any timeout at all as
there is no defined API in MBED to inform application of error cases.

@LMESTM LMESTM force-pushed the LMESTM:fix_increase_stm32_spi_timeout branch to 7d17532 May 12, 2017

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented May 12, 2017

I would agree with @0xc0170. With the current mbed HAL API there isn't a good way to convey timeout errors or to request one specifically.

One could argue that MBED API could be enhanced ...
Anyway, as we don't understand the root cause of the issue but you're asking for a fix anyway,
I do as I'm told and PR is updated.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 12, 2017

One could argue that MBED API could be enhanced ...

I absolutely agree on that point! 😄

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 12, 2017

Thanks @LMESTM . We will consider this feedback for future improvements

LGTM

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 12, 2017

/morph test

@0xc0170 0xc0170 added the needs: CI label May 12, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented May 12, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 197

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels May 12, 2017

@0xc0170 0xc0170 merged commit cb3531c into ARMmbed:master May 15, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment