[riot-notifications] [RIOT-OS/RIOT] tests/mtd_raw: add simple test for MTD (#15362)

Marian Buschsieweke notifications at github.com
Wed Jan 6 21:27:44 CET 2021


@maribu commented on this pull request.

looks good to me, some comments inline

> + * @param[in] offset    Adds an offset to the printed memory addresses.
+ * @param[in] offset    If the origin of the data is an address in memory,
+ *                      this can be used to print the real addresses together
+ *                      with the data.

Something went wrong here

> +
+    unsigned idx = atoi(argv[1]);
+
+    if (idx > MTD_NUMOF) {
+        printf("%s: invalid device: %s\n", argv[0], argv[1]);
+        *oob = true;
+        return NULL;
+    }
+
+    *oob = false;
+    return _get_mtd_dev(idx);
+}
+
+static uint64_t _get_size(mtd_dev_t *dev)
+{
+    return dev->sector_count * dev->pages_per_sector * dev->page_size;

If I recall correctly, non of the three is 64 bit wide. Hence, the multiplication is done in 32 bit and only afterwards extended to 64 bit, so the 32 most significant bits are always zero, even if the correct result is > 2^32 - 1.

> +    if (oob) {
+        return -1;
+    }

Is `dev == NULL` valid? If not, why is `oob` needed?

> +
+    int res = mtd_erase_sector(dev, sector, count);
+
+    if (res) {
+        printf("error: %i\n", res);
+    }
+
+    return res;
+}
+
+static void _print_info(mtd_dev_t *dev)
+{
+    printf("sectors: %"PRIu32"\n", dev->sector_count);
+    printf("pages per sector: %"PRIu32"\n", dev->pages_per_sector);
+    printf("page size: %"PRIu32"\n", dev->page_size);
+    printf("total: %lu\n", (unsigned long)_get_size(dev));

What wrong with?
```suggestion
    printf("total: %"PRIu64"\n", _get_size(dev));
```

> +        return -1;
+    }
+
+    _print_info(dev);
+
+    return 0;
+}
+
+static int cmd_power(int argc, char **argv)
+{
+    bool oob;
+    mtd_dev_t *dev = _get_dev(argc, argv, &oob);
+    enum mtd_power_state state;
+
+    if (argc < 3) {
+        goto error;

How about adding


```C
static int _print_usage(const char *progname) {
    printf("usage: %s <dev> <on|off>\n", progname);
    return -1;
}
```

and the replacing all `goto error;` by `return _print_usage(argv[0]);`?

-- 
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/15362#pullrequestreview-563004901
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210106/529ad788/attachment.htm>


More information about the notifications mailing list