Skip to content

testing: add periph I2C robot tests for HIL#10147

Closed
smlng wants to merge 6 commits intoRIOT-OS:masterfrom
smlng:pr/robot/i2c
Closed

testing: add periph I2C robot tests for HIL#10147
smlng wants to merge 6 commits intoRIOT-OS:masterfrom
smlng:pr/robot/i2c

Conversation

@smlng
Copy link
Member

@smlng smlng commented Oct 11, 2018

Contribution description

This adds a new test suite for the I2C peripheral drivers based on the RobotFramework and PHiLIP to allow for hardware-in-the-loop (HIL) testing.

Testing procedure

To run this you need a board (device under test) hooked up to a PHILIP test board, see https://github.com/riot-appstore/PHiLIP . Then run BOARD=<fill in> make -C tests/periph_i2c flash robot-test.

Issues/PRs references

depends on #10095

@smlng smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: tests Area: tests and testing framework Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR labels Oct 11, 2018
@smlng smlng requested review from MrKevinWeiss and cladmi October 11, 2018 10:03
@MrKevinWeiss
Copy link
Contributor

I ran the test on the nrf52dk (that supports reset on p21) with PHiLIP-N and all tests passed but make robot-open seems to have some problems with it.
When I tried everything together I received:
open: Unable to open /dev/tty2: Permission denied

When I tried with sudo and just robot-open I got:
objcopy not found. Hex file will not be created.

@smlng
Copy link
Member Author

smlng commented Oct 11, 2018

I think I remove the robot-open target, it was intended as a short-hand to open the generated html output and on macOS it works nicely, but Linux seems to have a different meaning of open.

@smlng smlng force-pushed the pr/robot/i2c branch 3 times, most recently from 2f87601 to 02c4730 Compare October 12, 2018 13:26
@smlng smlng force-pushed the pr/robot/i2c branch 4 times, most recently from 1d10d3a to 5717d91 Compare October 22, 2018 09:30
This introduces a new environment variable for a common directory
that holds all output of the build process, such as application or
package binaries. This would also allow to easily redirect output
to any other location, e.g. for out-of-source builds.

*** Keywords ***
DUT Must Have Periph I2C Application
API Call Should Succeed I2C Get ID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent initial error from connecting and resetting I think it would be good to send the I2C Get ID command once without any checks first, then send and check the id. Either include it here or do it before calling this keyword.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, will add a simple I2C Get ID without checking the result before this line


Suite Setup Run Keywords Reset DUT and PHILIP
... DUT Must Have Periph I2C Application
Test Setup Reset DUT and PHILIP
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think each test should include a DUT Must Have Periph I2C Application or at least send the I2C Get ID without checks. Maybe make a clear buffers command that does that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so basically Test Setup should be the same as Suite Setup

@MrKevinWeiss
Copy link
Contributor

Also I think it would be good for all data driven tests to have a acquire and release around it (maybe this can be done with the setup and teardown)

@MrKevinWeiss
Copy link
Contributor

also add the riot_pal version to the metadata

@MrKevinWeiss
Copy link
Contributor

... Also grep for verfiy, I think there are a few places that this occurs.

@MrKevinWeiss
Copy link
Contributor

hmmm, my suggestions still don't solve the remote-revb problems. Maybe a retry command n times would be better. Sometimes it takes 2 timeouts after the reset to recover.

@MrKevinWeiss
Copy link
Contributor

