Skip to content

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

Merged
aabadie merged 1 commit intoRIOT-OS:new_i2c_iffrom
MichelRottleuthner:pr_pn532_new_i2c
Jun 29, 2018
Merged

drivers/pn532: adapt to new i2c API (recreated)#9458
aabadie merged 1 commit intoRIOT-OS:new_i2c_iffrom
MichelRottleuthner:pr_pn532_new_i2c

Conversation

@MichelRottleuthner
Copy link
Copy Markdown
Contributor

@MichelRottleuthner MichelRottleuthner commented Jun 29, 2018

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

MichelRottleuthner commented Jun 29, 2018

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
Copy Markdown
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
Copy Markdown
Contributor

kaspar030 commented Jun 29, 2018

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

MichelRottleuthner commented Jun 29, 2018

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

@dylad
Copy link
Copy Markdown
Member

dylad commented Jun 29, 2018

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

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jun 29, 2018

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

Yes

@dylad
Copy link
Copy Markdown
Member

dylad commented Jun 29, 2018

Let's go then !

aabadie
aabadie reviewed Jun 29, 2018
View reviewed changes
drivers/pn532/pn532.c Outdated
#if ENABLE_DEBUG
#define PRINTBUFF printbuff
static void printbuff(char *buff, unsigned len)
static void printbuff(uint8_t *buff, unsigned len)
Copy link
Copy Markdown
Contributor

@aabadie aabadie Jun 29, 2018

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
Copy Markdown
Contributor Author

@MichelRottleuthner MichelRottleuthner Jun 29, 2018

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
Copy Markdown
Contributor

@aabadie aabadie Jun 29, 2018

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
Copy Markdown
Contributor Author

@MichelRottleuthner MichelRottleuthner Jun 29, 2018

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
Copy Markdown
Contributor

@aabadie aabadie Jun 29, 2018

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
Copy Markdown
Contributor

@aabadie aabadie Jun 29, 2018

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
Copy Markdown
Contributor Author

@MichelRottleuthner MichelRottleuthner Jun 29, 2018

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
Copy Markdown
Contributor

@aabadie aabadie Jun 29, 2018

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
Copy Markdown
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