-
Notifications
You must be signed in to change notification settings - Fork 2.1k
ieee802154/submac: add automatic ACK transmission when AUTO_ACK is not supported by driver #21533
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
Conversation
cpu/native/socket_zep/socket_zep.c
Outdated
| #include "debug.h" | ||
|
|
||
| #ifndef SOCKET_ZEP_AUTO_ACK | ||
| #define SOCKET_ZEP_AUTO_ACK 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
socket_zep is a pure software implementation, if the submac now provides an auto-ACK mechanism just drop it from socket_zep entirely - we don't need to maintain two code paths and the main goal of socket_zep is to test the common infrastructure, so we want to keep it as minimal as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For easier review and testing I did not delete it yet to make it easy to switch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either the feature works, then you can remove the functionality from the driver and rely on the submac.
Or the feature does not work yet, then we shouldn't merge the PR yet.
Keeping this clutter in the driver doesn't make review easier and testing should be finished when we merge the feature.
| } | ||
| case NETOPT_AUTOACK: | ||
| *((netopt_enable_t *)value) | ||
| = ieee802154_radio_has_capability(&submac->dev, IEEE802154_CAP_AUTO_ACK); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to be able to disable auto-ACK (e.g. for the sniffer application) independed from whether it is implemented by the hardware of by the submac
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RIOT/sys/net/gnrc/netif/ieee802154/gnrc_netif_ieee802154.c
Lines 214 to 221 in c09c7d1
| #if !IS_ACTIVE(CONFIG_IEEE802154_AUTO_ACK_DISABLE) | |
| if ((mhr[0] & IEEE802154_FCF_TYPE_MASK) == IEEE802154_FCF_TYPE_DATA && | |
| (mhr[0] & IEEE802154_FCF_ACK_REQ)) { | |
| netopt_enable_t auto_ack = NETOPT_DISABLE; | |
| netopt_enable_t promiscuous = NETOPT_DISABLE; | |
| if (dev->driver->get(dev, NETOPT_PROMISCUOUSMODE, &promiscuous, sizeof(promiscuous)) > 0 && | |
| dev->driver->get(dev, NETOPT_AUTOACK, &auto_ack, sizeof(auto_ack)) > 0 && | |
| auto_ack != NETOPT_ENABLE && promiscuous != NETOPT_ENABLE) { |
The idea as to not send automatic ACK when in promiscuous mode.
| netopt_enable_t promiscuous = NETOPT_DISABLE; | ||
| if (dev->driver->get(dev, NETOPT_PROMISCUOUSMODE, &promiscuous, sizeof(promiscuous)) > 0 && | ||
| dev->driver->get(dev, NETOPT_AUTOACK, &auto_ack, sizeof(auto_ack)) > 0 && | ||
| auto_ack != NETOPT_ENABLE && promiscuous != NETOPT_ENABLE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is sending the ACK done in gnrc_netif_ieee802154 instead of the submac?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seemed to be the right place to me because then
- all needed information is there
- ACK is only sent after the security check passed
- also works for drivers not supporting submac.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand, the ACK is purely for confirming the reception. If the security check didn't pass, retransmitting the packet won't change anything about that.
- also works for drivers not supporting submac.
This is a non-feature. The non-submac drivers are expected to implement the full MAC layer themselves / in hardware, so adding software fallbacks there seems to be going in the wrong direction. New drivers should just make use of submac.
Worse yet, with this the auto-ACK mechanism only works with GNRC. So the driver will behave differently depending on which network stack sits on top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish I could find a clear statement about that, but Google and AI tend to say only for frames that passed the security layer.
auto-ACK mechanism only works with GNRC
This means other stacks (openwsn and lwip) rely on hardware support as of now.
IMO a problem would only be when the hardware generates an ACK + the network stack generates an ACK (or none generates an ACK).
If it was implemented in submac the feature of software ACK would exist for any stack.
Yes, this would be better, but I would not call it a problem if only GNRC supports software ACK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not that much into IEEE 802.15.4 security, but when you say a frame does not pass the security layer, does that mean it was not encrypted with the right key?
If there is no ACK, the sender can not tell that apart from a frame that never arrived at the receiver - so it will just send the same frame again, it will still be rejected by the security layer. This will go on until retransmissions are exceeded - that doesn't seem like it's the intended behavior - or am I misunderstanding something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is I'd really keep the mac layer functionality in the submac and not concern GNRC with it. Moving the l2 ACK sending into GNRC seems like an unnecessary layering violation.
When you keep it in the submac, you also don't have to expose IEEE802154_CAP_AUTO_ACK via NETOPT_AUTOACK which is rather a hack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem I see now is that when it is not in submac, the drivers cannot just drop the ACK transmission logic, because that relies on a GNRC feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly - I really don't want to have two code paths in all drivers that want to use that, one for GNRC and one for !GNRC.
And if we need the !GNRC code path anyway, adding this feature to the submac becomes moot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On submac I would code it in here: https://github.com/fabian18/RIOT/blob/ed44a434641ff0516a01fea885c5e32625beb558/sys/net/link_layer/ieee802154/submac.c#L150-L152
A problem is, that I have to read the frame to know if it requires an ACK.
- For reading the frame I need a buffer which would normally be a
gnrc_pktsnip_tingnrc_netif_ieee802154.c - Once I read the frame from the driver, it will not be available in
gnrc_netif_ieee802154.c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4516ff1 to
b115377
Compare
b115377 to
dd7a503
Compare
| unsigned state = irq_disable(); | ||
| uint32_t flags = netdev_submac->isr_flags; | ||
| netdev_submac->isr_flags &= ~clear; | ||
| irq_restore(state); | ||
| return flags; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| unsigned state = irq_disable(); | |
| uint32_t flags = netdev_submac->isr_flags; | |
| netdev_submac->isr_flags &= ~clear; | |
| irq_restore(state); | |
| return flags; | |
| return atomic_fetch_and_u32(&netdev_submac->isr_flags, clear); |
| netdev_rx_info->lqi = rx_info.lqi; | ||
| } | ||
|
|
||
| #if !IS_ACTIVE(CONFIG_IEEE802154_AUTO_ACK_DISABLE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't put this behind a config but behind a pseudo-module that the driver can select if it needs software-ACKs.
Also better put it into a static helper function to not clutter _recv() too much.
That allows you to
if (ieee802154_radio_has_capability(&submac->dev, IEEE802154_CAP_AUTO_ACK) {
return;
}to keep the nesting of if blocks down.
|
|
||
| static const ieee802154_radio_ops_t cc2538_rf_ops = { | ||
| .caps = IEEE802154_CAP_24_GHZ | ||
| | IS_ACTIVE(CONFIG_IEEE802154_AUTO_ACK_DISABLE) ? 0 : IEEE802154_CAP_AUTO_ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This I don't get - the hardware doesn't loose the capability just because it's disabled by software - in fact the submac(?) will probably want to ensure to disable auto-ACKs in the driver if this config option is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The submac ACK is sent when the driver does not have IEEE802154_CAP_AUTO_ACK.
Should I change all the drivers to use the submac ACK or should all drivers which support auto ACK just have the compatibility flag. For ZEP I removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I understand CONFIG_IEEE802154_AUTO_ACK_DISABLE is that when it's set RIOT will not send ACKs, whether by software or by hardware.
Why one would need such an option I don't know, but it surely isn't meant to switch between software and hardware ACKs.
63fa3a0 to
f9aed0e
Compare
benpicco
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more nit, then I think this is good.
crasbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to be so late to the party 👀
Just a little nitpick.
d6b5b5b to
d62ac67
Compare
d62ac67 to
1dcc3db
Compare
1dcc3db to
fccb0ea
Compare
Contribution description
Adds transmission of ACK on the IEEE802154 layer, if the driver does not support automatic ACK.
A submac state is added. The ACK transmission is synchronous to the driver to not accept further frame transmissions in between.
Testing procedure
sudo ./dist/tools/tapsetup/tapsetupmake -C examples/networking/gnrc/gnrc_border_router flash term PORT=tap0USE_ZEP=1 make -C examples/networking/gnrc/gnrc_networking flash term PORT=tap1socket_zep.pcapng.zip
Issues/PRs references
split out from #21202