Skip to content

drivers/cc2420: Implemented missing drop case#10416

Merged
jia200x merged 2 commits intoRIOT-OS:masterfrom
maribu:cc2420
Jan 10, 2019
Merged

drivers/cc2420: Implemented missing drop case#10416
jia200x merged 2 commits intoRIOT-OS:masterfrom
maribu:cc2420

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Nov 16, 2018 •
edited
Loading

Contribution description

The netdev_driver_t::recv implementation of the cc2420 does not provide the drop feature. This PR adds it.

Testing procedure

It would be nice to add a test application for this, as many drivers are affected...

Issues/PRs references

#10410

Update 1: Removed warning about possible regression. Double flushing of the RX FIFO is indeed required and now is properly documented as to #10725. This PR was updated to perform double flushing of the RX FIFO using the new inline function in #10725

@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking Area: drivers Area: Device drivers labels Nov 16, 2018
@maribu maribu mentioned this pull request Nov 16, 2018
Open
jia200x
jia200x requested changes Jan 7, 2019
View reviewed changes
drivers/cc2420/cc2420.c Outdated
DEBUG("cc2420: recv: packet of length %i in RX FIFO\n", (int)len);
if (max_len != 0) {
/* dropping frame was requested */
cc2420_strobe(dev, CC2420_STROBE_FLUSHRX);
Copy link
Copy Markdown
Member

@jia200x jia200x Jan 7, 2019

Choose a reason for hiding this comment

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

AFAIS since the second cc2420_strobe is in the else block, this function will only get called once. Thus, the FIFO won't be flushed properly.

I guess that could be solved putting both cc2420_strobe in the end and adding return statement with the length here.

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jan 7, 2019

BTW we should indeed document the silicon bug (see CC2420 datasheet, section 14.3) in another PR :)

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jan 7, 2019

BTW we should indeed document the silicon bug (see CC2420 datasheet, section 14.3) in another PR :)

Maybe that should be done first and rebase this PR on it? I believe a static inline function to flush the FIFO will be more readable and less surprising instead of having two times the same command. This static inline function can be documented properly.

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jan 7, 2019

believe a static inline function to flush the FIFO will be more readable and less surprising instead of having two times the same command

Agreed!

Could you open a PR? I can also do it, let me know ;)

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jan 7, 2019

Today I'm too busy to attend to it. Feel free to create the PR. Otherwise I can do it tomorrow.

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jan 7, 2019

Today I'm too busy to attend to it. Feel free to create the PR. Otherwise I can do it tomorrow.

Ok, I will then.

@jia200x jia200x mentioned this pull request Jan 7, 2019
Merged
@jia200x jia200x requested a review from PeterKietzmann January 7, 2019 16:30
maribu added 2 commits January 8, 2019 23:42
@maribu
drivers/cc2420: Implemented missing drop feature
0784449 
The netdev_driver_t::recv implementation of the cc2420 does not provide the drop
feature. This commit adds it.
@maribu
drivers/cc2420: Fixed behavior on too small buffer
aac69c3 
As decided in RIOT-OS#10413 when
netdev_driver_t::recv() is called with a too small buffer the incoming frame
should be dropped and `-ENOBUFS` returned. This commit makes cc2420 compliant.
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jan 8, 2019

@jia200x: I rebased and also addressed issue #10413. Please re-review :-)

@jia200x jia200x added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Jan 9, 2019
@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jan 9, 2019

From docs:

If buf == NULL and len == 0, returns the packet size – or an upper bound estimation of the size – without dropping the packet. If buf == NULL and len > 0, drops the packet and returns the packet size.

If called with buf != NULL and len is smaller than the received packet:

    The received packet is dropped
    The content in buf becomes invalid. (The driver may use the memory to implement the dropping - or may not change it.)
    -ENOBUFS is returned

With this PR:

So, this fixes the initial issue

jia200x
jia200x approved these changes Jan 9, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@jia200x jia200x left a comment

Choose a reason for hiding this comment

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

I don't have hardware to test, but I'm OK giving an untested ACK

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Jan 10, 2019

I don't have hardware to test

@jia200x you do!

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jan 10, 2019

oh yes, I do 😅

@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 10, 2019
@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jan 10, 2019 •
edited
Loading

Tested on Zolertia Z1 and seems to work OK.

I modified the rx buffer to 30 bytes. I send ~100 packets (40 bytes) and they are dropped with -ENOBUFS. Then, sending ~100 packets (<= 30 bytes) still work.

Also the default example works as expected. I can give the tested ACK now

@jia200x jia200x added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jan 10, 2019
@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jan 10, 2019

All green. GO.

Thanks for your contribution!

@jia200x jia200x merged commit afa69c5 into RIOT-OS:master Jan 10, 2019
@aabadie aabadie added this to the Release 2019.01 milestone Jan 11, 2019
@maribu maribu deleted the cc2420 branch March 8, 2019 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jia200x jia200x jia200x approved these changes

@thomaseichinger thomaseichinger Awaiting requested review from thomaseichinger

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

No one assigned

Labels

Area: drivers Area: Device drivers Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2019.01

Development

Successfully merging this pull request may close these issues.

4 participants

@maribu @jia200x @PeterKietzmann @aabadie