[riot-notifications] [RIOT-OS/RIOT] ieee802154_submac: set TRX state only if SubMAC is idle (#15436)

José Alamos notifications at github.com
Thu Nov 12 17:31:28 CET 2020


<!--
The RIOT community cares a lot about code quality.
Therefore, before describing what your contribution is about, we would like
you to make sure that your modifications are compliant with the RIOT
coding conventions, see https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions.
-->

### Contribution description
This PR fixes a non valid Radio HAL API access from `ieee802154_submac`.
The SubMAC tries to go to its idle state on reception (either RX_ON or TRX_OFF). The problem is, the event is indicated first and right after the function returns the SubMAC tries to set the transceiver state.

If the upper layer wanted to send data (e.g `gnrc_netif_pktq` trying to dequeue a packet on RX_DONE), the SubMAC might call the `ieee802154_radio_set_trx_state` while the transmission is ongoing. This breaks the Radio HAL API.
<!--
Put here the description of your contribution:
- describe which part(s) of RIOT is (are) involved
- if it's a bug fix, describe the bug that it solves and how it is solved
- you can also give more information to reviewers about how to test your changes
-->


### Testing procedure
Try the following patch in master:
```diff --git a/sys/net/link_layer/ieee802154/submac.c b/sys/net/link_layer/ieee802154/submac.c
index 23653b58c4..5cb1e9f2b6 100644
--- a/sys/net/link_layer/ieee802154/submac.c
+++ b/sys/net/link_layer/ieee802154/submac.c
@@ -27,6 +27,8 @@
 #define CSMA_SENDER_BACKOFF_PERIOD_UNIT_MS  (320U)
 #define ACK_TIMEOUT_US                      (864U)
 
+bool _request;
+
 static void _handle_tx_no_ack(ieee802154_submac_t *submac);
 
 static void _tx_end(ieee802154_submac_t *submac, int status,
@@ -169,6 +171,7 @@ void ieee802154_submac_rx_done_cb(ieee802154_submac_t *submac)
     }
     else {
         submac->cb->rx_done(submac);
+        assert(_request == false);
 
         /* The Radio HAL will be in "FB Lock" state. We need to do a state
          * transition here in order to release it */
@@ -242,6 +245,7 @@ void ieee802154_submac_tx_done_cb(ieee802154_submac_t *submac)
     ieee802154_tx_info_t info;
 
     ieee802154_radio_confirm_transmit(dev, &info);
+    _request = false;
 
     switch (info.status) {
     case TX_STATUS_MEDIUM_BUSY:
@@ -267,6 +271,7 @@ int ieee802154_send(ieee802154_submac_t *submac, const iolist_t *iolist)
     uint8_t *buf = iolist->iol_base;
     bool cnf = buf[0] & IEEE802154_FCF_ACK_REQ;
 
+    _request = true;
     if (submac->state == IEEE802154_STATE_OFF) {
         return -ENETDOWN;
     }
```

Try stress ping. At some point, the assertion will be hit.

Then, try the following patch with this PR:
```diff --git a/sys/net/link_layer/ieee802154/submac.c b/sys/net/link_layer/ieee802154/submac.c
index 2ee90c79e0..bf3b4291fd 100644
--- a/sys/net/link_layer/ieee802154/submac.c
+++ b/sys/net/link_layer/ieee802154/submac.c
@@ -27,6 +27,8 @@
 #define CSMA_SENDER_BACKOFF_PERIOD_UNIT_MS  (320U)
 #define ACK_TIMEOUT_US                      (864U)
 
+bool _request;
+
 static void _handle_tx_no_ack(ieee802154_submac_t *submac);
 
 static void _tx_end(ieee802154_submac_t *submac, int status,
@@ -169,6 +171,7 @@ void ieee802154_submac_rx_done_cb(ieee802154_submac_t *submac)
     }
     else {
         submac->cb->rx_done(submac);
+        assert(_request == false);
 
         /* The only set the radio to the SubMAC default state only if the upper
          * layer didn't try to send more data. Otherwise there's risk of not
@@ -248,6 +251,7 @@ void ieee802154_submac_tx_done_cb(ieee802154_submac_t *submac)
     ieee802154_tx_info_t info;
 
     ieee802154_radio_confirm_transmit(dev, &info);
+    _request = false;
 
     switch (info.status) {
     case TX_STATUS_MEDIUM_BUSY:
@@ -273,6 +277,7 @@ int ieee802154_send(ieee802154_submac_t *submac, const iolist_t *iolist)
     uint8_t *buf = iolist->iol_base;
     bool cnf = buf[0] & IEEE802154_FCF_ACK_REQ;
 
+    _request = true;
     if (submac->state == IEEE802154_STATE_OFF) {
         return -ENETDOWN;
     }
```

There shouldn't be any assertion.
<!--
Details steps to test your contribution:
- which test/example to compile for which board and is there a 'test' command
- how to know that it was not working/available in master
- the expected success test output
-->


### Issues/PRs references
None so far
<!--
Examples: Fixes #1234. See also #5678. Depends on PR #9876.

Please use keywords (e.g., fixes, resolve) with the links to the issues you
resolved, this way they will be automatically closed when your pull request
is merged. See https://help.github.com/articles/closing-issues-using-keywords/.
-->

You can view, comment on, or merge this pull request online at:

  https://github.com/RIOT-OS/RIOT/pull/15436

-- Commit Summary --

  * ieee802154_submac: set TRX state only if SubMAC is idle

-- File Changes --

    M sys/net/link_layer/ieee802154/submac.c (26)

-- Patch Links --

https://github.com/RIOT-OS/RIOT/pull/15436.patch
https://github.com/RIOT-OS/RIOT/pull/15436.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/15436
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20201112/7d510142/attachment.htm>


More information about the notifications mailing list