It would be good to give more information on how to use philip in tests. This could just be a link or a full API description for Robot

This should be an independant command I think. Maybe we should also have a flag in the test to let us know to expect plots.

In case the user started a build with a faulty configuration, it might be useful to cancel it instead of waiting for the whole pipeline to finish. The ownership-plugin seems to allow this by using the @currentUserIsOwner or the @currentUserIsPrimaryOwner role. Link to plugin page.

The global admin will still be able to override that and cancel any job.

When I was testing PR #12955 as DUT with a nucleo-f103rb, I encountered a problem with tests/periph_uart. While all tests are working with ESP32 and the current RIOT master, the following tests fail with PR #12955 for the ESP32.

------------------------------------------------------------------------------
Extended Short Echo Should Succeed :: Verify echo of short string ... | FAIL |
[ t111 ] does not contain match for pattern 'u222'.
------------------------------------------------------------------------------
Extended Long Echo Should Succeed :: Verify echo of long string to... | FAIL |
[ t796676701934836782036789096074901985358332 ] does not contain match for pattern 'u8:7787812:4594789314789:1:7185:12:96469443'.
------------------------------------------------------------------------------

With the logic analyzer, no difference can be observed from the ESP32 side. The following snapshots show the reaction of ESP32 to the uart_write 1 flushcommand.

Figure 1: with current master

Figure 2: with PR #12955

The signals in these snapshots are

That is channel 3 and 5 are the signals generated by the ESP. The signal in channel 5 is the reaction of the ESP32 on the command uart_write 1 flush. In both cases, the test string flush is sent from ESP32 to PHiLIP-n. But in second case, PHiLIP-n doesn't answer with the incremented string.

Therefore, the uart_write 1 flush command is repeated after 2 seconds without any reaction from PHiLIP-n before PHiLIP-n restarts after a further second.

The only visible difference is that the ESP32 sends the test string on UART1 one character earlier relative to the output on UART0.

The complete captures from the logic analyzer:
master.logicdata.zip
deduplication.logicdata.zip

The Extended Short Echo test starts about 4 seconds after the first DUT_RST pulse in channel 6.

Currently the console gives feedback if something fails such as

==============================================================================
tests_periph_timer                                                            
==============================================================================
tests_periph_timer.Periph Timer Base :: Verify basic functionality of the P...
==============================================================================
Timer Init Should Succeed :: Verify timer_init return code            | FAIL |
'Error' does not contain 'Success'
------------------------------------------------------------------------------

It would be nice to have the full struct printed so we can see the values and the command in the console output, something like:

