drivers/tcs37727: apply unified params definition scheme #8683

aabadie · 2018-02-28T16:32:00Z

Contribution description

This PR update the params definitions scheme for the tcs37727 device driver.

Issues/PRs references

Initially done in #7937 and related to #7519

miri64 · 2018-03-07T13:14:28Z

drivers/include/tcs37727.h

-/**
- * @brief   Export the sensor's SAUL interface
- */
-extern const saul_driver_t tcs37727_saul_driver;


To be honest, now that I see this option, I think I prefer the extern definition to be in the drivers header. This way one just needs to include the header to get access to the driver, which seems to be for me the more natural way for a C-programmer to get access to global definitions. Also, a user not using auto_init would have to know what to get from extern. The (public) documentation for that is also removed here. Or is there anything speaking against this?

but SAUL is a higher layer API, and should not be dependency of the lower layer driver. That said, one should also remove the #include "saul.h" above.

That said, one should also remove the #include "saul.h" above.

Indeed, will do

but SAUL is a higher layer API, and should not be dependency of the lower layer driver. That said, one should also remove the #include "saul.h" above.

You are right.

Maybe have some <driver_name>_saul.h or <driver_name>/saul.h instead than?

inclusion of saul.h removed.

smlng · 2018-03-08T13:30:31Z

drivers/include/tcs37727.h

-/**
- * @brief   Export the sensor's SAUL interface
- */
-extern const saul_driver_t tcs37727_saul_driver;


but SAUL is a higher layer API, and should not be dependency of the lower layer driver. That said, one should also remove the #include "saul.h" above.

smlng · 2018-03-08T13:37:12Z

tested with pba-d-01-kw2x -> works

aabadie · 2018-03-21T06:28:38Z

@miri64, your comments are addressed here.

smlng · 2018-06-27T20:43:39Z

@miri64 can you've another look (or dismiss your review), thx! we should get I2C rework through to avoid blocking I2C changes/addition in master for too long.

smlng · 2018-07-04T08:13:59Z

ping @miri64 we want to get the I2C rework done -- if possible for the upcoming release, please check your review, THX!

Sorry, lost track on this one. Dismissed my review.

miri64 · 2018-07-04T09:47:36Z

On a separate note: does it make sense to push this effort for the upcoming release? Wouldn't it be safer to merge the I²C rework after the release so it can mature until the next release?

smlng · 2018-07-04T10:03:08Z

re-tested ACK

aabadie · 2018-07-04T10:05:55Z

does it make sense to push this effort for the upcoming release?

I'm not sure that's the best moment for this kind of question but ok.

For various reasons, the sooner, the better I think: a lot of effort, from a lot of people has already been put in this work, the I2C embargo is annoying because it blocks other contributions, there's a good momentum around this topic in the community and this I2C rework initiative was launched even before the previous release (the initial work started more than one year ago).

smlng · 2018-07-04T10:08:57Z

let's see how much we can achieve within the next few days, we are on a good track - but in the end its up to the release manager @cladmi to decide what's in and out. However, keep pushing to drop the embargo sooner than later.

miri64 · 2018-07-04T10:13:48Z

I'm not sure that's the best moment for this kind of question but ok.

I think it's the perfect moment to ask this question. Lot's of people under pressure and wanting to finish there work ASAP (which is for very the reasons @aabadie gave very understandable). But exactly because of that it is sometimes helpful to step back (or a third party to step in) and ask: is this still the right thing to do now. I'm just worried that we push it too quickly just because we want to get done on the risk of potentially breaking the a huge chunk of the next release. Don't get me wrong, I'd like to have the embargo dropped also sooner rather than later, but not on the condition of having a broken release.

aabadie · 2018-07-04T10:41:36Z

I think it's the perfect moment to ask this question

Do you really think the perfect moment exists ? I don't.
A better moment to ask this question would be IMHO just after a release. It gave us enough time for people to discuss and organize themselves and do the work. This is what was done after 2018.04.
Now we are finalizing the work: all but one CPU families were adapted, all device drivers are adapted, automatic testing is also on its way.

I'm just worried that we push it too quickly just because we want to get done on the risk of potentially breaking the a huge chunk of the next release

