Skip to content

Conversation

MichelRottleuthner
Copy link
Contributor

Contribution description

This adapts the ds1307 driver to the new i2c api. I don't have hardware to test.

Issues/PRs references

#6577

@MichelRottleuthner MichelRottleuthner added Area: drivers Area: Device drivers TF: I2C Marks issues and PRs related to the work of the I²C rework task force labels Jul 4, 2018
@MichelRottleuthner
Copy link
Contributor Author

Building tests/driver_ds1307 seems to fail because of a missing fix to embunit (#9242) which is not included in new_i2c_if branch. Is it safe to just cherry-pick the related commit to this PR?

@miri64
Copy link
Member

miri64 commented Jul 4, 2018

Don't have it in the office and have to see if I have it at home. Otherwise, I won't be able to test either :(.

@dylad
Copy link
Member

dylad commented Jul 4, 2018

Is it safe to just cherry-pick the related commit to this PR?

I guess you can do it, or open a separated PR into new_i2c_if, merge it and then rebased this branch on the new_i2c_if.

@miri64
Copy link
Member

miri64 commented Jul 4, 2018

I guess you can do it, or open a separated PR into new_i2c_if, merge it and then rebased this branch on the new_i2c_if.

We should rather just cherry-pick the fix for testing locally. Otherwise, the unittests get pulled into the embargo.

@MichelRottleuthner
Copy link
Contributor Author

hmm this indeed sounds bad I somehow thought this will be resolved by git because the commit would be exactly the same on both branches - but tbh I haven't actually tried if that would work...

@miri64
Copy link
Member

miri64 commented Jul 4, 2018

hmm this indeed sounds bad I somehow thought this will be resolved by git because the commit would be exactly the same on both branches - but tbh I haven't actually tried if that would work...

That should be the case, but one (highly unlikely, but still possible) follow-up change in master to embunit might make it not work anymore. We had a similar problem when refactoring gnrc_netif.

@miri64
Copy link
Member

miri64 commented Jul 4, 2018

Can't find the little thingy anymore btw :( so I'm not able to test.

@MichelRottleuthner
Copy link
Contributor Author

rebased to get rid of the unrelated cppcheck error

@MrKevinWeiss
Copy link
Contributor

So I think the adaptation to the new interface is correct according to the code review. @dylad would you like to merge and we put in on the "test once we have hardware" list?

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss 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!

@dylad
Copy link
Member

dylad commented Jul 9, 2018

@MrKevinWeiss let's move forward !

@dylad dylad merged commit 789b38f into RIOT-OS:new_i2c_if Jul 9, 2018
basilfx pushed a commit to basilfx/RIOT that referenced this pull request Jul 10, 2018
dylad added a commit to dylad/RIOT that referenced this pull request Jul 10, 2018
…_i2c

 drivers/ds1307: adapt to new i2c API
dylad added a commit that referenced this pull request Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants