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

_acquire() function added & removed duplication in format/freq calls #4635

Merged
merged 3 commits into from Jul 7, 2017

Conversation

Projects
None yet
8 participants
@deepikabhavnani
Contributor

deepikabhavnani commented Jun 26, 2017

Description

  1. Private _acquire() function is added to avoid multiple locking/unlocking
  2. format and frequenct functions updated to use appropriate function calls
    instead of a aquire()

Issue

#4626

Status

READY

@deepikabhavnani deepikabhavnani requested review from geky and 0xc0170 Jun 26, 2017

@geky

geky approved these changes Jun 26, 2017

Seems fine to me

_acquire() function added & no duplication in format/freq calls
1. Private _acquire() function is added to avoid multiple locking/unlocking
2. format and frequency functions updated to use appropriate function calls
instead of a aquire()

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:spi_acquire branch to de89be3 Jun 26, 2017

SPI::_owner = NULL; // Not that elegant, but works. rmeyer
aquire();
spi_frequency(&_spi, _hz);
_owner = this;

This comment has been minimized.

@c1728p9

c1728p9 Jun 26, 2017

Contributor

The owner can be changed here without ensuring that both frequency and format are set. The format setting maybe be incorrect on the next write since it was not updated.

A sequence similar to this would cause that:

spi_a.write(value_a)                 <-Internal call to _acquire will change owner and set frequency and format for SPI A
spi_b.frequency(freq_b)              <- Owner changed to B and frequency set
spi_b.write(value_b)                 <- Interal call to _acquire won't change setting. Frequency will be set correctly for B and format will be set for A
@c1728p9

Thanks for updating this

@@ -270,6 +270,12 @@ class SPI {
int _bits;
int _mode;
int _hz;
private:
/* Private acquire fucntion without locking/unlocking

This comment has been minimized.

@0xc0170

0xc0170 Jun 28, 2017

Member

typo fucntion

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 28, 2017

A question, now we use _acquire private method. The old one (aquire) is without a change . How does this affect current programs? I don't see any virtual for aquire so should not have any impacts, correct?

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Jun 29, 2017

Private _acquire has no locking, but since we lock before using this function behavior will be same. We do not have virtual function for aquire in code base, but some user might be having it, hence old aquire API is kept unchanged.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 29, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 29, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 697

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jun 30, 2017

@adbridge adbridge merged commit 28df3ae into ARMmbed:master Jul 7, 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

@sg- sg- removed the ready for merge label Jul 7, 2017

@deepikabhavnani deepikabhavnani deleted the deepikabhavnani:spi_acquire branch Jul 7, 2017

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