nrf802154: Add ACK handling capabilities

[bergzand]

Contribution description

This PR adds ack handling to the nrf802154 radio driver. Both ACKing incoming frames and ACK requests with retransmissions are supported. The number of retransmissions is configurable, but within the bounds specified by IEEE 802.15.4.

I can split this PR into the autoack and ack_req parts if preferred.

with this PR having +246 lines of code I took the liberty of adding myself to the authors list.

Testing procedure

Manual testing with a sniffer and another node is preferred here:

• Acks correctly transmitted when requested by a peer
• Acks not transmitted while requested by a peer when autoack is disabled
• Frame transmitted with ack_req bit set is not retransmitted when a proper ack is received
• Frame is retransmitted when no ack is received.
• Frame is not retransmitted when ack_req is disabled.
• Multicast frames are not retransmitted and don't have ack_req flag set while ack_req is set in the interface settings
• It must not be possible to set the max retransmissions above 7.

And not immediately critical, but nice to have tested:

• Must be at least SIFS symbols between frame received and ack transmitted.
• Must be {S|L}IFS symbols between ack reception and next transmission.
• driver reports NETDEV_EVENT_TX_NOACK when no ack received.
• driver always reports NETDEV_EVENT_TX_COMPLETE when ack_req is disabled.

Issues/PRs references

depends on #11146

[PeterKietzmann]

There once was #4795 which proposed a generic mechanism outside the driver which seems reasonable IMO, as it is a MAC feature. However, I can imagine it was timing issues that lead to your decision to implement the feature next to the hardware. Can you give short feedback?

[PeterKietzmann]

@jia200x wanna have a look at this too?

[bergzand]

@PeterKietzmann Timing was one of the issues, although that matters more for answering a frame with ack requests set than receiving ack. Assuming I understand the architecture proposed with #4795 the low level driver still has to handle ACK timeouts and signal ACK_TIMEOUT back to the retransmission code. Including the code to configure and handle retransmissions wasn't a lot of extra effort here. Being able to work directly on top of the low level driver code was a definite advantage here.

I agree that in the future, it might be possible to refactor the retransmission and maybe even the ack handling to a different module

[bergzand]

This should also validate the psdu length

[bergzand]

rebased and added additional checks to the ack reception handling

[SemjonWilke]

I just tested it and had some issues. I tested with samr21xpro and nrf5284dk all 16 combinations of ack_req, autoack, -ack_req and -autoack sending with both boards (multiple times) and wrote the results into a table.
I expected to receive some kind of a symmetric matrix, since combinations should have the same effect when swapping them between the boards, but this was clearly not the case.

Something that always worked was -ack_req on both boards.

Also I found some interesting bug with ping when sending from samr to nrf with combination [samr with autoack and -ack_req] and [nrf with autoack and ack_req]:
INFO # 2 packets transmitted, 3 packets received, 3 duplicates, 2147483598% packet loss
I've seen this bug before this PR and it occured with different combinations sometimes, but for the first time I could reproduce it everytime with this one combination. Can you confirm my finding, I would raise an issue then.

[SemjonWilke]

I know this will be annoying, but can you split this PR? I think it is easier to debug the modules independently and also it will probably go in much faster that way.

[bergzand]

I know this will be annoying, but can you split this PR? I think it is easier to debug the modules independently and also it will probably go in much faster that way.

Nah, you're totally right, should have splitted this from the start.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

[benpicco]

With #14950 and #14802 this should now be obsolete.

[SemjonWilke]

@benpicco you are not wrong, but maybe we should wait until nrf802154 is marked obsolete and is going to be removed.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

[SemjonWilke]

Since the module is still default for some boards, this PR is not completely obsolete - even though it would need a lot of work to revive.
https://github.com/RIOT-OS/RIOT/blob/master/boards/common/nrf52/Makefile.nrf802154.dep#L3
I think deprecating nrf802154 in favor of 15.4 submac should happen soonish.

[benpicco]

Since the module is still default for some boards, this PR is not completely obsolete

Which boards are that?
And what is the point of implementing something in a driver for which there is now a common layer?

[jeandudey]

https://github.com/RIOT-OS/RIOT/blob/master/boards/common/nrf52/Makefile.nrf802154.dep#L3
I think deprecating nrf802154 in favor of 15.4 submac should happen soonish.

nrf802154 defaults to the SubMac, nrf802154_netdev_legacy defaults to the direct netdev driver. So, unless specified directly on your board/application it uses the SubMac.

I think we should go forward with with the HAL/SubMac and add the features on top of it whenever possible if it benefits other drivers too.

See also #15886

And what is the point of implementing something in a driver for which there is now a common layer?

The ACK handling is still implemented on driver though, only for nrf5280154 and #13295 . I think some devices out there don't include Auto-ACK capabilities so it might be a thing to discuss for the SubMac.

[SemjonWilke]

The ACK handling is still implemented on driver though, only for nrf5280154 and 13295 . I think some devices out there don't include Auto-ACK capabilities so it might be a thing to discuss for the SubMac.

Sorry, that I am not up to date. From what you say and what I see in https://github.com/RIOT-OS/RIOT/blob/master/cpu/nrf52/radio/nrf802154/nrf802154_radio.c there is a working implementation of ACK for the SubMac of nrf52 CPUs and it's set as default radio.

In that case I'd suggest closing this PR with no further ado.

[benpicco]

obsoleted by #16630