Here is my brute force solution for the remote-revb problems (but this should also help many other initial connection problems

--- a/tests/periph_i2c/tests/01__periph_i2c_base.robot
+++ b/tests/periph_i2c/tests/01__periph_i2c_base.robot
@@ -2,8 +2,13 @@
 Documentation       Basic tests to verify functionality of the periph I2C API.

 Suite Setup         Run Keywords    Reset DUT and PHILIP
+...                                 I2C Get ID
+...                                 I2C Get ID
 ...                                 DUT Must Have Periph I2C Application
-Test Setup          Reset DUT and PHILIP
+Test Setup          Run Keywords    Reset DUT and PHILIP
+...                                 I2C Get ID
+...                                 I2C Get ID
+...                                 I2C Get ID

 Resource            periph_i2c.keywords.txt
 Resource            api_shell.keywords.txt
diff --git a/tests/periph_i2c/tests/02__periph_i2c_write_register.robot b/tests/periph_i2c/tests/02__periph_i2c_write_register.robot
index 206013009..c73658cb1 100644
--- a/tests/periph_i2c/tests/02__periph_i2c_write_register.robot
+++ b/tests/periph_i2c/tests/02__periph_i2c_write_register.robot
@@ -2,8 +2,14 @@
 Documentation       Data driven tests to verify writing to a valid register.

 Suite Setup         Run Keywords    Reset DUT and PHILIP
+...                                 I2C Get ID
+...                                 I2C Get ID
 ...                                 DUT Must Have Periph I2C Application
-Test Setup          Reset DUT and PHILIP
+Test Setup          Run Keywords    Reset DUT and PHILIP
+...                                 I2C Get ID
+...                                 I2C Get ID
+...                                 I2C Get ID
+
 Test Template       I2C Write Bytes To Register Should Succeed

 Resource            periph_i2c.keywords.txt
==============================================================================
tests periph i2c ON remote-revb
==============================================================================
tests periph i2c ON remote-revb.Periph I2C Base :: Basic tests to verify fu...
==============================================================================
Acquire and Release Should Succeed :: Verify I2C acquire and relea... | PASS |
------------------------------------------------------------------------------
Acquire after Release Should Succeed :: Verify acquiring an I2C bu... | PASS |
------------------------------------------------------------------------------
Double Acquire Should Timeout :: Verify that acquiring a locked I2... | PASS |
------------------------------------------------------------------------------
Unacquired Read Register Should Succeed :: Verfiy reading a regist... | PASS |
------------------------------------------------------------------------------
Read Register After Release Should Error :: Verfiy reading a regis... | FAIL |
'Success' does not contain 'Error'
------------------------------------------------------------------------------
Read Register After NACK Should Succeed :: Verify recovery of I2C ... | PASS |
------------------------------------------------------------------------------
tests periph i2c ON remote-revb.Periph I2C Base :: Basic tests to ... | FAIL |
6 critical tests, 5 passed, 1 failed
6 tests total, 5 passed, 1 failed
==============================================================================
tests periph i2c ON remote-revb.Periph I2C Write Register :: Data driven te...
==============================================================================
One Byte Should Succeed                                               | PASS |
------------------------------------------------------------------------------
Two Bytes Should Succeed                                              | PASS |
------------------------------------------------------------------------------
Ten Bytes Should Succeed                                              | PASS |
------------------------------------------------------------------------------
tests periph i2c ON remote-revb.Periph I2C Write Register :: Data ... | PASS |
3 critical tests, 3 passed, 0 failed
3 tests total, 3 passed, 0 failed
==============================================================================
tests periph i2c ON remote-revb                                       | FAIL |
9 critical tests, 8 passed, 1 failed
9 tests total, 8 passed, 1 failed
==============================================================================

@smlng
Copy link
Member Author

smlng commented Nov 8, 2018

Also I think it would be good for all data driven tests to have a acquire and release around it (maybe this can be done with the setup and teardown)

yeah Test Setup and Test Teardown would be the best way, so we do not clutter the actual test

@smlng
Copy link
Member Author

smlng commented Nov 8, 2018

... Also grep for verfiy, I think there are a few places that this occurs.

what do you mean?

@smlng
Copy link
Member Author

smlng commented Nov 8, 2018

hmmm, my suggestions still don't solve the remote-revb problems. Maybe a retry command n times would be better. Sometimes it takes 2 timeouts after the reset to recover.

you mean I2C Get ID should be called multiple times, before running the test? I guess then it would be better to have something like: retry as long as it times out, but max n times

@MrKevinWeiss
Copy link
Contributor

Very nice. I like the changes and it seems to improve the non i2c related issues.

This introduces a new way to write RIOT tests using the RobotFramework,
starting by adding new make targets for easy usage.
This adds a couple of tests already ported to RobotFramework.
Add simple wrapper to run static-tests using RobotFramework, this adds
some nice HTML-output for free.
Add initial support for the periph_i2c test for the RobotFramework.
@kaspar030
Copy link
Contributor

kaspar030 commented Dec 12, 2018

Would you mind creating a version that does not depend on robot? No need to stall HIL testing for a framework discussion.

@tcschmidt
Copy link
Member

Would you mind creating a version that does not depend on robot? No need to stall HIL testing for a framework discussion.

No issue here: HIL testing is deployed and working. No need to intermingle with the asynchronous framework discussion.

@kaspar030
Copy link
Contributor

HIL testing is deployed and working. No need to intermingle with the asynchronous framework discussion.

Then we can close this?

@MrKevinWeiss
Copy link
Contributor

@kaspar030 please don't, at the moment we are using this for the HiL results in the nighties (and I have been using it for i2c improvements that can use some review 😉)

@MrKevinWeiss
Copy link
Contributor

Would you mind creating a version that does not depend on robot?

Checkout the tests/periph_i2c/tests/test.py (it doesn't use test runner and was meant as an example of what should be tested but it is an independent test).

@kaspar030
Copy link
Contributor

kaspar030 commented Dec 12, 2018

please don't, at the moment we are using this for the HiL results in the nighties

I know, and I didn't intent to. I tried to clarify what @tcschmidt means by "this is deployed and working" vs. there's an open PR.

edit deleted accidental duplicate post

@tcschmidt
Copy link
Member

@kaspar030 the point is simple: Framework discussion is asynchronous, nothing more.

@smlng
Copy link
Member Author

smlng commented Jan 17, 2019

closing in favour of #10774

@smlng smlng closed this Jan 17, 2019
@smlng smlng deleted the pr/robot/i2c branch June 25, 2019 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: tests Area: tests and testing framework Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants