tests/periph_i2c: Update to new I2C API #9168

MrKevinWeiss · 2018-05-23T14:24:31Z

Updated test for command shell interface to new i2c api. Added some features and simplified a bit.

Contribution description

-update to new api
-standardize variable names
-add new commands to match api closer
-remove i2c clock speed functionality

Issues/PRs references

Issue #6577
Involves PR #9162, #6576, #9257

dylad

Nice job ! just a few mistakes.

dylad · 2018-05-23T14:53:07Z

tests/periph_i2c/main.c


-    if (speed < 0 || speed > 4) {
-        puts("Error: Speed value not in range (0 - 4)");
+    res = i2c_init(dev);


i2c_init must not be call there. It only belongs to drivers/periph_common/init.c

dylad · 2018-05-23T14:53:45Z

tests/periph_i2c/main.c

+        puts("Error: Init: Given device not available");
+        return 1;
+    }
+    else if (res == 0){


space before curly brace

MrKevinWeiss · 2018-05-23T15:13:46Z

I will wait for all changes to the api before fixing things

MrKevinWeiss · 2018-05-28T10:14:52Z

@dylad I tested with #9198 and it seems to function, can we merge?

dylad · 2018-05-28T11:12:17Z

Let me (or someone else ) test it first and give an ACK.
Plus I would like to test on two different CPUs to validate this.

MrKevinWeiss · 2018-06-05T14:44:07Z

@dylad So it is a bit heavy but should support most all API and have a few nice features for later. Eventually I will do a minimal shell (binary based) but for now this should do. Once everything is up to date and merged it would be nice to make buildtest and see if memory issues occur on any board (right now they all fail because the API isn't fully ported). Here is it in working with the samr21 (ie, the API is working).

dylad · 2018-06-06T09:18:33Z

tests/periph_i2c/main.c

+    return 1;
+}
+
+int cmd_i2c_acquire(int argc, char **argv) {


I don't think we should use this here. acquire and release functions are supposed to be handled by the driver not the user. In the current case, if the user use cmd_i2c_acquire, it will block the I2C.

Yup I know, I think if it is in the API it should be tested (the fact that it blocks shouldn't be a problem for the test since I should be able to expect timeout and provide resets). If you don't wish to have this tested in the automated test we can remove it but I think since it is exposed to the user it should be tests. Also some clarification on it's expected behavior would be nice (ie is it required to acquire before use? Can we release twice without getting an error?)

You're right I mess up things sorry. acquire doesn't belong to CPU driver but device driver so we must be able to test the acquire/release function.
You can leave it as it.

smlng

first review round.

smlng · 2018-06-08T11:06:49Z

tests/periph_i2c/main.c

+        printf("Error: ETIMEDOUT [%d]\n", -res);
+        return 1;
+    }
+    else if (res == I2C_ACK) {


From the code below, this will never be used but is handled in the function with specific success output.
Meaning: this could be removed, or not?

I'd like to leave it in, just in case somebody does something in the future it won't say unknown error since nothing specifies that one must handle the ACK first.

fine with me

smlng · 2018-06-08T11:09:26Z

tests/periph_i2c/main.c

+
+int print_i2c_error(int res) {
+    if (res == -EOPNOTSUPP) {
+        printf("Error: EOPNOTSUPP [%d]\n", -res);


here and below: does it make sense to print the number, too? IMHO simply using puts("Error: EOPNOTSUPP!"); should be fine and enough.

The number is more there if the python (or any other) interface wants to use it, since it is a standard errno it may be easy just to feed it into a standard decoding function and get more info (I also don't think it adds too much bloat).

smlng · 2018-06-08T11:11:43Z

tests/periph_i2c/main.c

-    int dev, speed, res;
+int check_dev(void) {
+    if (i2c_dev < 0) {
+        puts("Error: no I2C device was initialized");


there is ENODEV which you could use here and print similarly to the other errors? See print_i2c_error, too

Yup, nice catch, will do! [TODO]

smlng · 2018-06-08T11:16:12Z

tests/periph_i2c/main.c

    dev = atoi(argv[1]);
-    speed = atoi(argv[2]);
+    if (dev < 0 || dev >= (int)I2C_NUMOF) {


minor nit, but this can be simplified to:

if (dev < 0 || dev >= (int)I2C_NUMOF) { puts("Error: Given device not available"); i2c_dev = -1; return 1; } printf("Success: i2c_%i successfully assigned as master\n", dev); i2c_dev = dev; return 0;

omitting the else branch because of the return in the if part and also the final return 1 would've never been reached, too.

Yup, will do. [TODO]

smlng · 2018-06-08T11:21:01Z

tests/periph_i2c/main.c

+        return 1;
+    }
+
+    if (argc != 1) {


why bother? You don't expect nor use arguments, so (IMHO) no need to check the number.

OK, I a bit think an exact structure should be enforced but I will change it anyways. [TODO]

smlng · 2018-06-08T11:21:11Z

tests/periph_i2c/main.c

-    res = i2c_init_master(dev, map_speed[speed]);
-    if (res == -1) {
-        puts("Error: Init: Given device not available");
+    if (argc != 1) {


dito, see above

smlng · 2018-06-08T11:22:43Z

tests/periph_i2c/main.c

+
+
+    if (res == I2C_ACK) {
+        printf("Success: i2c_%i successfully read 1 byte from reg 0x%02x: [",


why not having one printf only, instead of 2 + puts?

Yup, legacy stuff. [TODO]

smlng · 2018-06-08T11:29:23Z

tests/periph_i2c/main.c

+    res = i2c_read_byte(i2c_dev, addr, &data, flags);
+
+    if (res == I2C_ACK) {
+        printf("Success: i2c_%i successfully read 1 byte: [", i2c_dev);


nit picky: omit successfully because its already a Success 😉

applies to other output above and below, too.

Yup legacy, will do. [TODO] x3

smlng · 2018-06-08T11:34:27Z

tests/periph_i2c/main.c

+    }
+
+    if (res == I2C_ACK) {
+        printf("Success: i2c_%i successfully read %i bytes from reg 0x%02x: [",


IMHO its a bit confusing, that the user puts in the address of a register in decimal and its printed in HEX here, or not? At least from what I found atoi expects numbers of base 10, but you could use strtol with base 16 instead.

and btw. maybe this printing could be a helper function? Something like:

static inline print_i2c_read(i2c_t dev, uint16_t *reg, uint8_t *buf, size_t len) { assert ((dev > 0) && (len > 0)); printf("Success: i2c_%i read %i byte(s) ", dev, len); if (reg != Null) { printf("from reg 0x%02x ", reg); } printf(": [ "); for (unsigned i = 0; i < len; i++) { printf("0x%02x ", data[i]); } puts("]"); }

and then be use in i2c_read_reg(s)/byte(s)

I may just do strtol (legacy used atoi), I know lots of times I like seeing the hex numbers (since most data sheets support that). With the interface it gets abstracted out though. Either way I'll improve somehow. [TODO]

smlng · 2018-06-08T11:44:52Z

tests/periph_i2c/main.c

    int res;
    uint8_t addr;
-    int length = argc - 2;
+    uint8_t reg;


is this enough, because the API uses uint16_t, or not?

Yup, legacy stuff [TODO]

keestux · 2018-06-12T18:22:33Z

tests/periph_i2c/main.c


-    dev = atoi(argv[1]);
-    speed = atoi(argv[2]);
+static inline int get_num(char *str) {


Please put function opening curly on a new line

keestux · 2018-06-12T18:26:05Z

tests/periph_i2c/main.c

+{
+    (void)argv;
+    if (argc != 1) {
+        puts("Error: Aquire: Invalid number of arguments!");


Typo: acquire

keestux · 2018-06-12T18:39:36Z

tests/periph_i2c/main.c

-int print_i2c_error(int res)
-{
+
+int check_dev(void) {


More curlies that should go on a new line

tcschmidt · 2018-06-13T08:12:09Z

@smlng @MrKevinWeiss can we resolve the controversial change request?

smlng

@MrKevinWeiss: @keestux is right. Can you fix this, i.e. opening braces of function should be on a separate line (on their own). Otherwise the code looks good and ready to merge - but I would rather leave this to @dylad or @aabadie, who seem to manage the I2C rework

dylad · 2018-06-13T09:30:03Z

If @smlng thinks this PR is good to go after the final comment is adressed then I guess we can go :)

smlng · 2018-06-13T11:34:57Z

tests/periph_i2c/main.c

+static inline void print_i2c_read(i2c_t dev, uint16_t *reg, uint8_t *buf,
+    size_t len)
+{
+    printf("Success: i2c_%i read %i byte(s) ", dev, len);


travis complains here, should printf("Success: i2c_%i read %i byte(s) ", dev, (int)len); for instance.

casting length from size_t to int is the easiest, I guess.

@MrKevinWeiss can you fix this small issue before merge, because it will hit us anyway when this branch will be merged back into master.

oops missed that, sure!

smlng · 2018-06-13T11:37:58Z

@dylad apart from the minor issue travis reports, this is good to go. And @MrKevinWeiss would be happy to get this done (over with 😬) and work on refining the test scripts eventually making I2C testing easier for everyone, that is: no need to merge different PRs together to run tests.

dylad · 2018-06-13T11:45:17Z

@smlng I'm not an expert in testing/CI and so, but this looks OK to me. If all comments + Travis are addressed, we can click on the big green button :)

keestux · 2018-06-13T20:34:00Z

tests/periph_i2c/main.c

-        puts("Error: Speed value not in range (0 - 4)");
+    (void)argv;
+    (void)argc;
+    if (check_dev() == 1){


space before curly (there are more like these)

keestux · 2018-06-13T20:48:13Z

tests/periph_i2c/main.c

-        puts("]");
-        return 0;
+
+    switch (GET_I2C_CLK_SPD(i2c_dev)) {


The indenting of this switch/case statement is somewhat unusual. The Coding Convention does not say anything specific about it, but I like to see an extra indentation for the statements besides the "case".

switch (GET_I2C_CLK_SPD(i2c_dev)) { case case I2C_SPEED_LOW: printf("Success: i2..."); break; case I2C_SPEED_NORMAL:

keestux · 2018-06-13T20:55:03Z

tests/periph_i2c/main.c

+        }
+        printf("0x%02x", buf[i]);
+    }
+    puts("]");


Why the mix of puts and printf?
Personally I prefer to see just printf's, for consistency.

keestux · 2018-06-13T20:55:14Z

tests/periph_i2c/main.c

+    }
+    printf(": [");
+    for (int i = 0; i < len; i++) {
+        if (i != 0){


space before curly

keestux · 2018-06-13T20:58:27Z

tests/periph_i2c/main.c


-int cmd_init_master(int argc, char **argv)
+static inline int get_num(char *str)


I'd like to see "const char *" for the argument

dylad · 2018-06-14T07:42:40Z

If everything is addressed, I think you can squash.

smlng · 2018-06-14T08:04:20Z

@dylad we squashed it, you wanna hit merge?

dylad · 2018-06-14T08:06:48Z

Here we go !
Good job guys !

tests/periph_i2c: Update to new I2C API

dylad added the TF: I2C Marks issues and PRs related to the work of the I²C rework task force label May 23, 2018

dylad reviewed May 23, 2018

View reviewed changes

haukepetersen mentioned this pull request May 28, 2018

periph/i2c: tracking the remodeling #6577

Closed

47 tasks

MrKevinWeiss self-assigned this May 28, 2018

MrKevinWeiss mentioned this pull request May 31, 2018

sam0_common/i2c.c extra byte fix #9257

Closed

aabadie added the Area: tests Area: tests and testing framework label Jun 3, 2018

dylad reviewed Jun 6, 2018

View reviewed changes

This was referenced Jun 8, 2018

tests/periph_i2c: python interface for shell #9311

Closed

tests/periph_i2c: Add test to run with bpt and the py if #9312

Closed

smlng suggested changes Jun 8, 2018

View reviewed changes

keestux reviewed Jun 12, 2018

View reviewed changes

smlng suggested changes Jun 13, 2018

View reviewed changes

MrKevinWeiss added State: waiting for other PR State: The PR requires another PR to be merged first and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Jun 13, 2018

smlng approved these changes Jun 13, 2018

View reviewed changes

smlng reviewed Jun 13, 2018

View reviewed changes

keestux reviewed Jun 13, 2018

View reviewed changes

tests/periph_i2c: Update to new I2C API

5365b08

MrKevinWeiss force-pushed the n/pr/tests/periph_i2c branch from 468d8e3 to 067bff4 Compare June 14, 2018 07:54

tests/periph_i2c: refactor and standardize output

0beaf30

MrKevinWeiss force-pushed the n/pr/tests/periph_i2c branch from 067bff4 to 0beaf30 Compare June 14, 2018 08:00

dylad merged commit a2ee6ea into RIOT-OS:new_i2c_if Jun 14, 2018

basilfx pushed a commit to basilfx/RIOT that referenced this pull request Jul 10, 2018

Merge pull request RIOT-OS#9168 from MrKevinWeiss/n/pr/tests/periph_i2c

031a544

tests/periph_i2c: Update to new I2C API

dylad added a commit to dylad/RIOT that referenced this pull request Jul 10, 2018

Merge pull request RIOT-OS#9168 from MrKevinWeiss/n/pr/tests/periph_i2c

ba2ce7b

tests/periph_i2c: Update to new I2C API

MrKevinWeiss deleted the n/pr/tests/periph_i2c branch July 11, 2018 08:06

dylad added a commit that referenced this pull request Jul 11, 2018

Merge pull request #9168 from MrKevinWeiss/n/pr/tests/periph_i2c

d673752

tests/periph_i2c: Update to new I2C API



		if (res == I2C_ACK) {
		printf("Success: i2c_%i successfully read 1 byte from reg 0x%02x: [",


		int cmd_init_master(int argc, char **argv)
		static inline int get_num(char *str)

tests/periph_i2c: Update to new I2C API #9168

tests/periph_i2c: Update to new I2C API #9168

Conversation

MrKevinWeiss commented May 23, 2018 • edited Loading

Contribution description

Issues/PRs references

dylad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrKevinWeiss commented May 23, 2018

MrKevinWeiss commented May 28, 2018

dylad commented May 28, 2018

MrKevinWeiss commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smlng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcschmidt commented Jun 13, 2018

smlng left a comment

Choose a reason for hiding this comment

dylad commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smlng commented Jun 13, 2018

dylad commented Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylad commented Jun 14, 2018

smlng commented Jun 14, 2018

dylad commented Jun 14, 2018

MrKevinWeiss commented May 23, 2018 •

edited

Loading

dylad commented Jun 13, 2018 •

edited

Loading