cpu/nrf52: optimized I2C driver by haukepetersen · Pull Request #8385 · RIOT-OS/RIOT

haukepetersen · 2018-01-19T21:26:14Z

Contribution description

This PR applies some enhancements to the I2C driver for the nrf52. It mainly consists of some minor fixes and a little re-structuring for better code re-usage. Further the driver is now using assertions for all API functions.

If I saw it correctly, the previous version of the driver had also a problem as it was not using a repeated start condition when reading register(s), this is also fixed now.

Issues/PRs references

none

PeterKietzmann

In which setup did you test this? I only gave it a quick try without logic analyzer (nrf52dk + srf02 with tests/driver_srf02). Initialization of the device seems successful. Single shot or periodic sampling directly hangs

PeterKietzmann · 2018-01-31T09:25:19Z

cpu/nrf52/periph/i2c.c

-
-    /* shortcuts configuration */
-    dev(bus)->SHORTS = (TWIM_SHORTS_LASTTX_STOP_Enabled << TWIM_SHORTS_LASTTX_STOP_Pos) |
-                       (TWIM_SHORTS_LASTRX_STOP_Enabled << TWIM_SHORTS_LASTRX_STOP_Pos);


Don't understand why you remove this initialization.

The shorts are set on-demand in the transfer functions...

PeterKietzmann · 2018-01-31T09:27:46Z

cpu/nrf52/periph/i2c.c

-    /* enable the device */
-    dev(bus)->ENABLE = (TWIM_ENABLE_ENABLE_Enabled << TWIM_ENABLE_ENABLE_Pos);
+    /* configure bus clock speed */
+    dev(bus)->FREQUENCY = speed;


You removed the shift because TWIM_ENABLE_ENABLE_Pos is 0 right?

PeterKietzmann · 2018-01-31T09:30:48Z

cpu/nrf52/periph/i2c.c

 int i2c_acquire(i2c_t bus)
 {
-    assert(bus <= I2C_NUMOF);
+    assert(bus < I2C_NUMOF);


Should have seen this before :/ ...

PeterKietzmann · 2018-01-31T09:36:18Z

cpu/nrf52/periph/i2c.c


-    /* start read sequence */
-    dev(bus)->EVENTS_STOPPED = 0;
+    dev(bus)->ADDRESS = addr;


Why did you remove the 7-bit-address mask?

because this way we save one redundant instruction. The ADRESS register does only use the 7 LSB anyway, so no need for manual masking the address.

haukepetersen · 2018-01-31T10:41:58Z

In which setup did you test this? I only gave it a quick try without logic analyzer (nrf52dk + srf02 with tests/driver_srf02). Initialization of the device seems successful. Single shot or periodic sampling directly hangs

I tested this mainly with the lis2dh12 and the bmx280 sensors on the ruuvitag. I will also take a look at the srf02 to see whats wrong...

haukepetersen · 2018-01-31T10:43:21Z

rebased (before I start debugging the srf02...).

PeterKietzmann · 2018-01-31T10:44:15Z

Thanks for your feedback!

haukepetersen · 2018-01-31T10:45:35Z

Thanks for your review!

haukepetersen · 2018-01-31T11:21:46Z

found the problem: The I2C driver expected the acquire function to be called after the init function, while the srf02 driver expected the order the other way around (see #8446 for this issue). So I now fixed the I2C driver to re-enable the i2c device in the end of init_master(). I also reabsed this PR on top of #8486, so the mutex initialization is fixed.

I will also look at the drivers for lis2dh12 and the bmx280, to make sure they use the correct acquire -> init_master order...

PeterKietzmann · 2018-01-31T16:15:38Z

Great, works now. I'm fine with this PR. Let's wait for #8486 and rebase/squash then

haukepetersen · 2018-02-01T11:38:37Z

Perfect. Would you mind to ACK #8486?!

PeterKietzmann · 2018-02-02T09:54:16Z

@haukepetersen please rebase

This function is supposed to be used for putting the CPU into sleep mode for short amounts of time (e.g. in typical polling loops used in periph drivers).

haukepetersen · 2018-02-02T10:34:11Z

rebased

haukepetersen · 2018-02-02T10:34:57Z

and squashed.

PeterKietzmann · 2018-02-02T11:22:19Z

Travis CI fails due to previously set "Waiting For Other PR" flag. How is this machine restarted, only by a new commit?

haukepetersen · 2018-02-02T11:25:18Z

As Travis is not needed for a final verdict, I'd say we simply ignore it in this case.

aabadie · 2018-02-02T11:25:36Z

Travis CI fails due to previously set "Waiting For Other PR" flag. How is this machine restarted, only by a new commit?

This needs to be restarted manually. It would be good to remove this check from travis.

haukepetersen added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation 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 labels Jan 19, 2018

haukepetersen assigned PeterKietzmann Jan 19, 2018

haukepetersen mentioned this pull request Jan 23, 2018

drivers/lis2dh12: add I2C mode #8419

Closed

PeterKietzmann reviewed Jan 31, 2018

View reviewed changes

haukepetersen force-pushed the opt_nrf52_i2c branch from 0028118 to 27479aa Compare January 31, 2018 10:43

haukepetersen force-pushed the opt_nrf52_i2c branch from 27479aa to ef6bed1 Compare January 31, 2018 11:19

haukepetersen added the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 31, 2018

haukepetersen added 2 commits February 2, 2018 11:33

cpu/nrf52: updated vendor header

959e838

cpu/nrf52: add CPU specific sleep function

787d69c

This function is supposed to be used for putting the CPU into sleep mode for short amounts of time (e.g. in typical polling loops used in periph drivers).

haukepetersen force-pushed the opt_nrf52_i2c branch from ef6bed1 to 3813f88 Compare February 2, 2018 10:34

haukepetersen added 2 commits February 2, 2018 11:34

cpu/nrf52: optimized i2c driver implementation

b1efc39

boards/nrf52-based: updated I2C configuration

2b2accf

haukepetersen force-pushed the opt_nrf52_i2c branch from 3813f88 to 2b2accf Compare February 2, 2018 10:34

PeterKietzmann added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 2, 2018

PeterKietzmann approved these changes Feb 2, 2018

View reviewed changes

PeterKietzmann merged commit b2c1ca8 into RIOT-OS:master Feb 2, 2018

haukepetersen deleted the opt_nrf52_i2c branch February 2, 2018 11:39

Conversation

haukepetersen commented Jan 19, 2018

Contribution description

Issues/PRs references

Uh oh!

PeterKietzmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Jan 31, 2018

Uh oh!

haukepetersen commented Jan 31, 2018

Uh oh!

PeterKietzmann commented Jan 31, 2018

Uh oh!

haukepetersen commented Jan 31, 2018

Uh oh!

haukepetersen commented Jan 31, 2018

Uh oh!

PeterKietzmann commented Jan 31, 2018

Uh oh!

haukepetersen commented Feb 1, 2018

Uh oh!

PeterKietzmann commented Feb 2, 2018

Uh oh!

haukepetersen commented Feb 2, 2018

Uh oh!

haukepetersen commented Feb 2, 2018

Uh oh!

PeterKietzmann commented Feb 2, 2018

Uh oh!

haukepetersen commented Feb 2, 2018

Uh oh!

aabadie commented Feb 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants