[riot-notifications] [RIOT-OS/RIOT] WIP: tests/ieee802154_hal: extend radio hal test (#15761)

José Alamos notifications at github.com
Wed Jun 9 14:28:17 CEST 2021


@jia200x requested changes on this pull request.

I think the overall architecture looks ok. Some of the global variables must stay there because they are referenced by different shell functions.
In the future (when this test supports multiple radios) we should simply keep them in structures and allocate as many structures as radios.

In any case, here's a list of suggestions. Feel free to commit them directly (they are not tested, so might need some tiny adjustments)

> @@ -56,17 +76,27 @@ static uint8_t seq;
 static void _print_packet(size_t size, uint8_t lqi, int16_t rssi)

We should probably rename this function to `_handle_packet`, since now it does more than printing.

>          for (unsigned i=0;i<size;i++) {
-            printf("%02x ", buffer[i]);
+            DEBUG("%02x ", buffer[i]);
+        }
+        received_packets++;
+        if (send_reply) {
+            uint8_t out[IEEE802154_LONG_ADDRESS_LEN];
+            unsigned j = 0;
+            for (unsigned i=20;i>12;i--) {

there's a [builtin function for extracting the source address](https://doc.riot-os.org/group__net__ieee802154.html#ga25610e6c53c638b14e36b2372e12e75d).

> +        if (send_reply) {
+            uint8_t out[IEEE802154_LONG_ADDRESS_LEN];
+            unsigned j = 0;
+            for (unsigned i=20;i>12;i--) {
+                out[j] = buffer[i];
+                j++;
+            }
+            send(out, IEEE802154_LONG_ADDRESS_LEN, iol_data_spam(), 1, 0);

```suggestion
        if (send_reply) {
            uint8_t out[IEEE802154_LONG_ADDRESS_LEN];
            le_uint16_t tmp;
            int src_len = ieee802154_get_src(buffer, out, &tmp);
            send(out, src_len, iol_data_spam(), 1, 0);
```

> +                                            dst, dst_len,
+                                            src_pan, dst_pan,
+                                            flags, seq++)) < 0) {
+            puts("txtsnd: Error preperaring frame");
+            return 1;
+        }
+
+        iolist_t iol_hdr = {
+            .iol_next = &iol_data,
+            .iol_base = mhr,
+            .iol_len = mhr_len,
+        };
+        _send(&iol_hdr);
+        xtimer_msleep(time);
+    }
+    if (ENABLE_DEBUG) {

I just realized this lines don't affect timings because they are displayed by the end of the test. Therefore I think it would be useful to keep them

> +    if (ENABLE_DEBUG) {
+        puts("-------Summary of the test-------");
+        printf("Send Packets: %d\n", send_packets);
+        printf("Acknowledged Packets: %d\n", received_acks);
+        printf("Percentage: %d\n", (received_acks * 100)/num);
+        printf("Received Packets: %d\n", received_packets);
+        puts("---------------------------------");
+    }

```suggestion
    puts("-------Summary of the test-------");
    printf("Send Packets: %d\n", send_packets);
    printf("Acknowledged Packets: %d\n", received_acks);
    printf("Percentage: %d\n", (received_acks * 100)/num);
    printf("Received Packets: %d\n", received_packets);
    puts("---------------------------------");
```

> +    }
+    res = _parse_addr(addr, sizeof(addr), argv[1]);
+    if (res == 0) {
+        puts("Usage: txtsnd <long_addr> <len>");
+        return 1;
+    }
+    len = atoi(argv[2]);
+    iolist_t iol_data = {
+        .iol_base = payload,
+        .iol_len = len,
+        .iol_next = NULL,
+    };
+    return send(addr, res, iol_data, 1, 0);
+}
+
+iolist_t iol_data_spam(void) {

This pattern here is a bit tricky to follow because it adds a reference to a global pointer. But I get the spirit of trying to reduce code.

Why don't you try something like this?:
```c
static inline void _populate_iolist(iolist_t *iol, void *buf, size_t len, iolist_t *next)
{
    iol->iol_base = buf;
    iol->iol_len = len;
    iol->iol_next = next;
}
```

Then you can simply call them like this (taken from `txtspam`):
```c
    num = atoi(argv[2]);
    time = atoi(argv[3]);
    iolist_t pkt;
    _populate_iolist(&pkt, &channel_number, sizeof(channel_number), NULL);
    return send(addr, res, iol_data_spam(), num, time);
```

> +    iolist_t iol_data = {
+        .iol_base = channel_ptr,
+        .iol_len = sizeof(*channel_ptr),
+        .iol_next = NULL,
+    };
+    return iol_data;
+}
+
+int txtspam(int argc, char **argv) {
+    uint8_t addr[IEEE802154_LONG_ADDRESS_LEN];
+    size_t res;
+    size_t num;
+    size_t time;
+
+    if (argc != 4) {
+        puts("Usage: txtspam <long_addr> <number of packets> <time in ms between packets>");

I think it would be useful to pass the payload length here too.
We can reuse the "Lorem Ipsum" logic here and keep the "channel payload" part for the channel test

> +    if (argc != 4) {
+        puts("Usage: txtspam <long_addr> <number of packets> <time in ms between packets>");
+        return 1;
+    }
+    res = _parse_addr(addr, sizeof(addr), argv[1]);
+    if (res == 0) {
+        puts("Usage: txtspam <long_addr> <number of packets> <time in ms between packets>");
+        return 1;
+    }

Regardless of the proposal described above, you can condense these snippets:
```suggestion
    if (argc != 4 || !(res = _parse_addr(addr, sizeof(addr), argv[1]))) {
        puts("Usage: txtspam <long_addr> <number of packets> <time in ms between packets>");
        return 1;
    }
```

> +    printf("Request: %d\n", request_counter);
+    printf("Confirm: %d\n", confirm_counter);

This left over code should be removed
```suggestion
```

> +uint16_t confirm_counter = 0;
+uint16_t request_counter = 0;

This left over code should be removed
```suggestion
```

> +        puts("-------Summary of the test-------");
+        printf("Send Packets: %d\n", send_packets);
+        printf("Acknowledged Packets: %d\n", received_acks);
+        printf("Percentage: %d\n", (received_acks * 100)/num);
+        printf("Received Packets: %d\n", received_packets);
+        puts("---------------------------------");

Some radios handle retransmissions automatically or don't forward the ACK frame to the upper layer.
For skipping this case, you can use the "has_xxx" functions of the Radio HAL.

In this specific case this should be enough:

```suggestion
        ieee802154_dev_t *dev = ieee802154_hal_test_get_dev(RADIO_DEFAULT_ID);
        puts("-------Summary of the test-------");
        printf("Send Packets: %d\n", send_packets);
        if (ieee802154_radio_set_frame_retrans(dev) || ieee802154_radio_has_irq_ack_timeout(dev)) {
            printf("Acknowledged Packets: %d\n", received_acks);
            printf("Percentage: %d\n", (received_acks * 100)/num);
        }
        printf("Received Packets: %d\n", received_packets);
        puts("---------------------------------");
```

> @@ -740,10 +865,9 @@ int main(void)
     mutex_init(&lock);
     mutex_lock(&lock);
     _init();
-
+    current_channel = 26;

```suggestion
    current_channel = CONFIG_IEEE802154_DEFAULT_CHANNEL;
```

> @@ -0,0 +1,32 @@
+# Used Devices

I think it would be good to write here what the riotctrl related functions are doing. At least `txtspam`, `check_last_packet`, etc

> @@ -218,7 +218,6 @@ static int _read(ieee802154_dev_t *dev, void *buf, size_t size, ieee802154_rx_in
         res = 0;
     }
 
-

ping

>          for (unsigned i=0;i<size;i++) {
-            printf("%02x ", buffer[i]);
+            DEBUG("%02x ", buffer[i]);
+        }
+        received_packets++;
+        if (send_reply) {
+            uint8_t out[IEEE802154_LONG_ADDRESS_LEN];
+            unsigned j = 0;
+            for (unsigned i=20;i>12;i--) {

See below

> +    if (argc != 4) {
+        puts("Usage: txtspam <long_addr> <number of packets> <time in ms between packets>");
+        return 1;
+    }
+    res = _parse_addr(addr, sizeof(addr), argv[1]);
+    if (res == 0) {
+        puts("Usage: txtspam <long_addr> <number of packets> <time in ms between packets>");
+        return 1;
+    }

I just noticed this comes from the old `txtsnd` function. You can either leave it as it is or try to condense them :)

-- 
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/15761#pullrequestreview-679541669
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210609/5dac092e/attachment-0001.htm>


More information about the notifications mailing list