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.

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

cpu/nrf52: allow accessing multiple i2c peripherals #12372

Merged
merged 1 commit into from
Oct 4, 2019
Merged

cpu/nrf52: allow accessing multiple i2c peripherals #12372

merged 1 commit into from
Oct 4, 2019

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Oct 4, 2019

Contribution description

While trying to find out what was wrong with #10793, I found that the i2c driver was always returning the pointer on the first device. This PR is fixing this problem, since the driver should be able to handle multiple configured devices.

Testing procedure

Try this PR with #10793, the new configured devices should work with the saul application.

Issues/PRs references

Found while debugging #10793.

@aabadie aabadie added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: cpu Area: CPU/MCU ports labels Oct 4, 2019
benpicco
benpicco approved these changes Oct 4, 2019
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

I don't have a device to test atm, but this is such an obvious fix.

i2c_config[dev] is used everywhere else in the driver too.

@MrKevinWeiss
Copy link
Contributor

I am trying it with the thingy52 with #10793 but I am not getting any response when going through either testing procedure. I do however think this is a no brainer.

MrKevinWeiss
MrKevinWeiss requested changes Oct 4, 2019
View reviewed changes
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.

Still some funny stuff going on when testing with
USEMODULE=i2c_scan make BOARD=thingy52 -C tests/shell/ flash term
or
make BOARD=thingy52 -C tests/periph_i2c/ flash term

Though this makes sense does the cpu properly support multiple i2c devices?

Blocking until someone tells me I am crazy and shows some output of a working test on i2c_dev 0.

@aabadie
Copy link
Contributor Author

aabadie commented Oct 4, 2019 •
edited
Loading

Blocking until someone tells me I am crazy and shows some output of a working test on i2c_dev 0.

I'm setting up a board (nrf52840-dk) with 2 I2C sensors on 2 different buses.

@MrKevinWeiss MrKevinWeiss mentioned this pull request Oct 4, 2019
Merged
MrKevinWeiss
MrKevinWeiss approved these changes Oct 4, 2019
View reviewed changes
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.

The failure I was experience was for a different reason. I believe this is correct now.

ACK!

@MrKevinWeiss
Copy link
Contributor

Details in #10793

@MrKevinWeiss
Copy link
Contributor

@aabadie I leave it to you to click the merge button just in case you wanted to do or add anything else. However I am happy.

@aabadie
Copy link
Contributor Author

aabadie commented Oct 4, 2019

I made a custom setup with nrf52840dk, one sensor on one i2c bus (TWIM0), one sensor on a second I2C bus (TWIM1). I had to find the right pins combination (apparently not all pins can be used with TWIM).
On master it doesn't work: only one sensor is visible. With this PR, all sensors are visible.

I also tried i2c_scan and it reports the right addresses on the right bus. Auto-ACK.

Also thanks for testing @MrKevinWeiss !

@aabadie aabadie merged commit 825303c into RIOT-OS:master Oct 4, 2019
@aabadie aabadie deleted the pr/cpu/nrf52_multiple_i2c branch October 4, 2019 13:58
@MrKevinWeiss
Copy link
Contributor

Nice job 📯

@kb2ma kb2ma added this to the Release 2019.10 milestone Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@MrKevinWeiss MrKevinWeiss MrKevinWeiss approved these changes

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2019.10
Development

Successfully merging this pull request may close these issues.
4 participants
@aabadie @MrKevinWeiss @benpicco @kb2ma