==============================================================================
tests_periph_timer                                                            
==============================================================================
tests_periph_timer.Periph Timer Base :: Verify basic functionality of the P...
==============================================================================
Timer Init Should Succeed :: Verify timer_init return code            | FAIL |
`Error' does not contain 'Success'
cmd: init_val(dev=0)
data: 5
msg: Initializing the periph timer
result: Error

------------------------------------------------------------------------------

There are a number of improves that should happen with the CI and specifically Jenkinsfile.

• Make Jenkinsfile declarative as it is better documented and seems to be preferred
• Run all tests on nodes before releasing them to prevent async behaviour
• Tune timeouts (total, node_total, per test)
• Proper error handling
• Cancel whole job quickly
• Build on master and only send binaries
  • Master can use build in docker to get binaries
  • Nodes must have a static RIOT folder /opt/RIOT used for flashing and make robot-test
  • Master only unstashes RobotFW and binaries in nodes for tests
• Only run nodes with unique boards
• Don't run tests if wiring isn't present
• Add params to only run selected boards
• Add params to only run selected applications
• Add params to run all cpu?
• Run RIOT tests?
• Add args for make as parameter

As the title says, what are the plans for ADC, DAC and PWM?

ADC, DAC and PWM pins are already listed in the environment configuration and also defined as PHiLIP ports. But it is not clear to me on how to map the the GPIO definition in the environment definition to an ADC/DAC line or a PWM channel.

Dependent on the plans for ADC, DAC and PWM, the GPIOs in esp32-wroom-32.env should be different. For example, using GPIOs 25, 26, 27 for GPIO tests would prevent to test a DAC pin.

It appears the attempt to flush on the interface level during connection causes some issues with the arduino. When sending characters in bootloader mode (the first 2 seconds during startup) it could cause the arduino to stay in bootloader mode for up to 10 seconds. The timeout is reset every time a new character is sent. So it would be good to remove the flash on connect (or make it optional) from the riot pal interface. If this is done then the arduino should only need a RESET_WAIT of 3 or so then everything should be good.

So far philip needs to get some modes set and read some register. It doesn't have any checks or logging to see if it works. We should fix that.

This should be fixed as the SDA is connected to PB9

Robot Framework 4.0 beta 1 was released on Oct 19, 2020 and the new major version has several incompatible changes which have to be addressed before a update can happen. The newest release notes contains all changes and necessary steps.

• Changes to test and suite statuses
• Old :FOR loop syntax is not supported anymore
• Meaning of @{var}[item] and &{var}[item] syntax has changed
• Argument conversion changes
• Libdoc spec changes
• Other backwards incompatible changes

Most important enhancements as listed in the README.

• New SKIP status
• Criticality has been removed
• Native IF/ELSE syntax
• Support for nested control structures
• Libdoc enhancements
• Argument conversion enhancements
• List and dictionary expansion with item access
• Positional-only arguments

Worth mentioning is the change from the concept of "critical tests" to a new "SKIP" status, which would need to review/rework every existing test, but in my opinion would be quite a improvement. There are several resources on the web about this feature:

• Release Notes (4.0 beta 1): Criticality has been removed
• New SKIP status #3622
• Remove criticality concept in favor of skip status #3624

Currently the nightlies fail Read Register After NACK Should Succeed test. It gets a nack successfully but then fails on the recovery.

When I try to do this manually on the desk and on the HiL rack (by calling the values from term), it recovers. Could this be a timing issue?

Sometimes we get EIO and other times it is ENXIO. I believe it should be ENXIO for a nack on ADDR and EIO for nack on data...

Make that a todo. It will break things though.

A problem was caught in the stm32_common/i2c_2.c that seem to make its way through the CI. We should add a way to catch similar bugs.

Feature request to add i2c_read_reg test, this may require an update to PHiLIP to catch this bug though...

Related issue

RIOT-OS/RIOT#12564

After running though the documentation a few things could be clarified.

• Document how to confirm PHiLIP flash was successful (by blinking LED)
• Reference how to get portnames on linux (/dev/tty*)
• Add troubleshooting if DUT portname is not default
• make sure you run from test folder or use -C tests/per...
• Add troubleshooting (tell people to double check pinout if only a few tests fail, point out you need to point to a riot version)
• Clarify example use of RIOTBASE

While analyzing why the following tests in tests/periph_i2c/tests/04__periph_i2c_flags fail for esp32-wroom-32

• Repeated Start Read Bytes 0 Should Succeed
  

  RobotFW-tests/tests/periph_i2c/tests/04__periph_i2c_flags.robot

  Lines 29 to 34 in 9d418dd

  	Repeated Start Read Bytes 0 Should Succeed
  	[Documentation] Verify DUT does not lockup on repeated start read
  	PHiLIP.write reg user_reg 0
  	API Call Should Succeed I2C Read Bytes leng=2 flag=${I2C_FLAG_NOSTOP}
  	API Call Should Error I2C Read Bytes leng=2
  	API Call Should Succeed I2C Read Bytes leng=2 flag=${I2C_FLAG_NOSTART}

• Repeated Start Read Bytes 0xFF Should Succeed
  

  RobotFW-tests/tests/periph_i2c/tests/04__periph_i2c_flags.robot

  Lines 36 to 41 in 9d418dd

  	Repeated Start Read Bytes 0xFF Should Succeed
  	[Documentation] Verify DUT does not lockup on repeated start read
  	PHiLIP.write reg user_reg 255
  	API Call Should Succeed I2C Read Bytes leng=2 flag=${I2C_FLAG_NOSTOP}
  	API Call Should Error I2C Read Bytes leng=2
  	API Call Should Succeed I2C Read Bytes leng=2 flag=${I2C_FLAG_NOSTART}

I was wondering why the second i2c_read_bytes call should fail?

In section 3.1.10, the I2C standard only specifies:

"A data transfer is always terminated by a STOP condition (P) generated by the master. However, if a master still wishes to communicate on the bus, it can generate a repeated START condition (Sr) and address another slave without first generating a STOP condition. Various combinations of read/write formats are then possible within such a transfer."

From my point of view, "address another slave ..." doesn't mean necessarily that a second read with the same slave address should fail. It just says, that the master can continue the communication on the bus with another slave. In fact, the slave address in combined format is the same and only the direction is changed. Furthermore, "Various combinations ..." does not imply that the combined read after write transfer must be the only one.

Furthermore, according to Note 4 in section 3.1.10

"I2C-bus compatible devices must reset their bus logic on receipt of a START or repeated START condition such that they all anticipate the sending of a slave address"

That is, all slaves including the former one should await an address field after a repeated start. So why shouldn't it be the same address as before?

Although it might not make sense to have consecutive read transfers from the same slave, the standard does not forbid this.

I have tested it with a chip PCF8575 chip from NXP, the owner of the I2C standard. The chip accepts consecutive i2c_read_bytes calls with repeated START conditions. So IMHO, the ESP32 I2C behaves correctly.