drivers/mpl3115a2: rework by smlng · Pull Request #7120 · RIOT-OS/RIOT

smlng · 2017-05-31T20:20:38Z

rework driver interface
adapt test
add saul support -> i.e., for pba-d-01-kw2x

smlng · 2017-06-01T07:11:18Z

drivers/mpl3115a2/include/mpl3115a2.h

+enum {
+    MPL3115A2_OK,                 /**< all good */
+    MPL3115A2_ERROR_I2C,          /**< I2C communication failed */
+    MPL3115A2_ERROR_DEV,          /**< Device MAG3110 not found */


note to self: typo /MAG3110/MPL3115A2/

smlng · 2017-06-01T07:12:10Z

drivers/mpl3115a2/include/mpl3115a2_params.h

+/**@}*/
+
+/**
+ * @brief   MAG3110 configuration


note to self: typo again

smlng · 2017-06-09T21:19:44Z

see #7152, too

smlng · 2017-06-15T09:29:38Z

anybody willing to review, this is would finally resolve (and close) issue #7152

aabadie

Quick review: see comments below.

aabadie · 2017-06-22T12:56:09Z

drivers/include/mpl3115a2.h

+ *
+ * The driver will initialize the sensor for pressure measurement. The
+ * conversion duration depends on oversample ratio. After initialization
+ * the sensor can be set activ to run periodic measurements. The oversample


aabadie · 2017-06-22T12:58:20Z

drivers/include/mpl3115a2.h

 *
- * @return                  0 on success
- * @return                  -1 on error
+ * @return                  0 (MPL3115A2_OK) on success


You can use MPL3115A2 on success (no need to specify 0)

aabadie · 2017-06-22T12:58:50Z

drivers/include/mpl3115a2.h

- * @return                  0 on success
- * @return                  -1 on error
+ * @return                  0 (MPL3115A2_OK) on success
+ * @return                  -1 (MPL3115A2_ERROR_I2C) on error


same as above: -MPL3115A2_ERROR_I2C on error

aabadie · 2017-06-22T12:59:03Z

drivers/include/mpl3115a2.h

 *
- * @return                  0 on success
- * @return                  -1 on error
+ * @return                  0 (MPL3115A2_OK) on success


same comment as above

aabadie · 2017-06-22T12:59:28Z

drivers/include/mpl3115a2.h

 *
- * @return                  0 on success
+ * @return                  0 (MPL3115A2_OK) on success
 * @return                  -1 on error


no enum value for this case ?

aabadie · 2017-06-22T13:00:16Z

drivers/mpl3115a2/include/mpl3115a2_params.h

+#define MPL3115A2_PARAM_ADDR        (MPL3115A2_I2C_ADDRESS)
+#endif
+#ifndef MPL3115A2_PARAMS_DEFAULT
+#define MPL3115A2_PARAMS_DEFAULT    {   .i2c  = MPL3115A2_PARAM_I2C, \


extra spaces before .i2c, one is enough

aabadie · 2017-06-22T13:02:11Z

drivers/mpl3115a2/mpl3115a2.c

+        /* Release the bus for other threads. */
+        i2c_release(BUS);
+        LOG_ERROR("mpl3115a2_init: I2C error!\n");
+        return -MPL3115A2_ERROR_I2C;


Maybe a specific enum for I2C read error could be used ?

aabadie · 2017-06-22T13:03:01Z

drivers/mpl3115a2/mpl3115a2.c

+    if (i2c_write_regs(BUS, ADDR, MPL3115A2_CTRL_REG1, &reg, 1) != 1) {
+        i2c_release(BUS);
+        LOG_ERROR("mpl3115a2_init: failed to set sample rate!\n");
+        return -MPL3115A2_ERROR_CNF;


If the I2C read error code is kept, this one could be renamed to MPL3115A2_ERROR_WRITE. What do you think ?

aabadie · 2017-06-22T13:04:40Z

drivers/mpl3115a2/mpl3115a2_saul.c

+ */
+
+/**
+ * @ingroup     driver_mpl3115a2


drivers_mpl3115a2

aabadie · 2017-06-22T15:23:06Z

drivers/include/mpl3115a2.h

+ * @return                  0 (MPL3115A2_OK) on success
 * @return                  -1 on error
 */
 int mpl3115a2_set_standby(mpl3115a2_t *dev);


Please make the device descriptors const when needed/possible.

smlng · 2017-06-26T06:50:04Z

@aabadie: I addressed your comments.

aabadie

Thanks @smlng.
I made another pass of review and have new comments.

aabadie · 2017-06-26T08:19:43Z

drivers/include/mpl3115a2.h

+ * @name    Oversample Ratio configuration
+ * @{
+ */
+#define MPL3115A2_OS_RATIO_1        (0x00) /**< Oversample Ratio 1, conversion time 6 ms */


Maybe this can be in a anonymous enum?

aabadie · 2017-06-26T08:21:06Z

drivers/include/mpl3115a2.h

-} mpl3115a2_t;
+    i2c_t i2c;                  /**< I2C bus the device is connected to */
+    uint8_t addr;               /**< I2C bus address of the device */
+    uint8_t rate;               /**< sampling rate */


is it oversample ratio ? I would call it oversampling ratio btw

smlng · 2017-06-29T07:48:11Z

@aabadie happy? Regarding your comment on i2c read/write errors and matching return value, we don't have that for any other i2c driver. I see you point that this distinction might be useful, however, IMO we should generalize i2c error codes for return anyway - that is: define them in the i2c header file instead of redefinition for each and every driver.

aabadie · 2017-06-29T08:02:48Z

@aabadie happy?

Yes !

aabadie · 2017-06-29T08:03:21Z

There are lots of things that could be improved/modified with drivers implementation.

aabadie

ACK

aabadie · 2017-06-29T08:03:40Z

and go!

smlng · 2017-06-29T08:10:21Z

There are lots of things that could be improved/modified with drivers implementation.

yes, I2C is missing a major overhaul - though, @haukepetersen is/was already working on this

smlng added Area: drivers Area: Device drivers Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels May 31, 2017

smlng self-assigned this May 31, 2017

smlng requested a review from PeterKietzmann May 31, 2017 20:20

smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 1, 2017

smlng commented Jun 1, 2017

View reviewed changes

smlng force-pushed the drivers/mpl3115a2/rework branch 2 times, most recently from 944f748 to 7aa65e2 Compare June 1, 2017 07:45

smlng 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 Jun 2, 2017

smlng requested review from haukepetersen and jfischer-no June 6, 2017 14:53

smlng mentioned this pull request Jun 7, 2017

SAUL: add full support for PhyNode (pba-d-01-kw2x) #7152

Closed

smlng requested a review from aabadie June 13, 2017 14:27

smlng force-pushed the drivers/mpl3115a2/rework branch 5 times, most recently from 3d70f6b to a3df41f Compare June 15, 2017 09:21

smlng mentioned this pull request Jun 15, 2017

sys,phydat: add missing prefix, and string conversions #7193

Merged

smlng force-pushed the drivers/mpl3115a2/rework branch 2 times, most recently from 0c139df to fe8509c Compare June 15, 2017 13:16

smlng added this to the Release 2017.07 milestone Jun 20, 2017

aabadie requested changes Jun 22, 2017

View reviewed changes

aabadie reviewed Jun 22, 2017

View reviewed changes

smlng force-pushed the drivers/mpl3115a2/rework branch 2 times, most recently from 27a7fb2 to 86a4b80 Compare June 25, 2017 08:04

smlng force-pushed the drivers/mpl3115a2/rework branch from 86a4b80 to 38125db Compare June 25, 2017 08:05

aabadie requested changes Jun 26, 2017

View reviewed changes

smlng force-pushed the drivers/mpl3115a2/rework branch 2 times, most recently from 0a6072c to 9d950f9 Compare June 26, 2017 18:30

smlng force-pushed the drivers/mpl3115a2/rework branch from 9d950f9 to 8453274 Compare June 29, 2017 07:52

smlng added 3 commits June 29, 2017 09:57

drivers: rework mpl3115a2 pressure sensor

c737552

tests: adapt driver_mpl3115a2

9bed191

saul: add mpl3115a2

addf643

smlng force-pushed the drivers/mpl3115a2/rework branch from 8453274 to addf643 Compare June 29, 2017 07:57

kaspar030 changed the title ~~drivers, mpl3115a2: rework~~ drivers/mpl3115a2: rework Jun 29, 2017

aabadie approved these changes Jun 29, 2017

View reviewed changes

aabadie merged commit 7251448 into RIOT-OS:master Jun 29, 2017

smlng deleted the drivers/mpl3115a2/rework branch June 29, 2017 08:47

Conversation

smlng commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng commented Jun 9, 2017

Uh oh!

smlng commented Jun 15, 2017

Uh oh!

aabadie 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng commented Jun 26, 2017

Uh oh!

aabadie 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!

smlng commented Jun 29, 2017

Uh oh!

aabadie commented Jun 29, 2017

Uh oh!

aabadie commented Jun 29, 2017

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

aabadie commented Jun 29, 2017

Uh oh!

smlng commented Jun 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants