Skip to content

cpu/nrf5x/nrfmin: fix isr termination #11395

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
Merged
miri64 merged 1 commit into from
Apr 16, 2019
Merged

cpu/nrf5x/nrfmin: fix isr termination #11395

miri64 merged 1 commit into from
Apr 16, 2019

Conversation

@SemjonWilke
Copy link
Member

@SemjonWilke SemjonWilke commented Apr 15, 2019

Contribution description

The nrfmin driver had a serious bug terminating the interrupt handler in case of an invalid CRC value.
Instead of closing with cortexm_isr_end(); it just returned, which resulted in unwanted behaviour.

This PR fixes one error mentioned in #10878 and is based on the solution of @pystub

Testing procedure

Testing should at least involve receiving and then sending packets with a valid CRC. However, it is appreciated if a reviewer makes sure that the behaviour is also correct with invalid CRC.

Make sure to test it using nrfmin with USEMODULE=nrfmin on nrf5x boards.

@SemjonWilke SemjonWilke added Area: drivers Area: Device drivers Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 15, 2019
@miri64 miri64 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: 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 labels Apr 15, 2019
miri64
miri64 reviewed Apr 15, 2019
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Seems obvious. But I still want to test. ACK on the rest though

@miri64 miri64 mentioned this pull request Apr 16, 2019
Merged
@miri64
Copy link
Member

miri64 commented Apr 16, 2019

Mh... I wasn't able to reproduce the issue, but during my testing I found the following problem: When I ping from two nodes simultaneously

ping6 ff02::1 -c 1000 -i 10

everything works fine, however when I then do

ping6 ff02::1 -c 10000 -i 0

the first command doesn't work anymore afterwards. I'll have a look if this is isolated to nrfmin and report in a separate issue. However, this has nothing to do with this PR, so ACK.

miri64
miri64 approved these changes Apr 16, 2019
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK

@miri64 miri64 added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Apr 16, 2019
@miri64 miri64 merged commit 5e789c6 into RIOT-OS:master Apr 16, 2019
@miri64
Copy link
Member

miri64 commented Apr 16, 2019

Please provide a backport to 2019.04-branch.

This was referenced Apr 16, 2019
Closed
Open
@SemjonWilke SemjonWilke mentioned this pull request Apr 17, 2019
Merged
@SemjonWilke
Copy link
Member Author

SemjonWilke commented Apr 17, 2019

Backport provided in #11408

@SemjonWilke SemjonWilke deleted the nrfmin_fix_isr branch April 17, 2019 09:49
@PeterKietzmann PeterKietzmann mentioned this pull request Aug 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 approved these changes

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 Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch 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

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SemjonWilke @miri64