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

drivers: mag3110 rework #6886

Merged
merged 4 commits into from Apr 14, 2017
Merged

drivers: mag3110 rework #6886

merged 4 commits into from Apr 14, 2017

Conversation

smlng
Copy link
Member

@smlng smlng commented Apr 10, 2017

  • rework driver mag3110 magnetometer
  • add SAUL support

@smlng smlng added Area: drivers Area: Device drivers Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Apr 10, 2017
@smlng smlng self-assigned this Apr 10, 2017
aabadie
aabadie requested changes Apr 11, 2017
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

A few nits and comments. I also think you could add an auto_init_mag3110 function in sys/auto_init/saul. Then one can test the driver with examples/saul by simply running:

USEMODULE=mag3110 make -C examples/saul flash term

drivers/mag3110/mag3110_saul.c
* @{
*
* @file
* @brief MAG3110 adaption to the RIOT actuator/sensor interface
Copy link
Contributor

Choose a reason for hiding this comment

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

adaptation

Copy link
Member Author

Choose a reason for hiding this comment

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

both are correct, I leave as is 'cause its written that way in every other saul adaption

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed, this is probably a French bias (it's also adaptation in French). According to this post, adaptation is the preferred form though.
But leave it like this it's fine.

tests/driver_mag3110/main.c Outdated
printf("Initializing MAG3110 magnetometer at I2C_%i... ", TEST_MAG3110_I2C);
if (mag3110_init(&dev, TEST_MAG3110_I2C, TEST_MAG3110_ADDR, MAG3110_DROS_DEFAULT) == 0) {
printf("Initializing MAG3110 magnetometer at I2C_%i... ", mag3110_params->i2c);
if (mag3110_init(&dev, mag3110_params) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here it should be &mag3110_params[0] instead of mag3110_params

drivers/mag3110/mag3110.c Outdated
if (i2c_init_master(i2c, I2C_SPEED) < 0) {
i2c_release(dev->i2c);
if (i2c_init_master(BUS, I2C_SPEED) < 0) {
i2c_release(BUS);
return -2;
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be also great if you could add an enum for return codes

drivers/mag3110/mag3110.c Outdated
if (reg != dev->params.type) {
i2c_release(BUS);
DEBUG("[mag3110] init - error: invalid WHO_AM_I value [0x%02x]\n",
(int)reg);
Copy link
Contributor

Choose a reason for hiding this comment

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

align

@aabadie aabadie added this to the Release 2017.04 milestone Apr 11, 2017
@smlng
Copy link
Member Author

smlng commented Apr 11, 2017 •
edited

@aabadie address[ed] your comments!

@smlng smlng force-pushed the driver/mag3110/rework branch 2 times, most recently from 08fb578 to 94563aa Compare April 11, 2017 18:26
@smlng
Copy link
Member Author

smlng commented Apr 13, 2017

@aabadie ping? Okay to squash and release Murdock on it?

aabadie
aabadie approved these changes Apr 13, 2017
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Yes, please squash @smlng.

ACK when Murdock is green

@smlng smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 13, 2017
@miri64 miri64 merged commit 812c557 into RIOT-OS:master Apr 14, 2017
@smlng smlng deleted the driver/mag3110/rework branch May 31, 2017 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabadie aabadie aabadie approved these changes

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

@smlng smlng

Labels
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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2017.04
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@smlng @aabadie @miri64