[riot-notifications] [RIOT-OS/RIOT] sys/analog_util: Refactor, add test (#8577)

Gaƫtan Harter notifications at github.com
Tue Feb 20 13:38:05 CET 2018


cladmi requested changes on this pull request.

Some remarks regarding the implementation.

>  }
 
 float adc_util_mapf(int sample, adc_res_t res, float min, float max)
 {
-    return ((((max - min) * sample) / val_max[res]) + min);
+    return ((((max - min) * sample) / (1 << _adc_res_bits(res))) + min);

As `_adc_res_bits(res)` can have a value of 16, I think this `1` should be set to be 32 bit wide or it will break on 16bit platforms.

Also right now, there is a by one difference from the previous version when dividing. Not sure if it matters but just noticing.

> +#include "analog_util.h"
+
+#define ENABLE_DEBUG (0)
+#include "debug.h"
+
+typedef struct {
+    int expected;
+    int sample;
+    int min;
+    int max;
+    adc_res_t res;
+} test_values_t;
+
+/* Arbitrarily chosen test vectors */
+/* TODO: Choose test vectors in a more qualified manner to catch any edge cases */
+static test_values_t test_data[] = {

I was wondering what it meant to have min >= max, and by checking the API I found its forbidden:

https://github.com/RIOT-OS/RIOT/blob/2018.01/sys/include/analog_util.h#L36

Really adding an `assert` there could be better.

> +
+#include "analog_util.h"
+
+#define ENABLE_DEBUG (0)
+#include "debug.h"
+
+typedef struct {
+    int expected;
+    int sample;
+    int min;
+    int max;
+    adc_res_t res;
+} test_values_t;
+
+/* Arbitrarily chosen test vectors */
+/* TODO: Choose test vectors in a more qualified manner to catch any edge cases */

Compared to the previous version, its dividing by a resolution value which is one bigger, would be good I think to show a edge case like this.

> +
+#include "analog_util.h"
+
+#define ENABLE_DEBUG (0)
+#include "debug.h"
+
+typedef struct {
+    int expected;
+    int sample;
+    int min;
+    int max;
+    adc_res_t res;
+} test_values_t;
+
+/* Arbitrarily chosen test vectors */
+/* TODO: Choose test vectors in a more qualified manner to catch any edge cases */

Also test does not pass on `samr21-xpro` with the previous version on commit `8edcbd1... unittests: Add analog_util tests`, so would be nice to have a working test fand an updated test when changing the function.

-- 
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/8577#pullrequestreview-97803641
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20180220/972a153d/attachment.html>


More information about the notifications mailing list