Skip to content

drivers/pn532: adapt to new i2c API (recreated) #9458

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 29, 2018
Merged

drivers/pn532: adapt to new i2c API (recreated) #9458

merged 1 commit into from
Jun 29, 2018

Conversation

MichelRottleuthner
Copy link
Contributor

Contribution description

clone of #9432 - with all the changes addressed. Accidentally dleted the old branch and wasn't able to reopen the PR.

Issues/PRs references

#6577

@MichelRottleuthner MichelRottleuthner requested a review from dylad June 29, 2018 10:10
@dylad dylad added Area: drivers Area: Device drivers TF: I2C Marks issues and PRs related to the work of the I²C rework task force labels Jun 29, 2018
dylad
dylad approved these changes Jun 29, 2018
View reviewed changes
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, previous comments from @smlng are addressed. Let's wait for Travis and merge.

@dylad
Copy link
Member

dylad commented Jun 29, 2018

@MichelRottleuthner Travis is complaining, there are some styles issues.
You can fix and squash directly.

@haukepetersen haukepetersen mentioned this pull request Jun 29, 2018
Closed
47 tasks
@MichelRottleuthner MichelRottleuthner force-pushed the pr_pn532_new_i2c branch 2 times, most recently from 7f4c4a5 to d688375 Compare June 29, 2018 12:25
@MichelRottleuthner
Copy link
Contributor Author

the style issues must have been there berfore, added a (not so nice) fix anyway.
Btw. why are we using such an old version of cppcheck? my local version (1.83) was smart enough to don't see this as style error.

@dylad
Copy link
Member

dylad commented Jun 29, 2018

@MichelRottleuthner I have no idea... maybe @kaspar030 has an idea ?
btw, Travis is complaining again with whitespace issue.

@kaspar030
Copy link
Contributor

why are we using such an old version of cppcheck?

Murdock uses the ubuntu 16.04 version, 1.72. Travis seems to be even older. Maybe that can be updated? @aabadie?

@dylad
Copy link
Member

dylad commented Jun 29, 2018

Yeah, it would be great to update it so we don't have to add voodoo things here.

@aabadie
Copy link
Contributor

aabadie commented Jun 29, 2018

Maybe that can be updated?

I prefer dropping Travis and switch to CircleCI like I did in #8729. The advantage is that it uses the official docker image riotdocker so the tools versions will be same has the ones on Murdock.

@dylad
Copy link
Member

dylad commented Jun 29, 2018

I prefer dropping Travis and switch to CircleCI like I did in #8729

How do it works for this PR ?

@aabadie
Copy link
Contributor

aabadie commented Jun 29, 2018

How do it works for this PR ?

Revert the changes related to cppcheck and only keep the changes related to I2C. If Travis fails, it won't block the merge button anyway.

@MichelRottleuthner
Copy link
Contributor Author

reverted cpp changes. Is the upgrade to the CI planned before the i2c rework branch will be merged?

@dylad
Copy link
Member

dylad commented Jun 29, 2018

@aabadie you mean we can merge it right now without taking care of Travis ?

@aabadie
Copy link
Contributor

aabadie commented Jun 29, 2018

you mean we can merge it right now without taking care of Travis ?

Yes

@dylad
Copy link
Member

dylad commented Jun 29, 2018

Let's go then !

aabadie
aabadie reviewed Jun 29, 2018
View reviewed changes
drivers/pn532/pn532.c Outdated
@@ -80,7 +80,7 @@

#if ENABLE_DEBUG
#define PRINTBUFF printbuff
static void printbuff(char *buff, unsigned len)
static void printbuff(uint8_t *buff, unsigned len)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a leftover of changes related to cppcheck ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope - it's a slightly unrelated change but without this compiling with debug enabled will fail

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also revert this and open a separate PR. A least, it should not be part of the same commit containing I2C changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted. shall I open the other PR against new_i2c_if too? (master should work though)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be fine against master.

aabadie
aabadie reviewed Jun 29, 2018
View reviewed changes
drivers/pn532/pn532.c
#endif
}
else {
if (mode == PN532_SPI) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be PN532_I2C, no ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. the condition is used to init chip select pin (SPI)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see

@dylad
Copy link
Member

dylad commented Jun 29, 2018

@aabadie I'll let you push the button then :)

@MichelRottleuthner MichelRottleuthner mentioned this pull request Jun 29, 2018
Merged
@aabadie aabadie merged commit c012f0d into RIOT-OS:new_i2c_if Jun 29, 2018
basilfx pushed a commit to basilfx/RIOT that referenced this pull request Jul 10, 2018
@aabadie @basilfx
Merge pull request RIOT-OS#9458 from MichelRottleuthner/pr_pn532_new_i2c
e874f60 
drivers/pn532: adapt to new i2c API (recreated)
dylad pushed a commit to dylad/RIOT that referenced this pull request Jul 10, 2018
@aabadie @dylad
Merge pull request RIOT-OS#9458 from MichelRottleuthner/pr_pn532_new_i2c
134b36d 
drivers/pn532: adapt to new i2c API (recreated)
dylad pushed a commit that referenced this pull request Jul 11, 2018
@aabadie @dylad
Merge pull request #9458 from MichelRottleuthner/pr_pn532_new_i2c
e73c31d 
drivers/pn532: adapt to new i2c API (recreated)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabadie aabadie aabadie left review comments

@dylad dylad dylad approved these changes

Assignees
No one assigned
Labels
Area: drivers Area: Device drivers TF: I2C Marks issues and PRs related to the work of the I²C rework task force
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MichelRottleuthner @dylad @kaspar030 @aabadie