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.

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

drivers - cc110x_ng: Propose new CC110x driver implementation #1772

Merged
merged 3 commits into from
Dec 1, 2014

Conversation

fnack
Copy link
Member

@fnack fnack commented Oct 7, 2014

This PR proposes a new implementation of the cc110x_ng driver and shows example usage for the MSB-IoT board. The new implementation features:

  • Usage of the low-level periph driver functionality (making board-specific spi communication implementations obsolete)
  • Implementation of the new netdev base interface (still WIP in some parts)

I know this implementation still leaves room for optimization because of a lot of duplicate code between the old and new driver. Proposals to slim the code are very welcome!

This PR depends on #1770

@miri64 miri64 added Area: network Area: Networking State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Area: drivers Area: Device drivers labels Oct 7, 2014
@miri64 miri64 added this to the Release NEXT MAJOR milestone Oct 7, 2014
@@ -0,0 +1,36 @@
/*
* Copyright (C) 2009 Freie Universität Berlin
* Copyright (C) 2013 INRIA
Copy link
Member

Choose a reason for hiding this comment

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

netdev was "invented" in 2014 by @authmillenon, employed by FUB at this time, so I think it's a safe bet to change this accordingly.

@OlegHahm
Copy link
Member

OlegHahm commented Oct 7, 2014

I'm definitely no expert on the low-layer stuff and couldn't test, but in general this looks good to me. I'm only a bit worried about the increasing confusion by RIOT now providing three different drivers for the cc110X, but I don't see how this could easily be solved. However, you might want to add some more documentation about how and when this driver can be used - and we should hope that someone's willing to port other boards with this radio chip to the new periph model.

@OlegHahm OlegHahm assigned haukepetersen and unassigned OlegHahm Oct 7, 2014
@miri64
Copy link
Member

miri64 commented Oct 7, 2014

The cleanest solution to reduce the number of cc110x drivers would be to fix #1540 (edit: for the boards in question) first and remove drivers/cc110x then, I guess ;-)

@LudwigKnuepfer
Copy link
Member

I know that naming is the hardest problem in CS, but I really dislike the naming scheme of $base, $base_ng, $base_v2. I propose to rename them all in a more sensible manner (e.g. $base_w_mac, $base_periph, ... or even $base_old, $base_older), if the old modules need to be kept.

@haukepetersen
Copy link
Contributor

I agree, that we should be careful with the naming of the drivers to save some confusion. How about something like renaming the old ones into $base_legacy and $base_legacy_w_mac and taking keeping the new one as $base? This way the old boards will still work and could slowly be migrated to the periph driver version of the driver...

@LudwigKnuepfer
Copy link
Member

Sounds good to me.

@OlegHahm
Copy link
Member

OlegHahm commented Oct 7, 2014

No objections from my side.

@fnack
Copy link
Member Author

fnack commented Oct 7, 2014

Definitely better than the existing names!

@LudwigKnuepfer
Copy link
Member

I think the name changing of the older drivers should be done in a fresh PR.

@LudwigKnuepfer
Copy link
Member

(and for all other legacy drivers on the go)

@LudwigKnuepfer
Copy link
Member

=> #1776

@LudwigKnuepfer
Copy link
Member

@fnack rebase?

@LudwigKnuepfer
Copy link
Member

(and rename)

@LudwigKnuepfer
Copy link
Member

Also, quickly looking over the commits, there appear to be some fixes/extensions of other modules that could be useful to move to separate PRs (the critical commenting error, SPI, possibly more).

@fnack
Copy link
Member Author

fnack commented Nov 1, 2014

I'll do that today! I'll squash and will think about moving some of the commits. The spi change for example was already merged in master with another PR.

@fnack fnack force-pushed the cc1101_if branch 4 times, most recently from 73045c6 to 5e01c75 Compare November 3, 2014 09:14
@fnack fnack removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 3, 2014
@fnack
Copy link
Member Author

fnack commented Nov 3, 2014

What about the cppcheck errors from my travis build? They are definitely false-positives. It looks like cppcheck has problems with bit field struct members like unsigned LL_ACK : 1;

Is this a known issue @N8Fear?

@miri64
Copy link
Member

miri64 commented Nov 4, 2014

Bit-field struct members are discouraged (see #1609)

@miri64
Copy link
Member

miri64 commented Nov 4, 2014

For other false-positives, see https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions#travis

@fnack
Copy link
Member Author

fnack commented Nov 4, 2014

Fixed the cppcheck-findings. Finally, Travis is happy :)

@fnack fnack force-pushed the cc1101_if branch 2 times, most recently from 1736f6b to fdf7069 Compare November 18, 2014 15:08
@fnack
Copy link
Member Author

fnack commented Nov 18, 2014

Rebased. I would really like to get this PR merged soon. What tasks are left to get an ACK @OlegHahm and @haukepetersen?

#define GPIO_2_PORT GPIOA
#define GPIO_2_PIN 4
#define GPIO_2_PORT GPIOA /* CC1101 GDO1 */
#define GPIO_2_PIN 6
Copy link
Member

Choose a reason for hiding this comment

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

broken indentation (I know it was broken before, too)

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you explain the problem to me? I don't really see what is wrong with the indentation x)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, is apparently correct - just a browser issue. Never mind!

@OlegHahm
Copy link
Member

Apart from minor comments, this PR looks good.

@fnack
Copy link
Member Author

fnack commented Nov 19, 2014

I addressed your comments.

@fnack fnack force-pushed the cc1101_if branch 3 times, most recently from 5d8daa0 to 894b0ea Compare November 28, 2014 07:35
@OlegHahm
Copy link
Member

OlegHahm commented Dec 1, 2014

Apart from minor comments, this PR looks good.

...and works (as locally demonstrated). (This is an ACK.)

@fnack fnack removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Dec 1, 2014
@fnack
Copy link
Member Author

fnack commented Dec 1, 2014

Rebased, squashed and Travis is happy too. Go...

fnack added a commit that referenced this pull request Dec 1, 2014
drivers - cc110x: Propose new CC110x driver implementation
@fnack fnack merged commit 78b6b74 into RIOT-OS:master Dec 1, 2014
@fnack fnack deleted the cc1101_if branch March 13, 2015 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: network Area: Networking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants