Skip to content

netopt: Add option for retrieving number of retransmissions#7447

Merged
jnohlgard merged 1 commit intoRIOT-OS:masterfrom
bergzand:netopt/tx-retries
Aug 17, 2017
Merged

netopt: Add option for retrieving number of retransmissions#7447
jnohlgard merged 1 commit intoRIOT-OS:masterfrom
bergzand:netopt/tx-retries

Conversation

@bergzand
Copy link
Copy Markdown
Member

@bergzand bergzand commented Aug 6, 2017

To request the number of retransmissions needed for the last transmission. Useful for ETX calculation. Can only be supported by radio's that report this information such as the at86rf233 and the mrf24j40.

This was referenced Aug 6, 2017
Merged
Closed
Closed
@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Aug 11, 2017

I think the documentation needs some clarification. What type of retries are meant here?
Is it only pure link layer retransmissions caused by missing ACK packets that are counted, or are retries because of failing CCA checks also counted (channel was busy when we tried to send)?

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Aug 13, 2017

Thanks for the feedback. You're completly right. This is supposed to be purely retransmission for missing ACK packets. I'll push a commit with some clarifications tomorrow

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Aug 17, 2017

@gebart I've clarified the documentation a bit. Is it clear this way?

jnohlgard
jnohlgard reviewed Aug 17, 2017
View reviewed changes
sys/include/net/netopt.h Outdated
* @brief Get retry amount from missing ACKs of the last transmission
*
* This retrieves the number of retries needed for the last transmissions.
* Only retries due to missing ACK packets are considered. Retries due to
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard Aug 17, 2017

Choose a reason for hiding this comment

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

to make this even more clear you could change this to
"Only retransmissions due to missing ACK packets are considered."

jnohlgard
jnohlgard approved these changes Aug 17, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

@bergzand much clearer! 👍
ACK
Feel free to squash.
Do as you wish with my suggested wording change above.

@jnohlgard jnohlgard added this to the Release 2017.10 milestone Aug 17, 2017
@jnohlgard jnohlgard self-assigned this Aug 17, 2017
@jnohlgard jnohlgard added Area: network Area: Networking Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 17, 2017
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Aug 17, 2017

@gebart thank you for your comment. I've reworded the doc to your suggestion and squashed the commits.

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Aug 17, 2017

Whitespace check failed 😢. Fixed it.

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Aug 17, 2017

I have no clue why the build is failing here. Is it an issue caused by me or is that specific test broken?

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Aug 17, 2017

you need to add the new option to the string list in sys/net/crosslayer/netopt/netopt.c

@jnohlgard jnohlgard changed the title Add netopt for retrieving number of retransmissions netopt: Add option for retrieving number of retransmissions Aug 17, 2017
@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Aug 17, 2017

@bergzand you can directly amend the string list fix to this commit

jnohlgard
jnohlgard requested changes Aug 17, 2017
View reviewed changes
sys/include/net/netopt.h Outdated
*
* This retrieves the number of retries needed for the last transmissions.
* Only retransmissions due to missing ACK packets are considered.
* Retransmissionsdue to CCA failures are not counted.
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard Aug 17, 2017

Choose a reason for hiding this comment

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

The CCA failures are not retransmissions, nothing is transmitted. Call them retries or backoffs instead, those are the words that are used in the 802.15.4 standard where they describe the CSMA/CA algorithm for 802.15.4 networks.

Copy link
Copy Markdown
Member Author

@bergzand bergzand Aug 17, 2017

Choose a reason for hiding this comment

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

Ack, fixed.

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Aug 17, 2017

Thanks for the pointer to that file, build is now successful 😄

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Aug 17, 2017

No problem, I had made the same mistake in #7481

@jnohlgard jnohlgard merged commit b5984d2 into RIOT-OS:master Aug 17, 2017
@bergzand bergzand mentioned this pull request Oct 12, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnohlgard jnohlgard jnohlgard approved these changes

Assignees

@jnohlgard jnohlgard

Labels

Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2017.10

Development

Successfully merging this pull request may close these issues.

2 participants

@bergzand @jnohlgard