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.

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

netdev/ieee802154_submac: add retransmission reporting #15208

Merged
merged 2 commits into from
Oct 15, 2020

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Oct 12, 2020

Contribution description

If we do software retransmissions, we already keep a count.
Allow to query it with NETOPT_TX_RETRIES_NEEDED.

Testing procedure

Issues/PRs references

#14448

@benpicco benpicco requested review from jia200x and bergzand October 12, 2020 11:48
@benpicco benpicco added Area: network Area: Networking Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Oct 12, 2020
@benpicco benpicco force-pushed the ieee802154_submac-etx branch from 4871b00 to 2fc600d Compare October 12, 2020 14:10
netdev_ieee802154_submac_t *netdev_submac = container_of(submac,
netdev_ieee802154_submac_t,
submac);
netdev_t *netdev = (netdev_t *)netdev_submac;

netdev_submac->retrans = info->retrans;
Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, indeed this works. But maybe we should handle the case where there are HW retrans but no tx_info in the SubMAC. That way, the SubMAC always gives this information

Copy link
Member

Choose a reason for hiding this comment

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

alternatively, we can return an error when NETOPT_TX_RETRIES_NEEDED is called in such case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I wasn't sure if info could be NULL.

Comment on lines 212 to 218
if (info) {
netdev_submac->retrans = info->retrans;
} else {
netdev_submac->retrans = -1;
}

Copy link
Member

Choose a reason for hiding this comment

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

Since you are there, could you add this patch?

diff --git a/sys/net/link_layer/ieee802154/submac.c b/sys/net/link_layer/ieee802154/submac.c
index 0584ba95a3..392714b995 100644
--- a/sys/net/link_layer/ieee802154/submac.c
+++ b/sys/net/link_layer/ieee802154/submac.c
@@ -194,6 +194,9 @@ static void _handle_tx_success(ieee802154_submac_t *submac,
 
     if (ieee802154_radio_has_frame_retrans(dev) ||
         ieee802154_radio_has_irq_ack_timeout(dev) || !submac->wait_for_ack) {
+        if (!ieee802154_radio_has_frame_retrans_info()) {
+            info->retrans = -1;
+        }
         _tx_end(submac, info->status, info);
     }
     else {

And change these lines to:

Suggested change
if (info) {
netdev_submac->retrans = info->retrans;
} else {
netdev_submac->retrans = -1;
}
if (info) {
netdev_submac->retrans = info->retrans;
}

?
That way the SubMAC will always give its best effort to report the retransmissions. And if there's no info (TX_MEDIUM_BUSY or TX_NO_ACK), then netdev_submac->retrans doesn't get updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think turning the _get_cap() function into a bitfield would be a good optimization.

Copy link
Member

Choose a reason for hiding this comment

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

yes, we could!

@jia200x
Copy link
Member

jia200x commented Oct 15, 2020

Looks good to me. Please squash.
Also, could you add its own commit for the one that touches the radio HAL API?

@benpicco benpicco force-pushed the ieee802154_submac-etx branch from 189d55b to 914be5b Compare October 15, 2020 09:48
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 15, 2020
If we do software retransmissions, we already keep a count.
Allow to query it with `NETOPT_TX_RETRIES_NEEDED`.
@benpicco benpicco force-pushed the ieee802154_submac-etx branch from 914be5b to 87151db Compare October 15, 2020 11:26
Copy link
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.

ACK

@benpicco benpicco merged commit 4edf5c7 into RIOT-OS:master Oct 15, 2020
@benpicco benpicco deleted the ieee802154_submac-etx branch October 15, 2020 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

2 participants