Any valuable active help is always welcome to avoid this, volunteer ?

miri64 · 2018-07-04T11:30:26Z

Any valuable active help is always welcome to avoid this, volunteer ?

Apart from testing on the little bit of hardware I have, I think there isn't much I can contribute with regarding that kind of help. But if you have questions regarding process or Git I am happy to help :-)

drivers/tcs37727: apply unified params definition scheme

aabadie added Type: cleanup The issue proposes a clean-up / The PR cleans-up 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 Feb 28, 2018

aabadie assigned kYc0o Feb 28, 2018

aabadie mentioned this pull request Mar 1, 2018

boards/pba-d-01-kw2x: fix driver params usage #8709

Merged

miri64 previously requested changes Mar 7, 2018

View reviewed changes

smlng suggested changes Mar 8, 2018

View reviewed changes

aabadie force-pushed the pr/drivers/params/tcs37727 branch from 26c0e4f to 469dc53 Compare March 8, 2018 14:47

smlng approved these changes Mar 8, 2018

View reviewed changes

aabadie mentioned this pull request May 23, 2018

drivers: unify default parameter definition scheme #7519

Closed

haukepetersen mentioned this pull request May 25, 2018

periph/i2c: tracking the remodeling #6577

Closed

47 tasks

aabadie changed the base branch from master to new_i2c_if May 27, 2018 21:14

aabadie force-pushed the pr/drivers/params/tcs37727 branch from 469dc53 to 28dac25 Compare May 27, 2018 21:16

aabadie added TF: I2C Marks issues and PRs related to the work of the I²C rework task force and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 27, 2018

aabadie force-pushed the pr/drivers/params/tcs37727 branch from 28dac25 to ba7c40a Compare June 7, 2018 21:50

aabadie added 3 commits June 18, 2018 13:33

drivers/tcs37727: use new driver params scheme

39ccd07

sys/auto_init: update tcs37727 auto_init

b48706b

drivers/tcs37727: adapt to new I2C api

739aeac

aabadie force-pushed the pr/drivers/params/tcs37727 branch from ba7c40a to 739aeac Compare June 18, 2018 11:34

smlng merged commit 76e11ee into RIOT-OS:new_i2c_if Jul 4, 2018

basilfx pushed a commit to basilfx/RIOT that referenced this pull request Jul 10, 2018

Merge pull request RIOT-OS#8683 from aabadie/pr/drivers/params/tcs37727

f4fcf20

drivers/tcs37727: apply unified params definition scheme

dylad pushed a commit to dylad/RIOT that referenced this pull request Jul 10, 2018

Merge pull request RIOT-OS#8683 from aabadie/pr/drivers/params/tcs37727

96e4f69

drivers/tcs37727: apply unified params definition scheme

dylad pushed a commit that referenced this pull request Jul 11, 2018

Merge pull request #8683 from aabadie/pr/drivers/params/tcs37727

5d49054

drivers/tcs37727: apply unified params definition scheme

aabadie deleted the pr/drivers/params/tcs37727 branch September 23, 2018 08:47

aabadie added this to the Release 2018.10 milestone Nov 5, 2018

drivers/tcs37727: apply unified params definition scheme #8683

drivers/tcs37727: apply unified params definition scheme #8683

Uh oh!

Conversation

aabadie commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Uh oh!

miri64 Mar 7, 2018

Choose a reason for hiding this comment

Uh oh!

smlng Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

aabadie Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

miri64 Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

miri64 Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

aabadie Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

smlng Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

smlng commented Mar 8, 2018

Uh oh!

aabadie commented Mar 21, 2018

Uh oh!

smlng commented Jun 27, 2018

Uh oh!

smlng commented Jul 4, 2018

Uh oh!

miri64 commented Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smlng commented Jul 4, 2018

Uh oh!

aabadie commented Jul 4, 2018

Uh oh!

smlng commented Jul 4, 2018

Uh oh!

miri64 commented Jul 4, 2018

Uh oh!

aabadie commented Jul 4, 2018

Uh oh!

miri64 commented Jul 4, 2018

Uh oh!

Uh oh!

aabadie commented Feb 28, 2018 •

edited

Loading

miri64 commented Jul 4, 2018 •

edited

Loading