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

samd21/i2c: add feedback in case transfer failed #3751

Merged
merged 1 commit into from
Sep 29, 2015

Conversation

daniel-k
Copy link
Member

According to the I2C interface the return value of any read or write function is either 'the bytes transfered' or -1 in case of undefined device.

With this PR those functions now return 0 in case of general failure, so the user/driver code can react accordingly.

@daniel-k daniel-k added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: drivers Area: Device drivers labels Aug 31, 2015
@biboc
Copy link
Member

biboc commented Sep 4, 2015

I definitely agree with PR, this is great that you had a look at it! I don't see any errors here.
Just why do we need to return -4 for a kind of error if we don't return it afterwards?
I mean, you wrote:

static inline int _write(SercomI2cm *dev, char *data, int length)
...
return -4;

And then

if(_write(i2c, (char *)(&reg), 1) < 0) return 0;

-4 will never be handled by the device driver. So I don't know if it is really useful to return different value for different errors. What's your opinion?

@daniel-k
Copy link
Member Author

daniel-k commented Sep 4, 2015

The Riot I2C interface doesn't provide detailed feedback in case of failure, but internally the driver has some knowledge about the failure. It was just that you could use this information maybe somewhere, but then it turned out that it's not the case. I don't have an opinion on this as long as the I2C interface doesn't provide more detailed feedback.

@biboc
Copy link
Member

biboc commented Sep 4, 2015

Ok so we should merge your PR and someone might want to edit the I2C driver and use those feedback values.
Will try it on Monday

@biboc biboc added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 11, 2015
@biboc
Copy link
Member

biboc commented Sep 11, 2015

Ok great! Let's wait for Travis and merge

@biboc
Copy link
Member

biboc commented Sep 11, 2015

@OlegHahm, may I merge this PR even if it's not for the release? It's a very small add.

@cgundogan
Copy link
Member

If I may put my two cents in, I'd say only bug fixes and discussed features should be included in the coming release. Looking at the number of PRs and issues that are still open and tagged for this release.. it is strangely growing

@OlegHahm
Copy link
Member

I agree with @cgundogan. Even if it's only a small patch, we have to cut the line somewhere. Hopefully, we will have the release by the end of next week.

@biboc
Copy link
Member

biboc commented Sep 16, 2015

Ok, will wait the release then

@LudwigKnuepfer
Copy link
Member

Related:
#3366

@PeterKietzmann
Copy link
Member

@bapclenet, feature freeze is over. We should find out why the build test fails for M0 group and merge the PR then.

@daniel-k
Copy link
Member Author

We should find out why the build test fails for M0 group

Doesn't fail anymore. Maybe some travis issue some time ago?

@PeterKietzmann
Copy link
Member

Ah nice. Well, as @bapclenet already ACKed I'm gonna hit the button now

PeterKietzmann added a commit that referenced this pull request Sep 29, 2015
samd21/i2c: add feedback in case transfer failed
@PeterKietzmann PeterKietzmann merged commit d081ade into RIOT-OS:master Sep 29, 2015
@biboc
Copy link
Member

biboc commented Sep 30, 2015

Thanks, @PeterKietzmann :-)

@OlegHahm OlegHahm added this to the Release NEXT MAJOR milestone Sep 30, 2015
@daniel-k daniel-k deleted the pr/samd21_i2c branch October 9, 2015 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants