Skip to content

drivers/cc2420: move flush sequence to inline function #10725

Merged
maribu merged 2 commits intoRIOT-OS:masterfrom
jia200x:pr/cc2420_inline_flush
Jan 8, 2019
Merged

drivers/cc2420: move flush sequence to inline function #10725
maribu merged 2 commits intoRIOT-OS:masterfrom
jia200x:pr/cc2420_inline_flush

Conversation

@jia200x
Copy link
Copy Markdown
Member

@jia200x jia200x commented Jan 7, 2019

Contribution description

See #10416 (comment)

Testing procedure

Check it compiles properly. Try to receive packets and check they are received correctly (if FIFO is not flushed, new packets won't be received)

Issues/PRs references

#10416

@jia200x jia200x requested a review from maribu January 7, 2019 16:27
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Jan 7, 2019

(I renamed _flush_fifo to _flush_rx_fifo to make it more clear)

@jia200x jia200x requested a review from PeterKietzmann January 7, 2019 16:31
@smlng smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances 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 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 Jan 8, 2019
smlng
smlng reviewed Jan 8, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

untested ACK

@smlng
Copy link
Copy Markdown
Member

smlng commented Jan 8, 2019

I guess its safe to squash already

maribu
maribu approved these changes Jan 8, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you very much :-)

@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 8, 2019

I guess its safe to squash already

+1

@jia200x jia200x force-pushed the pr/cc2420_inline_flush branch from 1cd077d to 87e9172 Compare January 8, 2019 12:51
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Jan 8, 2019

done!

@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 8, 2019

If Travis and Murdock don't find any issues, I would just merge this PR untested. I really cannot see how this could break anything - I would bet that the compiler would generate the exact same machine code before and after this PR.

@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 8, 2019

@smlng, @PeterKietzmann: Are you fine with untested merge of this PR?

@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 8, 2019

OK. I did just compile it with and without the PR and disassembled cc2420.o. The result is the same - so indeed the same machine code is generated. I think this should be as good as testing. (I cannot test because I'm missing the hardware.)

@maribu maribu added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jan 8, 2019
@maribu maribu merged commit 1907374 into RIOT-OS:master Jan 8, 2019
@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 8, 2019

Huh! A moment ago all three checks were green - now Codacy is running again... Strange. (But I guess that test will pass again, as no code was change since it was green and I hit merge.)

@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 8, 2019

For the record Codacy says the PR is up to standards now. Next time I'll hit Ctrl+F5 instead of F5 before clicking on merge. Maybe the browser chache served me ill this time.

@maribu maribu mentioned this pull request Jan 8, 2019
Merged
@aabadie aabadie added this to the Release 2019.01 milestone Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maribu maribu maribu approved these changes

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

+1 more reviewer

@smlng smlng smlng left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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 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: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2019.01

Development

Successfully merging this pull request may close these issues.

4 participants

@jia200x @smlng @maribu @aabadie