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.

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

drivers/kw2xrf: Use TMR3 instead TMR4 to detect ACK timeout #12287

Merged
merged 1 commit into from
Oct 2, 2019
Merged

drivers/kw2xrf: Use TMR3 instead TMR4 to detect ACK timeout #12287

merged 1 commit into from
Oct 2, 2019

Conversation

Ilmu011
Copy link

@Ilmu011 Ilmu011 commented Sep 23, 2019

Originally posted by @PeterKietzmann in #12083

@Ilmu011 for the TMR3 vs TMR4 discussion, please keep that one separate in an own issue or even better in a fixing PR :-)

This driver currently uses TMR4 to detect ACK timeouts in TR operations. There is a dedicated timer for detecting timeouts in RX-operations and that is TMR3. I am working on #10364 right now and the kw41z driver uses TMR3 for this purpose, which seems to make more sense.

See section 7.6.2 of the Reference Manual "Using T3CMP to Abort an RX operation"

@Ilmu011 Ilmu011 mentioned this pull request Sep 23, 2019
Closed
@benpicco benpicco added 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 Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Sep 23, 2019
PeterKietzmann
PeterKietzmann requested changes Oct 2, 2019
View reviewed changes
Copy link
Member

@PeterKietzmann PeterKietzmann left a comment

Choose a reason for hiding this comment

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

@benpicco proposed some deduplication which I would like to see too. There is more room for improvement in this driver, but for the first step this change should be good. @Ilmu please address @benpicco's comments and answer open questions. Furthermore, could leave some short comments next to your code (stuff like enable/disable timer X, ...) because the existing functions that you re-use are not very intuitive and also not well documented.

I'll run some tests very soon.

@Ilmu011
Copy link
Author

Ilmu011 commented Oct 2, 2019 •
edited
Loading

@benpicco It looks like the difference between kw2xrf_set_idle_sequence() and kw2xrf_set_sequence is that kw2xrf_set_idle_sequence() checks whether there are still pending transmissions and doesn't abort the sequence if that is the case.
I'm using kw2xrf_set_idle_sequence() now.

@PeterKietzmann I also removed the duplicate code and added some comments.

@benpicco
Copy link
Contributor

benpicco commented Oct 2, 2019

Looks much better & easier to understand now with the smaller if blocks!

@benpicco
Copy link
Contributor

benpicco commented Oct 2, 2019

Feel free to squash.

@Ilmu011
Copy link
Author

Ilmu011 commented Oct 2, 2019

Squashed it :)

@PeterKietzmann PeterKietzmann 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: 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 labels Oct 2, 2019
PeterKietzmann
PeterKietzmann approved these changes Oct 2, 2019
View reviewed changes
Copy link
Member

@PeterKietzmann PeterKietzmann left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@Ilmu011 thanks for addressing all comments. The code looks good now! I've tested it by placing LED0_ON and LED0_OFF here and here and measuring the timeout value with a logic analyzer (to do so I flashed gnrc_networking and did a ping6 to a non existing address). Both master and this PR behave similar and take a bit more than 900us on a phytec board.

@PeterKietzmann
Copy link
Member

@Ilmu011 I think one commit should be enough here. Please squash all together and name it something alike the first commit.

@Ilmu011
Copy link
Author

Ilmu011 commented Oct 2, 2019

@PeterKietzmann Done.

@PeterKietzmann PeterKietzmann merged commit ed75d63 into RIOT-OS:master Oct 2, 2019
@PeterKietzmann
Copy link
Member

Congrats to your first RIOT contribution!

@kb2ma kb2ma added this to the Release 2019.10 milestone Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco left review comments

@PeterKietzmann PeterKietzmann PeterKietzmann approved these changes

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: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2019.10
Development

Successfully merging this pull request may close these issues.
5 participants
@Ilmu011 @benpicco @PeterKietzmann @kb2ma @AdemCanAgdas